Post-merge grading review of #641 (PR for #604) found one Critical and two Major defects in the control-journaling half. The cursor codec and runs events reader are sound and stay. Until this fix lands, treat live runs steer/interrupt against journal-backed runs as unsafe.
Critical: cross-process journal appends have no mutual exclusion
run_journal._append_critical_section is a threading.Lock plus signal mask (run_journal.py:75, 114-126) - process-local. #641 makes runs steer/interrupt append control.* events from an external CLI process (run_control_journal.py:220-266) while the run owner appends dispatch facts and transitions mid-run (run_lifecycle.py:434-441, 642-649, 756-763, with no retry on StaleSequenceError). Failure modes:
- Both processes read tail sequence N and both append N+1 to the O_APPEND file: forked hash chain. Every subsequent read reports chain errors and a journal-authoritative run is permanently fail-closed. No recovery path exists for a forked chain (recover_partial_tail handles partial lines only).
- The CLI append lands between the owner's read and append: the owner's lifecycle write dies with LifecycleJournalError, un-retried.
interrupt co-schedules the two writers by design (CLI appends control.observed at the same moment the owner reacts to the aborted turn).
- Same race against redaction: an external append between redaction's _verified_events read and _replace_journal (run_redaction.py:1430, 1492) silently drops the control event or forks the chain.
The docstring claim at run_control_journal.py:229-233 that checkpoint_event_pair prevents pair-splitting is false across processes; run_lifecycle.py:154-166 documents it as process-level.
Fix: an fcntl lock file beside the journal, taken inside _append_critical_section by EVERY append path (owner, control CLI, redaction, recovery), plus owner-side retry on stale sequence. If this cannot land promptly, revert the control-journaling half of #641 and keep the read-only cursor/events half.
Major: no status or ownership guard on control journaling
execute_control_request checks only journal_present (run_control_journal.py:312-313) and control_transport_from_run reads persisted metadata that survives completion (run_control.py:436-454). runs steer against a completed run appends control.requested + control.failed to a terminal authoritative journal and rewrites shadow evidence as a non-owner (run_control_journal.py:206-217). Guard on run status and lock ownership before any control append.
Major: unfiltered transport/provider text in control.failed payloads
detail is filled from str(exc) on transport exceptions (run_control_journal.py:363-369) and verbatim from the app-server response["error"] (:423-427), truncated to 512 chars; the allowlist legitimizes it (run_events.py:124). #604's criterion was bounded metadata and digests, not provider bodies. Replace with a bounded error-class enum plus digest reference, matching the established payload discipline.
Also fold in (Minor, same surface)
- Concurrent same-request-id callers both see "absent" and both invoke the transport; compare the committed event against the submission to detect the idempotent replay.
runs events stops at the first terminal event, making post-terminal events (run.redaction.recorded, control.*) unreachable through the cursor reader.
- Follow mode never revalidates chain continuity between polls; a redaction rewrite mid-follow emits a suffix that does not chain onto previously emitted events.
Regression tests must include a real two-process append race (fork or subprocess, not monkeypatch) proving the lock serializes writers, and a steer-against-completed-run refusal.
From the post-merge grading review of PR #641. Refs #604.
Post-merge grading review of #641 (PR for #604) found one Critical and two Major defects in the control-journaling half. The cursor codec and
runs eventsreader are sound and stay. Until this fix lands, treat liveruns steer/interruptagainst journal-backed runs as unsafe.Critical: cross-process journal appends have no mutual exclusion
run_journal._append_critical_sectionis athreading.Lockplus signal mask (run_journal.py:75, 114-126) - process-local. #641 makesruns steer/interruptappendcontrol.*events from an external CLI process (run_control_journal.py:220-266) while the run owner appends dispatch facts and transitions mid-run (run_lifecycle.py:434-441, 642-649, 756-763, with no retry on StaleSequenceError). Failure modes:interruptco-schedules the two writers by design (CLI appends control.observed at the same moment the owner reacts to the aborted turn).The docstring claim at run_control_journal.py:229-233 that checkpoint_event_pair prevents pair-splitting is false across processes; run_lifecycle.py:154-166 documents it as process-level.
Fix: an fcntl lock file beside the journal, taken inside _append_critical_section by EVERY append path (owner, control CLI, redaction, recovery), plus owner-side retry on stale sequence. If this cannot land promptly, revert the control-journaling half of #641 and keep the read-only cursor/events half.
Major: no status or ownership guard on control journaling
execute_control_request checks only journal_present (run_control_journal.py:312-313) and control_transport_from_run reads persisted metadata that survives completion (run_control.py:436-454).
runs steeragainst a completed run appends control.requested + control.failed to a terminal authoritative journal and rewrites shadow evidence as a non-owner (run_control_journal.py:206-217). Guard on run status and lock ownership before any control append.Major: unfiltered transport/provider text in control.failed payloads
detail is filled from str(exc) on transport exceptions (run_control_journal.py:363-369) and verbatim from the app-server response["error"] (:423-427), truncated to 512 chars; the allowlist legitimizes it (run_events.py:124). #604's criterion was bounded metadata and digests, not provider bodies. Replace with a bounded error-class enum plus digest reference, matching the established payload discipline.
Also fold in (Minor, same surface)
runs eventsstops at the first terminal event, making post-terminal events (run.redaction.recorded, control.*) unreachable through the cursor reader.Regression tests must include a real two-process append race (fork or subprocess, not monkeypatch) proving the lock serializes writers, and a steer-against-completed-run refusal.
From the post-merge grading review of PR #641. Refs #604.