Skip to content

Align Go and Rust reader positions with libsystemd - #14

Open
ktsaou wants to merge 9 commits into
sow/agent-process-backlogfrom
sow-0045/reader-positions
Open

ktsaou wants to merge 9 commits into
sow/agent-process-backlogfrom
sow-0045/reader-positions

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

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() or QueryUnique() at EOF, or on an empty file, skipped the first newly appended entry. Related gaps:

  • directory readers exposed a stale entry after a realtime seek;
  • realtime-search ENTRY read errors were swallowed;
  • reader positions after boundary moves differed from libsystemd.

Changes (Go and Rust)

  • File, directory and facade readers follow the libsystemd position table:
    • an explicit head or tail has no current entry;
    • a failed move keeps the last returned entry current, in directory readers too;
    • a directory realtime seek clears the current entry.
  • Live appends after tail, after EOF and under installed filters are observed exactly once. Filters rebuild their posting state when the entry population grows.
  • 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 they can be rebuilt after the file grows.

Validation

  • New position-contract tests in both languages:
    • 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/... and cargo test --workspace pass.
  • Interoperability against systemd 255:
    • live matrix 8/8;
    • directory matrix PASS;
    • mixed-directory matrix 42/42.
  • run_journalctl_query_matrix.py against a pinned systemd v262 journalctl: 3586/3586 PASS.
  • Benchmarks:
    • Rust restart paths execute no more instructions than the baseline.
    • Go live backward restart adds about 23 instructions per restart (+0.4%); this cost was accepted and is recorded in the SOW.

Summary by cubic

Fixes a regression where a healthy Go Refresh() or QueryUnique() at EOF skipped the first newly appended entry, and aligns Go and Rust journal reader positions with libsystemd.

  • File, directory, and facade readers follow the libsystemd position contract: an explicit head or tail has no current entry, and a failed move keeps the last returned entry current.
  • Live appends after tail, after EOF, and under installed filters are observed exactly once; filters rebuild their posting state when the entry population grows, and directional cursor seeks check for growth first.
  • Realtime-search ENTRY read errors are returned instead of swallowed.
  • Go mmap live checks read header fields from a dedicated header mapping so an unchanged file costs no system call; Rust filters keep their source matches so a grown population can rebuild them.
  • Go Previous after Close at 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.
  • Adds shared position-contract tests in both languages, including cursor-seek live growth coverage for directory and facade readers.

Written for commit 21132ec. Summary will update on new commits.

View guided diff Turn on auto-fix

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.
@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1d308b4f-0c15-49af-8617-e5adb1990ea4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@codacy-production

codacy-production Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 374 complexity · 38 duplication

Metric Results
Complexity 374
Duplication 38

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Loading

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread go/journal/reader.go Resolved
Comment thread go/journal/reader_filter.go Resolved
Comment thread .agents/sow/specs/product-scope.md Resolved
- 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.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file (changes from recent commits).

Confidence score: 4/5

  • .agents/sow/SOW-status.md conflicts 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

Comment thread .agents/sow/SOW-status.md Resolved
Comment thread .agents/sow/SOW-status.md Resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant