[jussielo-amd]

Summary

Repairing a torn tail of an automation JSONL store (events, audit, proposals) rewrites the whole store and renames it over the original. If that rewrite cannot complete, nothing else was tried: the debris stayed, and every later append to the same store hit the same failure. Because rocmd treats an append error as fatal (record_event at daemon start and in the watcher loop), a store stuck this way could keep the daemon from starting, and every rocm command that records an audit event printed a warning on each invocation.

The rewrite can fail on a full disk (no room for the temp copy), and on Windows when another program such as a virus scanner or indexer holds the store open without delete sharing, which blocks the rename. This change falls back to cutting the debris in place, under the store's lock, only when the replace itself fails, and then does a normal append. A truncate needs no free space and no rename, so it can succeed where the replace cannot. Fixes #748.

What changes for users

• A torn store whose replace fails is now repaired silently by the fallback. The repeated audit-event warnings stop and rocmd starts. Which records survive is unchanged: the debris is dropped exactly as before.
• The normal path is untouched: a successful replace is followed by no truncate, and clean or merely unterminated stores are still a plain append.
• If both the replace and the truncate fail, the error is one line naming the store and both causes. If the cut works but the append after it fails, that error also carries the replace failure, since it is the root cause.
• No flags, output formats or --help text change. The fallback prints nothing, like a successful repair.

Notes for reviewers

The issue says the store is never rotated or capped. That was true when it was filed but #744 has since added rotation for the events and audit stores and compaction for proposals, so the whole-store rewrite is bounded (about 8 MiB for events and audit). The failure mode is real; the "needs space for an unbounded store" framing no longer is.

This is the first of the two directions listed in the issue. I did not take the sentinel approach: readers deliberately error on any unparseable line that is not the last one, so resealing debris in the middle of the store changes the store format and would break reads for older rocm/rocmd sharing it.

The fallback knowingly reopens, on this already-failing path only, the shrink that #738 closed for the normal path. The reader's guards narrow that window without closing it: a scan that comes back short is refused or retried, but a scan that already read part of a debris line longer than one scan chunk and then finds the file regrown past the offset the scan has reached is not detected and could splice the two reads. That needs a failing replace, a debris line over 64 KiB, and a reader mid-scan at once. The issue accepts the narrower window on this rare path; the doc comment on repair_torn_tail_and_append states it precisely, and the reader-side docs no longer claim a retry covers every shrink.

Out of scope: the proposal status rewrite also depends on write_atomic, but it is a compaction rewrite with no in-place alternative, so it is a different class. Making rocmd tolerate an append failure at start is a separate error-policy decision; a genuinely full disk can still fail the append of the record itself.

Testing

The torn-tail repair is a helper that takes the replace and truncate steps as parameters, so each failure can be injected. New unit tests cover: the fallback cuts the debris in place and appends (same file identity afterwards, replace attempted first with the full repaired store); a working replace is never followed by a truncate; both steps failing reports both causes with their full context chains and leaves the store as it was; a failed append after the cut still reports the replace failure; and appends after a fallback repair are plain appends. Two tests use a real fault instead of an injected one: on Unix, a store directory that takes no new files (skipped when run as a user the permission bits do not bind, such as root; CI runs it as a regular user), and on Windows, another handle holding the store open without delete sharing.

I broke the code under test to confirm the tests fail: removing the fallback, always falling back, dropping the # from one cause so its context chain is lost, and dropping the replace failure from the append error each turn the matching tests red. cargo test -p rocm-core, cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --check pass locally on Linux.

Not run locally: cargo xtask coverage and scripts/smoke_local.py. The Windows test has no Windows host here, so its first run was the Windows CI job, and I could not show that dropping the separate write handle turns it red.

New scenario automations-10 runs the fallback through an audited CLI command, rocm config set-permissions: with a torn audit log and an audit directory that takes no new files, the command must confirm the change, print no audit-log warning, and leave an audit log holding the earlier record and the new one with the torn line gone. Run against a build with the fallback removed it fails on the audit-log warning, which is the reported symptom. That fault is a directory permission, which does not bind root, and the self-hosted WSL2 guest runs the suite as root, so the scenario carries a new @requires-dir-permissions tag backed by a capability probe (modelled on @requires-case-sensitive-fs) and resolves to a skip there instead of passing without testing anything. The probe and the tag resolution have unit tests, including that the OS reason is reported before the permissions one when both premises fail, and I mutated each to confirm they fail. The tag is documented in the e2e README, and docs/testing.md now covers the repair tests and the scenario. Existing scenario automations-06 covers the read side of a torn tail and is unchanged.

