Skip to content

Close single-ownership follow-ups against systemd - #27

Open
ktsaou wants to merge 15 commits into
sow-0097/cli-sdk-single-ownerfrom
sow-0176/single-ownership-followups
Open

ktsaou wants to merge 15 commits into
sow-0097/cli-sdk-single-ownerfrom
sow-0176/single-ownership-followups

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Stacked on #26 (SOW-0097). Merge after it.

Closes the follow-ups from the single-ownership work, each checked against systemd and delivered in Go and Rust:

  • --list-boots and -b match v262 on journals without _BOOT_ID DATA; callers opt into the old header fallback with BootListOptions, and listing errors are returned.
  • Verification keys follow the v262 grammar through one public SDK parser per language; the CLI copies are gone. Keys are judged per file, and a rejected key stops the run with v262's message.
  • An unreadable DATA object or a payload without = costs one field, not the entry, in both languages.
  • Go retention counts *.journal~ and deletes them first; Rust covers chain-named disposed files too.
  • Rust: open_files deduplicates, single-value maps keep the first repeated value, a missing main entry array is detected at open, and boot ranges stop at the visible tail.
  • Non-UTF-8 source names convert the same way in both languages.

Validation against a systemd 262 oracle built with sealing: Go and Rust unit tests, byte identity, verify, directory, mixed directory, live, query, reader hardening and parser parity matrices. A differential of 29 verification keys on sealed and plain files shows no exit-code differences.

Known remaining difference: v262 prints a => Validated from ... summary after a sealed PASS; that needs a new SDK result type and is tracked by SOW-0177.


Summary by cubic

Closes the single-ownership follow-ups by aligning Go and Rust journal behavior with systemd v262 across boot listing, verification keys, damaged entries, retention, and source-name handling.

  • --list-boots and -b now match v262 on journals without _BOOT_ID DATA: no boots are listed by default, and callers opt into the old header fallback with BootListOptions; listing errors are returned.
  • Verification keys follow the v262 grammar through one public SDK parser per language, replacing the CLI copies; keys are judged per file and a rejected key stops the run with v262's message.
  • An unreadable DATA object or a payload without = costs one field, not the entry, in both languages.
  • Go retention counts *.journal~ files and deletes them before archived history; Rust covers chain-named disposed files too.
  • Rust open_files deduplicates paths, single-value maps keep the first repeated value, a missing main entry array is detected at open, and boot ranges stop at the visible tail.
  • Non-UTF-8 source names convert the same way in both languages.

The one remaining difference is the => Validated from ... summary v262 prints after a sealed PASS; that needs a new SDK result type and is tracked in SOW-0177.

Written for commit b0751f3. Summary will update on new commits.

View guided diff Turn on auto-fix

ktsaou added 12 commits November 8, 2026 00:09
…as Go does

DirectoryReader::list_boots and SdJournal::list_boots now return a Result
instead of falling back to one boot per file header when files cannot be
read. Activates SOW-0176 with the user's decisions on the legacy export
contract (kept) and this signature.
…ership-followups

# Conflicts:
#	rust/README.md
Go no longer fails a whole entry on a bad DATA object, and Rust skips the same
damage systemd does. Rust single-value maps keep the first repeated value, and
Rust open_files ignores repeated paths, matching Go and systemd.
The default boot list follows v262 for journals without _BOOT_ID DATA, with an
opt-in header fallback. Go retention now bounds disposed journal files as Rust
and journald do, Rust detects a missing main entry array at open and bounds boot
postings by the visible tail, and invalid source-name bytes become U+FFFD.
@coderabbitai

coderabbitai Bot commented Nov 8, 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: 7c18106e-2212-4421-b0a8-e90c17e4327d

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

@codacy-production

codacy-production Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Not up to standards ⛔

🔴 Issues 1 critical

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
Complexity 1 critical

View in Codacy

🟢 Metrics 127 complexity · 13 duplication

Metric Results
Complexity 127
Duplication 13

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

⚠️ Resolve issues before merging

A high-risk race remains in retention cleanup: a scanned pathname can be replaced before deletion, allowing an unrelated file to be removed. This should be fixed before merging.

The sealed verification summary remains explicitly deferred rather than silently treated as complete. Coverage data was unavailable, so line-level validation cannot be independently confirmed.

