fix(acp): recover when compact is interrupted - #275
Conversation
![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: 7f04747b86
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd5e30c20
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| isGoalActive, | ||
| canPauseGoal, | ||
| }: CanStopAgentOptions): boolean => | ||
| (isContextCompacting && activeAssistantTurnId != null) || |
There was a problem hiding this comment.
After a daemon restart or interrupted stream leaves both an in-progress compaction marker and an unfinished assistant entry in durable history, this condition still exposes Stop even though nothing is cancellable. Fresh evidence beyond the prior comment is that resolveActiveAssistantTurnId accepts any history entry lacking finished/endedAt, whereas cancelSession requires that ID in its in-memory active/execution maps and otherwise returns success without updating history; consequently, clicking Stop silently no-ops and the button remains permanently visible. Base this branch on a live cancellability signal, or finalize the stale history when cancellation finds no matching runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. The fix gates the compaction Stop branch on a non-null activeAssistantTurnId, but as noted that ID is history-derived and may not be live-cancellable after a daemon restart or interrupted stream. Finalizing stale history at cancellation time would require resolving the user turn ID in cancelSession and routing it through markDispatchCancelled, which is a backend change beyond this PR's frontend scope. Happy to take it on as a follow-up if the maintainers agree the live-cancellability signal or stale-finalization path is the right direction.
ccd5e30 to
1c4f120
Compare
93c540d to
1fc4d01
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fc4d0143c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.deps.logger.debug( | ||
| `[${sessionId}] Finalizing stale unfinished turn ${turnId} after stop request found no live runtime` | ||
| ); | ||
| this.deps.clearSessionActivePresence(sessionId); |
There was a problem hiding this comment.
When a delayed or multi-client cancellation for an old compaction arrives after a newer turn has started, this branch is entered because the active/execution IDs do not match the requested old turn, but it still clears the session-wide presence and finalizeCancelledTurn sets the whole session to idle. SessionActivePresenceController.setPhase is a no-op after clear, so the newer turn remains invisible and loses its Stop control and heartbeat for the rest of its execution. Only perform these session-wide cleanup operations when there is no current turn; otherwise restrict the repair to the stale history entry.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L107-L114
Useful? React with 👍 / 👎.
1fc4d01 to
694e6c0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 694e6c0264
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await this.finalizeCancelledTurn({ | ||
| sessionId, | ||
| sessionDoc, | ||
| turnId, | ||
| reportTurnError: false, |
There was a problem hiding this comment.
When this stale compaction coexists with a newer pending user turn that has not yet acquired a runtime, currentTurnId is still null, so this calls finalizeCancelledTurn without the stale turn's userTurnId. Its markDispatchCancelled fallback then selects existingMeta.latestUserMsgId, marks that unrelated newer turn canceled, and advances lastHandledUserMsgId, causing the newly submitted prompt to be skipped. Restrict this path to repairing the stale assistant entry and clearing its cancel request rather than invoking a finalizer that infers ownership from current dispatch metadata.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L152-L157
Useful? React with 👍 / 👎.
|
I pushed a follow-up that keeps the compaction Stop control tied to a live cancellable turn and adds a recovery path for stale history. When a compaction marker and unfinished assistant entry survive without a live runtime, cancellation now marks the compaction failed, finalizes the assistant entry, clears the session state, and leaves newer turns untouched. The adapter dependency was also rebased onto its current main while retaining the non-turn prompt cancellation fix. Focused tests passed, and the PR is mergeable again. |
|
I pushed 59174ce to fix the failing stale-turn path. Durable history is now inspected only when no live turn owns the session, and an in-memory runtime also counts as current ownership, so a stale stop request cannot call |
github-actions
Bot
added
status:needs-pr-attention
59174ce to
dfa1a55
Compare
github-actions
Bot
added
the
status:needs-pr-attention
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa1a551e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (currentTurnId == null) { | ||
| const history = await sessionDoc.getHistory(); |
There was a problem hiding this comment.
When a new turn starts while getHistory() is awaited, this pre-await currentTurnId == null snapshot remains true, so the stale-repair branch can clear the new turn's presence and later set its session status to idle or cancel its dispatch metadata. Fresh evidence after the prior ownership finding is that this added guard still does not acquire tryAcquireSessionRewriteConflictLease, although new-turn creation does, leaving the check and session-wide cleanup non-atomic; hold that lease or recheck live ownership immediately before cleanup.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
github-actions
Bot
removed
the
status:needs-pr-attention
1fac12e to
9d8b080
Compare
|
I rebased the branch onto current main to clear the merge conflicts, and pushed a follow-up (9d8b080) that closes the remaining race in the stale-repair path: the session rewrite conflict lease is now held across the awaited |
|
@UniversePeak please fix the tests |
Model: gpt-5.6-luna
Chatgpt-codex-connector[bot] P2 review (2026-09-01T16:05:21Z) on session-chat-interface.tsx:3348: when a pending/in-progress compaction marker remains in history but its assistant entry is already finished (restart, interrupted notification stream), isContextCompacting stays true while activeAssistantTurnId is null. The previous canStopAgent branch showed Stop in that state, but handleStop rejects the click as missing_active_turn, leaving an idle session with a permanently nonfunctional Stop button. Gate the compaction case on a cancellable assistant turn by extracting canStopAgentEnabled into session-context-compaction.ts and adding regression tests. Model: gpt-5.6-luna
Model: gpt-5.6-luna
Model: gpt-5.6-luna
Model: gpt-5.6-luna
Holding the conflict lease across the awaited getHistory() read closes the race where a newer turn starts while stale repair is mid-await: dispatch defers on the busy lease, and the recheck after acquisition keeps the newer turn's presence and dispatch metadata untouched. Model: glm-5.3-flash
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4890479ed1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await sessionDoc.updateHistory((nextHistory) => { | ||
| for (const entry of nextHistory) { | ||
| if (entry.id !== turnId) continue; | ||
| entry.finished = true; |
There was a problem hiding this comment.
When the stale compaction belongs to an Operation target turn, this history write publishes a terminal assistant entry before finalizeCancelledTurn records the canceled user status. LodyOperationCoordinator checks for cancellation first but then treats any terminal assistant as succeeded (apps/cli/src/orchestration/operation-coordinator.ts:538-547), so its history watcher can durably finish the operation as successful during this window, and the later cancellation cannot undo that completion. Preserve the existing cancellation-first ordering by recording the owning user turn's cancellation before setting finished/endedAt, or make the updates atomic.
Useful? React with 👍 / 👎.
4890479 to
546fb73
Compare
|
The failing test came from upstream, not from this PR's changes: #609 replaced I rebased the branch onto current main, which brings in the rewritten sparkle-packaging test from #539 that no longer depends on the workflow file. On the rebased head (546fb73), the sparkle-packaging suite passes (5 tests), and the focused |
|
@Leeeon233 Tests are fixed in 4890479. The |
3 participants
Related issue
Closes #267
Problem / pressure
Stopping
/compactwhile Codex is showing the context-compaction activity leaves the ACP prompt waiting forthread/compacted. Because compaction is a non-turn command, the existing cancellation path cannot find a turn to interrupt, so the prompt remains active and later prompts fail withA Codex prompt is already active.Summary
cancelledand release its active-prompt guard./compactbefore the provider's terminal compaction notification arrives and verifies a late notification is harmless.The ACP adapter portion is contributed in the companion change LodyAI/acp-extension-codex#30, and this PR updates the repository submodule pointer to that tested revision.
Visual explanation
The change preserves ordinary turn interruption while adding an ownership-aware recovery path for non-turn compaction and stale durable history:
sequenceDiagram participant User participant SessionUI participant ExecutionService participant ACP participant History User->>SessionUI: Stop during /compact SessionUI->>ExecutionService: cancelSession(sessionId) alt live compaction owner exists ExecutionService->>ACP: cancel owning non-turn prompt ACP-->>ExecutionService: cancelled ExecutionService-->>SessionUI: clear active compaction state else no live owner, stale compaction history exists ExecutionService->>History: finalize stale assistant entry as failed ExecutionService-->>SessionUI: clear stale state only end ACP-->>ExecutionService: late thread/compacted (optional) Note over ExecutionService: ignore safely; never clear a newer turnA newer live or queued turn remains authoritative: stale cleanup repairs only the old assistant entry and does not finalize, cancel, or hide the newer turn.
Before / after
/compacthas no turn id to interrupt, so the ACP prompt stays active and future prompts are rejected.Test plan
corepack pnpm --filter acp-extension-core build— passed.corepack pnpm --filter acp-extension-codex typecheck— passed.corepack pnpm --filter acp-extension-codex exec vitest run src/__tests__/CodexACPAgent/CodexAcpClient.test.ts -t "cancels a compact slash command without waiting for compaction completion" --no-file-parallelism— passed.corepack pnpm --filter @lody/components exec vitest run tests/session-context-compaction.test.ts— 2 tests passed.corepack pnpm --filter @lody/components typecheck— passed.git diff --check— passed.Context handoff
Instructions for reviewing agents
packages/acp-extension-codex/src/CodexAcpServer.tscancellation handling andCodexCommands.tscompaction lifecycle callbacks, then verify the parent submodule pointer and session UI stop gate.thread/compact/startoperation, and that a latethread/compactednotification remains harmless.Authoring context
Original user prompt
Show original prompt