[Copilot]

🟡 Changes recommended

The replacement-failure fallback still lacks an audited CLI Gherkin scenario verifying that audit warnings stop, clean records are preserved, and the new event is saved.

1 open issue

• Add audited CLI coverage for failed replacement fallback · New

What changed in this PR

Adds a fallback for torn JSONL tails in automation stores, addressing #748 when atomic replacement fails while preserving the existing normal repair path.

Changes:

• Falls back to truncating debris under the store lock, then appending the new record.
• Preserves both failure causes in errors and documents the accepted concurrent-reader risk.
• Adds injected-failure tests and platform-specific tests for directory permissions and Windows sharing restrictions.

File	Description
crates/​rocm-core/​src/​automation.rs	Implements the truncate-and-append fallback, updates reader-safety documentation, and adds regression tests.

_{Review effort: Balanced}

[jussielo-amd]

Added in fbef4b2: scenario automations-10 drives the fallback through an audited CLI command. With a torn audit log and an audit directory that takes no new files, rocm config set-permissions must confirm the change, print no audit-log warning, and leave a log holding the earlier record and the new one with the torn line gone. Run against a build with the fallback removed, it fails on the "failed to write audit event" warning, which is the reported symptom.

Because that fault is a directory permission, which does not bind root (the self-hosted WSL2 guest runs the suite as root), the scenario is Linux-gated and also carries a new @requires-dir-permissions tag backed by a capability probe, so it skips there rather than pass without testing anything. The tag is documented in the e2e README and docs/testing.md, and the PR description is updated to match.

[Copilot]

🔵 Needs a closer look

A maintainer must confirm that the fallback’s deliberate restoration of an in-place shrink/regrow race and record-corruption risk for lock-free JSONL readers is acceptable.

1 open issue

• Add audited CLI coverage for failed replacement fallback

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 9cbf004

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change. Implements the truncate-fallback direction of issue #748 (torn-tail repair of a JSONL store has no fallback when the atomic replace can't complete); the PR explicitly scopes out #748's alternative "sentinel" direction with stated reasoning, and separately scopes out the related rocmd-tolerates-append-failure policy question as out of scope. "Fixes #748" is accurate: the issue asked for a fallback, not both directions it raised.

Blocking

None

Non-blocking

