fix(grok): Harden ACP resume with replay-idle load readiness (#3156)#129
Conversation
Co-authored-by: Julius Marminge <julius@mac.lan>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…gg#3156) Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44d905a6dd
ℹ️ 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 runningTurnId = thread.session?.status === "running" ? thread.session.activeTurnId : null; | ||
| return { | ||
| threadId: thread.id, | ||
| ...(runningTurnId !== null ? { turnId: runningTurnId } : {}), |
There was a problem hiding this comment.
Forward the scoped turn id past the client command
On the normal ChatView path this turnId never reaches the provider: interruptThreadTurn dispatches thread.turn.interrupt, but ProviderCommandReactor.processTurnInterruptRequested still calls providerService.interruptTurn({ threadId }) and drops event.payload.turnId. In the scenario this change is trying to guard—Stop for turn A being processed after turn B has become active—the Grok adapter receives an unscoped interrupt and can cancel B anyway, so please forward the payload turn id through that reactor.
Useful? React with 👍 / 👎.
| yield* acp.agent | ||
| .cancel({ sessionId: started.sessionId }) | ||
| .pipe(Effect.ignore, Effect.forkIn(runtimeScope)); |
There was a problem hiding this comment.
Await cancel before releasing the next prompt
Here cancel interrupts the local prompt fiber and then forks/ignores session/cancel, so the effect can complete before the cancel notification is written. When a user stops a hung prompt and immediately sends a follow-up, the next session/prompt can be enqueued before the delayed cancel; ACP cancellation is session-scoped, so that late cancel can hit the follow-up or leave the original provider run alive. Please await or serialize the cancel send before allowing another prompt.
Useful? React with 👍 / 👎.
| prompt: (payload) => | ||
| getStartedState.pipe( | ||
| Effect.flatMap((started) => { | ||
| promptSerializationSemaphore.withPermit( |
There was a problem hiding this comment.
Preserve concurrent Grok steering prompts
This semaphore serializes every ACP session/prompt, but GrokAdapter still treats a second sendTurn while ctx.promptsInFlight > 0 as a steer that should be folded into the active Grok run. With a long-running or hung first prompt, the follow-up steer now waits for that first prompt to finish instead of being delivered during the run, so users cannot steer an active Grok turn and the request can hang indefinitely until Stop is used.
Useful? React with 👍 / 👎.
| if (Option.isSome(gate) && gate.value.active) { | ||
| const lastActivityAtMillis = yield* Clock.currentTimeMillis; |
There was a problem hiding this comment.
Scope replay-idle readiness to the loaded session
While session/load is gated, this branch treats any session/update as replay progress before checking notification.sessionId below. If Grok emits an update for a child/foreign session while the requested resume session is still pending, the idle timer can synthesize a successful load for the wrong/stale session and also drops that foreign update; the gate should only advance for updates belonging to the session being loaded.
Useful? React with 👍 / 👎.
Co-authored-by: Julius Marminge <julius@mac.lan>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…gg#3156) Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
…gg#3156) Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
# Conflicts: # apps/server/src/provider/Layers/CursorAdapter.test.ts # apps/server/src/provider/Layers/CursorAdapter.ts # apps/server/src/provider/acp/AcpRuntimeModel.ts # apps/server/src/provider/acp/AcpSessionRuntime.ts # pnpm-lock.yaml # pnpm-workspace.yaml
d2d615f to
ddcd138
Compare
44d905a to
41db910
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41db910c6a
ℹ️ 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 loaded = yield* Effect.raceFirst( | ||
| acp.agent.loadSession(loadPayload), | ||
| Fiber.join(idleFiber), |
There was a problem hiding this comment.
Fall back when stale ACP loads fail
When a stored resumeSessionId is stale and session/load returns an error, the failure from this race now bubbles out of start() and prevents Grok from opening the thread at all. The previous path recovered by creating a new session after a failed load, so users whose agent cache was pruned or whose resume cursor is invalid now have a bricked thread until the cursor is cleared; please keep the session/new fallback for load errors while still using replay-idle for hung loads.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| const activeTurnId = ctx.activeTurnId ?? ctx.session.activeTurnId; | ||
| if (turnId !== undefined && activeTurnId !== undefined && activeTurnId !== turnId) { |
There was a problem hiding this comment.
Ignore scoped stops when no turn is active
When a delayed Stop carries a turnId for an already-completed turn, activeTurnId is undefined, so this guard falls through and the code below still sends a session-scoped ctx.acp.cancel. That cancel is no longer tied to the requested turn; if the user immediately submits a follow-up after turn A completes, the stale Stop for A can be delivered while turn B is starting and cancel the follow-up. Treat a scoped interrupt with no matching active turn as stale rather than sending session/cancel.
Useful? React with 👍 / 👎.
| for (let yieldAttempt = 0; yieldAttempt < 8; yieldAttempt += 1) { | ||
| yield* Effect.yieldNow; | ||
| } | ||
| yield* prepared.acp.drainEvents; |
There was a problem hiding this comment.
Synchronize xAI completion with trailing updates
When Grok sends _x.ai/session/prompt_complete before the final session/update chunks, the prompt fallback can resume here before those later notifications have been parsed and queued. drainEvents only waits for events already in eventQueue, so the fixed eight yields are not a real ordering guarantee; under load a trailing chunk can be enqueued after the barrier, then the turn is marked ready and that chunk is dropped because there is no active turn. Please drain based on protocol/batch ordering or another explicit notification barrier rather than a fixed yield count.
Useful? React with 👍 / 👎.
| const index = | ||
| notification.promptId !== undefined | ||
| ? pending.findIndex( | ||
| (entry) => | ||
| entry.sessionId === notification.sessionId && | ||
| entry.promptId === notification.promptId, | ||
| ) | ||
| : pending.findIndex((entry) => entry.sessionId === notification.sessionId); |
There was a problem hiding this comment.
Avoid resolving ambiguous xAI completions by session only
Because promptId is optional in XAiPromptCompleteNotification, this fallback resolves the first pending prompt for the session when Grok omits it. With Grok steering there can be multiple pending prompts on the same ACP session; an out-of-order or stale prompt-complete notification without a prompt id can unblock the wrong deferred and leave the prompt that actually completed hung forever. Please only use the session-only fallback when it is unambiguous, or ignore no-id completions while more than one prompt can match.
Useful? React with 👍 / 👎.

What Changed
Why
UI Changes
Checklist