[jussielo-amd]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No rows reference ROCMAI-721 or ROCMAI-665.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (No new subcommand or subsystem; the change stays inside crates/rocm-core/src/automation.rs.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state, per AGENTS.md §3. (No user-facing message is added or changed.)

Why

Fixes ROCMAI-721; partially addresses ROCMAI-665, which shares its root cause. 665 also asks for store rotation and tail-only proposal reads, both deferred to ROCMAI-729, so this PR does not close it.

The three rocm-core JSONL stores (automation events, audit events, review proposals) were updated by reading the whole file, changing it in memory, and renaming a rewritten copy over the original via write_atomic, with no lock. The rocm CLI and rocmd write these stores concurrently: both append to the audit store, and rocm sets proposal status while rocmd's watchers append new proposals. Two writers whose read-modify-write cycles overlapped kept only the last rename, and the other record was dropped with no error, log line or test signal. Two threads of one process could additionally collide on the same temp-file name, because it is qualified only by pid and millisecond.

The same design made every append cost O(store size): one full read plus one full rewrite per record. With the gpu-metrics watcher on, rocmd records about one event a minute into both the events and audit stores, so each grows without bound and every append gets slower. Reads had the same problem, since load_recent_jsonl_records parsed the whole file to keep the last limit records.

Reproduced before the fix with the new regression tests: six writer processes appending 20 records each to the audit store left 86 of the 120 records. Status updates racing 100 proposal appends left 29 of the 101 proposals. Eight threads appending concurrently failed outright on the shared temp-file name.

What changes

One lock per store. Every mutation of a store now holds a sibling <store>.lock file through the existing rocm_core::FileLock, which wraps std::fs::File::lock (flock on Linux, LockFileEx on Windows) and excludes threads and processes alike.

• Appends are, in the common case, a single append-mode write of one line under that lock. They no longer read or rewrite the store, so their cost does not grow with it in that case. Appends need the lock even though append mode alone would not interleave them: a torn-tail repair (next) and the proposal-status rewrite both replace the file by rename, so an append that had already opened the old file would land in the replaced copy and vanish. A torn tail — crash debris, or left by a write that failed partway through a record — is never truncated in place, because a lock-free reader's reverse scan could otherwise observe the file shrink and then regrow as the next append lands. Instead, the next append writes the store's clean prefix plus its own new record as a single rename over the store (costing O(store size) for that one append, same as the proposal-status rewrite), so a reader only ever sees the pre-repair length or the post-append length, never one in between. An unterminated last line that is valid JSON — from a hand edit, or from a write that failed one byte short of its own trailing newline — is kept and terminated rather than cut; a write that fails short of landing a complete record leaves debris for the next append to fold in, and a write that fails but still lands a complete record is reported as a success rather than an error, so a caller that retries on error cannot write a duplicate.
• A proposal status update keeps its whole-file rewrite (it changes a record in place, so it cannot be an append), but its entire read-modify-write now runs under the same lock.
• Readers stay lock-free. A reader may catch an append mid-write, so an unterminated last line that is not valid JSON is skipped instead of failing rocm status or the dashboard. Complete JSON of the wrong shape is still reported as an error, exactly as before. The proposals reader now parses bytes instead of a String, so a record cut inside a multi-byte UTF-8 character cannot fail the whole file.
• load_recent_jsonl_records (events and audit) scans backwards from the end in 64 KiB chunks and stops after limit records. Its cost now tracks limit rather than the store size. A scan that fails while the file's length moved under it is retried from the new end; nothing this module's own writers do can shrink a store in place, so that case is for a store edited or truncated out-of-band, not a concurrent writer. One behaviour shifts as a consequence of the backward scan: a corrupt line older than the requested window is no longer reported, while one inside the window still is.

write_atomic's doc comment now states that it serializes nothing, and the prune_managed_service_records comment that lists every FileLock::acquire call in the tree now includes these locks.

Two doc comments outside this file (process.rs, and a comment in examine.feature) that still described every append as rewriting the whole store are corrected to match the append-mode write path. The Windows file-identity test helper used to prove the torn-tail repair replaces rather than truncates the store is shared from test_support.rs rather than duplicated.

Mixed versions: a rocm or rocmd binary from before this change does not take the lock. While an old and a new binary run side by side, for example an old rocmd still running after an upgrade, the old one's rewrites can still drop a record. The fix is complete once both sides are on this version.

Verification

• Regression tests, written first and failing before the fix (in automation.rs): N threads appending to all three stores; N separate processes, made by re-executing the test binary, appending to all three stores; and proposal status updates racing proposal appends. Each asserts the exact set of records afterwards. All three failed on main as described above and pass with the fix. The two concurrency tests and the status race also passed 15 consecutive local runs.
• Mutation check: removing the lock from either the append path or the status-update path makes proposal_status_updates_racing_appends_lose_neither fail. Both locks are therefore covered by a test.
• File-identity tests (inode on Unix, volume serial plus file index on Windows) prove the torn-tail repair actually replaces the store rather than truncating it in place, and a sibling pair proves the common (no-torn-tail) case stays a plain append on the same file. A test reproducing the original truncate-then-append shape fails both of these.
• A directly-tested pure function (landed_despite_the_error) decides whether a failed write still landed a complete record, so the duplicate-avoidance behaviour above does not depend on inducing a real write failure to exercise its decision logic.
• Deterministic tests for the edge paths: torn tail folded into the next append, valid unterminated tail kept (including a whitespace-only one), reader skip of a torn tail (including one cut inside a UTF-8 character), a wrong-shape unterminated line still reported and never dropped by a rewrite, ReverseLines against a forward split at chunk sizes 1 to 8 (and against its own reported line offsets), the scan window, and a scan that finds the file shorter than it measured.
• A torn-tail repair racing concurrent appenders (the exact scenario the per-store lock's own doc comment gives as its reason to exist, not just plain appends racing each other) is seeded and stressed the same way as the other concurrency tests, and passes.
• load_recent_jsonl_records's retry-after-a-failed-scan path, and append_or_tolerate_a_missing_newline's glue around a write that actually fails rather than the pure decision function alone, are each driven directly: a test seam forces an out-of-band shrink strictly between two of ReverseLines's own chunked reads (not just before the scan starts, which is already tolerated without a retry), and two Unix tests force a genuine short write via RLIMIT_FSIZE with SIGXFSZ ignored — one where the write lands everything but its trailing newline (the append must still succeed, with no duplicate and no silently-complete write), and one where the limit leaves no room for even the first byte (the append must report a real failure, not fold it into a false success).
• cargo test --workspace --all-targets --no-fail-fast: 4419 passed, 0 failed. cargo build --workspace --all-targets and cargo test -p rocm-core --lib both stay clean with the torn-tail repair and the duplicate-avoidance check in place.
• cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --all --check: clean.
• python scripts/smoke_local.py: ok.
• cargo xtask e2e -- -n "service-cleanup-|automations-": 13 scenarios passed. This includes @id:service-cleanup-names-a-file-it-could-not-remove, which asserts that a prune is recorded in the audit store, so it runs the new append path end to end, and the new @id:automations-tolerates-torn-audit-tail (automations-06), which seeds a torn audit-log tail and asserts rocm automations still lists the record before it.

No scenario for the race itself. What it fixes is a race between writers that sits inside a window of microseconds. A scenario that races whole CLI processes would rarely hit that window, because process start-up dominates, so it could not fail reliably before the fix. The cross-process unit test reproduces the race deterministically instead, by calling the same rocm_core functions the CLI and rocmd call from separate processes. The new *.jsonl.lock files are zero-byte rendezvous files beside the stores in the data directory, the same pattern as the existing launch.lock and autostart.lock.

A separate, deterministic, user-observable behaviour does have one, covered by @id:automations-tolerates-torn-audit-tail (automations-06): render_automations_text reads the audit store, directly surfacing the raw text via the rocm automations CLI subcommand and the TUI's automations-manager screen (which shells out to rocm automations list as a subprocess) — it's also reachable in-process through rocm chat's and the TUI's slash-command rocm_command tool and through rocmd's own MCP rocm_command tool (a further subprocess), though neither of those shows the raw text the same way. render_automations_text already never exits non-zero on a read error — it swallows one via .unwrap_or_default() — so what this scenario pins is that the clean record ahead of a torn tail still shows up, rather than that error wiping out the whole audit section the way it would without this PR's read-path skip. The other half of the same behaviour change — a corrupt line older than the read window no longer being reported — stays a named unit test (load_recent_stops_before_corrupt_lines_older_than_the_window) rather than a second scenario, since rocm automations uses a fixed internal limit rather than a user-supplied one, so there is no CLI-level lever to place a corrupt line on one side of the window or the other.

Windows is exercised by windows-build-and-test (a local cross-compile was not possible without a MinGW toolchain). On windows-latest, every new test passes, including the cross-process writers, the status-update race, and the torn-tail repair, which replaces the store via rename through the same handle semantics as the proposal-status rewrite. The Windows-specific choices rest on these documented semantics:

• FileLock is LockFileEx and already guards launch.lock and autostart.lock on Windows.
• Rust's append mode on Windows opens with FILE_APPEND_DATA only, and std documents that writes land at the current end of file.
• std opens files with FILE_SHARE_DELETE by default, so a lock-free reader does not block a rename — the proposal-status rewrite's, or a torn-tail repair's.

Scope

Deferred to ROCMAI-729:

• Size cap or rotation for the three stores. The stores still grow without bound, although appends no longer slow down as they grow in the common case. Rotation has to run under the same lock, and proposals cannot be rotated naively, because a pending proposal must stay findable for approve and reject. That needs a retention policy, not just code.
• Tail-only reads of the proposals store. Proposals written without an id get one derived from their absolute line index (legacy-<at>-<index>), which a backward scan cannot compute. load_automation_proposals, load_recent_automation_proposals and find_automation_proposal therefore still read the whole file.

Known limitation, tracked in #748: a torn-tail repair that cannot complete its rename — sustained disk pressure, since the repair now needs free space roughly equal to the whole store rather than one line, or a Windows handle without delete-sharing — has no fallback, and every subsequent append to that store keeps retrying the same whole-store rewrite rather than degrading the way the old in-place truncate did.

One guarantee this reverses: the previous whole-file-rewrite-per-append design meant a crash or full disk could never leave a store with a truncated or malformed trailing line — the rename that published a change was all-or-nothing. Append-mode writes trade that away for O(1) appends and no dropped records: a crash or a failed write mid-append can now leave a torn tail, handled by folding it into the next append (see the torn-tail paragraph above) rather than by the store never being able to have one. Issue #666 is this earlier guarantee; this PR knowingly moves away from it.

Out of scope: fsync durability, unchanged from before.

[Copilot]

🟡 Changes recommended

A confirmed shrink-and-regrow race can make lock-free readers return fabricated records or transient errors.

1 open issue

• Coordinate tail repair with readers to prevent invalid JSON · New

What changed in this PR

This PR addresses concurrent-writer record loss in the core JSONL stores shared by rocm and rocmd, while reducing append and recent-read costs.

Changes:

• Serializes store mutations with per-store locks and replaces whole-file appends with append-mode writes.
• Adds torn-tail recovery, backward scanning for recent records, and concurrency regression tests.
• Clarifies locking guarantees and documents the new lock usage.

Verification here used a standalone race model; project tests were not run in this review environment.

File	Description
crates/​rocm-core/​src/​lib.rs	Clarifies that write_atomic does not serialize writers.
crates/​rocm-core/​src/​automation.rs	Implements store locking, append recovery, reverse reads, and regression tests.
apps/​rocm/​src/​main.rs	Updates the prune-lock documentation to account for store locks.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · 87fcf8c

Review — no blocking findings

Full review of the whole change.
Implements the concurrency fix described in ROCMAI-721/ROCMAI-665 (both already named in the PR title/commits); the deferred follow-up work (store rotation, tail-only proposal reads, and the torn-tail-repair-has-no-fallback edge case) is explicitly scoped out to a tracked follow-up, consistent with the code.

Blocking

None

Non-blocking

• FileLock's doc comment doesn't mention its new primary use site — crates/rocm-core/src/process.rs (~line 385-392, unchanged by this PR)
  The doc comment enumerates FileLock's use sites as "the daemon autostart decision and the managed-serve GPU select-then-claim," both check-then-act races. This PR adds a third, now very prominent use (serializing JSONL-store append/rewrite in automation.rs via jsonl_lock_path) that is a different shape (plain mutual exclusion over a write, not a check-then-act race) and isn't reflected in the enumeration. Confirmed the doc text is unchanged in this diff and the new use site is real and heavily used.
  Confidence 70 · mechanical · Fix: extend the doc's use-site list in process.rs to include the per-store JSONL locks.

Decisions for the author

• inspect_tail's backward-chunk scan duplicates ReverseLines's scanning logic — non-blocking-improvement
  Both independently implement "find the last \n scanning backwards in fixed-size chunks" (crates/rocm-core/src/automation.rs). They agree today and each is tested (ReverseLines exhaustively via reverse_lines_matches_a_forward_split_at_every_chunk_size; inspect_tail's multi-chunk path only via the torn-tail case), but a future change to one could silently diverge from the other. Consider having inspect_tail reuse ReverseLines for its one-line lookup.
• reads_racing_appends_never_fail would also pass against the pre-fix code — non-blocking-improvement
  The test (automation.rs) asserts concurrent reads never error while appends race, but the pre-PR code's every-append-is-an-atomic-rename behavior also never let a reader observe a torn write, so this test doesn't by itself distinguish the fix from a revert. The in-code comment already discloses this ("this is a smoke test... pinned deterministically by readers_skip_an_unterminated_last_line_that_does_not_parse"), so it reads as an honest, self-aware smoke test rather than a hidden gap — noted only as something the author may want to strengthen (e.g. shrinking the scan chunk size under test to make a torn read more probable), not as a defect.

Positive signals

• The file-identity tests (append_of_a_torn_tail_replaces_rather_than_truncates_the_file_in_place, Unix inode / Windows volume-serial+file-index) correctly recognize that a byte-content assertion alone cannot distinguish an atomic-rename repair from an in-place truncate-then-rewrite, and add the one check that can.
• append_or_tolerate_a_missing_newline's "did it actually land" check for a write that fails exactly at its own trailing newline is a genuinely subtle case (it avoids a caller-side retry creating a duplicate record), and it is backed by a dedicated, directly-tested pure function (landed_despite_the_error) rather than only being exercised indirectly.
• Every doc comment this change adds or edits (module header, jsonl_lock_path, append_jsonl_record, read_range, write_atomic, the main.rs lock-reachability note) was checked against the implementation and is accurate — including the specific, checkable claim that no code path appends to a store, or calls into another lock, while holding a different FileLock.

Deployment notes

• Mixed-version rollout: a rocm/rocmd binary built before this change does not take the new per-store lock, so while an old and a new binary write the same store concurrently (e.g. an old rocmd still running through an upgrade), the old side's writes can still silently drop a record. This is already called out in the PR description; both sides need to be on this version for the fix to hold.
• Each store gains a sibling zero-byte *.jsonl.lock file in the data directory (same pattern as the existing launch.lock/autostart.lock); no migration is needed since it's created on first use.

What this covered

Full review of both PR commits (base 7eb4bfacd9 to head 87fcf8c50c), all 7 changed files read completely at the PR head: crates/rocm-core/src/automation.rs (the 1672-line core of the change, plus the pre-change version for comparison), apps/rocm/src/main.rs, crates/rocm-core/src/lib.rs, crates/rocm-core/src/process.rs (including FileLock's implementation), crates/rocm-core/src/config.rs, crates/rocm-core/src/test_support.rs, and tests/e2e-cucumber/features/examine.feature. Also read every call site of the three store-accessor functions across apps/rocm and apps/rocmd to confirm no caller bypasses the new lock, and every other FileLock::acquire call site in the tree to confirm no nested-acquire/deadlock path exists between the new per-store locks and the pre-existing launch/autostart/install locks.

Four independent fan-out passes ran (core automation.rs logic and its cross-file doc claims; the mechanical windows_file_identity relocation; this repo's own AGENTS.md adherence; file history/blame plus code-comment staleness across the touched files and a repo-wide search for stale references to the old behavior), consolidated against direct tracing of the lock/call-graph. CI was checked twice, since one job (windows-build-and-test) was still in progress at the first check; all checks are green at the reviewed head. The PR description and its two named tickets were read after the findings above were reached, and are consistent with the code and with each other.

Did not run: no test or mutation check was executed directly in this review; it relies on CI's green cargo test --workspace --all-targets and the PR's own stated mutation check (removing either lock makes proposal_status_updates_racing_appends_lose_neither fail). The history pass found only one prior commit touching automation.rs (a pure extraction), so there was no earlier concurrency-related review history to compare against. Not reconciled against this PR's own GitHub review threads or comments.

[tomastola]

Thanks for the thorough fix — the cross-process regression test that re-executes the test binary, and the inode / file-identity tests that tell "replaced by rename" apart from "rewritten in place", are both excellent. Two pieces of new behaviour are not exercised by any test, though, so requesting changes on those.

Blocking

1. The retry loop in load_recent_jsonl_records is never exercised — crates/rocm-core/src/automation.rs:491, :502-509

Setting ATTEMPTS to 1 leaves every automation:: test green (cargo test -p rocm-core automation::, 27/27). a_scan_that_finds_the_file_shorter_than_it_was_still_succeeds calls scan_recent_jsonl_records directly with a stale length, so it never reaches the retry. The description lists this behaviour ("A scan that fails while the file's length moved under it is retried from the new end"), but nothing tests it.

Suggested test: add a seam between the length read and the scan (or inject a shrink between the two file_len() calls) and assert that the load still succeeds after a retry.

2. The "tolerate a missing newline" wiring is untested; only its pure decision function is — automation.rs:310-342

Replacing the body of append_or_tolerate_a_missing_newline with a plain file.write_all(line).context(..) leaves every test green. landed_despite_the_error is tested. The code around it is not: the read-back from start, the .max(start), and the error folding. That untested code is what decides whether a caller that retries on error writes a duplicate.

A real partial write is practical to produce. The cross-process test already re-executes the test binary; a Unix child run the same way, with RLIMIT_FSIZE set one byte short of the record and SIGXFSZ ignored, gets a short write. Assert that the append returns Ok and the store holds exactly one copy of the record.

Non-blocking

• No test races an append against a torn-tail repair (automation.rs:229-234, :266). Removing the lock from append_jsonl_record leaves both concurrent_appends_from_many_{threads,processes} green, because append mode alone keeps small appends from interleaving. Only proposal_status_updates_racing_appends_lose_neither catches it, and only for the proposals store. Nothing seeds a torn tail and then runs concurrent appenders, which is the scenario the lock's doc comment gives as its reason. Seeding a torn tail in the events or audit store before the threaded appenders would cover it.
• The blank unterminated-tail branch is untested (automation.rs:407). Dropping is_blank(&tail) || leaves all tests green. A whitespace-only tail would then be cut by a whole-store rewrite instead of being kept.
• Redundant code. records.truncate(limit) at automation.rs:543 can never change anything: the tail push adds at most one record, the loop stops at limit, and every limit == 0 caller returns early. unterminated && in parse_jsonl_records (:451, :460) is also redundant: when the bytes end in \n, the last split segment is empty and is skipped as blank first. Removing either leaves all tests green. Remove them, or comment why they stay as guards.
• #744 is stacked on this PR's first commit only. Its automation.rs still has repair_unterminated_tail / truncate_file, the in-place truncation this PR's second commit removed. #744 also makes the new "never rotated or capped" comments (process.rs:640, process.rs:1868-1869, examine.feature:370, automation.rs:25) false once it lands. It needs a rebase onto this head before either merges. Per AGENTS.md, a stacked PR should also stay in draft.
• The base is behind main, and #735 there marks write_atomic as not permanent. This PR adds a new write_atomic call site (the torn-tail repair, automation.rs:287) and a doc paragraph on it (lib.rs:499-502). After rebasing, make sure whatever replaces write_atomic covers this call site too.
• The first commit's message describes a design the second commit removed (c081bcd: "A failed write is rolled back to the previous length, and a torn last line … cut by the next append"). Squash the two commits or reword that message.

Decisions for the author

• No Gherkin scenario, although the change creates new paths in the data directory. AGENTS.md §3 asks for one when a change alters the files or paths the CLI creates, and every audit write now creates events.jsonl.lock. The description's reasoning is sound: the race window is too small for a CLI-level scenario, and the lock files follow the existing launch.lock / autostart.lock pattern. Please confirm that maintainers accept that reasoning.
• The torn-tail repair renames over a store the appender still holds open (automation.rs:267-287). #748 already tracks the failure on volumes without POSIX rename semantics, where the repair fails every time, every later append to that store fails, and rocmd treats an append error as fatal. One cheap mitigation fits in this PR: drop(file) before write_atomic in the Torn arm, which is safe because the store lock is still held. Also, the description says the repair uses "the same handle semantics as the proposal-status rewrite", but the status rewrite has no handle open when it renames.
• Append-mode writes give up #666's "a crash or full disk never leaves a truncated store" guarantee. That trade is reasonable: you get O(1) appends and no lost records, at the cost of tail-repair logic on a rarely run path. The module doc states it, but the description doesn't mention that it reverses #666. "fsync durability, unchanged" reads as though nothing about crash safety moved.
• The store lock blocks with no timeout. Only local file I/O runs under it, so the risk is low. Still, before this change nothing could block an append; now a stopped holder or a hung network mount stalls CLI commands and rocmd's watcher tick. rocmd takes the lock synchronously inside a tokio::select! arm, so it cannot respond to shutdown while it waits.

Deployment notes

• An old rocmd still running after an upgrade takes no lock and can still drop records, as the description says. Restart rocmd together with the CLI upgrade.
• New zero-byte events.jsonl.lock and proposals.jsonl.lock files appear next to the stores under the audit/ and automations/ data directories.

[juhovainio]

I read through this and found no blocking correctness defects: the lock coverage on appends and the proposal status rewrite, the reader tolerance for a torn tail, the rename-based repair and the reverse scan all hold up, and CI is green. I'm requesting changes because a few things should be settled before merge:

1. Scenario coverage for the reader change. The PR's justification for having no Gherkin scenario covers the race, which is non-deterministic at the CLI level. It does not cover the other user-observable change: rocm status and the dashboard now skip a torn tail, and corrupt lines older than the limit window are no longer reported. That behaviour is deterministic (seed a store with a torn tail, run the command). Please add a small scenario, or name the unit tests that prove it in the PR text.
2. Write-failed-but-landed path. Only the pure landed_despite_the_error is tested; the glue around it is not. See the inline comment.
3. Ticket wording. The PR says it fixes ROCMAI-665, but that ticket also asks for rotation and tail-only proposal reads, which are deferred to ROCMAI-729. "Fixes 721; partially addresses 665" would avoid closing it early.
4. Docs. I only had the diff, so please grep README, docs/ and docs/testing.md for stale wording such as "append-only" or "rewrites the whole file".
5. Commit history. The first commit message still describes rollback and truncate, which the second commit replaced. Fine if squashed; misleading otherwise.

Also worth considering: FileLock::acquire has no timeout, so a stalled holder now blocks callers. Please confirm no append runs on an async worker without spawn_blocking.

Smaller notes are inline.

[juhovainio]

Torn-tail repair reads the whole clean prefix into memory and rewrites it. If the rename fails, every later append to this store retries the same rewrite and errors (known, #748). Since rotation is deferred, a bounded fallback may be worth having before stores grow large.

[jussielo-amd]

Added one concrete, low-risk mitigation in scope for this PR: drop(file) before write_atomic in the Torn arm, making the hand-off from the pre-repair handle to the replacement file explicit rather than implicit (it already worked without this, since std's default Windows share mode includes delete-sharing, but this states the intent directly rather than leaning on that default). A full bounded fallback for a rename that can't complete stays out of scope for this PR, tracked in #748 along with the rest of that limitation.

[tomastola]

Re-review of 87fcf8c..c4a045e. The two new commits reuse ReverseLines in inspect_tail and update the FileLock docs. Neither addresses the two blocking findings from the previous review, and both still hold at c4a045e.

Blocking (still open)

1. The retry loop in load_recent_jsonl_records is never exercised — crates/rocm-core/src/automation.rs:485, :496-503

Still open. At c4a045e, setting ATTEMPTS to 1 leaves all 28 automation:: tests passing (cargo test -p rocm-core --lib automation::). The new reverse_lines_tolerates_a_shrink_before_the_first_read tests ReverseLines directly, so like a_scan_that_finds_the_file_shorter_than_it_was_still_succeeds it never goes through the retry. Suggested test, unchanged: add a seam between the length read and the scan (or force a shrink between the two file_len() calls), and assert that the load succeeds after a retry.

2. The "tolerate a missing newline" wiring is untested; only its pure decision function is — append_or_tolerate_a_missing_newline, automation.rs:310-342

Still open. Replacing its body with a plain file.write_all(line).with_context(..) leaves all 28 tests passing. The untested part is the read-back from start after a failed write, which decides whether a caller that retries on error writes a duplicate. Suggested test, unchanged: on Unix, re-execute the test binary as a child (as the cross-process test already does) with RLIMIT_FSIZE set one byte short of the record and SIGXFSZ ignored. Then assert that the append returns Ok and the store holds exactly one copy.

New commits

No new problems. The (start, bytes) offsets from ReverseLines::next_line are covered: an off-by-one in the start computation fails five tests, including the strengthened reverse_lines_matches_a_forward_split_at_every_chunk_size and the new shrink test. Reusing ReverseLines in inspect_tail keeps the torn-tail repair offset unchanged.

Non-blocking (unchanged from the previous review)

• No test races an append against a torn-tail repair, which is the reason the append lock gives for existing (automation.rs:229-234).
• The blank unterminated-tail branch (is_blank(&tail) || in inspect_tail) is untested.
• records.truncate(limit) in scan_recent_jsonl_records and unterminated && in parse_jsonl_records are redundant.
• #744 is still stacked on this PR's first commit, and it brings back the in-place truncate that this PR removed.
• The base is behind main (#735), and this PR adds a new write_atomic call site.
• The first commit's message still describes the rollback design that the second commit replaced.

The author decisions and deployment notes from the previous review are unchanged.

[nowycondro]

🔴 Automated review · pr-review-watcher · c4a045e

Review

Read the full diff from merge-base 7eb4bfacd9 to head c4a045e2 (all 7 changed files), with particular attention to crates/rocm-core/src/automation.rs (read completely at head, 1718 lines) and to the two commits added since the previous automated pass (efafdba7, c4a045e2). The locking design — a per-store .lock file serializing append/rewrite, append-mode writes replacing whole-file rewrites, and rename-based torn-tail repair instead of in-place truncation — holds up under direct reading, and the concurrency regression tests (multi-thread and multi-process) exercise the actual failure mode the PR describes. Two test-coverage gaps already raised on this PR remain open at this exact head; I independently re-verified both by reading the current code rather than taking the open status on faith.

Blocking

• The retry loop in load_recent_jsonl_records has no test that drives it — crates/rocm-core/src/automation.rs:480-504
  The function reads the file length, scans, and on failure retries up to ATTEMPTS = 3 times if the length changed in between (lines 496-503). Every test that exercises a stale/shrunk length (a_scan_that_finds_the_file_shorter_than_it_was_still_succeeds, reverse_lines_tolerates_a_shrink_before_the_first_read) calls scan_recent_jsonl_records or ReverseLines directly, bypassing this wrapper entirely, so the retry-and-succeed path this function exists for is never actually run by the suite. Confirmed by reading the test module end to end — no test seeds a shrink between the loop's two file_len() calls.
  Confidence 85 · logic · Fix: add a seam (or a test-only hook) between the length read and the scan so a test can shrink the file mid-call and assert the retry still succeeds.
• append_or_tolerate_a_missing_newline's wiring around a failed write is untested; only its pure decision function is — crates/rocm-core/src/automation.rs:310-342 (decision function at 349-353, tested at 1672-1700)
  landed_despite_the_error has a direct unit test, but the surrounding glue — the read-back from start after write_all fails, the .max(start), and which of the two errors gets surfaced — has no test inducing an actual partial write. That glue is what decides whether a caller retrying on error ends up writing a duplicate record.
  Confidence 85 · logic · Fix: on Unix, re-execute the test binary as a child (the cross-process concurrency test already does this) with a tight RLIMIT_FSIZE and SIGXFSZ ignored to force a one-byte-short write, then assert the append still returns Ok and the store holds exactly one copy.

Non-blocking

None beyond what's already open on this PR's review threads (a couple of redundant guards, and no test that races an append against an in-flight torn-tail repair) — re-read at this head and still accurate, not repeating the details here.

Decisions for the author

• Append-mode writes trade away the earlier "a crash or full disk never leaves a truncated store" guarantee for O(1) appends and no dropped records, moving the cost to a rarely-run tail-repair path — tradeoff. The module doc states this plainly; it's a reasonable trade given the unbounded-growth problem being fixed, but worth the author explicitly confirming nothing downstream still assumes the old guarantee.
• Store rotation, tail-only proposal reads, and the no-fallback case for a rename that fails on a filesystem without POSIX rename semantics are explicitly deferred to follow-up work rather than this PR — tradeoff. Scope looks reasonable for a focused concurrency fix; flagging only so a reader of this review knows it was a deliberate cut, not an oversight.

Positive signals

• The new inspect_tail → ReverseLines reuse (the latest commit) removes the duplicate backward-scan implementation that existed at the previous head, and the refactor is verified by a strengthened version of reverse_lines_matches_a_forward_split_at_every_chunk_size (now checking exact line offsets) plus a new reverse_lines_tolerates_a_shrink_before_the_first_read test — the offset arithmetic checks out against the prior loop-based implementation it replaced.
• The inode/file-identity tests (append_of_a_torn_tail_replaces_rather_than_truncates_the_file_in_place, both Unix and Windows variants) correctly recognize that a byte-content assertion alone can't distinguish a rename-based repair from an in-place truncate-then-rewrite, and add the one check that can.
• The FileLock doc comment in process.rs and the lock-reachability note in apps/rocm/src/main.rs were both updated in this round to name the new JSONL-store lock use site, and both read as accurate against the actual call sites.

Deployment notes

• CI as of this review: all checks green except E2E tests and windows-build-and-test, both still pending/in progress at the time of this review — no failing checks observed.
• An old rocm/rocmd binary built before this change takes no per-store lock, so a mixed-version rollout (e.g. an old rocmd still running through an upgrade) can still silently drop a record from an old-side write; the PR description already calls this out.
• Each JSONL store gains a sibling zero-byte *.jsonl.lock file in its data directory, following the existing lock-file pattern elsewhere in the codebase; created on first use, no migration needed.

What this covered

Full diff 7eb4bfacd9...c4a045e2, all 7 changed files, with crates/rocm-core/src/automation.rs read completely at head and the two newest commits diffed in isolation against the previous head to confirm they introduce no regressions. Also checked docs/*.md, docs/testing.md, and README.md for stale wording about the old whole-file-rewrite behavior this PR replaces — found none.

Did not run: no Rust toolchain (cargo/rustc) is available in this environment, so no tests, clippy, or build were run directly here; CI's own green cargo test/clippy runs are taken at face value per the checks above. Findings rest on direct reading of the code and tests, not execution.

Not reconciled against this PR's existing review threads as a goal in itself — the two blocking items above happen to match open points raised earlier on this PR, but were independently re-derived by reading the current code at this exact head rather than copied from those threads.

[tomastola]

Re-review of c4a045e..24d266b. This push only rebases onto main (bfa3c04). git range-diff shows all four commits unchanged, so neither blocking finding from the previous review has been addressed. I re-ran both checks at 24d266b to confirm.

Blocking (still open)

1. No test exercises the retry loop in load_recent_jsonl_records (crates/rocm-core/src/automation.rs:485, :496-503)

Setting ATTEMPTS to 1 still leaves all 28 automation:: tests passing (cargo test -p rocm-core --lib automation::). The suggested fix is unchanged: add a seam between the length read and the scan, so a test can shrink the file between the two file_len() calls. Then assert that the load succeeds after a retry.

2. The read-back in append_or_tolerate_a_missing_newline is untested (automation.rs:310-342)

Only the pure decision function landed_despite_the_error has a test. Replacing the body with a plain file.write_all(line).with_context(..) still passes all 28 tests; the only signal is an unused-variable warning for start. The suggested fix is unchanged: on Unix, re-run the test binary as a child process, as the cross-process test already does. Set RLIMIT_FSIZE one byte short of the record and ignore SIGXFSZ. Then assert that the append returns Ok and the store holds exactly one copy.

Resolved by this push

• The base is no longer behind main. #735 is now included, and the new write_atomic call site builds against it.
• #744 is now stacked on all four of this PR's commits, not just the first. It no longer brings back the in-place truncate: it repairs a torn tail by renaming, the same as this PR.

Non-blocking (unchanged)

• No test races an append against a torn-tail repair. That race is the reason the append lock gives for existing (automation.rs:229-234).
• The blank unterminated-tail branch (is_blank(&tail) || in inspect_tail) is untested.
• records.truncate(limit) in scan_recent_jsonl_records and unterminated && in parse_jsonl_records are redundant.
• The first commit's message still describes the rollback design that the second commit replaced.

[nowycondro]

🔴 Automated review · pr-review-watcher · 24d266b

Review — needs work

Full review of the whole change (all 7 files, read completely at the PR head against the merge-base with origin/main).
Fixes ROCMAI-721 (confirmed via the tracker: an open bug describing exactly this concurrent-writer data-loss). Explicitly does not close the related ROCMAI-665, deferring store rotation and tail-only proposal reads to ROCMAI-729 — a reasonable, honestly-scoped split.

Blocking

• The PR's own "Verification" section names three specific test/scenario artifacts that do not exist anywhere in this diff or the repository at head — and the two code gaps those claims paper over are the highest-risk untested behavior in the change. path/file:line for each:
  • crates/rocm-core/src/automation.rs (test module, lines ~1061-1719): the description states "A torn-tail repair racing concurrent appenders ... is seeded and stressed the same way as the other concurrency tests, and passes." No such test exists — every #[test] in the file was enumerated; the two multi-thread/multi-process tests (concurrent_appends_from_many_threads_lose_no_records :1061, concurrent_appends_from_many_processes_lose_no_records :1105) never produce a torn tail, so every append in them takes the Clean/ValidUnterminated path, which plain OS-level append-mode writes (O_APPEND/FILE_APPEND_DATA) already make safe without the new per-store lock. The actual hazard the lock exists to prevent — a reader's reverse scan observing the store mid rename-based repair — is argued convincingly in code comments and the module doc, but is exercised by no concurrent test.
  • crates/rocm-core/src/automation.rs:496-501 (the attempt < ATTEMPTS && file_len()? != len retry branch in load_recent_jsonl_records, confirmed new to this PR by diffing against the pre-PR file, which has no such loop): the description claims "two Unix tests force a genuine short write via RLIMIT_FSIZE with SIGXFSZ ignored." git grep -n "RLIMIT|SIGXFSZ" across the entire repository at this head returns nothing.
  • tests/e2e-cucumber/features/automations.feature: the description cites a "new @id:automations-tolerates-torn-audit-tail (automations-06)" scenario. This file has zero diff in this PR (git diff --name-status confirms it is not among the 7 changed files) and contains no such scenario ID at base or head.
  • Evidence: direct git grep/git diff against merge-base bfa3c043a3 and head 24d266b7, re-confirmed live immediately before publishing this review. Confidence 90 — the artifacts' absence is certain; that the two underlying behaviors are genuinely untested elsewhere follows from an exhaustive enumeration of every test function in the file.
  • Fix: either land the three described tests/scenario so the diff matches "Verification," or correct that section to stop claiming coverage that isn't there. Given the torn-tail-repair-vs-reader race is exactly what an earlier commit of this same PR was flagged for (independently confirmed via git log that commit 0342aa9, "repair a torn JSONL tail by rename, not truncate," fixed the underlying code before this head — so the production fix itself looks sound), closing the test gap is the more valuable of the two remedies: it is the one guard against a future regression of the exact bug this PR exists to fix.
  • Category: architectural (test-coverage + description-accuracy).

Non-blocking

• Minor tense mismatch in an updated comment — tests/e2e-cucumber/features/examine.feature:371 ("Unbounded, the logs grew by the payload's size every minute, forever") mixes past tense with "forever," where the near-identical sibling edit in crates/rocm-core/src/process.rs:645 uses present tense ("grows... forever"), which reads more clearly. Confidence 70 · mechanical · Fix: match the tense used in process.rs.

Decisions for the author

• write_automation_proposals's "caller must hold the lock" contract is enforced only by doc comment, not the type system — tradeoff. Today it has exactly one call site (inside update_automation_proposal_status, which does hold the lock), so there's no live bug, but a future internal caller could violate the invariant silently. Threading a lock-guard reference through the signature would make it structural at the cost of ceremony for a function with one caller today.
• This PR knowingly reverses the "a crash can never leave a torn tail" guarantee the old whole-file-rewrite design had (replaced with append-mode writes plus torn-tail repair) — tradeoff, already unusually well-disclosed in the description itself (including a cross-link to the original guarantee and a tracked known-limitation issue for the no-fallback-under-disk-pressure case). Flagging only so the author/reviewers explicitly confirm this is the accepted trade, not because the disclosure is lacking.
• No Gherkin scenario races the lock itself — non-blocking-improvement / already-justified. The description argues convincingly that a microsecond-scale race can't be forced reliably through black-box CLI process spawns, and substitutes a deterministic cross-process unit test instead. That reasoning holds; no scenario is needed for the race itself (distinct from the untested torn-tail-vs-reader interaction above, which is a gap in the unit-level coverage, not a missing e2e scenario).

Positive signals

• The core safety invariant — this module never shrinks a store in place, so a lock-free reader can never observe a mid-repair transient state — is correctly implemented via rename-based repair through write_atomic; traced concretely (stable fd pinned to its open-time inode, repairs swap inodes via atomic rename) with no interleaving found that produces an invalid read.
• is_torn_record is shared verbatim between the writer's inspect_tail and the readers' parsing paths, preventing classification drift between the two sides of the same contract.
• landed_despite_the_error precisely narrows "tolerate a failed write" to exactly "missing only the trailing newline," backed by a focused, readable dedicated unit test.
• ReverseLines is tested unusually rigorously: a forward-split equivalence check across 8 chunk sizes and 8 content shapes (including a line crossing a chunk boundary), plus dedicated tests for a shrink before vs. after the first chunk read.
• Every doc-comment update this PR makes (in lib.rs, process.rs, main.rs, examine.feature) was independently checked against the actual code and found accurate — careful, correctly cross-referenced documentation work.
• The windows_file_identity move from config.rs into shared test_support.rs is a clean, justified (genuinely used by ≥2 test modules), byte-for-byte-preserving refactor with no orphaned imports.
• No breaking public API changes across any function this PR touches; lock acquisition order is correct everywhere checked (acquired before any mutation, held via RAII, no nested-acquire hazard, no cross-store lock-path collision).

Deployment notes

• Mixed-version rollout risk, author-disclosed and worth repeating: a rocm/rocmd binary from before this change does not take the per-store lock. While an old and new binary run side by side during a rolling upgrade, the old one's writes can still silently drop a record; the fix is only complete once every writer is on this version.
• New *.jsonl.lock sibling files appear beside each store (same pattern as the existing launch.lock/autostart.lock) — no new operational concern expected, but worth knowing for anyone enumerating the data directory.

What this covered

Full read of all 7 changed files at PR head 24d266b73222f1f8bd2d3b42390d564bdc52f4fc against merge-base bfa3c043a344c9090024972d3a28db4d552b3b67, via three independent fan-out workers (core automation.rs logic; the supporting doc-comment/refactor files; agent-instruction/history/prior-changes/code-comment passes), plus my own direct verification of the two highest-stakes claims — the torn-tail-repair-vs-reader race, and whether the flagship concurrency tests actually depend on the new lock — and a full grep/diff audit of every specific, checkable claim in the PR's "Verification" section.

CI: all checks green per live statusCheckRollup (build-and-test, clippy, windows-build-and-test, E2E tests, coverage ratchet, commit-signatures, CodeQL, etc. all SUCCESS) — no failing-check finding.

Did not run: no local cargo build/test/clippy invocation in this environment (relied on direct code reading plus CI's own green results); Step 7 design questions were reasoned through directly rather than spawned to separate workers, given the volume of concrete evidence already in hand.

Not reconciled against any discussion on this PR. One incidental exposure to note for transparency: an early tool call made only to read PR review metadata (authors and commit SHAs, to detect whether this re-review would duplicate one already done at the current head) returned raw JSON whose preview incidentally included a fragment of one automated reviewer's comment text. That fragment was not used as input to any finding above — the corresponding code concern (the torn-tail-repair race) was independently re-derived here from reading automation.rs directly, and separately confirmed via git log that it was fixed at the code level by commit 0342aa9 before the current head, which is exactly the point of Blocking finding #1 above (the fix landed; the regression test for it did not). No other review thread, comment, or prior round's report on this PR was read.

[jussielo-amd]

Addressed the review feedback from @tomastola, @juhovainio, and the automated reviewer at head 1980a00.

Blocking findings (both confirmed open at the previous head, now fixed):

1. load_recent_jsonl_records's retry-after-a-failed-scan path was never exercised. Added a test seam (test_hooks, mirroring uv.rs's own test-hook pattern) that forces an out-of-band shrink strictly between two of ReverseLines's own chunked reads — not just before the scan starts, which was already tolerated without needing the retry. load_recent_retries_past_a_shrink_mid_scan_and_succeeds seeds a store that needs more than one physical chunk read, registers the hook to fire on the second read, and asserts the wrapper recovers via retry rather than propagating the error.

2. append_or_tolerate_a_missing_newline's glue around a real failed write was untested. Added two Unix tests that force a genuine short write via RLIMIT_FSIZE (SIGXFSZ ignored) rather than simulating one: append_tolerates_a_short_write_from_an_fsize_limit (the write lands everything but its trailing \n; must still report Ok with no duplicate) and append_reports_a_real_failure_when_nothing_landed (zero slack — the very first write(2) call lands nothing at all; must report a genuine failure, not fold it into a false success). Both were mutation-verified: a glue mutation collapsing the two outcomes into an unconditional Ok(()) makes the second test fail while the first still passes.

Non-blocking findings:

• Torn-tail repair racing concurrent appenders — the exact scenario the per-store lock's own doc comment gives as its reason to exist — had no test; concurrent_appends_after_a_torn_tail_lose_no_records seeds a torn tail in all three stores and stresses it the same way the existing thread-based concurrency test does.
• A whitespace-only unterminated tail had no test pinning that it's kept and terminated rather than cut as torn debris; append_terminates_a_blank_unterminated_last_line_without_cutting_it covers it.
• The two "redundant" guards (records.truncate(limit), the unterminated && check) are each provably dead in the current call graph, but are real safety nets against a future direct caller bypassing the guard that makes them dead today — kept, with a comment saying so rather than removed.
• drop(file) added before write_atomic in the Torn arm, making the hand-off from the pre-repair handle explicit.
• FileLock's doc comment now names the per-store JSONL lock.
• The first commit's message no longer describes the rollback-to-previous-length design the second commit replaced (reworded via git rebase, force-pushed — content unchanged, verified byte-identical against a backup before pushing).
• Grepped README/docs/docs-testing.md for stale "whole-file rewrite" wording per @juhovainio's ask — no hits; the one place that did have it (examine.feature) was already fixed earlier in this PR.

Scenario coverage: @juhovainio noted that naming unit tests doesn't discharge AGENTS.md §3's scenario requirement for user-observable behavior. Added @id:automations-tolerates-torn-audit-tail (automations-06), which seeds a torn audit-log tail and asserts rocm automations still lists the clean record before it — render_automations_text is the only production reader of load_recent_audit_events in the workspace, confirmed by grep. The corrupt-line-outside-the-window half stays a named unit test, since rocm automations has no user-supplied limit to exploit.

Ticket wording: PR description now says "Fixes ROCMAI-721; partially addresses ROCMAI-665" rather than implying 665 closes on merge.

On the FileLock-has-no-timeout / spawn_blocking question (@tomastola, @juhovainio): confirmed true — every rocmd call into the per-store lock runs synchronously on the tokio worker thread, including inside run_daemon's tokio::select! arms, with zero spawn_blocking calls anywhere in the workspace. Treating a proper fix (wrapping dozens of call sites, with its own risk around shared &mut state across a blocking-pool boundary) as out of scope for this focused concurrency-correctness PR — happy to open a tracked follow-up issue for it if that's the right call.

All CI checks are green at the current head (1980a00), including windows-build-and-test and the full E2E suite.

[nowycondro]

🔴 Automated review · pr-review-watcher · 35f3638

Review — needs work

Full review of the whole change (79f6e08c...35f36382, base main). Fixes ROCMAI-721 (concurrent-writer record loss in the JSONL stores); partially addresses ROCMAI-665 (O(n) append-cost redesign), explicitly deferring its store-rotation/tail-read-proposals scope to ROCMAI-729. All three ticket relationships were checked against Jira and match the change's actual content.

Blocking

• An append can report failure even though the record landed, letting a retrying caller write a duplicate — crates/rocm-core/src/automation.rs:345-350, contradicts the contract stated at :252-256.
  append_or_tolerate_a_missing_newline is built specifically so a write that fails but still lands a complete record is reported as success rather than error, "so a caller that retries on error cannot write a duplicate." But if write_all fails and the follow-up stat/read-back used to check what landed (:330-337) also fails (e.g. a second I/O error on a failing disk), the function falls into the Err(check_error) arm and returns Err regardless of whether the record actually landed. That breaks the function's own documented guarantee ("seeing Err here means this specific record did not land") for this one sub-case, and a caller retrying on error per that contract can duplicate a record it already wrote. Low likelihood (needs two I/O faults in a row), but it sits directly in the path this PR's design exists to make safe.
  Confidence 85 · logic · Fix: narrow the doc's claim to exclude this sub-case, or return a distinguishable "landed status unknown" error so callers can choose not to retry on it.

• No test distinguishes a torn last line from a corrupt line anywhere else in the proposals store — crates/rocm-core/src/automation.rs:450-479 (guard at :470), reached via load_automation_proposals/update_automation_proposal_status.
  parse_jsonl_records's new byte-level parsing only tolerates an unparseable line when it is both unterminated and the file's last line (index + 1 == line_count && is_torn_record(line)); this position-aware exception is new in this PR. Every existing corruption test (readers_skip_an_unterminated_last_line_that_does_not_parse, proposal_reads_skip_a_torn_line_cut_inside_a_utf8_character, an_unterminated_last_line_of_the_wrong_shape_is_reported_not_dropped) places the bad line last. Deleting the index + 1 == line_count half of the guard — so any unparseable line anywhere is silently skipped — would still pass every one of those tests, yet would let update_automation_proposal_status's whole-file rewrite silently drop a corrupted non-final proposal line from disk, the opposite of the "wrong-shape data must be reported, never dropped" guarantee this same file documents at :571-573.
  Confidence 85 · logic (missing coverage for behaviour this change introduces) · Fix: add a proposals-store test with a corrupt/invalid line before the last line and assert both the load and the status-update fail, with the on-disk file unchanged.

Non-blocking

• write_atomic's new doc bullet understates a hazard it's documenting — crates/rocm-core/src/lib.rs:357-360 vs. the temp-name scheme at :367-371.
  The new bullet says overlapping read-modify-write cycles merely "lose one update (last rename wins)." Two callers in the same process within the same millisecond share the temp path <path>.<ext>.tmp-<millis>-<pid>; concurrent fs::writes truncating that one path can publish mixed bytes, and the losing rename fails with ENOENT rather than just losing an update. automation.rs:1139 itself separately notes this exact same-process temp-name collision as a bug the per-store lock fixes — the doc bullet should say so too. Low severity here since every JSONL caller now holds the store lock, so this only matters for non-JSONL write_atomic callers (e.g. config.rs, service_record.rs) that this PR doesn't touch.
  Confidence 70 · mechanical · Fix: add the same-process/same-millisecond case to the bullet.

• reads_racing_appends_never_fail would still pass with the fix reverted — crates/rocm-core/src/automation.rs:1591-1619.
  The test's stores stay well under one 64 KiB scan chunk, so ReverseLines never crosses a chunk boundary and a small append is essentially never observed half-written on a local filesystem in-process. The test can effectively only fail on a panic. Its own comment already calls it a smoke test and points elsewhere for the real pinning test, so this is a naming/strength mismatch rather than a coverage gap.
  Confidence 70 · test-coverage · Fix: rename to reflect that it's a smoke test, or make the fixture large enough / records big enough that a genuine partial-write race becomes observable.

• The new torn-audit-tail scenario's rationale is duplicated near-verbatim in three places — tests/e2e-cucumber/features/automations.feature:59-78, tests/e2e-cucumber/tests/e2e/automations_steps.rs:112-120 and :207-215.
  All three repeat the same ".unwrap_or_default() means exit code proves nothing" point and the same caller inventory (rocm automations, the TUI automations-manager screen, rocm chat/TUI slash rocm_command, rocmd's MCP tool) — a list the feature file itself admits "isn't meant as an exhaustive map of every caller." Three copies of a caller list is a maintenance liability: the next caller added or removed won't get all three updated.
  Confidence 85 · mechanical · Fix: keep the rationale once (in the feature file) and trim the step-definition comments to what each step does.

• prune_managed_service_records's doc keeps extending a hand-maintained, already-incomplete list of lock sites — apps/rocm/src/main.rs:4566-4576.
  This PR had to add the new per-store locks to an existing "the only other FileLock::acquire calls in the tree are..." comment in an unrelated function, in another crate. Every claim added is true, but the list already omits at least one other call site documented elsewhere in the same file (main.rs:8504), and the next lock added anywhere will again require remembering to update this comment, with no mechanism to catch a miss.
  Confidence 70 · mechanical · Fix: state the invariant ("nothing reachable from either phase acquires this lock path; other locks use different files") instead of maintaining an exhaustive list.

Decisions for the author

• Generic JSONL-store machinery living inside automation.rs — tradeoff. Locking, the reverse-scan reader, and torn-tail repair (roughly half of this file's 2153 lines) are store-agnostic machinery with nothing automation-specific about them, sitting in a file whose docs/architecture.md charter is "the built-in automation-watcher catalog... and its on-disk event/proposal/audit records." That charter does cover this file's own store I/O, so this isn't a clear guardrail violation, but a future JSONL-backed store elsewhere in the tree (there's an open, related PR #610 generalizing write_atomic) would have to duplicate or import from here rather than from a neutral module. Worth confirming this placement is deliberate rather than just where the diff happened to land.
• FileLock::acquire has no timeout — non-blocking-improvement. A wedged writer stalls every subsequent append to that store forever. This matches the existing pattern for launch.lock/autostart.lock elsewhere in the tree, so it isn't a new risk this PR introduces, just one it now applies to higher-frequency store writes (the gpu-metrics watcher appends roughly once a minute).
• Proposals reads stay O(store size) — tradeoff, already explicitly scoped to ROCMAI-729 in the PR's own text. Flagging only that the asymmetry with the now-O(limit) events/audit readers isn't called out inline in the proposals reader's own doc comment.

Positive signals

• append_jsonl_record/update_automation_proposal_status acquire the store lock before opening the file and drop the file handle before the lock (explicit drop(file) at automation.rs:296, not incidental ordering), closing exactly the "append lands in a copy already replaced by rename" race the design exists to prevent.
• is_torn_record deliberately tests "not valid JSON" rather than "not a valid record," so complete-but-wrong-shape data is still surfaced as an error instead of being silently dropped by a rewrite — backed by a test (an_unterminated_last_line_of_the_wrong_shape_is_reported_not_dropped) that asserts the on-disk bytes are byte-identical after the rewrite refuses.
• The reverse-scan reader parses on raw bytes rather than String, so a record cut mid-way through a multi-byte UTF-8 character can't fail the whole read — verified by a dedicated test that cuts exactly inside a UTF-8 continuation byte.
• The short-write edge-case tests use real, forced OS-level short writes (RLIMIT_FSIZE/SIGXFSZ), not mocks, for both the "landed despite the error" and "genuine failure" cases.

Deployment notes

• Mixed-version rollout window: during a rolling upgrade, any rocm/rocmd instance still on the pre-fix binary keeps using the old lock-free path and can still drop a concurrent writer's record until every instance is upgraded. Already called out in the PR description; repeating here for whoever schedules the rollout.
• New *.jsonl.lock sibling files appear next to each of the three stores in the data directory, following the existing launch.lock/autostart.lock pattern — no action needed, just worth knowing for anyone enumerating or backing up the data directory.
• Already-tracked, not new: GitHub issue #748 (confirmed via lookup, filed against this same PR during an earlier review round) documents that a torn-tail repair which can't complete its replacing rename — sustained disk pressure, or a Windows handle without delete-sharing — has no fallback, and every subsequent append to that store then retries the same whole-store rewrite. Explicitly accepted as a known tradeoff with a follow-up ticket; not a new finding from this pass.

What this covered

Every file in the diff, read completely: crates/rocm-core/src/automation.rs in full (both at pr-738-head and the pre-change version for comparison), crates/rocm-core/src/process.rs's FileLock plus its doc-only diff, crates/rocm-core/src/lib.rs's write_atomic body plus its diff, crates/rocm-core/src/test_support.rs and crates/rocm-core/src/config.rs diffs, apps/rocm/src/main.rs's changed doc comment and its surrounding function, and all three e2e cucumber files (automations.feature, examine.feature, automations_steps.rs). Also read: the repo's AGENTS.md in full, the relevant docs/architecture.md passages for automation.rs's ownership, coverage-floors.toml, and this file's own git history. Reviewed against 79f6e08c42b9b2e6cf7f21edad059c96cb9983f1...35f36382764428ff38760238bf323e2de636817c.

Two independent fan-out workers ran: one over the core automation.rs/process.rs/lib.rs/test_support.rs concurrency logic, one over the secondary files and e2e tests. A third planned worker (for the agent-instruction-adherence/history/code-comments source-reading passes) could not be launched — a concurrent-subagent-slot ceiling shared across sessions in this environment was hit on both the first attempt and one retry — so those three passes were run directly in this driver context instead, blind (before reading either fan-out worker's findings), rather than as an independent subagent; that makes them less independent than the two code passes, though still blind to the change's own framing.

CI was read via gh pr view --json statusCheckRollup and taken at face value: every check (build-and-test, windows-build-and-test, clippy, fmt, Test (affected crates), Coverage (rocm-dash crates, ratcheted), E2E tests, Commit signatures + sign-off, CodeQL, etc.) reports SUCCESS as of the reviewed head commit; none failing.

Not reconciled against any discussion, prior review, or review thread on this PR — this review was conducted blind per the method and did not read any existing review comments.