fix: stop native Codex compaction on cancellation - #618
Conversation
Model: gpt-5
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. |
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.
Retry reconciliation after the active turn disappears
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.
Keep reconciliation off the fast control lane
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.
Requeue goal turns after releasing the rewrite barrier
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.
Retry active results when the same daemon releases the turn
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.
Recheck live ownership after the awaited metadata read
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
Related issue
Follow-up to #573.
Problem / pressure
Stop during manual Codex
/compactcould return an ACP cancellation while native compression kept running. The adapter treated the empty start acknowledgement as evidence that compaction had no interruptible turn. Codex 0.153.4 actually emits the ordinary native turn lifecycle.Summary
Update the Codex adapter to capture the compaction turn, interrupt it on Stop, and retain prompt ownership until native completion or connection closure. The existing CLI finalization then persists unresolved compaction as failed before releasing execution ownership.
Adapter implementation: LodyAI/acp-extension-codex#41 (merge dependency). The submodule points to its published commit.
The final PR contains the adapter submodule update, the draft history-write contract, and its owning Agent Note. Session rendering and RPC transports match the base branch; opening a conversation does not repair historical data.
Visual explanation
Before / after
Test plan
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
CodexAppServerClient.tsand cancellation ownership inCodexAcpServer.ts.Authoring context