diff --git a/devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md b/devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md index 7b6b968df0d..6f7179c8d89 100644 --- a/devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md +++ b/devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md @@ -215,7 +215,11 @@ cd docs-site && bun run build as a server follow-up candidate in 000 and in the PR. - **A8 In-flight terminal replies win.** The hook (and finalizer) processes a terminal reply from a status request already in flight even after Cancel; a later 404 still takes the neutral path (A3). A1's promise - becomes: success is shown only after a terminal `done` status reply is observed. + becomes: success is shown only after a terminal `done` status reply is observed. The handoff owns one + parsed status-read operation rather than cloned `Response` bodies. Its 45 s budget covers fetch, body EOF + and JSON parsing; the finalizer also cancels that reader when the flow-wide deadline wins and never waits + a full retry interval beyond the deadline. This preserves the already-sent terminal reply without letting + a stalled body retain the module-scoped singleflight entry indefinitely (#6021). - **A9 Settle split at the guard.** Two helpers: `reloadAccountsAfterLogin(provider)` (awaited `fetchAccountSets`) and `refreshDerivedAfterLogin()` (`fetchConfig`, `fetchProviderQuotas(true)`, `bumpModelsRefresh`). The existing loop keeps its generation/mounted guard **between** them, keeps its diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index 1d51fc11d2a..61b0e377a2a 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -451,7 +451,7 @@ an ambiguous token selection), when `KIROCLI_DB_PATH` / `KIRO_CLI_DB_FILE` redir from the live CLI store, or when an existing primary CLI database has no recognized token row. Repair or remove the unreadable database under the normal `kiro-cli` data path, unset those import selectors, then retry. Signing in from a machine with no existing `kiro-cli` session is unaffected. -The native dashboard choices are add-only and do not sign out `kiro-cli`. A device dialog shows the code and verification destination. Only recognized Kiro or Builder ID hosts are opened as links; an unexpected destination is shown as copyable text for review. +The native dashboard choices are add-only and do not sign out `kiro-cli`. A device dialog shows the code and verification destination. Only recognized Kiro or Builder ID hosts are opened as links; an unexpected destination is shown as copyable text for review. Closing the dialog sends cancellation and leaves a bounded background status check to reconcile a commit already in progress. A stalled status response is retried; exhausting the flow deadline produces the neutral ended outcome rather than claiming success. The account list marks Kiro accounts excluded from automatic selection with a reason, when available. ## 3. API-key catalog diff --git a/gui/src/components/use-kiro-device-login.ts b/gui/src/components/use-kiro-device-login.ts index 1e8d2c92379..e4c7e978d39 100644 --- a/gui/src/components/use-kiro-device-login.ts +++ b/gui/src/components/use-kiro-device-login.ts @@ -1,12 +1,15 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { afterOAuthCancellation } from "../oauth-cancellation-barrier"; -import { finalizeKiroDeviceFlow, observeKiroDeviceFinal, type KiroFinalOutcome } from "../kiro-device-login-finalizer"; +import { + finalizeKiroDeviceFlow, observeKiroDeviceFinal, readKiroDeviceStatus, + type KiroFinalOutcome, type KiroStatusRead, +} from "../kiro-device-login-finalizer"; import { parseKiroDeviceView, type KiroDeviceMethod, type KiroDeviceView } from "../kiro-device-login-helpers"; type Phase = "idle" | "starting" | "pending" | "done" | "expired" | "failed" | "cancelled" | "ended"; export type KiroLoginState = { phase: Phase; view?: KiroDeviceView; error?: "start" | "network" | "invalid" }; -type Session = { closed: boolean; view?: KiroDeviceView; inFlight?: Promise; - waitController?: AbortController; terminal?: KiroFinalOutcome }; +type Session = { closed: boolean; closedController: AbortController; view?: KiroDeviceView; + inFlight?: KiroStatusRead; waitController?: AbortController; terminal?: KiroFinalOutcome }; const wait = (ms: number, signal: AbortSignal) => new Promise(resolve => { if (signal.aborted) { resolve(); return; } const timer = setTimeout(() => { signal.removeEventListener("abort", stop); resolve(); }, ms); @@ -16,7 +19,19 @@ const wait = (ms: number, signal: AbortSignal) => new Promise(resolve => { const CLOSED = Symbol("closed"); /** The awaited value, or CLOSED when the session closed while it was pending (a late reply belongs to the finalizer). */ const unlessClosed = (session: Session, value: Promise): Promise => - value.then(result => (session.closed ? CLOSED : result)); + new Promise(resolve => { + if (session.closed) { resolve(CLOSED); return; } + let done = false; + const settle = (result: T | typeof CLOSED) => { + if (done) return; + done = true; + session.closedController.signal.removeEventListener("abort", stop); + resolve(result); + }; + const stop = () => settle(CLOSED); + session.closedController.signal.addEventListener("abort", stop, { once: true }); + void value.then(result => settle(session.closed ? CLOSED : result), () => settle(CLOSED)); + }); export function useKiroDeviceLogin(apiBase: string, onSettled?: (provider: string, outcome: KiroFinalOutcome) => void, pollDelay: (ms: number, signal: AbortSignal) => Promise = wait) { @@ -44,6 +59,7 @@ export function useKiroDeviceLogin(apiBase: string, onSettled?: (provider: strin const session = sessionRef.current; if (!session || session.closed) return; session.closed = true; + session.closedController.abort(); sessionRef.current = null; session.waitController?.abort(); if (mountedRef.current) setState({ phase: "cancelled" }); @@ -64,7 +80,7 @@ export function useKiroDeviceLogin(apiBase: string, onSettled?: (provider: strin const start = useCallback(async (method: KiroDeviceMethod) => { if (sessionRef.current) return; - const session: Session = { closed: false }; + const session: Session = { closed: false, closedController: new AbortController() }; sessionRef.current = session; setState({ phase: "starting" }); let response: Response | undefined; @@ -114,21 +130,18 @@ export function useKiroDeviceLogin(apiBase: string, onSettled?: (provider: strin break; } const flowId = view.flowId; - const request = fetch(`${apiBase}/api/oauth/status?provider=kiro&flowId=${encodeURIComponent(flowId)}`).catch(() => null); - session.inFlight = request.then(response => response?.clone() ?? null); - const status = await unlessClosed(session, request); + const request = readKiroDeviceStatus(apiBase, flowId); + session.inFlight = request; + const status = await unlessClosed(session, request.result); if (status === CLOSED) break; - if (status?.status === 404) { - session.inFlight = undefined; + session.inFlight = undefined; + if (status.kind === "missing") { session.terminal = "ended"; settledRef.current?.("kiro", "ended"); setState({ phase: "ended", view: session.view }); break; } - const body = status?.ok ? await unlessClosed(session, status.json().catch(() => null)) : null; - if (body === CLOSED) break; - const next = body === null ? null : parseKiroDeviceView(body); - session.inFlight = undefined; + const next = status.kind === "view" ? status.view : null; if (!next || next.flowId !== flowId) continue; session.view = next; if (next.state === "pending") { setState({ phase: "pending", view: next }); continue; } diff --git a/gui/src/kiro-device-login-finalizer.ts b/gui/src/kiro-device-login-finalizer.ts index a2327ea397f..dff5ad25890 100644 --- a/gui/src/kiro-device-login-finalizer.ts +++ b/gui/src/kiro-device-login-finalizer.ts @@ -1,16 +1,84 @@ import { parseKiroDeviceView, type KiroDeviceView } from "./kiro-device-login-helpers"; export type KiroFinalOutcome = "added" | "ended" | "failed"; +export type KiroStatusResult = + | { kind: "view"; view: KiroDeviceView } + | { kind: "missing" } + | { kind: "retry" }; +export type KiroStatusRead = { result: Promise; cancel: () => void }; type Listener = (outcome: KiroFinalOutcome) => void; const active = new Map>(); const terminal = new Map(); const listeners = new Map>(); const keyFor = (apiBase: string, flowId: string) => JSON.stringify([apiBase, flowId]); const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); -async function boundedRead(read: Promise, ms: number): Promise { +const MAX_STATUS_BODY_BYTES = 64 * 1024; + +function cancelBody(response: Response): void { + try { void response.body?.cancel().catch(() => {}); } catch { /* best effort */ } +} + +/** Own fetch, body EOF and parsing as one cancellable operation; headers alone are not completion. */ +export function readKiroDeviceStatus(apiBase: string, flowId: string, timeoutMs = 45_000): KiroStatusRead { + const controller = new AbortController(); + let reader: ReadableStreamDefaultReader | undefined; + let settled = false; + let finish!: (result: KiroStatusResult) => void; + const result = new Promise(resolve => { finish = resolve; }); + const settle = (value: KiroStatusResult) => { + if (settled) return; + settled = true; + clearTimeout(timer); + finish(value); + }; + const cancel = () => { + if (settled) return; + controller.abort(); + try { void reader?.cancel().catch(() => {}); } catch { /* best effort */ } + // Transport abort and stream cancellation are cooperative. The caller's deadline is not. + settle({ kind: "retry" }); + }; + const timer = setTimeout(cancel, Math.max(0, timeoutMs)); + void (async () => { + try { + const response = await fetch( + `${apiBase}/api/oauth/status?provider=kiro&flowId=${encodeURIComponent(flowId)}`, + { signal: controller.signal }, + ); + if (settled) { cancelBody(response); return; } + if (response.status === 404) { cancelBody(response); settle({ kind: "missing" }); return; } + if (!response.ok || !response.body) { cancelBody(response); settle({ kind: "retry" }); return; } + reader = response.body.getReader(); + const decoder = new TextDecoder(); + let text = ""; + let bytes = 0; + while (true) { + const chunk = await reader.read(); + if (settled) return; + if (chunk.done) break; + bytes += chunk.value.byteLength; + if (bytes > MAX_STATUS_BODY_BYTES) { cancel(); return; } + text += decoder.decode(chunk.value, { stream: true }); + } + text += decoder.decode(); + let decoded: unknown; + try { decoded = JSON.parse(text); } catch { settle({ kind: "retry" }); return; } + const view = parseKiroDeviceView(decoded); + settle(view ? { kind: "view", view } : { kind: "retry" }); + } catch { settle({ kind: "retry" }); } + })(); + return { result, cancel }; +} + +async function awaitStatusRead(read: KiroStatusRead, ms: number): Promise { let timer: ReturnType | undefined; try { - return await Promise.race([read, new Promise(resolve => { timer = setTimeout(() => resolve(null), ms); })]); + return await Promise.race([ + read.result, + new Promise(resolve => { + timer = setTimeout(() => { read.cancel(); resolve({ kind: "retry" }); }, Math.max(0, ms)); + }), + ]); } finally { if (timer) clearTimeout(timer); } } @@ -45,7 +113,7 @@ export function observeKiroDeviceFinal(apiBase: string, flowId: string, view: Ki /** Detached reconciliation survives dialog and page unmount. One loop per flowId. */ export function finalizeKiroDeviceFlow(apiBase: string, flowId: string, expiresAt?: number, - inFlight?: Promise): Promise { + inFlight?: KiroStatusRead): Promise { const key = keyFor(apiBase, flowId); const prior = active.get(key); if (prior) return prior; @@ -55,27 +123,23 @@ export function finalizeKiroDeviceFlow(apiBase: string, flowId: string, expiresA const run = (async () => { let pending = inFlight; while (Date.now() < deadline) { - let response: Response | null; - try { - const remaining = deadline - Date.now(); - const timeout = Math.min(45_000, remaining); - response = await boundedRead(pending ?? fetch( - `${apiBase}/api/oauth/status?provider=kiro&flowId=${encodeURIComponent(flowId)}`, - { signal: AbortSignal.timeout(timeout) }, - ), timeout); - } catch { response = null; } + const remaining = deadline - Date.now(); + const read = pending ?? readKiroDeviceStatus(apiBase, flowId, Math.min(45_000, remaining)); pending = undefined; + const status = await awaitStatusRead(read, remaining); if (terminal.has(key)) return terminal.get(key)!; - if (response?.status === 404) return finish(apiBase, flowId, "ended"); - if (response?.ok) { - const view = parseKiroDeviceView(await response.json().catch(() => null)); - if (view) { - const result = observeKiroDeviceFinal(apiBase, flowId, view); - if (result) return result; - } + if (status.kind === "missing") return finish(apiBase, flowId, "ended"); + if (status.kind === "view") { + const result = observeKiroDeviceFinal(apiBase, flowId, status.view); + if (result) return result; } - await delay(2_000); + const retryRemaining = deadline - Date.now(); + if (retryRemaining <= 0) break; + await delay(Math.min(2_000, retryRemaining)); } + // A close can hand off after expiry. Do not leave that inherited transport alive merely + // because there was no remaining loop iteration in which the deadline wrapper could cancel it. + pending?.cancel(); return finish(apiBase, flowId, "ended"); })().finally(() => { active.delete(key); }); active.set(key, run); diff --git a/gui/tests/kiro-device-login.test.tsx b/gui/tests/kiro-device-login.test.tsx index 8be8c8326e3..203b1d5ee6c 100644 --- a/gui/tests/kiro-device-login.test.tsx +++ b/gui/tests/kiro-device-login.test.tsx @@ -11,8 +11,9 @@ import type { ProviderAuthHandlers } from "../src/components/provider-workspace/ import { en } from "../src/i18n/en"; import { interpolate, type TFn } from "../src/i18n/shared"; import { useProvidersOAuth } from "../src/pages/use-providers-oauth"; -import { finalizeKiroDeviceFlow } from "../src/kiro-device-login-finalizer"; -import { subscribeKiroDeviceFinal } from "../src/kiro-device-login-finalizer"; +import { + finalizeKiroDeviceFlow, readKiroDeviceStatus, subscribeKiroDeviceFinal, +} from "../src/kiro-device-login-finalizer"; const globals = ["document", "window", "navigator", "localStorage", "fetch", "IS_REACT_ACT_ENVIRONMENT"] as const; let previous: Record<(typeof globals)[number], unknown>; @@ -334,14 +335,19 @@ test("close during start dispatches cancel before detached status", async () => unsubscribe(); }); -test("close during status body parsing leaves finalizer as sole outcome owner", async () => { - const body = deferred(); +test("close during a delayed status body transfers its sole reader to the finalizer", async () => { + let bodyController!: ReadableStreamDefaultController; let readingBody = false; responder = async url => { if (url.includes("/api/oauth/status?")) { - const response = json(view("done")); - Object.defineProperty(response, "json", { value: () => { readingBody = true; return body.promise; } }); - return response; + return new Response(new ReadableStream({ + start(controller) { + bodyController = controller; + readingBody = true; + controller.enqueue(new TextEncoder().encode(JSON.stringify(view("done")))); + // The terminal JSON is not complete until EOF; close it only after the component unmounts. + }, + }), { headers: { "Content-Type": "application/json" } }); } return url.endsWith("/api/oauth/login/cancel") ? json(view("cancelled")) : json(view("pending")); }; @@ -352,13 +358,79 @@ test("close during status body parsing leaves finalizer as sole outcome owner", expect(readingBody).toBe(true); await act(async () => { root!.unmount(); root = null; }); await flush(); - expect(received).toEqual(["added"]); - await act(async () => { body.resolve(view("done")); }); await flush(); + expect(received).toEqual([]); + await act(async () => { bodyController.close(); }); await flush(); expect(settled).toEqual([]); expect(received).toEqual(["added"]); + expect(requests.filter(r => r.url.includes("/api/oauth/status?"))).toHaveLength(1); unsubscribe(); }); +test("status read deadline covers a body that never reaches EOF", async () => { + let bodyCancelled = 0; + responder = async url => url.includes("/api/oauth/status?") + ? new Response(new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(JSON.stringify(view("done")))); + }, + cancel() { bodyCancelled++; }, + }), { headers: { "Content-Type": "application/json" } }) + : json({}); + const read = readKiroDeviceStatus("", flowId, 20); + expect(await read.result).toEqual({ kind: "retry" }); + expect(bodyCancelled).toBe(1); +}); + +test("status read deadline settles when fetch ignores abort and discards its late body", async () => { + const pending = deferred(); + let lateBodyCancelled = 0; + responder = async url => url.includes("/api/oauth/status?") ? pending.promise : json({}); + const read = readKiroDeviceStatus("", flowId, 20); + expect(await read.result).toEqual({ kind: "retry" }); + pending.resolve(new Response(new ReadableStream({ + cancel() { lateBodyCancelled++; }, + }))); + await Promise.resolve(); + await Promise.resolve(); + await new Promise(resolve => setTimeout(resolve, 0)); + expect(lateBodyCancelled).toBe(1); +}); + +test("the overall finalizer deadline cancels a longer inherited body read", async () => { + let bodyCancelled = 0; + responder = async url => url.includes("/api/oauth/status?") + ? new Response(new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(JSON.stringify(view("done")))); + }, + cancel() { bodyCancelled++; }, + }), { headers: { "Content-Type": "application/json" } }) + : json({}); + const inherited = readKiroDeviceStatus("", flowId, 45_000); + const first = finalizeKiroDeviceFlow("", flowId, Date.now() - 59_980, inherited); + expect(await first).toBe("ended"); + expect(bodyCancelled).toBe(1); + const afterCleanup = finalizeKiroDeviceFlow("", flowId, Date.now() + 60_000); + expect(afterCleanup).not.toBe(first); + expect(await afterCleanup).toBe("ended"); +}); + +test("an already-expired finalizer cancels its inherited read without polling", async () => { + let bodyCancelled = 0; + responder = async url => url.includes("/api/oauth/status?") + ? new Response(new ReadableStream({ + cancel() { bodyCancelled++; }, + })) + : json({}); + const inherited = readKiroDeviceStatus("", flowId, 45_000); + expect(await finalizeKiroDeviceFlow("", flowId, Date.now() - 60_001, inherited)).toBe("ended"); + await Promise.resolve(); + await Promise.resolve(); + await new Promise(resolve => setTimeout(resolve, 0)); + expect(bodyCancelled).toBe(1); + expect(requests.filter(r => r.url.includes("/api/oauth/status?"))).toHaveLength(1); +}); + test("closing aborts the pending timer and never dispatches a hook status fetch", async () => { let waitSignal: AbortSignal | undefined; const cancellableDelay = (_ms: number, signal: AbortSignal) => new Promise(resolve => { diff --git a/structure/decisions/ADR-6021-kiro-status-read-ownership.md b/structure/decisions/ADR-6021-kiro-status-read-ownership.md new file mode 100644 index 00000000000..599e7b9acf4 --- /dev/null +++ b/structure/decisions/ADR-6021-kiro-status-read-ownership.md @@ -0,0 +1,12 @@ +# ADR-6021 — Kiro status-read ownership + +- Contract owner: [GUI and management API](../gui-and-management-api.md#authentication-boundaries) + +## Decision record + +- Purpose and intent: Let Kiro device-login reconciliation survive dialog unmount without allowing a stalled response body to outlive the documented read and flow deadlines. +- Existing implementation and constraints: The hook consumed an original `Response` while the detached finalizer consumed a clone. The 45-second race ended when headers arrived, so either body could then wait forever. The already-sent status request must still be able to prove a terminal credential commit after Cancel; simply aborting it on unmount would lose that evidence and permit a later 404 to win. +- Alternatives considered: Abort the hook request and start a fresh finalizer request; retain cloned responses but race each `json()` call; transfer one operation that owns transport, body, parsing and cancellation. +- Chosen approach: Create one status-read operation before dispatch. It owns fetch, a bounded 64 KiB body reader, JSON decoding and public-view validation under one 45-second timer. Closing the dialog transfers that operation to the module-scoped finalizer. The finalizer separately caps its wait by the remaining flow deadline, cancels an inherited reader when that bound wins, and clamps retry sleep to the remaining time. +- Why this approach: One reader preserves terminal-reply precedence without duplicate body consumers. Explicit stream ownership lets timeout settle independently of cooperative transport abort and releases the finalizer singleflight even when EOF never arrives. +- Benefits, costs and impact: Status reads have deterministic memory and lifetime bounds, and a delayed terminal EOF can still reconcile after unmount. The helper is intentionally scoped to status polling; initial-login and cancellation response bodies retain their existing behavior. A status body larger than 64 KiB is treated as retryable invalid input rather than retained. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index fc13b4b5969..318a6a48dd5 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -61,11 +61,13 @@ unsupported; deployments that previously relied on such embedding must open it a Kiro management login starts the native device flow only when `POST /api/oauth/login` supplies `method: "builder-id"`, `"google"`, or `"github"`. A method-less request retains -the Kiro CLI flow used by the dashboard chooser. The KiroDeviceLoginDialog and useKiroDeviceLogin GUI modules own the native chooser and polling. The kiro-device-login-finalizer GUI module continues terminal status reads after dialog unmount; a provisional cancel result is never treated as confirmed success. Native status and cancellation require `flowId`; +the Kiro CLI flow used by the dashboard chooser. The KiroDeviceLoginDialog and useKiroDeviceLogin GUI modules own the native chooser and polling. The kiro-device-login-finalizer GUI module continues terminal status reads after dialog unmount; a provisional cancel result is never treated as confirmed success. Hook and finalizer share one status-read operation that owns fetch, bounded body consumption, parsing and cancellation. The complete read has a 45-second ceiling, the detached loop remains bounded by flow expiry plus 60 seconds (and 16 minutes maximum), and closing the dialog transfers rather than clones an in-flight reader so an already-observed terminal reply can still win. Native status and cancellation require `flowId`; provider-keyed status and cancellation retain their previous behavior. Device-flow responses contain only the flow handle, method, public verification fields, state, expiry, and a duplicate-profile warning when applicable. The dashboard renders a native device dialog and links only exact Builder ID or Kiro-owned verification hosts. Kiro account rows show automatic-selection exclusion reasons when present. +> Decision record: [ADR-6021](decisions/ADR-6021-kiro-status-read-ownership.md) + OpenCodex uses three mutually exclusive reusable admission credential classes: | Credential class | Sources | Allowed surface |