Test suggestions

  • Default and opt-in header-fallback boot listing, including noncanonical _BOOT_ID DATA
  • Boot listing preserves reader position and matches
  • Verification-key parser accepts and rejects systemd-compatible grammar cases in both languages
  • Invalid verification keys are judged only after a file is reached and stop verification
  • Damaged DATA objects and missing separators preserve the entry while excluding the damaged field
  • Disposed files, including chain-named files, are counted and deleted before archives
  • Recent disposed files do not prevent older files from satisfying max-age retention
  • Rust repeated paths are opened once and repeated scalar fields retain the first value
  • Missing main entry arrays hide entries for filtered and unfiltered reads
  • Boot ranges stop at the reader's visible tail
  • Non-UTF-8 source names use equivalent replacement behavior
  • Sealed verification output includes the systemd validated-range summary
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Sealed verification output includes the systemd validated-range summary
Low confidence findings
  • Coverage results are unavailable, so the claimed validation cannot be independently checked at line level.

TIP How was this review? Give us feedback

Comment thread go/journal/log_retention.go Resolved

@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 44 files

Architecture diagram
sequenceDiagram
    participant CLI as Go/Rust journalctl CLI
    participant SDK as SDK Reader API (Go/Rust)
    participant BootSvc as Boot Listing Service
    participant VerifySvc as Verification Key Service
    participant DataReader as Entry Data Reader
    participant RetentSvc as Retention Service
    participant Netdata as Netdata Source Name

    Note over CLI,Netdata: Single-ownership follow-ups aligned with systemd v262

    CLI->>SDK: ListBoots() / list_boots()
    SDK->>BootSvc: Query _BOOT_ID DATA
    alt No _BOOT_ID DATA and no fallback
        BootSvc-->>CLI: No boots listed (v262 match)
    else HeaderFallback enabled
        BootSvc->>BootSvc: Fallback to entry header boot IDs
        BootSvc-->>CLI: Boots from headers
    end
    BootSvc-->>SDK: BootInfo list (or error on read failure)

    CLI->>VerifySvc: ParseVerificationKey() / parse_verification_key()
    VerifySvc->>VerifySvc: Validate key with v262 grammar
    alt Key rejected
        VerifySvc-->>CLI: "Key was rejected by service"
        CLI->>CLI: Stop run with v262 message
    else Key accepted
        VerifySvc-->>CLI: Parsed verification key
        CLI->>SDK: VerifyFileWithKey() / verify_file_with_key()
        SDK->>SDK: Verify sealed journal per file
    end

    DataReader->>DataReader: Read entry DATA objects
    alt Unreadable or no '=' in payload
        DataReader->>DataReader: NEW: Skip damaged field, keep entry
        DataReader-->>SDK: Entry with remaining fields
    else Missing main entry array
        DataReader->>DataReader: Detect at open, hide all entries
        DataReader-->>SDK: No entries returned
    end

    RetentSvc->>RetentSvc: Scan journal files including *.journal~
    RetentSvc->>RetentSvc: Count disposed files, delete oldest first
    RetentSvc-->>CLI: Retention enforced (disposed before archives)

    Netdata->>Netdata: Convert non-UTF-8 source name
    Netdata->>Netdata: Use lossyUTF8() / to_string_lossy()
    Netdata-->>Netdata: Consistent source name across Go/Rust
Loading

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

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

Comment thread go/journal/log_retention.go Resolved
Comment thread docs/Writer-APIs.md Resolved
Comment thread docs/Reader-APIs.md Resolved
Comment thread docs/Rust-API.md Resolved
Comment thread go/journal/reader_entry.go Resolved
Comment thread go/journal/reader_entry.go Resolved
Comment thread .agents/sow/SOW-status.md Resolved

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

2 issues found across 14 files (changes from recent commits).

Confidence score: 4/5

  • .agents/sow/done/SOW-0176-20261108-single-ownership-followups.md marks the SOW complete while compressed-DATA failure behavior is still undecided. Record the user’s decision and disposition the behavior, or keep the SOW open and track the follow-up.
  • entry_damage.rs checks payload counts but not the returned values, so the tests could pass without proving the damaged field is skipped and MESSAGE=kept remains in the collect and enumerate APIs. Assert the payload values too.
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/done/SOW-0176-20261108-single-ownership-followups.md">

<violation number="1" location=".agents/sow/done/SOW-0176-20261108-single-ownership-followups.md:347">
P2: This completed SOW still leaves compressed-DATA failure semantics undecided. Record the user’s decision and disposition this behavior, or keep the SOW open and track the follow-up before marking it completed.</violation>
</file>

<file name="rust/src/journal/src/tests/entry_damage.rs">

<violation number="1" location="rust/src/journal/src/tests/entry_damage.rs:76">
P3: These assertions verify only payload counts, so they do not prove that the damaged field is skipped while `MESSAGE=kept` remains in the collect and enumerate APIs. Assert the returned payload values as well, otherwise those APIs can return the wrong field set without failing this regression test.</violation>
</file>

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

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

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