Skip to content

Drop only the field when compressed DATA cannot be decoded - #28

Open
ktsaou wants to merge 5 commits into
sow-0176/single-ownership-followupsfrom
sow-0178/undecompressible-data-field
Open

ktsaou wants to merge 5 commits into
sow-0176/single-ownership-followupsfrom
sow-0178/undecompressible-data-field

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Stacked on #27 (SOW-0176). Merge after it.

A compressed DATA object that cannot be decoded now costs only its field, not the whole entry, in Go and Rust:

  • Entry reads, payload visitors, entry enumeration, facade data getters, Explorer/Netdata facets and the Rust log-query row projection skip the field.
  • Unique-value enumeration and --verify still report the damage, as systemd does.
  • Rust: the "bad DATA behind one entry item" rule moves to JournalError::is_recoverable_entry_data in journal-core, so the SDK and journal-engine share one owner.

Behavior against a systemd 262 oracle on files with one corrupted compressed field:

Codec systemd 262 Go and Rust
zstd, lz4 rows kept, field dropped same
xz read fails (Cannot allocate memory) rows kept, field dropped (intended difference, documented)

run_reader_hardening.py now expects stock's rows for the corrupt and oversized zstd streams and checks them against stock -o export; strict verification still rejects those files.

Validation: Go and Rust unit tests, reader hardening, compression, verify, mixed directory, live and query matrices, docs examples and the duplication gate.


Summary by cubic

A compressed DATA object that cannot be decoded now costs only its field, not the whole entry, matching systemd 262 for zstd and lz4. Entry reads, payload visitors, entry enumeration, facade data getters, Explorer/Netdata facets, and the Rust log-query row projection skip the damaged field, while unique-value enumeration and --verify still report it; corrupt xz drops the field where systemd fails the whole read (documented difference).

Refactors

  • The Rust bad-DATA rule moved to JournalError::is_recoverable_entry_data in journal-core, giving the SDK and journal-engine a single owner.
  • The Rust tests share the damaged compressed entry setup between the field-drop and Explorer tests.

Bug Fixes

  • The hardening oracle now parses stock -o export output and compares per-row realtime and payloads instead of row counts, asserting the damaged MESSAGE field is dropped.

Written for commit 2daaa0b. Summary will update on new commits.

View guided diff Turn on auto-fix

@coderabbitai

coderabbitai Bot commented Nov 9, 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: 68ee296d-f426-42a8-bf30-77c21300c073

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 9, 2026 •

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 43 complexity · 4 duplication

Metric Results
Complexity 43
Duplication 4

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 blocking implementation findings were identified. The review does note missing direct regression coverage for payload visitors, Netdata, Rust query projection, and unique-value error propagation; these independently specified behaviors should be covered to reduce regression risk.

About this PR

  • Add focused automated coverage for the currently unverified payload, Netdata, Rust projection, and unique-value behaviors.

Test suggestions

  • GetEntry and entry payload collection preserve readable fields for corrupted zstd, xz, and lz4 DATA
  • Payload visitors, entry enumeration, and facade data getters skip only the damaged field
  • Explorer facets and rows survive corrupted compressed DATA
  • Netdata row expansion skips damaged compressed fields
  • Rust journal-engine query projection skips damaged fields
  • Unique-value enumeration still returns a decompression error
  • Strict Go and Rust verification reject damaged compressed DATA
  • Hardening and stock-oracle checks validate preserved rows and dropped fields
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Payload visitors, entry enumeration, and facade data getters skip only the damaged field
2. Netdata row expansion skips damaged compressed fields
3. Rust journal-engine query projection skips damaged fields
4. Unique-value enumeration still returns a decompression error

Make this review better

TIP Add a ticket key to the branch or title so the reviewer can check the PR against it. Or tell us which ticket tracker you use here.

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.

8 issues found and verified against the latest diff

Confidence score: 3/5

  • product-scope.md now says malformed payloads are field-local, but the sentence below still says they remain errors. Resolve that contradiction so the intended failure behavior is clear.
  • The reader tests can miss incorrect field drops or returned rows: run_reader_hardening.py checks only counts, and reader_decompress_damage_test.go does not assert that MESSAGE is unset. Compare the relevant export fields and assert the damaged field is dropped.
  • The completed SOW has stale differential results presented as current and omits the required failure-handling record. Label the baseline results and document how validation failures are recorded and resolved.
  • verify_graph.go wraps the decompression error with %v, which loses the error chain. Preserve it with %w so callers can inspect the underlying error.
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-0178-20261109-undecompressible-data-costs-one-field.md">

<violation number="1" location=".agents/sow/done/SOW-0178-20261109-undecompressible-data-costs-one-field.md:34">
P3: The differential facts are stale baseline results but are not labeled as pre-change, contradicting the later validation that Go and Rust retain rows and drop the damaged xz field. Mark this differential and the hardening expectation as baseline observations.</violation>

<violation number="2" location=".agents/sow/done/SOW-0178-20261109-undecompressible-data-costs-one-field.md:71">
P3: The analysis claims `docs/Rust-API.md` was part of the documentation change, but this PR updates only `Reader-APIs.md`. Remove it from the changed-docs list or record why the Rust API page needs no update.</violation>

<violation number="3" location=".agents/sow/done/SOW-0178-20261109-undecompressible-data-costs-one-field.md:164">
P3: This completed SOW omits the template’s required `Failure handling` record. Add how failed validation or audits are recorded, repaired, rerun, and prevented from closing the SOW.</violation>
</file>

<file name=".agents/sow/specs/product-scope.md">

<violation number="1" location=".agents/sow/specs/product-scope.md:807">
P2: This new paragraph makes bad DATA type/size and undecodable compressed payloads field-local, but the unchanged sentence just below still says wrong types, malformed nonzero sizes and decompression failures remain errors. Reconcile the two by scoping the older sentence to ENTRY/ENTRY-array tails and the other error classes, or by rewording it to exclude the field-local DATA cases.</violation>
</file>

<file name=".agents/sow/SOW-status.md">

<violation number="1" location=".agents/sow/SOW-status.md:85">
P3: This summary entry is cryptic: 'decisions 1B, xz = B' refers to options that only exist in the SOW file. State the outcome instead, including that the xz behavior intentionally differs from systemd.</violation>
</file>

<file name="go/journal/verify_graph.go">

<violation number="1" location="go/journal/verify_graph.go:1184">
P3: The wrapped decompression error uses `%v` for the underlying error, so its chain is lost. Wrap it with a second `%w` to keep it inspectable.</violation>
</file>

<file name="tests/interoperability/run_reader_hardening.py">

<violation number="1" location="tests/interoperability/run_reader_hardening.py:293">
P2: This oracle check can pass while stock export returns different rows or drops the wrong field, because it validates only row and `MESSAGE` counts. Parse the export records and compare the relevant fields and row identity against `expected` so the hardening test detects incorrect recovery behavior.</violation>
</file>

<file name="go/journal/reader_decompress_damage_test.go">

<violation number="1" location="go/journal/reader_decompress_damage_test.go:101">
P2: This test does not assert that the damaged `MESSAGE` field is actually dropped. Add assertions that the returned row excludes `MESSAGE` and that its facet is reported as unset rather than as the corrupted value; exercise the indexed strategy separately if that path is covered by this behavior.</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/specs/product-scope.md Resolved
Comment thread .agents/sow/SOW-status.md Resolved
Comment thread go/journal/verify_graph.go Resolved
The reader-hardening oracle now parses journalctl export output and compares
each row's realtime and payloads instead of counting rows and MESSAGE lines.
Both explorer damage tests assert MESSAGE is absent from the row and counted
only as unset. Go keeps the decompressor error in the wrapped chain. The spec
scopes the remaining-errors sentence to tail recovery, and the SOW labels its
baseline facts, corrects the docs list and records failure handling.

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