[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, reading the whole proposals store rather than a recent window.

• 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 store, 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 restart_proposal_already_pending info event, whose text says a restart request awaiting review is already queued, is asserted together with the single pending proposal in server_recover_does_not_duplicate_pending_restart_proposals; the restart_proposal_check_failed error event is asserted together with nothing having been queued in server_recover_reports_an_unreadable_proposals_store_instead_of_failing.)

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 print a one-shot warning per store, 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. The check reads the whole proposals store rather than a recent window, because a pending proposal is never dropped and can be arbitrarily old; compaction keeps reviewed proposals to a fixed count, so the read stays small. A store that cannot be read is reported as a restart_proposal_check_failed error event and nothing is queued that tick: an error out of a watcher ends the daemon loop, so it must not propagate. The trade-off is that while the store stays unreadable, server-recover logs that error event on each recovery check and queues no restart, since it cannot tell whether one is already pending; before this change the append path never parsed the store, so a restart would still have been queued.

docs/automations.md gains a "History And Retention" section stating the files, cap and retention, and a note on the restart dedupe, its two events and the unreadable-store behaviour; 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 -p rocm-core -p rocmd (924 and 161 passed, 0 failed), cargo clippy -p rocm-core -p rocmd --all-targets -- -D warnings and cargo fmt --check clean; the workspace-wide suite runs in CI.
• 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.

• Unit tests in apps/rocmd/src/watchers.rs for the server-recover dedupe, each tied to a failure it catches; the second and third were confirmed locally to fail against the broken behaviour:

Test	Mutation it catches
server_recover_does_not_duplicate_pending_restart_proposals	Queueing a second restart for the same unhealthy service: asserts exactly one pending proposal after two recovery ticks, and the event text.
server_recover_dedupe_sees_a_pending_restart_older_than_the_recent_window	Reading only the 100 most recent proposals: 101 newer proposals push the pending restart out of the window and a second one is queued (2 restarts, not 1).
server_recover_dedupe_ignores_proposals_that_do_not_block_a_restart	Dropping the status, watcher_id or service_id filter from the check: a reviewed restart, a restart from another watcher, or a restart for another service then suppresses a restart that should queue. Each of the three removals fails it.
server_recover_reports_an_unreadable_proposals_store_instead_of_failing	Propagating the read error, queueing anyway, or changing the event: the handler must return Ok, record the error event and leave the store untouched.

• a_rotation_failure_is_reported_once_per_store in automation.rs pins that the rotation-failure warning is decided per store path (two stores sharing a file name under different directories each get one) and names the path and the cause. a_failed_rotation_prints_its_warning_once_to_stderr covers the eprintln! itself: it re-runs the test binary as a child process and reads its stderr, and two appends to a store stuck at its cap print exactly one warning (removing the eprintln! fails it).
• 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. gpu-thermal-protect's stop-proposal check (pending_stop_proposal_exists) is left as it was: it still reads the 100 most recent proposals and propagates a read error, so it has the same two limits this change removes for restart.
• No Gherkin scenario covers the restart dedupe: the e2e harness has no step that drives a rocmd watcher tick against an unhealthy managed service, so the handler is covered by the unit tests above, which run it against the real proposals store.

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

fixed

[juhovainio]

I went through the rotation/compaction changes and found no correctness blockers. The lock ordering, the lock-free reader snapshot, and compaction (pending kept, just-reviewed pinned, legacy ids persisted) all hold up, and the new automations-07/08/09 scenarios assert files and state together. The Strix Halo Ubuntu E2E failures I saw (bench-06, "device state NOT drained") look like runner contention rather than this diff.

Requesting changes for a few things worth addressing before merge, left inline:

• docs/testing.md names the wrong scenarios for the rotation coverage (06 is the torn-tail scenario; it should be 07-09).
• The new watcher event text says "a reviewed restart request is already waiting", but the proposal is pending, not reviewed, and the test only pins the action name.
• The restart dedupe only scans the latest 100 proposals, so a long-lived pending restart can age out and the flood returns. This watcher fix is also outside ROCMAI-729's scope and has no scenario for the new restart_proposal_already_pending event; either add one, justify its absence in the PR text, or split it into its own PR.
• The rotation-failure warning is process-wide, so a failed events-store rotation silences the audit-store warning.

[juhovainio]

The rotation and compaction scenarios are automations-07, automations-08 and automations-09; automations-06 is the torn audit-log tail scenario. Please update the reference.

[jussielo-amd]

Fixed in 1ae1b85: the reference now reads automations-07 through automations-09.

[juhovainio]