• Doc comment overstates how much the store must regrow for the accepted splice hazard to go undetected — crates/rocm-core/src/automation.rs:466 (the PR's own "Notes for reviewers" additionally claims this comment "states it precisely")
  The comment says an in-flight reader's buffered read "finds the file regrown to at least its old length" before the short-read guard is defeated. Tracing ReverseLines::next_line (automation.rs:982-1017) and its guard (bytes.len() as u64 != self.pos - start && !self.pending.is_empty()) shows the guard only compares against self.pos at the current iteration, which strictly decreases as the backward scan progresses — so an undetected splice only needs the file to regrow to at least the scan's current offset, not to its original full length. The qualitative "narrow, accepted risk" conclusion is unaffected, and the underlying design choice is sound (confirmed against issue #748, which explicitly accepted this exact tradeoff in review of #738), but the written risk characterization permits a narrower window than the hazard actually has.
  Confidence 75 · architectural · Fix: loosen the comment's wording from "its old length" to "the scan's current offset" (or state the bound relative to chunk size directly).

• New @requires-dir-permissions gate has no regression test pinning that requires_os is still checked first — tests/e2e-cucumber/src/expectation.rs (resolve(), the new check added after the requires_os check) and tests/e2e-cucumber/features/automations.feature (automations-10, tagged with both @requires-os:linux and @requires-dir-permissions)
  Today's ordering is correct (confirmed by reading resolve(): requires_os is checked before requires_dir_permissions), and the new tag's own pass/skip resolution is tested. But the sibling tag it's explicitly modeled on, @requires-case-sensitive-fs, shipped with exactly this ordering wrong once (commit 2fdff46c5, sub-commit "report the OS before the case-folding reason when both gates fail": "On native Windows a scenario tagged both @requires-os:linux and @requires-case-sensitive-fs failed both premises, and the grid reported the filesystem reason."), and that fix came with a dedicated pinning test, os_reason_wins_over_case_folding_reason_on_a_non_matching_os. No equivalent test exists for the requires_dir_permissions/requires_os combination, so nothing would catch a future refactor silently reintroducing the same bug class for this tag.
  Confidence 85 · mechanical · Fix: add a test mirroring os_reason_wins_over_case_folding_reason_on_a_non_matching_os for requires_dir_permissions.

Decisions for the author

• The fallback repair prints nothing, even though its trigger (full disk / Windows sharing violation) is operator-relevant and the analogous rotation-rename failure path does warn — tradeoff
  rotate_jsonl_generations's rename failure gets a once-per-store eprintln! via rotation_failure_warning, but repair_torn_tail_and_append's replace failure — the same class of failure, and one that additionally reopens a documented reader-splice risk — produces no signal at all (confirmed by reading the whole function body). This is deliberate and stated in the PR text ("The fallback prints nothing, like a successful repair."). The counter-argument is that a repair which fully recovers the data shouldn't alarm the operator differently depending on which repair strategy happened to succeed. Both sides are reasonable; worth confirming this was weighed against rotation_failure_warning's precedent rather than just inherited from the normal replace path's silence by default.

Positive signals

• repair_torn_tail_and_append holds the store's lock continuously from tail_start computation through the full repair — no TOCTOU between inspecting the tail and cutting it.
• Error-chain causality is deliberately engineered and tested: dedicated tests pin the exact message text for both-steps-fail and cut-succeeds-but-append-fails, including the replace failure surviving as root cause in the latter.
• Two of the five new automation.rs tests drive the real production fallback (a genuinely read-only directory on Unix, a genuinely open handle without delete-sharing on Windows) rather than only injected stub failures, and use file-identity checks (inode / volume-serial+file-index) that can't be faked by a subtly wrong implementation.
• The e2e capability-probe/tag addition (dir_permissions_bind/@requires-dir-permissions) is threaded completely through every site its template tag (case_sensitive_fs) touches, and the new scenario's final assertion checks the exact ordered record list plus trailing-newline, which would catch a reordered or duplicated append.

Deployment notes

None.

What this covered

Read completely: crates/rocm-core/src/automation.rs in full (current + diff against merge-base), and all seven other changed files (docs/testing.md, tests/e2e-cucumber/README.md, tests/e2e-cucumber/features/automations.feature, tests/e2e-cucumber/src/capability.rs, tests/e2e-cucumber/src/expectation.rs, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/automations_steps.rs), plus a workspace-wide grep for callers of every changed symbol (none outside this file) and the repo's git history for the touched regions. Reviewed as two coupled file-groups (core fix; e2e test infra) by independent fan-out workers, plus independent agent-instruction-adherence and history/prior-changes passes; findings above were re-verified directly against the source before scoring, not taken on a worker's word. Diff range: b9d5fa9d93d7c950a1b6453c1705aac9dc7921de...9cbf0042896486c47980e8cd1ec59baa7b1981be.

CI at the reviewed commit: all completed checks are green (build-and-test, windows-build-and-test, Test (affected crates), clippy, CodeQL, coverage, license/notices, markdown links, commit-signatures, and the primary "E2E tests"/"E2E consolidated report" run). A separate, later-triggered batch of platform-specific E2E jobs (Strix Halo/MI300X/W7900/Windows, nightly and release lanes) was still in-progress/queued at review time; none had failed, but their outcome could not be confirmed here.

AGENTS.md (root agent-instruction file) was read and checked in full; no violations found. Not reconciled against any discussion on this PR (out of scope per method — only the description and issue #748 were read, never review threads/comments).

---

🤖 Generated with Claude Code

[juhovainio]

I read the diff and found nothing blocking. The torn-tail fallback looks correct, and the new tests cover both the blocked-replace case and the audited-command path.

Two small nits are left inline: a couple of reflowed comment lines are now overlong, and the e2e step hardcodes the lock file name instead of deriving it.

Two things to keep in mind, neither a change request: the truncate fallback reopens a narrow window where a lock-free reader can see the file shrink (documented, and accepted in #748), and the real-fault unit test and scenario automations-10 skip when run as root, so they don't exercise the fault there.

This reflects a read of the diff only, not a local run; the hardware E2E lanes were still pending when I looked.

[juhovainio]

Nit: this reflowed doc line is now well over the line width (rustfmt leaves comments alone); please rewrap the paragraph.

[jussielo-amd]

Rewrapped in 9d36372.

[juhovainio]

Nit: same here, the reflowed test comment line is overlong; rewrap.

[jussielo-amd]

Rewrapped in 9d36372.

[juhovainio]

Nit: the lock file name is hardcoded; if jsonl_lock_path changes, this creates the wrong file. Consider deriving it from the same helper or noting the coupling.

[jussielo-amd]

Fixed in 9d36372: jsonl_lock_path is now public and re-exported from rocm_core, and this step creates the lock file through it (jsonl_lock_path(&paths.audit_events_path())), so it cannot drift from the name the writers lock on. I ran automations-10 locally against the change and it passes.

[jussielo-amd]

Follow-up on the automated review of 9cbf004, addressed in 9d36372.

Doc comment on the regrowth condition. Confirmed against ReverseLines::next_line: the short-read check compares each read with self.pos - start, and self.pos only decreases as the scan proceeds, so an undetected splice needs the file to regrow past the offset the scan has reached, not to its old length. The doc comment on repair_torn_tail_and_append now says that. The PR description made the same "old length" claim, so I corrected it there too.

Ordering test for @requires-dir-permissions. Added os_reason_wins_over_dir_permissions_reason_on_a_non_matching_os, modelled on the case-folding twin. Its fixture fails both premises (a non-Linux host where the permission bits do not bind), and it first asserts both tags parsed so it cannot pass vacuously. To confirm it tests the ordering, I disabled the OS check in resolve(): this test and the existing case-folding one both went red with the other premise's reason, and both pass again with the check restored.

Should the fallback print a warning, as the rotation-rename failure does? I kept it silent, deliberately, for two reasons. First, the two failures leave different states behind: a failed rotation leaves the store stuck over its cap and growing, which nothing else surfaces, while the fallback fully recovers the store and the records it keeps are identical to a successful replace. A warning would report a repair that worked. Second, the rotation warning is deduplicated by a process-local static, and rocm commands are short-lived, so the same dedupe here would still print once per invocation. That is the repeated audit-event warning this PR sets out to remove. The replace failure is not lost when it matters: if the cut or the append then fails, the error carries it as a cause.

[nowycondro]

🤖 Automated review (Claude Code) · 9d36372

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change. Implements the truncate-fallback direction of issue #748 (torn-tail repair of a JSONL store has no fallback when the atomic replace can't complete); the PR explicitly scopes out #748's alternative "sentinel" direction with stated reasoning, and separately scopes out the related rocmd-tolerates-append-failure policy question as out of scope. "Fixes #748" is accurate: the issue asked for a fallback, not both directions it raised.

Blocking

None

Non-blocking

• jsonl_lock_path's doc comment overextends the "lands in a copy and vanish" consequence to the truncate fallback, which doesn't create a copy — crates/rocm-core/src/automation.rs:243-254
  The comment lists three reasons the lock guards against a stale open handle: replace-by-rename, the new truncate-fallback, and rotation-by-rename — then concludes "an append that had already opened the old file would otherwise land in the replaced or rotated-away copy and vanish." That consequence only follows from the rename-based mechanisms (a new inode replaces the old one, so a stale handle's write lands in an orphaned, unread copy). The truncate fallback mutates the same file in place — confirmed by the file-identity assertion in a_torn_tail_is_cut_in_place_when_replacing_the_store_fails, which pins before == after — so a stale handle's append would land in the live file, not vanish into a copy. Every other doc comment this diff touches (read_range, load_recent_jsonl_records_from, TailState::Torn, repair_torn_tail_and_append's own doc) carefully scopes its claims with an "except the fallback…" qualifier; this one, the first one edited in the diff, does not, and now implies a consequence of the truncate path that isn't true.
  Confidence 70 · mechanical · Fix: separate the truncate-fallback mention from the "lands in a copy and vanish" clause, since the lock's role there is purely the first-listed reason (serializing against other appends), not protecting against a vanishing write.

• probe_dir_permissions_bind's permission restore isn't failure-safe on its own error path — tests/e2e-cucumber/src/capability.rs:449-461
  The restore call (std::fs::set_permissions(&probe_dir, ... 0o755)?) uses ?, so if it errors, the closure returns Err before permissions are restored, and probe().unwrap_or(false) swallows the error — the probe directory is left at 0o555 with nothing attempting to fix it afterward. The sibling helper added in the same PR for the same purpose, restore_audit_dir_permissions in tests/e2e-cucumber/tests/e2e/automations_steps.rs, deliberately makes its own identical restore call best-effort (let _ = std::fs::set_permissions(...)) so the attempt is never skipped — the inconsistency between the two sites suggests this is an oversight rather than a deliberate choice. Blast radius is narrow (this is the capability probe's own disposable, one-off TempDir, not a scenario's isolated root, and triggering it needs the first, structurally identical set_permissions call to succeed while the second fails moments later), so this is a robustness gap rather than a live bug.
  Confidence 70 · logic · Fix: make the restore unconditional and best-effort, e.g. let _ = std::fs::set_permissions(&probe_dir, std::fs::Permissions::from_mode(0o755)); before returning, mirroring restore_audit_dir_permissions.

Decisions for the author

• The fallback repair prints nothing, even though its trigger (full disk / Windows sharing violation) is operator-relevant and the analogous rotation-rename failure path does warn — tradeoff
  rotate_jsonl_generations's rename failure gets a once-per-store eprintln! via rotation_failure_warning, but repair_torn_tail_and_append's replace failure — the same class of failure, and one that additionally reopens a documented reader-splice risk — produces no signal at all. This is deliberate and stated in the PR text ("The fallback prints nothing, like a successful repair."). The counter-argument is that a repair which fully recovers the data shouldn't alarm the operator differently depending on which repair strategy happened to succeed. Both sides are reasonable; worth confirming this was weighed against rotation_failure_warning's precedent rather than inherited by default from the normal replace path's silence.

Positive signals

• repair_torn_tail_and_append holds the store's lock continuously across the whole repair-and-append sequence — traced through append_jsonl_record's _lock binding, which stays in scope across the entire match — so there is no writer/writer TOCTOU window reopened by this change.
• The reader-splice doc comment is now precise: tracing ReverseLines::next_line's short-read guard (bytes.len() as u64 != self.pos - start && !self.pending.is_empty()) confirms self.pos only decreases across a scan, so an undetected splice needs the file to regrow only past the scan's current offset, matching the comment's corrected wording exactly (this round's commit explicitly fixed an over-broad "its old length" phrasing from an earlier draft).
• Error-chain causality is deliberately engineered and tested: dedicated tests pin the exact message text for both-steps-fail and cut-succeeds-but-append-fails, including the replace failure surviving as root cause in the latter.
• Two of the five new automation.rs tests drive the real production fallback (a genuinely read-only directory on Unix, a genuinely open handle without delete-sharing on Windows) rather than only injected stub failures, using file-identity checks (inode / volume-serial+file-index) that can't be faked by a subtly wrong implementation.
• resolve() in expectation.rs checks requires_os strictly before requires_dir_permissions (confirmed by reading the function body in order), and this round added a dedicated test, os_reason_wins_over_dir_permissions_reason_on_a_non_matching_os, constructing a host that fails both premises and asserting the OS reason wins — mirroring the equivalent test for the sibling @requires-case-sensitive-fs tag.
• Tag-based skip genuinely prevents the step panic in audit_dir_takes_no_new_files from ever running under non-matching conditions: tests/e2e.rs's filter_run resolves Expectation per scenario and excludes Skip scenarios before any step executes, so the panic is an unreachable backstop under correct tagging, not a race.

Deployment notes

None.

What this covered

Read completely at the reviewed head: crates/rocm-core/src/automation.rs (3653 lines) and crates/rocm-core/src/lib.rs in full, plus all six other changed files (docs/testing.md, tests/e2e-cucumber/README.md, tests/e2e-cucumber/features/automations.feature, tests/e2e-cucumber/src/capability.rs, tests/e2e-cucumber/src/expectation.rs, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/automations_steps.rs). Reviewed as two coupled file-groups (core fix; e2e test infra) by independent fan-out workers, each reading its group's files completely, plus a direct driver-level trace of repair_torn_tail_and_append, ReverseLines::next_line's short-read guard, and resolve()'s check ordering; findings were re-verified directly against the source before scoring. Also checked: git history for automation.rs's torn-tail region (PR #738's review history, which introduced the rename-based repair and explicitly deferred this fallback to issue #748), a grep for all other callers of jsonl_lock_path (none affected by it becoming pub), and the AGENTS.md root agent-instruction file in full (no violations found — in particular its requirements for a regression test proving the fix, a Gherkin scenario for user-observable behavior, and a platform-gated test on its own gate are all met).

No Rust toolchain (cargo/rustc) was available in this environment, so no test was compiled or run here; all findings rest on manual tracing of the source, and no finding is scored above 85. In its place: CI at the reviewed commit (9d363722) was read live and all 24 checks in the status rollup are green, including Test (affected crates), build-and-test, windows-build-and-test (covers the Windows-specific share-violation test), and E2E tests/E2E consolidated report. Hardware-specific/nightly E2E lanes are not part of this rollup and their status could not be confirmed from here.

Not reconciled against any discussion on this PR — only the description and issue #748 were read for Step 9, never review threads or comments.

---

🤖 Generated with Claude Code