Skip to content

fix(run-control): serialize journal appends across processes (#651) - #660

Merged
solomonneas merged 3 commits into
mainfrom
fix/651-cross-process-journal-lock
Aug 1, 2026
Merged

fix(run-control): serialize journal appends across processes (#651)#660
solomonneas merged 3 commits into
mainfrom
fix/651-cross-process-journal-lock

Conversation

@solomonneas

Copy link
Copy Markdown
Collaborator

Fixes #651.

Closes the Critical defect found in the post-merge grading review of #641: the runs steer/interrupt control CLI appends to a run's lifecycle journal from a second OS process, while _append_critical_section held only a threading.Lock plus signal mask. Two processes could each read tail sequence N and append N+1 to the O_APPEND file, forking the hash chain and permanently fail-closing a journal-authoritative run with no recovery path.

Fix and regression map

Item Fix Named regression tests
Cross-process append race Sibling lifecycle.jsonl.lock acquired with fcntl.flock(LOCK_EX) inside the existing critical section, taken by every append path: owner lifecycle writes, the control CLI, redaction, and recovery. Nesting is SIGTERM mask, then process lock, then file lock; unlock precedes close and a primary exception is preserved through cleanup. Hosts without fcntl or pthread_sigmask keep the prior in-process guards. test_cross_process_append_race_serializes (real subprocess children, not monkeypatch)
Owner-side stale retry Lifecycle writes retry on StaleSequenceError instead of dying when a control append lands between read and write. test_owner_retries_stale_sequence_after_external_append
Status / ownership guard Control appends refuse on a completed run and when the caller is not the active lock owner, instead of journaling into a terminal chain and rewriting shadow evidence as a non-owner. test_steer_against_completed_run_refuses_without_journaling
Bounded control.failed payloads Free transport and app-server error text replaced with a bounded error-class enum plus digest reference, matching the payload discipline in run_events.EVENT_TYPES. test_control_failed_payload_carries_bounded_error_class
Minor: duplicate control effect Concurrent same-request-id callers detected by comparing the committed event against the submission. covered in tests/test_run_events_cursor_control.py
Minor: post-terminal events unreachable runs events no longer stops at the first terminal event, so run.redaction.recorded and control.* are reachable through the cursor reader. covered in tests/test_run_events_cursor_control.py
Minor: follow-mode chain continuity Follow mode revalidates chain continuity between polls, so a redaction rewrite mid-follow cannot emit a suffix that fails to chain onto emitted events. covered in tests/test_run_events_cursor_control.py

No changes to event canonicalization, existing event types, or golden fixtures.

Verification

  • brigade work verify run --target . --command "./scripts/verify" --capture brigade-work
  • Receipt 20260801-200416-work-verify-135e03, exit 0: 5,675 passed, 3 skipped
  • Targeted receipt 20260801-200338-work-verify-138809: 308 passed across the four affected suites, including all 13 previously-red tests

Provenance note for the reviewer

This branch was assembled in two salvage steps, both from changes.patch artifacts of brigade run invocations that terminated non-successfully after producing good uncommitted work and then had their worktrees pruned (see #656):

  • 0dcbe970 red suite, from run 20260801-182926-facaecff (ended incomplete; a worker misread its base commit and cascaded skipped: prerequisite failed)
  • 711e1fef implementation, from run 20260801-191101-6612a08a (ended timeout at 1500s after the implementation was complete)

The red suite was verified failing against the pre-fix tree before the implementation was applied: AssertionError: cross-process append race forked the journal: ['sequence gap/duplicate: expected 202, got 201'].

13 failing tests written by the brigade run red phase: real two-process
append race via subprocess (reproduces the chain fork), sibling
lifecycle.jsonl.lock expectations, steer-against-completed-run refusal,
bounded control.failed payloads, and the three minor items from #651.
Implementation follows; tests must pass without being weakened.
Add a sibling lifecycle.jsonl.lock acquired with fcntl.flock inside the
existing append critical section, so the run owner, the steer/interrupt
control CLI, redaction, and recovery serialize across OS processes rather
than only within one. Lock nesting is SIGTERM mask, then the process lock,
then the file lock, with unlock before close and the primary exception
preserved.

Guard control appends on run status and lock ownership so steering a
completed run refuses instead of journaling into a terminal chain, and
replace free transport text in control.failed payloads with a bounded
error class plus digest reference.

Salvaged from run 20260801-191101-6612a08a, which produced a complete
implementation before its worker timed out at 1500s and its worktree was
pruned (see #656).
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • coderabbit-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 340d4c89-4b40-47d4-855d-ce9ad225a1c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@solomonneas solomonneas self-assigned this Aug 1, 2026
@solomonneas

Copy link
Copy Markdown
Collaborator Author

Grader: APPROVE, merging.

Verified independently rather than trusting the green suite, since I assembled this branch from two salvaged changes.patch artifacts and the tests share provenance with the implementation:

  1. The race test is honest. Disabling fcntl.flock in a scratch copy makes test_append_event_fcntl_lock_serializes_two_subprocess_writers FAIL at test_run_journal.py:1990; restoring it makes the test pass. It spawns two real subprocess children synchronized through a filesystem barrier so both pass the stale check on the same sequence, across multiple rounds. It is not a vacuous pass.
  2. Every append path takes the cross-process lock. Four call sites of _append_critical_section: append_event, lookup_idempotent_event, recover_partial_tail, and journal_mutation. Redaction routes through journal_mutation (run_redaction.py:1424, :2071) and the control CLI through run_control_journal.py:305. recover_partial_tail is now covered, closing an open Minor from the slice 1-6 review.
  3. Full gate green: receipt 20260801-200416-work-verify-135e03, 5,675 passed, 3 skipped, exit 0. CI fully green.

This closes the Critical introduced by #641. Live steer/interrupt on journal-backed runs is safe again.

…-journal-lock

# Conflicts:
#	src/brigade/run_redaction.py
@solomonneas
solomonneas merged commit 9da6cf0 into main Aug 1, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

run-control: cross-process journal appends race the owner and can fork the chain (urgent fix-forward for #641)

1 participant