From 258e7f15267893ee7d5fae564eb0459bc7900ca7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:49:40 -0400 Subject: [PATCH] refactor(web): let trySetPendingStepUp own the step-up retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing the retry from the mirror effect raced: an async command continuation can open a prompt and install its retry before a previous commit's passive effect flushes, and that effect's stale `pendingStepUp === null` snapshot would then delete the new prompt's retry. Install it in `trySetPendingStepUp` instead, in the same synchronous step that opens the prompt — `null` for an ambient prompt, the operation for a command-scoped one. Every prompt comes through there and `handleStepUpAuthorize` is the only reader, so a slot cleared as a side effect can leave a stale closure behind harmlessly: the next prompt overwrites it. A refused prompt still installs nothing (#2165). Copilot round 3 on #2237. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall --- clients/web/src/hooks/useOAuthRecovery.ts | 76 +++++++++++++---------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 1acb59a0..4dbbd342 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -317,19 +317,6 @@ export function useOAuthRecovery({ useEffect(() => { sessionRef.current.pendingStepUp = pendingStepUp; sessionRef.current.pendingReauth = pendingReauth; - // The retry belongs to the prompt, so it dies with it. Every arm that - // dismisses a prompt deliberately drops the retry inline — but the two - // that clear the slot as a *consequence* of something else (the - // server-switch reset below, and `resetOAuthRecoveryState` on disconnect) - // would otherwise leave it installed, and the next prompt to open without - // one of its own would inherit it: authorizing an ambient step-up on - // server B would run the command server A was left mid-way through - // (Copilot on #2237). Clearing it here covers both without any of them - // having to remember. It is a ref write in an effect, which is where a ref - // write belongs — the render-phase reset above must stay pure. - if (pendingStepUp === null) { - pendingStepUpRetryRef.current = null; - } }); // Both slots are server-scoped, so switching servers has to clear whichever @@ -352,8 +339,32 @@ export function useOAuthRecovery({ ); }); + /** + * Opens the step-up prompt, or refuses when one is already up. + * + * The retry is installed here rather than by the caller because it belongs + * to the prompt and must be written in the same synchronous step that opens + * it (#2237). Two things follow, and both are load-bearing: + * + * - A **refused** prompt installs nothing, so a command that was just told + * it cannot start does not replace the open prompt's operation (#2165). + * - An **ambient** prompt passes `null` and so *overwrites* whatever the + * previous prompt left in the ref. Nothing else has to clear it: the paths + * that drop the slot as a consequence of something else — the + * server-switch reset, `resetOAuthRecoveryState` on disconnect — can leave + * a stale closure behind, because the only thing that reads it is + * `handleStepUpAuthorize`, which needs a prompt, and every prompt comes + * through here. Clearing it from an effect instead would race: an async + * continuation can open a prompt and install its retry before a previous + * commit's passive effect flushes, and that effect's stale + * `pendingStepUp === null` snapshot would then delete the new prompt's + * retry (Copilot on #2237). + */ const trySetPendingStepUp = useCallback( - (next: PendingStepUp): boolean => { + ( + next: PendingStepUp, + retryOperation: (() => Promise) | null, + ): boolean => { if (sessionRef.current.pendingStepUp !== null) { notifications.show({ title: "Step-up authorization in progress", @@ -365,6 +376,7 @@ export function useOAuthRecovery({ return false; } setPendingStepUp(next); + pendingStepUpRetryRef.current = retryOperation; return true; }, [sessionRef], @@ -642,13 +654,18 @@ export function useOAuthRecovery({ }) => { const server = sessionRef.current.servers.find((s) => s.id === serverId); if (isStepUpConfirmation(challenge, server)) { - trySetPendingStepUp({ - challenge, - authorizationUrl, - serverId, - source, - enterpriseManaged: isEmaStepUp(challenge, server), - }); + // Ambient: nothing to retry, and passing `null` is what evicts a + // previous prompt's closure. + trySetPendingStepUp( + { + challenge, + authorizationUrl, + serverId, + source, + enterpriseManaged: isEmaStepUp(challenge, server), + }, + null, + ); return; } prepareOAuthRedirect({ @@ -713,23 +730,16 @@ export function useOAuthRecovery({ ) { return true; } - // The retry belongs to the prompt, so it is installed only once the - // prompt is actually open (#2165). `trySetPendingStepUp` REFUSES a - // second prompt while one is already up — writing the ref first meant - // the refused command's operation replaced the open prompt's, and - // authorizing that prompt then ran the command the user was just told - // could not start. - if ( - trySetPendingStepUp({ + trySetPendingStepUp( + { challenge: error.authChallenge, authorizationUrl: error.authorizationUrl, serverId: options.serverId, source: options.source, enterpriseManaged: isEmaStepUp(error.authChallenge, server), - }) - ) { - pendingStepUpRetryRef.current = options.retryOperation ?? null; - } + }, + options.retryOperation ?? null, + ); return false; }