"a reviewed restart request is already waiting": the proposal is pending (awaiting review), not reviewed. Suggest "a restart request awaiting review". The test pins only the action name, so this wording claim is unverified.

[jussielo-amd]

Changed to "a restart request awaiting review is already queued" in e5b2361. The test now asserts that text together with the single proposal still being pending (it previously pinned only the action name).

[juhovainio]

The dedupe only scans the 100 most recent proposals. A pending restart older than that is missed and the flood returns. Also, this watcher fix is outside ROCMAI-729's scope and has no Gherkin scenario for the new restart_proposal_already_pending event; please add one, state the justification in the PR text, or split it out.

[jussielo-amd]

Fixed in e5b2361. The check now reads the whole proposals store (a pending proposal is never dropped, and compaction keeps the reviewed ones to 200), and a new test queues 101 newer proposals behind the pending restart; it fails against the 100-record window (a second restart is queued) and passes now. I also made an unreadable proposals store a reported restart_proposal_check_failed error event rather than an error out of the watcher, since that would end the daemon loop.

On scope and the scenario: I kept the fix here because the flood was also pushing older pending proposals out of the window other dedupe checks read. There is no scenario because the e2e harness has no step that drives a rocmd watcher tick against an unhealthy managed service, so it is covered by unit tests against the real proposals store; the PR text says so now. gpu-thermal-protect's pending_stop_proposal_exists has the same 100-record window and error propagation. I left it as it was, as outside this change, and noted that in the Scope section; it would be a separate change.

[juhovainio]

This Once is process-wide, not per path: a failed events-store rotation suppresses the audit-store warning, and the message names only the first path. Fine if deliberate (say so in the doc comment), otherwise key it per path.

[jussielo-amd]

Keyed per path in 50b06b2: each store now gets its own one-shot warning, and the message names that store. The new test uses two paths that share a file name under different directories, like the real events and audit stores.

[jussielo-amd]

@juhovainio the four points in your review are answered in their threads: the scenario reference (1ae1b85), the "awaiting review" wording plus a status assertion and the restart dedupe reading the whole proposals store (e5b2361), and the per-store rotation warning (50b06b2). I left the threads open for you to resolve. The PR description now covers the dedupe change, why it has no scenario, and that the stop-proposal check is left as it was.

fixed

[juhovainio]

I went through this PR end to end. The rotation and compaction work looks correct to me: the rotation ordering under the per-store lock, the lock-free multi-generation reader, and the proposal compaction with legacy-id migration all hold up, and the tests cover them well (concurrent writers, mid-rotation reads, part-way rotation, chain shift and oldest-drop in automations-07 through 09). CI is fully green. I found nothing broken, but there is one item I would like addressed before merge, so I am requesting changes on it.

To address before merge

1. The server-recover watcher dedupe in apps/rocmd/src/watchers.rs is a separate change from rotation and compaction, and ROCMAI-729 does not mention it. It adds user-visible behavior: repeat restart proposals are suppressed, and two new events (restart_proposal_already_pending, restart_proposal_check_failed) appear in the automation event log that rocm automations surfaces. It is covered only by unit tests, with no scenario under tests/e2e-cucumber/features/, and docs/automations.md does not mention it. Please either split it into its own PR, or add a scenario and a docs note, or say in the PR text why a scenario is not possible.

Non-blocking

2. With a corrupt proposals store, server-recover now logs an error event every tick and never queues a restart. Before, the append path did not parse the store, so queueing kept working. Reporting instead of propagating is the right call, but please state the trade-off in the PR or docs.
3. The eprintln! for the rotation warning is only tested through rotation_failure_warning. a_rotation_that_fails_still_appends_the_record does not look at stderr, so removing the call would leave every test green.
4. reads_racing_rotations_see_each_record_once_and_in_order (4000 appends, 8-retry reader) is timing-sensitive. If a loaded runner exhausts the 8 attempts the test fails, so keep an eye on it for flakes.
5. When the reader gives up after 8 retries, the .unwrap_or_default() callers turn that into an empty list. I know this is documented and accepted; just flagging that it is silent.
6. The dedupe reads the whole proposals store every tick for each unhealthy service. That is bounded only by reviewed compaction, since pending proposals are uncapped. This is fine today, but worth revisiting if another watcher can pile up pending proposals.
7. Nit: the module-doc reflow in automation.rs splits the AutomationTriggerEvent sentence mid-line for no reason.

[juhovainio]

This adds user-visible events (restart_proposal_already_pending, restart_proposal_check_failed) and suppresses repeat restart proposals, but only unit tests cover it. User-observable behavior needs a scenario in tests/e2e-cucumber/features/, or a stated reason it cannot have one. It is also outside ROCMAI-729's scope: consider splitting it out, and mention it in docs/automations.md.

[jussielo-amd]

Kept in this PR, with the two things you asked for in lieu of a scenario: a note in docs/automations.md (a8b8669) naming the two events and the behaviour, and the reason there is no scenario stated in the PR text. The e2e harness has no step that drives a rocmd watcher tick against an unhealthy managed service, so the dedupe is covered by the four server_recover_* unit tests in this file, which run the handler against the real proposals store. I kept it here rather than splitting because the flood it stops was also pushing older pending proposals out of the window the other dedupe checks read, and because the store-size caps in this PR are only meaningful once something stops queueing a new proposal every recovery tick. Happy to split it out if you would still rather.

[juhovainio]

Trade-off worth stating in the PR: with a corrupt proposals store this arm logs an error event every tick and never queues a restart, where before the append path did not parse the store and queueing kept working. Skipping rather than propagating is right (an Err ends the daemon loop); just make the behavior explicit.

[jussielo-amd]

Stated in the PR text (the server-recover paragraph under "What changes") and in the new docs/automations.md note (a8b8669): while the proposals store stays unreadable, each recovery check logs a restart_proposal_check_failed error event and no restart is queued, because it cannot tell whether one is already pending. The error is reported rather than raised so the daemon loop keeps running.

[juhovainio]

The one-shot rotation warning is only tested through rotation_failure_warning; nothing fails if this eprintln! is dropped, because a_rotation_that_fails_still_appends_the_record does not look at stderr. Low risk, but the wiring is unasserted.

[jussielo-amd]

Agreed, that was an unasserted wire. Added a_failed_rotation_prints_its_warning_once_to_stderr in a8b8669: it re-runs the test binary as a child process (the same approach as the short-write tests in this file) and reads its stderr, and two appends to a store stuck at its cap must print exactly one warning: failed to rotate line naming the store. Confirmed locally that replacing the eprintln! with a no-op fails exactly this test.

[juhovainio]

Nit: the module-doc reflow splits the AutomationTriggerEvent sentence mid-line ("defined here, but this" / "module persists"); restoring the original wrap would keep the diff minimal.

[jussielo-amd]

Restored the original wrap in a8b8669, so the diff for that paragraph is now just the removed "not rotated or capped" sentence and the new rotation paragraph.

[jussielo-amd]

@juhovainio thanks for the thorough pass. Pushed a8b8669; each inline thread has its own reply, and I left them open for you.

• 1 (restart dedupe): kept in this PR. It now has a docs/automations.md note, and the PR text states why there is no scenario (no harness step drives a rocmd watcher tick against an unhealthy service; the four server_recover_* unit tests run the handler against the real proposals store). I can split it into its own PR if you prefer.
• 2 (corrupt proposals store): the trade-off is now stated in the PR text and the docs note.
• 3 (eprintln! wiring): new child-process test reads stderr; confirmed it fails without the eprintln!.
• 7 (nit): original wrap restored.
• 4 (reads_racing_rotations_... timing): no change. It is a deliberately heavy race test and passed 15 consecutive local runs plus the CI lanes on this head; if it flakes on a loaded runner, raising the reader's retry budget in the test is the first thing to try.
• 5 (silent unwrap_or_default()): accepted as documented on open_jsonl_generations; no change.
• 6 (whole-store read per tick): agreed it is bounded only by reviewed compaction. Pending proposals are uncapped by design (the PR text says so), so it is worth revisiting if another watcher can pile them up; not changed here.

fixed

[juhovainio]

Looks good to me. I read through the full diff and found nothing blocking.

• Rotation, compaction and the legacy-id migration are covered by the new automations-07 through 09 scenarios, and docs/automations.md and docs/testing.md are updated to match.
• The tradeoffs the description calls out (proposals are never rotated, pending proposals are uncapped, the dedupe reads the whole proposals store each tick) are reasonable for this change.
• I traced the lock-free reader's reopen-and-compare file-identity check and didn't find a gap.
• One non-blocking note is left inline.

Caveat: this is a read of the diff only. I did not run the Windows rename path locally; CI's windows-build-and-test is green at this head.

[juhovainio]

Non-blocking: while the restart stays pending this records an info event on every recovery tick (~30s). Previously each tick also logged an event, so this is not a regression, but the events store is now capped, so these entries will push gpu-metrics history out and crowd the "recent background activity" listing. Consider skipping or rate-limiting the event when a restart is already pending.