[jussielo-amd]

#738 (ROCMAI-665/721, the per-store lock and single-line appends this PR builds on) has merged; this PR is rebased onto main directly and is no longer stacked.

Since the last review

• Rebased onto fix(core): stop JSONL stores losing records to concurrent writers #738's current tip (c4a045e2), which replaced the old truncate-based torn-tail repair with an atomic rename-based one (inspect_tail / TailState / write_atomic). The rotation/compaction feature is reintegrated on top of that: constants, rotate_jsonl_generations, open_jsonl_generations, the multi-generation load_recent_jsonl_records, and proposal compaction all port forward unchanged; only append_jsonl_record itself needed real rework.

• Fixed the ordering bug the review found in append_jsonl_record: the size-cap check now runs against the live file's raw length before inspect_tail runs, not after. An at-cap torn store is now rotated whole (debris and all) rather than first paid for with an O(store) write_atomic repair that then gets rotated away a moment later.

• Added events_store_rotates_through_the_public_wrapper and audit_log_rotates_through_the_public_wrapper: both go through append_automation_event/append_audit_event and load_recent_automation_events/load_recent_audit_events at the real EVENT_LOG_ROTATE_BYTES cap, closing the gap the review demonstrated (flipping those wrappers back to None/0 previously left every existing test green).

• Updated the module doc, jsonl_lock_path's doc, and open_jsonl_generations' doc so they describe both reasons an append can replace or rename the live file (torn-tail repair and rotation), not just one.

• Fixed apps/rocmd/src/watchers.rs's server-recover watcher queuing an unbounded restart proposal roughly every 30s for a service that stays unhealthy (a non-blocking finding from the review): it now checks for an existing pending restart proposal first, mirroring gpu-thermal-protect's existing pending_stop_proposal_exists dedupe.

• Lock-free multi-generation readers: kept as-is. Both reviews independently found the open/reopen/compare-identity snapshot correct; switching to a locked read for 64–100-record windows would trade a working, tested design for an unforced simplification.

• Repeated rotation failures are no longer silent: append_jsonl_record's best-effort rotate now prints a one-shot eprintln! warning per process if the rename keeps failing (matching this crate's existing eprintln!-for-diagnostics convention; no tracing/log dependency exists here).

• docs/automations.md's retention section was re-checked against the new cap-check ordering — it already described the rotate-whole-on-cap behavior correctly, no change needed. docs/testing.md now also notes the public-wrapper rotation tests.

• docs/manual-testing.md has no automation-retention section to update (checked, not just assumed).

• events_store_rotates_through_the_public_wrapper and audit_log_rotates_through_the_public_wrapper read with limit: 2, not 1 (a second review finding): at limit: 1 the newest record in the live file alone always satisfies the read, so the public wrappers' read-side generations wiring (load_recent_automation_events/load_recent_audit_events passing EVENT_LOG_ROTATED_GENERATIONS) was never actually exercised — only the append side was, and flipping the read side back to 0 generations still left both tests green. limit: 2 forces the read to cross into the rotated .1.

• audit_records (the e2e test helper that parses the audit store back) now names the path and 1-based line number on a parse failure instead of an opaque .expect panic.

• Added automations-09: automations-07 only forces a single rollover (a live file at the cap gets a .1); the shift-and-drop-oldest chain beyond that — .1 to .2, .2 to .3, .3 to .4, previous .4 dropped — was otherwise pinned only by rocm-core's unit tests, which a review flagged against AGENTS.md §3's user-observable-files rule. The new scenario seeds a full chain of four already-rotated generations plus a live file at the cap, forces one more rotation, and checks the whole chain shifted down by one and the oldest generation's content is gone.

• fix(core): stop JSONL stores losing records to concurrent writers #738 merged into main (squashed), so this PR is rebased directly onto main and no longer carries the ROCMAI-721 commits — its own six ROCMAI-729 commits replay unchanged against the now-merged work. Out of draft accordingly.

• 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-729.)

• 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, plus the small apps/rocmd/src/watchers.rs dedupe fix above.)

• 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. (The new restart_proposal_already_pending info event is asserted together with the proposal count in server_recover_does_not_duplicate_pending_restart_proposals, mirroring gpu_thermal_protect_does_not_duplicate_pending_stop_proposals.)

Why

ROCMAI-729, the follow-up deferred from ROCMAI-665/721. After #738, appends to the three rocm-core JSONL stores (automations/events.jsonl, audit/events.jsonl, automations/proposals.jsonl) are O(1), but nothing bounds their size: with the gpu-metrics watcher enabled, rocmd adds a record a minute to both the events and the audit store (about 600 KB a day to the audit log), forever. And the recent-proposal read still parses the whole proposals file, because a legacy proposal without a stored proposal_id gets an id derived from its absolute line number (legacy-<at>-<index>), which a tail read cannot know.

What changes

Events and audit: size-based rotation. The append that finds the live file at the cap first renames it to <store>.1, shifting .1 to .2 and so on and dropping the oldest, then appends to a fresh file.

• It runs under the store's existing .lock, the same lock as every append, so a rename can never strand an in-flight append — the failure fix(core): stop JSONL stores losing records to concurrent writers #738 fixed for the proposal rewrite.
• Only the unbroken run of generations from .1 up is shifted. A missing generation is a free slot, so a rotation that stopped part-way (for example, the live file could not be renamed on Windows because another program held it open without delete sharing) is completed by the next one without dropping an extra generation.
• Rotation is best effort: if a rename fails, the record is still appended to the unrotated file and the next append tries again. Losing a record to a failed rename would be worse than one oversized file. Repeated failures now print a one-shot warning per process instead of staying silent.
• The cap check runs against the live file's raw length before any torn-tail repair, so an at-cap store — torn or not — is rotated whole rather than repaired first and rotated away immediately after.

Lock-free readers that span the rollover. load_recent_automation_events / load_recent_audit_events read the live file and then .1, .2, … only while still short of limit, so a read just after a rotation still returns the records rotated aside. Readers stay lock-free as in #738, which raises one hazard: a rotation (or a torn-tail repair, which also replaces the live file by rename) can rename files between a reader's opens, so it could open the same file under two names (reading its records twice) or skip a generation. The reader therefore opens every generation, reopens every path, and checks each still names the same file (inode, or volume serial + file index on Windows, via the same-file crate, already in the dependency tree through walkdir and already in THIRD_PARTY_NOTICES.txt); if any moved, it reopens the set. Because a rotation only moves files to higher generations and renames the live file last, the matched handles are a consistent snapshot.

Proposals: compaction instead of rotation. Rotating the proposals store would let a pending proposal fall out of reach of approve/reject, so it is never rotated. Instead, the status rewrite (which already rewrites the whole file under the lock) compacts it: every pending proposal is kept, plus the most recently reviewed ones; older reviewed proposals are dropped. Reviewed proposals only arise from that rewrite (every producer appends pending), so this bounds them exactly. The proposal the update just reviewed always ranks newest, so a review never removes the record it just wrote, even if the clock moved backwards.

Legacy proposal ids and tail-only reads. The same rewrite persists each legacy proposal's derived id, computed from its line number before compaction removes any line, so a legacy id never changes. load_recent_automation_proposals (used by rocm automations and by rocmd's thermal watcher every tick) now reads only the tail, and falls back to a whole-file read only while a legacy proposal without a stored id is inside the window — which stops after the first status update. find_automation_proposal still reads the whole store, since a pending proposal can be arbitrarily old; compaction keeps that bounded by the pending proposals plus the reviewed cap.

server-recover's proposal flood. Unlike gpu-thermal-protect, the server-recover watcher queued a new restart proposal on every recovery tick (about every 30s) for a service that stayed unhealthy, instead of checking for one already pending — which also pushed older pending proposals out of the window other dedupe checks read. It now checks first, mirroring the existing pattern.

docs/automations.md gains a "History And Retention" section stating the files, cap and retention; docs/testing.md lists the focused tests.

Policy decisions

Store	Policy	Value	Why
events, audit	rotate at	8 MiB (EVENT_LOG_ROTATE_BYTES)	About two weeks per file at the gpu-metrics rate, months without it; small enough to open a rotated file in an editor or read it whole in a script.
events, audit	rotated files kept	4 (EVENT_LOG_ROTATED_GENERATIONS)	Caps each store at about 40 MiB, about two months of history at the busiest documented rate. The callers read at most 64 recent records, so retention, not read cost, is what this sets. Mirrors the bounded-history intent of the client log rotation in apps/rocm/src/logging.rs (MAX_RETAINED_LOGS = 7), but size-based, because these stores are written by two processes and have no natural daily boundary.
proposals	rotation	never	A pending proposal must stay findable by id until it is reviewed.
proposals	reviewed kept on each status update	200 (REVIEWED_PROPOSALS_RETAINED)	Well above the largest recent-proposal window any caller reads (100); pending proposals are always kept.

Pending proposals themselves are not capped: dropping one would silently discard a decision the user has not made yet. A store whose pending proposals are never reviewed still grows, at the rate the watchers queue them — server-recover's own flood above was one source of that growth, now fixed.

A store that grew past 8 MiB before this change is rotated aside whole on its first append, so it falls under the cap only once that file is itself dropped, four rotations later; the docs say so.

Verification

• cargo test --workspace --all-targets --no-fail-fast: 36 suites, 4551 passed, 0 failed, 17 ignored (count grew with main's own test additions since the figure above was last taken; this PR's own tests are unchanged in count except the two wrapper-test assertions it tightened). cargo clippy --workspace --all-targets -- -D warnings clean.
• New unit tests in automation.rs, each tied to a failure it catches (mutation evidence, run locally):

Test	Mutation it catches
rotation_under_concurrent_threads_loses_no_records, rotation_under_concurrent_processes_loses_no_records (8 threads / 6 re-executed processes, rotating every ~14 records)	Rotating outside the store lock: records lost in 3 of 3 runs.
reads_racing_rotations_see_each_record_once_and_in_order (one sequential writer rotating while a reader polls)	Dropping the reopen-and-compare check: duplicate records read in 5 of 6 runs.
a_rotation_after_one_left_part_way_drops_no_generation	Shifting every generation regardless of gaps: one generation lost.
legacy_proposal_ids_survive_compaction_and_are_persisted	Not persisting legacy ids before compaction: the legacy proposal's id stops resolving.
recent_proposals_read_only_the_tail_but_fall_back_for_legacy_ids	Dropping the legacy fallback: wrong legacy id returned.
events_store_rotates_through_the_public_wrapper, audit_log_rotates_through_the_public_wrapper	Passing None/0 instead of Some(EVENT_LOG_ROTATION)/EVENT_LOG_ROTATED_GENERATIONS from the public wrappers: previously left every other test green.

Plus deterministic tests for rotation retention, reads spanning a rollover, a store caught mid-rotation (no live file / gap at .1), a failed rotation still appending the record, and compaction keeping every pending proposal. The concurrency tests passed 15 consecutive local runs.

• New unit test in apps/rocmd/src/watchers.rs: server_recover_does_not_duplicate_pending_restart_proposals, mirroring the existing gpu_thermal_protect_does_not_duplicate_pending_stop_proposals — asserts exactly one proposal after two recovery ticks for the same unhealthy service, plus the restart_proposal_already_pending event.
• e2e (cargo xtask e2e -- -n "automations-", all 9 automations scenarios pass, mock lane, no GPU needed — the scenario numbers below shifted by one after the rebase, since main's own torn-audit-tail scenario took automations-06):
  • @id:automations-audit-log-rotates-at-cap (automations-07): seeds the audit log at the cap, makes an audited change (rocm config set-permissions ask), and asserts the full log is now events.jsonl.1, the change starts a fresh log, and rocm automations still lists four pre-rotation entries — which now exist only in the rotated file.
  • @id:automations-review-compaction-keeps-pending (automations-08): seeds 205 reviewed proposals between two pending ones, approves the newer one through rocm mcp-call proposal_action, and asserts 200 reviewed remain, the approval is among them, and the older pending proposal is still found by id.
  • @id:automations-rotation-chain-shifts-and-drops-oldest (automations-09): seeds a full chain of four already-rotated generations (.1-.4) plus a live file at the cap, forces one more rotation with the same set-permissions trigger, and asserts the whole chain shifted down by one and the previous .4's content survives nowhere.
• Conflict-marker grep and the internal-content leak scan against this PR's diff both came back clean after the rebase.

Not verified:

• Windows behaviour was not run: renaming and replacing files other processes hold open relies on Rust's default FILE_SHARE_DELETE share mode for the rocm processes' own handles, and on rotation being best effort when some other program holds the file without it. The Windows CI lanes will exercise the unit tests.
• The three new e2e scenarios were not run against the binary from before this change; each asserts files or generation counts (events.jsonl.1-.4, the 200-reviewed cap) that only this change creates.
• cargo xtask tpn --check was not run (cargo-about is not installed locally); same-file 1.0.6 is already listed in THIRD_PARTY_NOTICES.txt.

Scope

• Proposals are not rotated, and pending proposals are not capped (see above).
• Old rocm/rocmd binaries running alongside new ones do not rotate; mixed versions behave as described in fix(core): stop JSONL stores losing records to concurrent writers #738.
• server-recover's dedupe fix is scoped to that watcher's own restart-proposal flood; it does not change apps/rocmd/src/watchers.rs's other watchers or the 30s SERVER_RECOVER_BACKOFF_MS cadence itself.

[nowycondro]

🔴 Automated review · pr-review-watcher · 5a827f0

Review — needs work

Full review of the whole change (base 6dfcb161…head 5a827f01), blind to this PR's own description until findings were settled. Implements ROCMAI-729 (cap automation stores by rotation and compaction) as its own self-contained change; the ticket itself could not be retrieved from this environment (no tracker access here), so this assesses only what the PR states and what the code does, not the ticket's full scope.

Blocking

• The 4-generation rotation chain (shift-and-drop-oldest) has no e2e coverage, only unit tests — tests/e2e-cucumber/features/automations.feature:59-72, tests/e2e-cucumber/tests/e2e/automations_steps.rs:127-151,352-375
  The project's own AGENTS.md §3 states: "User-observable behavior needs a scenario, not only a unit test... If the change alters what a user of the CLI can observe — command output, exit codes, files or paths the CLI creates... then that behavior must be covered by a Gherkin scenario... a unit test asserting the internal helper does NOT discharge this." The new automations-audit-log-rotates-at-cap scenario only forces a single rotation (seeds one live file at the cap, rotates once, asserts .1 exists and .2 does not) — EVENT_LOG_ROTATED_GENERATIONS (the documented cap of 4) is never referenced anywhere in either file. The shift-and-drop-oldest chain across multiple rotations is exercised only by unit tests in automation.rs (which are thorough and mutation-tested per the PR's own verification table — this is not a correctness doubt). Confidence 82 · architectural · Fix: either extend/add a scenario that pre-seeds existing rotated generations and forces one more rotation to assert the chain shift and the oldest generation's deletion, or add a sentence to the PR text explaining why this slice is intentionally left to unit-test-only coverage (AGENTS.md explicitly allows that path for a stated reason).

Non-blocking

• audit_records test helper panics with no line context on a parse failure — tests/e2e-cucumber/tests/e2e/automations_steps.rs:344-350
.expect("audit line parses") gives no line index or content if a record fails to parse. On this new merge-across-rotated-generations read path, a genuine corruption bug would surface as an opaque panic rather than an actionable one. Confidence 72 · mechanical · Fix: include the line index/content in the panic message.

Decisions for the author

None

Positive signals

• The gap-tolerant rotation shift (rotate_jsonl_generations, automation.rs:315-330) correctly handles a rotation that previously stopped part-way (a gap at .1) without an off-by-one — traced all three structural cases (full chain, mid-gap, missing .1) by hand against the code.
• The lock-free reader's reopen-and-compare protocol (open_jsonl_generations) correctly re-verifies every generation's file identity via same-file::Handle equality (not just the live file), with a bounded retry count (8 attempts, then a clear error) rather than an unbounded loop.
• Proposal compaction's sort key (a (bool, u128, usize) tuple keyed first on "is this the proposal just reviewed") guarantees the just-reviewed proposal is always retained even if the system clock moved backwards, without a separate special case in the retain logic.
• Reviewing a proposal is independently audit-logged (apps/rocm/src/chat.rs's proposal_action handler calls record_cli_audit_event alongside update_automation_proposal_status), so compacting old reviewed proposals out of the proposals store does not lose the durable record of what was decided — it survives in the separately-retained audit log.
• The PR description's "Not verified" section is unusually candid about real gaps (Windows behavior, the new scenarios not run against the pre-change binary) — exactly the kind of explicit scope honesty that makes a review easier to trust.

Deployment notes

• Adds a new direct dependency, same-file (already present transitively and already listed in the repo's third-party notices file, per the PR's own note).
• An audit/events log that is already past the 8 MiB cap at upgrade time is rotated aside whole on its first post-upgrade append; the oversized backlog only clears once that file ages out four rotations later (documented in docs/automations.md).
• Mixed old/new binary versions: an old binary never rotates a store; this matches the existing mixed-version behavior from the PR this one is stacked on and is explicitly called out in scope.
• Pending proposals are never capped by design — a watcher that queues proposals nobody reviews grows that store without bound. This is an explicitly acknowledged, deliberate limitation in the PR text, not an oversight.

What this covered

Read the whole diff between the PR's base (6dfcb161, the tip of the branch this PR stacks on) and head (5a827f01) across all 8 changed files. crates/rocm-core/src/automation.rs (2279 lines at head) was read completely by an independent pass plus my own manual trace of the rotation/reader/compaction core. tests/e2e-cucumber/features/automations.feature and tests/e2e-cucumber/tests/e2e/automations_steps.rs were read completely by a second independent pass plus my own spot-read of the new scenarios and step implementations. Cargo.lock, crates/rocm-core/Cargo.toml, crates/rocm-core/src/lib.rs, docs/automations.md, and docs/testing.md were reviewed directly. Checked this repo's AGENTS.md for applicable rules. Checked git history on automation.rs and the still-open review finding on the PR this one is stacked on, for interaction with the new logic (found independent/orthogonal by direct tracing — rotated generations are never reopened for writing once renamed). Checked all production call sites of the touched public functions across apps/rocm and apps/rocmd — all use unchanged signatures, no breakage. Checked CI (gh pr view --json statusCheckRollup): every check reports SUCCESS or SKIPPED, nothing failing. The ticket named in the PR title could not be retrieved from this environment (no tracker access here) — that check could not be made. Did not independently run cargo test/cargo clippy locally; CI already reports these green for this exact head commit. Not reconciled against any prior review discussion on this PR — this is a first, blind pass.

[tomastola]

🔴 Automated review · pr-review-watcher · 5a827f0

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 — needs work

Reviewed this PR's own commits (6dfcb16..5a827f0). The rotation, the lock-free readers and the compaction logic look correct. The two blocking items are a conflict with the base branch and a missing test.

Blocking

• The PR conflicts with its base branch, and resolving it needs a design decision — crates/rocm-core/src/automation.rs (append_jsonl_record, module doc, load_recent_jsonl_records)

  • This PR sits on an older state of fix-jsonl-store-concurrent-writers. Since then the base gained 87fcf8c ("repair a torn JSONL tail by rename, not truncate"), which replaces repair_unterminated_tail and the truncate rollback with inspect_tail plus write_atomic.
  • GitHub reports the PR as conflicting, and git merge-tree against the base tip gives 12 conflict hunks in automation.rs.
  • Most hunks are doc text. The append_jsonl_record body is not: the cap check has to sit after inspect_tail and before the Torn arm's write_atomic.
    • If it runs after that write, an at-cap torn store is first rewritten in full (8 MiB) and then renamed straight to .1.
    • If a rotation fails, the code drops into the Torn arm and rewrites 8 MiB on every append.
  • Several of this PR's doc comments become false after the merge. For example, "A failed write is rolled back to the pre-append length": 87fcf8c removed that rollback.
  • The open_jsonl_generations doc says only rotations move files. After 87fcf8c, a torn-tail repair also replaces the live events and audit file. The snapshot argument still holds (the identity check just forces one retry), but the comment should name repair too.
  • AGENTS.md §11 asks for stacked PRs to stay in draft until their dependencies merge. This one depends on #738 and is marked ready.
  • Fix: rebase onto 87fcf8c, place the cap check before the torn-repair write, update the affected doc comments, and keep the PR in draft until #738 lands.

• Nothing tests that the automation events store actually rotates — crates/rocm-core/src/automation.rs (append_automation_event passing Some(EVENT_LOG_ROTATION); load_recent_automation_events passing EVENT_LOG_ROTATED_GENERATIONS)

  • The new unit tests call append_jsonl_record and load_recent_jsonl_records directly, with local policies. None of them goes through the public event-store wrappers, and e2e automations-06 covers only the audit log.
  • With append_automation_event changed to pass None and load_recent_automation_events changed to pass 0 generations, all 37 automation::tests still pass.
  • So the events store could quietly go back to growing without limit while CI stays green, even though capping it is half of what this PR claims.
  • Fix: add a unit test that fills automation_events_path() to EVENT_LOG_ROTATE_BYTES, appends through append_automation_event, and asserts that .1 exists and that load_recent_automation_events reads across it. The same shape for the audit wrappers is cheap to add.

Non-blocking

• server-recover can still grow the proposals store without bound, at about 2,880 records a day — apps/rocmd/src/watchers.rs (QueueProposal arm of handle_server_recover_event_with_record; pending_stop_proposal_exists)

  • This predates the PR, but it bears directly on what the PR is meant to cap.
  • In the opt-in propose mode, server-recover queues a new restart proposal each time recovery is due: every 30 s (SERVER_RECOVER_BACKOFF_MS) for a service that stays unhealthy. Unlike gpu-thermal-protect, it never checks for an existing pending proposal.
  • Compaction never drops a pending proposal, and both find_automation_proposal and update_automation_proposal_status read the whole file.
  • The flood also pushes an older pending stop proposal out of the 100-record window that pending_stop_proposal_exists reads. Once that happens, the thermal watcher's dedupe stops working.
  • The description does say that pending proposals are uncapped. But this case comes from a missing dedupe, not from a slow review backlog.
  • Fix: give server-recover a per-service pending-proposal check, either here or in a follow-up linked from the description.

• Repeated rotation failures are invisible — crates/rocm-core/src/automation.rs (let _ = rotate_jsonl_generations(...) in append_jsonl_record)

  • Best effort is the right call. But if renames keep failing (for example, another program holds the file open without delete sharing on Windows), the store grows past the documented ~40 MiB and nothing records it.
  • Fix: warn, or record an automation event, the first time rotation fails in a process.

Decisions for the author

• Lock-free readers or a shared read lock — tradeoff

  • The open, reopen and compare-identity snapshot is correct, and reads_racing_rotations_see_each_record_once_and_in_order would catch a regression in it. It also costs about 90 lines of subtle code and a new direct dependency.
  • The readers run rarely and read at most 64–100 records. Taking the store lock for those reads would remove the race entirely, at the cost of making readers wait behind an append or a rotation.
  • Worth confirming that keeping the readers lock-free is worth the complexity.

• What to do with a torn store at the cap during the rebase — tradeoff

  • Option 1: rotate it whole. The torn bytes end up as .1's unterminated last line, which readers already skip, and nothing gets rewritten.
  • Option 2: repair first and then rotate. That pays for an 8 MiB rewrite.
  • Option 1 is cheaper, and the read path already tolerates its result.

Deployment notes

• On the first append after upgrading, an existing events or audit log already over 8 MiB is renamed whole to .1. It only drops out four rotations later. docs/automations.md says so.
• Older rocm/rocmd binaries running alongside new ones never rotate, as the description notes.
• New direct dependency: same-file = "1" in rocm-core. It was already in the tree through walkdir.

[juhovainio]

I read the whole diff and found no correctness problems in the rotation, compaction or legacy-id migration code; the snapshot-by-reopen approach holds up, and the rotation/rollover tests plus the new automations-06/07 scenarios cover the observable behavior well. CI is green.

The change request is about merge state, not the code:

• The PR is currently conflicting and its base is the #738 branch, which is still open, so the diff also carries the ROCMAI-721 commit. Please rebase onto main once #738 merges, and keep this PR in draft until then.

Non-blocking, for your judgement:

• Two small inline notes on automation.rs (read cost of opening every generation, and the swallowed "kept being rotated" error).
• docs/manual-testing.md was not touched; if it has a section on automation store retention it should mention rotation and compaction.
• Per the PR description, rotation on Windows is unverified by hand. The replace-rename over an oldest generation that a concurrent reader still holds open is the path I would try there, even though the Windows CI lane passes.

[jussielo-amd]

@tomastola @juhovainio thanks both — addressed the blocking items and the process note:

• Rebased onto fix(core): stop JSONL stores losing records to concurrent writers #738's current tip (c4a045e2) in 2a07098, resolving the conflict and reordering append_jsonl_record's cap check to run before inspect_tail, so an at-cap torn store is rotated whole instead of being repaired (an O(store) write_atomic rewrite) and then rotated away a moment later.
• Added the missing public-wrapper rotation tests in ec40193 (events_store_rotates_through_the_public_wrapper, audit_log_rotates_through_the_public_wrapper) — confirmed locally that flipping append_automation_event/append_audit_event back to None/0 makes exactly these two fail while every other test stays green.
• Back in draft per AGENTS.md §11, since fix(core): stop JSONL stores losing records to concurrent writers #738 is still open.
• Also fixed the non-blocking server-recover proposal-flood finding in 7f27b4c, and replied to the two inline notes separately.

Kept the lock-free multi-generation readers as-is — both of you independently found that design correct, and I'd rather not trade a working, tested snapshot for an unforced simplification. Full details in the PR description's "Since the last review" section.

[tomastola]

🟢 Automated review · pr-review-watcher · 195fb93

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.

Re-review — the earlier blockers are fixed

This covers what changed since 5a827f0: the rebase onto c4a045e, plus ec40193, 7f27b4c, b171e7f and 195fb93.

What the last review raised

• Conflict with the base branch (was blocking): fixed.
  • The PR is rebased onto c4a045e and merges cleanly. It is back in draft, and the description says it waits for #738.
  • In append_jsonl_record, the cap check now reads the file length before inspect_tail runs. So a store at the cap with a torn tail is renamed to .1 as it is, and never rewritten first. The torn bytes become the unterminated last line of .1, which the reader already skips one file at a time. This is the cheaper option I suggested.
  • If a rotation fails, the append goes through the normal inspect_tail path. A torn tail is repaired once, and later appends are ordinary appends, so there is no 8 MiB rewrite on every append.
  • The doc comments that went stale are rewritten: the module doc, jsonl_lock_path, append_jsonl_record, and open_jsonl_generations, which now names torn-tail repair as a second way the live file gets replaced.
• No test that the events store rotates (was blocking): fixed on the write side, not on the read side. Details under Non-blocking.
• server-recover proposal flood (non-blocking): fixed in 7f27b4c.
  • pending_restart_proposal_exists follows the existing pending_stop_proposal_exists pattern.
  • When a restart proposal is already pending, the early return still calls record_event. That keeps last_event_unix_ms, and with it the 30 s server_recover_due backoff, working.
  • There is a new test that runs two ticks and checks that only one proposal is queued.
• Silent rotation failures (non-blocking): fixed. warn_once_rotation_failed writes one stderr warning per process.

Non-blocking

• No test checks that the events reader reads past the live file — crates/rocm-core/src/automation.rs (events_store_rotates_through_the_public_wrapper; load_recent_automation_events passing EVENT_LOG_ROTATED_GENERATIONS)
  • The new wrapper tests read with limit = 1, and the newly appended live file always satisfies that. So the read side of the wiring is never checked.
  • I changed both load_recent_automation_events and load_recent_audit_events to pass 0 generations, and all 43 automation::tests still passed.
  • For audit, automations-06 reads across .1 end to end, so that side is covered. For events, nothing is.
  • The new test comment says "Flip either side back to None/0 … These two tests close that gap". That is true for the append side (changing it to None fails events_store_rotates_through_the_public_wrapper) but not for the read side.
  • Fix: in both wrapper tests, read with limit = 2 and assert ["filler", "new"]. Then the test only passes if the reader actually spans .1.

Decisions for the author

• The lock-free readers versus a shared read lock tradeoff from the last review still stands. The description now spells out the snapshot argument, so this is fine to leave as it is.

What this covered

• Compared the old and new versions of the rebased commits, and read ec40193, 7f27b4c and b171e7f in full.
• Read 195fb93: it only adds a doc comment to open_jsonl_generations accepting that its retry-limit error reads as an empty activity list to .unwrap_or_default() callers. No code changes.
• Ran cargo test -p rocm-core --lib automation::tests locally with two changes:
  • Read side changed to 0 generations: all 43 tests passed.
  • Events append changed to None: the new wrapper test failed.
• CI on 195fb93: the Read the Docs build and the CodeQL actions and python analyses pass, and the CodeQL rust analysis is still running. No build or test job appears on this head, probably because the PR targets #738's branch rather than main. Green CI should not be read as proof the Rust tests ran.

[tomastola]

🟢 Automated review · pr-review-watcher · e4c82a4

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.

Re-review — rebase only, nothing new to review

This covers what changed since 195fb93. The new head e4c82a4 is a rebase and contains no new work.

What changed

• The base branch moved. #738's branch (fix-jsonl-store-concurrent-writers) was rebased from 7eb4bfa onto main at bfa3c04. Its own four commits are unchanged: git range-diff shows them as identical (c4a045e → 24d266b).
• This PR's six commits are byte-identical after the rebase. git range-diff c4a045e..195fb93 24d266b..e4c82a4 marks five of them =. The only difference in 23aae4a is one line of hunk context in crates/rocm-core/src/lib.rs, because main added pub mod windows_driver. The overall diff against the base is the same size as before: 9 files, +1426/−49.
• No overlap with the new main. Main's new commits share three files with this PR: Cargo.lock, crates/rocm-core/src/lib.rs and docs/testing.md. In each one the changes sit in separate places: a new same-file dependency entry, a new module line, and a separate section of the docs. None of them touches automation.rs or watchers.rs.

What the last review raised

• All the earlier blocking items are still fixed. The code is the same code I checked last time.
• Still open (non-blocking): no test checks that the events reader reads past the live file.
  • Where: events_store_rotates_through_the_public_wrapper and audit_log_rotates_through_the_public_wrapper in crates/rocm-core/src/automation.rs.
  • Both tests still read with limit = 1. The newest record is always in the live file, so that read never needs .1, and the read side is never tested.
  • For audit, automations-06 covers the read side end to end. For events, nothing does.
  • The test comment says flipping either side back to None/0 makes these tests fail. That is true for the append side only.
  • Fix: read with limit = 2 and assert ["filler", "new"].
• Decision for the author, unchanged: the readers still take no lock, and the description explains why that is safe. Fine to leave as it is.

What this covered

• Compared the old and new commits with git range-diff, for this PR and for the base #738.
• Listed the files that both this PR and the new main change, and read each overlapping change.
• Ran tests locally on e4c82a4:
  • cargo test -p rocm-core --lib automation::: 43 passed.
  • cargo test -p rocmd server_recover: 6 passed, including server_recover_does_not_duplicate_pending_restart_proposals.
• CI on e4c82a4: still running. Lint, clippy, coverage, signatures and CodeQL (actions and python) have passed. build-and-test, Test (affected crates), windows-build-and-test, Build Linux PR binaries, the containerised remote-control job and CodeQL rust have not finished. Unlike on 195fb93, the build and test jobs do run on this head.

[jussielo-amd]

Reverting this back to draft — it got marked ready-for-review while #738 is still open, which contradicts juhovainio's open change request (keep in draft until #738 merges) and our own note above. Will mark ready again once #738 lands and this is rebased onto main.

[jussielo-amd]

@nowycondro @tomastola addressed the remaining findings from the earlier review bodies (these aren't inline threads, so replying here):

• nowycondro's blocking finding — the shift-and-drop-oldest rotation chain was only pinned by unit tests, not an e2e scenario: added automations-09 in 5387c06. It seeds a full chain of four already-rotated generations (.1-.4) plus a live file at the cap, forces one more rotation through the existing config set-permissions trigger, and asserts the whole chain shifted down by one and the previous .4's content survives nowhere.
• tomastola's non-blocking finding, raised on two re-reviews — events_store_rotates_through_the_public_wrapper and audit_log_rotates_through_the_public_wrapper read with limit: 1, so the newest record in the live file alone always satisfied the read and the public wrappers' read-side EVENT_LOG_ROTATED_GENERATIONS wiring was never actually exercised: fixed in 31ae875 (limit: 2, asserting the result spans the rotated filler record and the new one).
• nowycondro's non-blocking finding — audit_records' .expect("audit line parses") gave no line context on a parse failure: fixed in 9d44d27 (names the path, 1-based line number, and the raw line).

PR description's "Since the last review" and "Verification" sections updated to match (scenario numbers also corrected there — they'd drifted by one after the rebase onto #738's now-merged tip, which folded in its own automations-06 scenario).

[jussielo-amd]

@juhovainio the merge-state change request should be clear now too: #738 merged, this PR is rebased directly onto main (no longer stacked, no longer carrying the ROCMAI-721 commits), and it's out of draft. CI is green on the new head.