fix: retain session ownership until compaction cancellation completes - #618
Conversation
Model: gpt-5
github-actions
Bot
added
scope: cli
scope: shared
scope: components
scope: streams-rpc
labels
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8a48169ada
ℹ️ 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".
| .then((result) => { | ||
| if (!result || result.outcome === 'unknown') { | ||
| compactionReconciliationAttemptsRef.current.delete(attemptKey); | ||
| } |
There was a problem hiding this comment.
When reconciliation returns active and that daemon subsequently exits before persisting the compaction's terminal update, this attempt key remains in the set permanently. The offline/online transition reruns the effect, but line 2717 suppresses the request to the replacement daemon, leaving the durable pending/in_progress item and its spinner stuck until the component is unmounted and reopened. Clear or invalidate active attempts when live ownership changes so restart recovery can actually reconcile the stale turn.
AGENTS.md reference: AGENTS.md:L82-L87
Useful? React with 👍 / 👎.
Model: gpt-5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a357b744e4
ℹ️ 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".
| 'machine/acp-capabilities-refresh-cancel', | ||
| 'session/cancel', | ||
| 'session/live-status', | ||
| 'session/reconcile-context-compaction', |
There was a problem hiding this comment.
When four stale sessions are mounted together, their reconciliation requests can occupy every slot in the four-request control pool: unlike the other fast handlers, each one awaits document acquisition, a history write, and waitUntilSynced() before returning. That queues session/cancel, session/dispatch-turn, status, and lifecycle requests behind potentially multi-second synchronization; because expiry is checked only after acquiring the lane, those user-control requests can time out or be discarded. Run this repair on the ordinary lane or acknowledge it before performing the durable work.
AGENTS.md reference: packages/loro-streams-rpc/AGENTS.md:L48-L55
Useful? React with 👍 / 👎.
Model: gpt-5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e2680aedd
ℹ️ 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".
| return response('unknown', { error: 'Session history is not synced with its owner.' }); | ||
| } | ||
|
|
||
| const releaseBarrier = this.executionService.tryAcquireSessionRewriteBarrier(args.sessionId); |
There was a problem hiding this comment.
When a set or resume goal request arrives after this barrier is acquired but before it is released, queueGoalTurn immediately tries continueSession; runVisibleSessionTurn returns rewrite-barrier, leaving claimed false, and lines 1304–1317 of session-execution-service.ts then delete the accepted pending goal and record a pre-prompt failure. The finally block only wakes SessionDispatchWatcher, which does not own pendingGoalTurnBySession, so the promised Lody-owned goal prompt never runs; explicitly retry or wake queued goal work when reconciliation releases the barrier.
AGENTS.md reference: packages/shared/AGENTS.md:L78-L83
Useful? React with 👍 / 👎.
Model: gpt-5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fe6689323
ℹ️ 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".
| .then((result) => { | ||
| if (!isDurableContextCompactionReconciliation(result)) { | ||
| compactionReconciliationAttemptsRef.current.delete(attemptKey); | ||
| } |
There was a problem hiding this comment.
When this request returns active during the interval between host finalization and provider termination, deleting the attempt key does not itself rerender the component or rerun this effect. If the same daemon subsequently releases the turn without persisting a terminal compaction update, neither history, connectivity, nor the machine instance ID changes, so the stale spinner remains indefinitely. The new owner-instance key only fixes daemon replacement; fresh evidence in this revision is that session live-presence/ownership state is still absent from the effect dependencies. Trigger a retry when same-daemon session activity ends.
Useful? React with 👍 / 👎.
Model: gpt-5
Model: gpt-5
Model: gpt-5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5faa4d9f27
ℹ️ 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".
| return response('retry', { error: 'The daemon still has live Session work.' }); | ||
| } | ||
|
|
||
| if (!(await verifyOwnership())) { |
There was a problem hiding this comment.
When a durable activation is recovered while reconciliation is in flight, hasLiveSessionWork() can return false and then this awaited ownership lookup yields while the control-lane dispatch registers the recovered turn. Recovery reuses the deterministic assistant:<userTurnId> entry, so the following targeted update can mark the compaction failed in that now-active same turn. Recheck live work after the final await, or serialize this mutation with turn acquisition, before changing history.
AGENTS.md reference: apps/cli/src/lib/acp/AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
Model: gpt-5
wibus-wee
changed the title
github-actions
Bot
added
scope: agent-runtime
and removed
scope: cli
scope: shared
scope: components
scope: streams-rpc
labels
wibus-wee
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b43c948f
ℹ️ 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".
| } | ||
| const ownedPromptRun = runtime.activePromptRun; | ||
| if (!ownedPromptRun || ownedPromptRun.turnId !== runtime.turnId) { | ||
| if (runtime.cancelRequested || !ownedPromptRun || ownedPromptRun.turnId !== runtime.turnId) { |
There was a problem hiding this comment.
When Stop arrives after this guard but while await steerRun.applied is pending, the new Stop path deliberately keeps the runtime alive, and the post-ack ownership check does not examine cancelRequested. A late successful acknowledgement therefore settles the stopped source turn as handled, transfers runtime and dispatch ownership to the steer, and then lets cancellation finalization cancel that new turn instead. Serialize Stop with steer acceptance or reject the ownership transfer when cancellation won this interval.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L60
Useful? React with 👍 / 👎.
Keep the merged session guidelines below the documentation size limit without changing their contracts. Model: gpt-6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae24a7235b
ℹ️ 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".
| // Provider acceptance forbids replay; Stop keeps the source cancellation owner. | ||
| return reject( | ||
| 'stale-turn', | ||
| 'Steer application arrived after cancellation or ownership changed' | ||
| ); |
There was a problem hiding this comment.
When an acknowledged steer has already been submitted and Stop sets cancelRequested before its acceptance resolves, this branch returns stale-turn without changing the steer entry from pending_apply. The renderer promotes only no-active-turn responses, while dispatch deliberately skips pending_apply entries without a matching activation pointer, so this accepted-but-cancelled message remains permanently nonterminal and noneditable after the source turn releases. Mark the steer as cancelled without requeueing it or transferring ownership.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L60
Useful? React with 👍 / 👎.
1 participant
Related issue
Follow-up to #573.
Problem / pressure
Stop during manual Codex
/compactmust keep both the adapter prompt and Lody's execution owner until native termination. The adapter previously cancelled only its local wait. Lody also interrupted its owner fiber immediately, entering history finalization before ACP completed.Lody's existing finalizer already retained the runtime through a raw-request drain; this was not unconditional early owner release. Normal Stop now keeps the owner in the live prompt while retaining #571's five-second termination fallback for an unresponsive provider.
Summary
Update the Codex adapter to capture the compaction turn, interrupt it on Stop, and retain prompt ownership until native completion or connection closure. In Lody, Stop during an in-flight prompt with a ready ACP session sets the existing cancellation flag and sends provider cancel without interrupting the owner fiber. Cancellation history finalization then runs after ACP returns, before releasing execution ownership.
Adapter implementation: LodyAI/acp-extension-codex#41 (merge dependency). The submodule points to its published commit.
The active Stop branch sends provider cancel without interrupting its fiber, then starts the existing five-second raw-request drain. A pending prompt at the deadline triggers termination of the old session; connection closure lets the prompt reject naturally. Failed termination retains ownership until ACP ends. Stop and the cancellation finalizer share one drain promise, so repeated Stop cannot reset the deadline or terminate twice.
Steer checks cancellation both before submission and after its acceptance ACK. If Stop wins while that ACK is pending, the accepted steer cannot change the source invocation, settle it as handled, or take ownership. Lody marks that exact steer user turn
canceledwithout changing dispatch pointers, then returnsstale-turn; neither daemon nor client requeues it. The source remains the cancellation owner through provider terminal. Only a provably undelivered steer becomes a pending ordinary turn. No retry, reconciliation protocol or new barrier is introduced.Visual explanation
sequenceDiagram participant Host as Lody participant ACP as Codex adapter participant Native as Codex app-server Host->>ACP: prompt /compact ACP->>Native: thread/compact/start Native-->>ACP: turn/started Host->>ACP: Stop Note over Host: Retain owner fiber and unfinished history ACP->>Native: turn/interrupt Native-->>ACP: interrupt ACK Note over ACP: Retain active prompt alt Native terminal within five seconds Native-->>ACP: turn/completed (interrupted) ACP-->>Host: prompt cancelled else Raw prompt still pending Host->>ACP: terminate old session Note over Host,ACP: Failed termination keeps waiting; no owner release ACP-->>Host: connection close rejects prompt end Host->>Host: persist terminal history, release executionBefore / after
Test plan
/compact, Stop, a pending next message, then provider terminal. The steer race explicitly submits steer, withholds its applied ACK, sends Stop, and resolves acceptance: the steer becomescanceled, source history remains unfinished, invocation and dispatch ownership stay with the source, and no handled settlement or replay occurs. Provider terminal cancels the source before release; the steer remains terminal. The steer status assertion failed onae24a723, where it remainedpending_apply. Fake timers also cover the five-second termination deadline, withheld cancel ACK, repeated Stop without deadline reset, and failed termination retaining ownership. Existing external-interruption and steer admission regressions remain covered.interruptedprecedes ACPcancelled.pnpm checkpasses all preceding workspace suites, then Electron reports 103/104 passing: the relay suite cannot import Electron because the local binary is not installed.Context handoff
Instructions for reviewing agents
canceledwithout handoff, dispatch pointer changes or replay; the source must retain its cancellation outcome.Authoring context