Repository navigation
Conversation
Repair the SOW-0045 regression: healthy refresh at EOF or on an empty file skipped the first appended entry. File, directory and facade readers in both languages now follow the libsystemd position table: - explicit head/tail boundaries without a current entry; failed moves keep the last returned entry current, including in directory readers - live appends after tail, EOF and under installed filters are observed exactly once - directory realtime seek clears the current entry - realtime-search ENTRY read errors are returned Go mmap live checks read n_entries and the tail entry-array fields from a dedicated header mapping, so an unchanged file costs no system call; ReadAt keeps one header pread. Rust filters record their source matches without an extra copy so a grown population can rebuild them. Adds the shared position contract tests for Go and Rust. Validated with the full test suites, the interoperability matrices and the systemd v262 journalctl query matrix.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 374 |
| Duplication | 38 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
✅ No blocking issues noticed
No merge-blocking findings were identified. Two recommended test scenarios remain missing: direct realtime-search error propagation/retry coverage and Rust filter-source rebuild coverage. The complex filtering implementation and matrix tests increase regression risk, but the available findings do not establish a required functional fix before merging.
About this PR
- Consider adding focused coverage for the unverified behaviors and separating archival/status updates from the implementation-focused change to make scope and provenance easier to review.
2 comments outside of the diff
go/journal/reader_filter_native_test.go
line 281🟡 MEDIUM RISK
Suggestion: Split this matrix test into focused scenario helpers or table-driven subtests for sequential movement, merged candidates, filtered movement, and injected entry-load failures, while sharing fixture construction.
go/journal/reader_filter_step.go
line 25🔴 HIGH RISK
Suggestion: This method now owns several independent state-machine responsibilities, making position and recovery behavior difficult to validate. Consider extracting helpers for live refresh, filter installation, recovery retry, and EOF parking while preserving error propagation and allocation behavior.
Test suggestions
- Cross-language file, directory, and facade position tables covering head, tail, EOF, reversal, filtering, compact layout, and access modes
- Healthy append after an explicit Refresh or QueryUnique at empty-file and forward-EOF positions
- Installed filters observe values absent at installation time after a healthy append
- Installed filters observe healthy appends after a failed backward preparation or recovery retry
- Directory realtime seek clears getters until the next successful movement
- Realtime-search ENTRY read errors propagate in both Go and Rust and succeed after repair
- Go mmap and ReadAt live-header checks avoid repeated mapping/read work on unchanged files and balance mappings on close
- Rust filter-source rebuild preserves Boolean match composition and does not add per-install copying
- Cross-language journal interoperability and CLI query/follow parity
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Healthy append after an explicit Refresh or QueryUnique at empty-file and forward-EOF positions
2. Realtime-search ENTRY read errors propagate in both Go and Rust and succeed after repair
3. Rust filter-source rebuild preserves Boolean match composition and does not add per-install copying
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Architecture diagram
sequenceDiagram
participant Client as Client Code
participant GoReader as Go Reader
participant DirReader as Directory Reader
participant Facade as libsystemd Facade
participant RustReader as Rust Reader
participant RustDir as Rust Directory Reader
participant RustFacade as Rust SdJournal Facade
participant Mmap as mmap Header Window
participant ReadAt as ReadAt Preload
participant File as Journal File
participant Filter as Filter Engine
participant Contract as Position Contract Tests
Note over Client,Contract: Runtime Position Contract (SOW-0045)
Client->>GoReader: Step()/StepBack()
Client->>DirReader: Step()/StepBack()
Client->>Facade: SdJournalNext()/SdJournalPrevious()
Client->>RustReader: next()/previous()
Client->>RustDir: next()/previous()
Client->>RustFacade: next()/previous()
Note over GoReader,File: Live tail/EOF detection
GoReader->>Mmap: liveEntryState() - reads n_entries + tail array fields
alt Mmap mode
Mmap-->>GoReader: header fields (no syscall)
else ReadAt mode
GoReader->>ReadAt: one pread for header
ReadAt-->>GoReader: header fields
end
GoReader->>GoReader: Compare against loaded header
alt Entry population unchanged
GoReader-->>Client: No refresh, return cached result
else Population changed
GoReader->>File: Refresh entry offsets
File-->>GoReader: New entry ranges
GoReader->>Filter: Mark stale, rebuild posting state
end
Note over GoReader: Edge state machine
GoReader->>GoReader: edge = tail/after/before
alt Forward from tail
GoReader-->>Client: ErrEndOfEntries (no current entry)
else Backward from tail
GoReader->>File: Refresh if grown
GoReader-->>Client: Newest entry
end
Note over DirReader: Boundary + park logic
DirReader->>DirReader: boundary = head/tail/none
DirReader->>DirReader: rememberKeyPosition()
alt Failed move
DirReader->>DirReader: parkOnCurrentKey() - restore last entry
DirReader-->>Client: Keep last entry current
else Successful move
DirReader-->>Client: New entry
end
Note over Facade: Seek/position semantics
Facade->>GoReader: SeekTail()/SeekHead()
GoReader->>GoReader: Set edge state
GoReader-->>Facade: Position with no current entry
Facade->>GoReader: GetRealtimeUsec()
alt Edge tail
GoReader-->>Facade: ErrNoEntry
else Parked entry
GoReader-->>Facade: Last returned entry
end
Note over RustReader: Filter rebuild on growth
RustReader->>RustReader: installed_source tracks filter + entry count
alt Entry count changed + forward at EOF
RustReader->>RustReader: refresh_installed_filter() rebuild from source
RustReader->>File: Re-resolve DATA by hash
File-->>RustReader: New posting population
RustReader-->>Client: Observe new entries
else Unchanged
RustReader-->>Client: Continue from park position
end
Note over RustDir: Directory boundary semantics
RustDir->>RustDir: boundary enum (Head/Tail/None)
RustDir->>RustDir: park_on_current_key() restore
alt Previous at head or Next at tail
RustDir-->>Client: No entry
else Failed move
RustDir-->>Client: Keep last key current
end
Note over RustFacade: Matches Go facade behavior
RustFacade->>RustReader: Same step/position logic
RustFacade-->>Client: NoEntry or last entry after failed move
Note over Contract: Cross-language validation
Contract->>GoReader: 576-cell libsystemd position table
Contract->>DirReader: Same table
Contract->>Facade: Same table
Contract->>RustReader: Same table
Contract->>RustDir: Same table
Contract->>RustFacade: Same table
Note over GoReader,Filter: Absent match resolution
GoReader->>Filter: falseExpr keeps payload
Filter->>File: Re-resolve DATA on refresh
File-->>Filter: New DATA found after append
Filter-->>GoReader: Match now succeeds
Note over RustReader,Filter: Match source preservation
RustReader->>RustReader: take_source() keeps match bytes
RustReader->>RustReader: Operation log preserved
RustReader->>Filter: Rebuild from source on growth
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
- Go: liveEntriesChanged returns an error on a closed reader instead of dereferencing the cleared accessor, so Previous after Close at an explicit tail no longer panics. - Go and Rust: directional cursor seeks check for live growth first, as steps do, so an installed filter sees rows appended after it was built. - Go: the DATA hash-table guard moves into resolveFilterMatch, so filter refreshes reject a missing table as builds do. - Position-contract test setups now fail on movement errors. - Spec and SOW-0045: state the per-language live checks and matrix sizes, and record the user-authorized build-package exception.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
# Conflicts: # .agents/sow/current/SOW-0045-20260528-go-reader-alignment-optimization.md
# Conflicts: # .agents/sow/SOW-status.md
# Conflicts: # .agents/sow/SOW-status.md
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 4/5
.agents/sow/SOW-status.mdconflicts with SOW-0160’s decision record, leaving it unclear whether the 256 KiB choice is authorized. Record the decision and its scope in SOW-0160, or remove the decision claim here.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".agents/sow/SOW-status.md">
<violation number="1" location=".agents/sow/SOW-status.md:17">
P2: This ledger contradicts SOW-0160's own decision record, so readers cannot tell whether the 256 KiB choice is authorized. Record the decision and its scope in SOW-0160, or remove the decision claim here.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Stacked on #13; merge #13 first. Completes SOW-0045 (
.agents/sow/done/SOW-0045-20260528-go-reader-alignment-optimization.md).Problem
A healthy Go
Refresh()orQueryUnique()at EOF, or on an empty file, skipped the first newly appended entry. Related gaps:Changes (Go and Rust)
n_entriesand the tail entry-array fields from a dedicated header mapping, so an unchanged file costs no system call. ReadAt keeps one headerpread.Validation
go/journal/reader_position_contract_test.go, a 576-cell libsystemd table;rust/src/journal/src/tests/position_contract.rs.CGO_ENABLED=0 go test ./...,CGO_ENABLED=1 go test ./cmd/...andcargo test --workspacepass.run_journalctl_query_matrix.pyagainst a pinned systemd v262 journalctl: 3586/3586 PASS.Summary by cubic
Fixes a regression where a healthy Go
Refresh()orQueryUnique()at EOF skipped the first newly appended entry, and aligns Go and Rust journal reader positions with libsystemd.PreviousafterCloseat an explicit tail returns an error instead of panicking, and the DATA hash-table guard moved into filter match resolution so refreshes reject a missing table.Written for commit 21132ec. Summary will update on new commits.