fix(auth): reconcile committed OAuth cancellation

.changeset/fix-pi-0-80-8-modelruntime-auth.md

@@ -2,4 +2,4 @@
 "@jmfederico/pi-web": patch
 ---
 
-Restore session-daemon startup and authentication on supported Pi `>=0.80.8 <0.81` releases by migrating model and credential handling to `ModelRuntime`. Login options now follow each provider's interactive API-key and OAuth capabilities, OAuth prompts retain their input, selection, and device-code semantics, and unsupported multi-step API-key setup fails safely instead of storing malformed credentials. PI WEB now requires Node.js `>=22.19.0`.
+Restore session-daemon startup and authentication on supported Pi `>=0.80.8 <0.81` releases by migrating model and credential handling to `ModelRuntime`. Login options now follow each provider's interactive API-key and OAuth capabilities, OAuth prompts retain their input, selection, and device-code semantics, committed OAuth login remains truthful when cancellation races the final refresh, and unsupported multi-step API-key setup fails safely instead of storing malformed credentials. PI WEB now requires Node.js `>=22.19.0`.

src/server/sessions/authService.test.ts

@@ -41,8 +41,9 @@ describe("AuthService", () => {
     auth.dispose();
   });
 
-  it("persists an API key and attempts every listener when propagation fails", async () => {
+  it("persists an API key and attempts every listener when failure logging throws", async () => {
-    const error = vi.fn();
+    const loggingFailure = new Error("auth logger failed");
+    const error = vi.fn(() => { throw loggingFailure; });
     const logger: AuthServiceLogger = { error };
     const { auth, credentials, changes } = await createAuthService({}, logger);
     const failure = new Error("session auth refresh failed");
@@ -260,6 +261,44 @@ describe("AuthService", () => {
     auth.dispose();
   });
 
+  it("reconciles cancellation after ModelRuntime persists OAuth but before its refresh completes", async () => {
+    const { auth, runtime, credentials, changes } = await createAuthService();
+    const provider = runtime.getProviders().find((option) => option.id === "anthropic" && option.auth.oauth !== undefined);
+    if (provider?.auth.oauth === undefined) throw new Error("Expected built-in OAuth provider");
+    const credential: Credential = {
+      type: "oauth",
+      refresh: "refresh-token",
+      access: "access-token",
+      expires: Date.now() + 60_000,
+    };
+    vi.spyOn(provider.auth.oauth, "login").mockResolvedValue(credential);
+    vi.spyOn(runtime, "reloadConfig").mockResolvedValue(undefined);
+    const refreshStarted = deferred<undefined>();
+    const finishRefresh = deferred<undefined>();
+    const refresh = vi.spyOn(runtime, "refresh").mockImplementation(async () => {
+      refreshStarted.resolve(undefined);
+      await finishRefresh.promise;
+      return { aborted: false, errors: new Map() };
+    });
+
+    const state = await auth.startOAuthLogin(provider.id);
+    await refreshStarted.promise;
+
+    await expect(credentials.read(provider.id)).resolves.toEqual(credential);
+    expect(auth.cancelOAuthFlow(state.flowId)).toMatchObject({ status: "cancelled", error: "Login cancelled" });
+    expect(changes).toEqual([]);
+
+    finishRefresh.resolve(undefined);
+    await vi.waitFor(() => { expect(auth.oauthFlow(state.flowId).status).toBe("complete"); });
+
+    expect(auth.oauthFlow(state.flowId)).toMatchObject({ status: "complete", progress: ["Login complete"] });
+    expect(auth.oauthFlow(state.flowId)).not.toHaveProperty("error");
+    await expect(credentials.read(provider.id)).resolves.toEqual(credential);
+    expect(changes).toEqual([{}]);
+    expect(refresh).toHaveBeenCalledOnce();
+    auth.dispose();
+  });
+
   it("emits an auth change after OAuth login completes without refreshing twice", async () => {
     const runtime = await ModelRuntime.create({
       credentials: new InMemoryCredentialStore(),
@@ -293,8 +332,9 @@ describe("AuthService", () => {
     expect(authFlows.disposed).toBe(true);
   });
 
-  it("completes OAuth when an auth-change listener rejects", async () => {
+  it("completes OAuth when an auth-change listener and failure logging throw", async () => {
-    const error = vi.fn();
+    const loggingFailure = new Error("auth logger failed");
+    const error = vi.fn(() => { throw loggingFailure; });
     const logger: AuthServiceLogger = { error };
     const { auth, runtime, changes } = await createAuthService({}, logger);
     const provider = runtime.getProviders().find((option) => option.id === "anthropic" && option.auth.oauth !== undefined);
@@ -353,6 +393,16 @@ async function tempAgentDir(): Promise<string> {
   return dir;
 }
 
+function deferred<T>() {
+  let resolveValue: (value: T) => void = () => undefined;
+  let rejectValue: (reason?: unknown) => void = () => undefined;
+  const promise = new Promise<T>((resolve, reject) => {
+    resolveValue = resolve;
+    rejectValue = reject;
+  });
+  return { promise, resolve: resolveValue, reject: rejectValue };
+}
+
 function radiusModelsConfig(name: string): string {
   return JSON.stringify({
     providers: {

src/server/sessions/authService.ts

@@ -53,8 +53,9 @@ export class AuthService {
 
   static async create(deps: AuthServiceDependencies = {}): Promise<AuthService> {
     const runtime = deps.runtime ?? (deps.agentDir === undefined ? await ModelRuntime.create({}) : await createModelRuntimeForAgentDir(deps.agentDir));
-    const authFlows = deps.authFlows ?? new OAuthLoginFlowService();
+    const logger = deps.logger ?? noopLogger;
-    return new AuthService(runtime, authFlows, deps.logger ?? noopLogger);
+    const authFlows = deps.authFlows ?? new OAuthLoginFlowService({ logger });
+    return new AuthService(runtime, authFlows, logger);
   }
 
   subscribe(listener: AuthChangeListener): () => void {
@@ -130,11 +131,19 @@ export class AuthService {
     const results = await Promise.allSettled([...this.listeners].map(async (listener) => listener(change)));
     for (const result of results) {
       if (result.status === "rejected") {
-        this.logger.error({ err: result.reason, ...context }, "auth-change listener failed");
+        this.logErrorNoThrow({ err: result.reason, ...context }, "auth-change listener failed");
       }
     }
   }
 
+  private logErrorNoThrow(details: Record<string, unknown>, message: string): void {
+    try {
+      this.logger.error(details, message);
+    } catch {
+      // A diagnostic failure cannot turn an already-committed auth mutation into an API failure.
+    }
+  }
+
   private async requireApiKeyLoginProvider(providerId: string) {
     await this.runtime.reloadConfig();
     const provider = getLoginProviderOptions(this.runtime, "api_key").find((option) => option.id === providerId);

src/server/sessions/oauthLoginFlowService.test.ts

@@ -60,6 +60,30 @@ describe("OAuthLoginFlowService", () => {
     service.dispose();
   });
 
+  it("keeps a committed login complete when its completion callback and logger throw", async () => {
+    const completionFailure = new Error("completion propagation failed");
+    const loggingFailure = new Error("OAuth logger failed");
+    const error = vi.fn(() => { throw loggingFailure; });
+    const onComplete = vi.fn(() => { throw completionFailure; });
+    const service = new OAuthLoginFlowService({ logger: { error } });
+    const state = service.start({
+      providerId: "test-provider",
+      providerName: "Test Provider",
+      runtime: fakeRuntime(() => Promise.resolve()),
+      onComplete,
+    });
+
+    await vi.waitFor(() => { expect(service.get(state.flowId).status).toBe("complete"); });
+
+    expect(service.get(state.flowId)).toMatchObject({ status: "complete", progress: ["Login complete"] });
+    expect(onComplete).toHaveBeenCalledOnce();
+    expect(error).toHaveBeenCalledWith(
+      { err: completionFailure, flowId: state.flowId, providerId: "test-provider" },
+      "OAuth login completion callback failed",
+    );
+    service.dispose();
+  });
+
   it("allows blank text responses for providers that use blank as a default", async () => {
     let domain: string | undefined;
     const service = new OAuthLoginFlowService();

src/server/sessions/oauthLoginFlowService.ts

@@ -27,25 +27,33 @@ interface OAuthFlowRecord {
   cleanupTimer?: TimerHandle;
 }
 
+export interface OAuthLoginFlowLogger {
+  error(details: Record<string, unknown>, message: string): void;
+}
+
 export interface OAuthLoginFlowServiceOptions {
   terminalTtlMs?: number;
   runningTtlMs?: number;
   now?: () => number;
+  logger?: OAuthLoginFlowLogger;
 }
 
 const DEFAULT_TERMINAL_TTL_MS = 5 * 60 * 1000;
 const DEFAULT_RUNNING_TTL_MS = 30 * 60 * 1000;
+const noopLogger: OAuthLoginFlowLogger = { error() { /* no-op */ } };
 
 export class OAuthLoginFlowService {
   private readonly flows = new Map<string, OAuthFlowRecord>();
   private readonly terminalTtlMs: number;
   private readonly runningTtlMs: number;
   private readonly now: () => number;
+  private readonly logger: OAuthLoginFlowLogger;
 
   constructor(options: OAuthLoginFlowServiceOptions = {}) {
     this.terminalTtlMs = options.terminalTtlMs ?? DEFAULT_TERMINAL_TTL_MS;
     this.runningTtlMs = options.runningTtlMs ?? DEFAULT_RUNNING_TTL_MS;
     this.now = options.now ?? (() => Date.now());
+    this.logger = options.logger ?? noopLogger;
   }
 
   start(options: {
@@ -80,20 +88,15 @@ export class OAuthLoginFlowService {
       notify: (event) => { this.handleEvent(record, event); },
     };
 
-    void options.runtime.login(options.providerId, "oauth", interaction)
+    void options.runtime.login(options.providerId, "oauth", interaction).then(
-      .then(async () => {
+      () => this.reconcileCommittedLogin(record, options.onComplete),
-        if (!this.isCurrentRunning(record)) return;
+      (error: unknown) => {
-        this.clearPending(record);
+        if (!this.isCurrent(record)) return;
-        await options.onComplete?.();
-        if (!this.isCurrentRunning(record)) return;
-        this.markTerminal(record, { ...withoutInteraction(record.state), status: "complete", progress: [...record.state.progress, "Login complete"] });
-      })
-      .catch((error: unknown) => {
-        if (this.flows.get(record.flowId) !== record) return;
         this.clearPending(record);
         if (record.state.status !== "running") return;
         this.markTerminal(record, { ...withoutInteraction(record.state), status: "error", error: error instanceof Error ? error.message : String(error) });
-      });
+      },
+    );
 
     return this.get(flowId);
   }
@@ -274,8 +277,39 @@ export class OAuthLoginFlowService {
     return pending;
   }
 
+  // ModelRuntime persists the credential before its post-login refresh. If a
+  // cancellation lands during that refresh, the resolved login is committed
+  // truth and must supersede the transient cancelled state.
+  private async reconcileCommittedLogin(record: OAuthFlowRecord, onComplete?: () => void | Promise<void>): Promise<void> {
+    if (this.isCurrent(record)) this.clearPending(record);
+    try {
+      await onComplete?.();
+    } catch (error) {
+      this.logErrorNoThrow(
+        { err: error, flowId: record.flowId, providerId: record.state.providerId },
+        "OAuth login completion callback failed",
+      );
+    }
+    if (!this.isCurrent(record)) return;
+    const completed = withoutInteraction(record.state);
+    delete completed.error;
+    this.markTerminal(record, { ...completed, status: "complete", progress: [...record.state.progress, "Login complete"] });
+  }
+
+  private isCurrent(record: OAuthFlowRecord): boolean {
+    return this.flows.get(record.flowId) === record;
+  }
+
   private isCurrentRunning(record: OAuthFlowRecord): boolean {
-    return this.flows.get(record.flowId) === record && record.state.status === "running";
+    return this.isCurrent(record) && record.state.status === "running";
+  }
+
+  private logErrorNoThrow(details: Record<string, unknown>, message: string): void {
+    try {
+      this.logger.error(details, message);
+    } catch {
+      // Logging is post-commit diagnostics and must never change auth truth.
+    }
   }
 
   private updateState(record: OAuthFlowRecord, state: OAuthFlowState): void {