Fix terminal run publication atomicity - #1070
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8161dd4435
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| journal.dispatch(delivery) | ||
| if terminalCommit != nil { | ||
| terminalCommit() |
There was a problem hiding this comment.
When terminal persistence or recorder draining is slow, prepareLocked has already set state.terminated, but the status remains active until this delayed commit. During that window CancelRun still accepts the run based solely on its non-terminal status and calls killBackgroundJobsForRun, so cancelling a run whose completed event has already won can destroy intentionally detached jobs even though the run ultimately reports completed. Status-gated control paths should also reject a sealed/transitioning run.
Useful? React with 👍 / 👎.
8161dd4 to
78d07ad
Compare
|
Rebased cleanly onto current |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d07ada77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if r.terminalBeforeDispatchHook != nil { | ||
| r.terminalBeforeDispatchHook(runID, eventType) | ||
| } | ||
| journal.dispatch(delivery) |
There was a problem hiding this comment.
When RolloutDir is enabled and the terminal recorder is slow or stalled, this dispatch call holds the runner-wide conversationEventMu while waiting up to 30 seconds to send and another 30 seconds to drain. Every unrelated emit and SubscribeConversationFrom also needs that mutex, so one run's terminal filesystem delay can freeze event publication and conversation SSE registration across the entire runner; previously terminal recorder dispatch happened after releasing this lock. Preserve the same-conversation persistence/fanout ordering without keeping the global journal mutex held through the bounded recorder waits.
Useful? React with 👍 / 👎.
78d07ad to
5f106be
Compare
# Conflicts: # docs/plans/active-plan.md # internal/harness/runner.go # internal/harness/runner_event_journal.go
1 participant
Closes #1067
Summary
Makes terminal run publication one-way consistent: whenever
GetRunexposescompleted,failed, orcancelled, the matching terminal event is alreadyreplayable. Terminal event persistence, conversation replay, recorder drain,
final status persistence, in-memory commit, and fanout now use one serialized
transition path.
The reviewed failure policy is explicit:
completes bounded live publication;
and protects it from pruning;
Start/Continue admission with HTTP 503
terminal_durability_unavailable;store I/O under runner, status, journal, or conversation locks;
StorageModeNoneremains the intentional event-suppression exception andno-store runs do not activate durable backpressure.
Current-main semantic integration
This head semantically integrates the complete #1067 transition series with
current main's #1054 waiting/pending publication lifecycle. Both now share one
context-aware per-run status lock.
Frontier review caught and repaired an initial merge regression: nonterminal
status persistence had been moved ahead of the live commit, so a failed
best-effort
UpdateRuncould leave a real AskUser question registered whileAPI, TUI, and GUI still saw
running. Nonterminal state now commits live first,retains the bounded store attempt, and returns failure so strict
waiting_for_userstatus/event publication can retry. Terminal publicationretains event-before-status ordering.
Tests and evidence
Final integrated head:
b45b4334aea593704581ed5c0b38a95b942e3193,based on
origin/maine6e255d8ba60d1ae3035031f1342ae22b0c5d74d.TestNonTerminalStatusPersistenceFailureKeepsLiveStateMoving.-count=100— PASS.
internal/checkpoints,internal/harness, andinternal/servernormal and race suites — PASS.
go veton affected packages — PASS.TMPDIR=/private/tmp GOCACHE=/private/tmp/gocode-go-cache ./scripts/test-regression.sh— PASS: repository normal, full race, coverage 85.7%, minimum 80.0%, zero
uncovered functions.
focused harness normal
-count=20and race-count=10also passed.Full retained #1067 regressions cover terminal status/replay ordering,
append/update failure and timeouts, recorder drain, redaction suppression,
same-conversation ordering, delayed nonterminal transitions, bounded degraded
admission, retry recovery, HTTP 503 behavior, retention/pruning, and concurrent
continuation reservations.
Hosted gate
Both hosted gates passed on exact head
b45b4334aea593704581ed5c0b38a95b942e3193:test-fastrun30672325842— PASS.test-racerun30672325850— PASS.Scope and rollout
No route schema, event type, persistence schema, cron/callback behavior, TUI,
or macOS code changes. Roll out as an ordinary daemon update. Monitor store
append/update failures and
terminal_durability_unavailable. Roll back ifterminal fanout deadlocks, healthy stores spuriously reject admission,
status/replay ordering regresses, or retention grows without bound. During a
real store outage, stop producers and preserve the process before rollback so
truthful in-memory terminal state is not lost.
Checklist