fix(server): cancel pending questions on stop - #7987
Conversation
github-actions
Bot
added
vouch:unvouched
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: b6e1edf330
ℹ️ 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".
| yield* Effect.forEach(pendingUserInputRequests(thread.activities), ({ requestId, turnId }) => | ||
| appendCancelledUserInputActivity({ |
There was a problem hiding this comment.
This cleanup runs only for thread.turn-interrupt-requested. When thread.session.stop is dispatched directly—such as archive/settle cleanup or the branch selector—the event instead reaches processSessionStopRequested, which stops the provider and marks the session stopped without appending any user-input.resolved activity. If the session was blocked on a question, reloading or later reopening the thread therefore still restores the now-unanswerable prompt. Apply the same durable cancellation cleanup after a successful session stop.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6e1edf. Configure here.
| turnId, | ||
| createdAt: event.payload.createdAt, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Stale snapshot double-cancels
Low Severity
pendingUserInputRequests runs against the thread snapshot taken before interruptTurn. Claude and Cursor already settle open prompts and emit user-input.resolved during interrupt, so the reactor still appends a second user-input.resolved with cancelled: true for requests that are no longer open.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b6e1edf. Configure here.
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. Not approved because:
|
b6e1edf to
37eaf59
Compare
The stop test now verifies an already-answered question and one whose response failed as stale are left alone, a question the provider resolves during the interrupt keeps its real resolution without a cancelled duplicate on top, and a still-open question gets a durable cancelled resolution stamped after the request even when the interrupt command predates it. Dispatches run in one Effect.gen so the file stays under the manual-effect-runner lint baseline.
37eaf59 to
de3647d
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| // while handling the interrupt. Cancellations get server-side timestamps | ||
| // so they order after the request activity even when the interrupt | ||
| // command predates it. | ||
| const postInterruptThread = yield* resolveThread(event.payload.threadId); |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderCommandReactor.ts:1276
The interrupt path can append a user-input.resolved cancellation before the provider's real resolution, leaving a false cancellation and duplicate resolution in the durable activity log. resolveThread at this point may run before the callback fiber resumes and ProviderRuntimeIngestion projects the resolution; coordinate with callback completion or make cancellation resolution idempotent.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 1276:
The interrupt path can append a `user-input.resolved` cancellation before the provider's real resolution, leaving a false cancellation and duplicate resolution in the durable activity log. `resolveThread` at this point may run before the callback fiber resumes and `ProviderRuntimeIngestion` projects the resolution; coordinate with callback completion or make cancellation resolution idempotent.
| activities: OrchestrationThread["activities"], | ||
| ): ReadonlyArray<{ readonly requestId: string; readonly turnId: TurnId | null }> { | ||
| const openRequests = new Map<string, TurnId | null>(); | ||
| const ordered = [...activities].toSorted( |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderCommandReactor.ts:288
When user-input.requested and user-input.resolved share a createdAt timestamp, an ID ordering where the resolution sorts first causes pendingUserInputRequests to delete the request and then reopen it. The function therefore returns an already-resolved question as pending, so an interrupt appends a false cancellation. Use a durable append sequence/order for equal-timestamp lifecycle events instead of activity.id.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 288:
When `user-input.requested` and `user-input.resolved` share a `createdAt` timestamp, an ID ordering where the resolution sorts first causes `pendingUserInputRequests` to delete the request and then reopen it. The function therefore returns an already-resolved question as pending, so an interrupt appends a false cancellation. Use a durable append sequence/order for equal-timestamp lifecycle events instead of `activity.id`.
1 participant
Problem
Stopping a turn that is blocked on a user question left the question prompt stuck on screen. The UI cleared it optimistically, but nothing durable recorded the resolution, so a reload or another client brought the dead prompt back and any answer to it failed as stale.
Fix
After a successful provider interrupt, the ProviderCommandReactor re-reads the thread's activity log and derives the still-open user-input requests from that fresh snapshot (honoring prior resolutions and stale-response failures), so a resolution the provider appends while handling the interrupt is never followed by a false cancelled duplicate. Each durable
user-input.resolvedactivity markedcancelled: trueis stamped with server-side time rather than the stop command's timestamp, so it orders after the request activity even when the stop command carries an older one. The pending-UI derivation in every client then clears the prompt from the event stream itself, with no client-side special casing.The reactor test covers an open request being cancelled despite an interrupt timestamp older than the request, a question the provider resolved during the interrupt keeping its real resolution untouched, and already-answered or stale-failed requests staying that way.
Note
Medium Risk
Touches the turn-interrupt path and writes new orchestration activities. Wrong pending-request derivation could cancel live questions or leave dead prompts in the log.
Overview
Stopping a turn that is waiting on a user question now records a durable cancellation so the prompt does not come back after reload or on another client.
After a successful provider interrupt,
ProviderCommandReactorwalks the thread activity log, finds still-open user-input requests (skipping already-resolved ones and stale-response failures), and appendsuser-input.resolvedactivities withcancelled: true. Clients already treat that kind as closed, so the prompt clears from the event stream without extra UI logic.Reviewed by Cursor Bugbot for commit b6e1edf. Configure here.
Note
Cancel pending user-input requests on
thread.turn.interruptinProviderCommandReactorthread.turn.interrupt-requested, after callingproviderService.interruptTurn, the reactor now appendsuser-input.resolvedactivities with{cancelled: true}for any still-open user-input requests inferred from the activity log.pendingUserInputRequeststo scan sorted thread activities and compute open request IDs, closing them on resolution or whenprovider.user-input.respond.failedmarks the request as stale or unknown.appendCancelledUserInputActivityto append the cancellation activity with summary "User input cancelled", preserving the originalturnIdand using the interrupt event'screatedAt.user-input.resolvedactivities in ProviderCommandReactor.ts should handle the new{cancelled: true}payload.📊 Macroscope summarized de3647d. 1 file reviewed, 2 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues