fix(#502): run mandatory authenticated-session teardown on involuntary auth loss too

CHANGELOG.md

@@ -19,6 +19,38 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   flip above a row near the viewport bottom, and oversized menus scroll within
   the viewport instead of placing Add/Cancel off-screen.
 
+### Fixed
+- **An involuntary auth loss (expired/invalid token, failed refresh, or a
+  concurrent first-contact authentication failure) now runs the same
+  mandatory authenticated-session teardown as an explicit sign-out** (#502).
+  Previously only `app.signOut` cancelled the in-flight Workbench
+  request (and issued its server-side `KILL QUERY`), cancelled the schema
+  graph and both export modes, invalidated the catalog, and closed the
+  documentation pane before rendering login; the `onAuthLost` path skipped
+  all of it, so a mid-flight query/export/lineage stream — or a still-completing
+  request that would go on to record History, bound parameters, or a schema
+  reload — could survive past the login screen. The teardown is now a single
+  idempotent `teardownAuthenticatedSession()` helper shared by both paths.
+  Making this actually work required deeper changes to
+  `connection-session.ts`: `getToken()` no longer clears credentials itself
+  on a failed proactive refresh (it did, previously — before its caller ever
+  got a chance to run the teardown), and `chCtx.onSignedOut` now (a)
+  notifies the teardown *before* clearing credentials, not after, freezing
+  `getToken` for that window to resolve the still-retained credential (even
+  one within the proactive-refresh skew, or already past real expiry — a
+  best-effort `KILL QUERY` costs nothing extra either way) rather than
+  discarding it, so the teardown's `KILL QUERY`/export-cancellation requests
+  reach the server instead of silently no-op'ing on a token that looks gone;
+  (b) latches once it starts handling a dead session so the teardown's own
+  concurrent authenticated calls rediscovering the same dead token can't
+  recursively re-run it, resetting only on a genuine fresh sign-in; and (c)
+  still clears credentials even if the injected `onAuthLost` callback
+  throws. A detached/expanded Data Pane view's own refresh preflight
+  (`src/ui/results.ts`) also now reports auth loss the same way every other
+  caller does — previously it neither cleared nor reported it, so a dead
+  token could linger un-torn-down indefinitely if that was a user's only
+  live activity.
+
 ### Removed
 - **The unused saved-query repair planner has been removed** (#429 phase 6 /
   #500). Direct, ownership-safe Panel and Dashboard trash actions are the

src/application/connection-session.ts

@@ -124,6 +124,14 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection
   let token: string | null = ss.getItem('oauth_id_token');
   let refreshTok: string | null = ss.getItem('oauth_refresh_token');
   let authMode: 'oauth' | 'basic' = ss.getItem('ch_basic_auth') ? 'basic' : 'oauth';
+  // #502: latches once `chCtx.onSignedOut` starts handling a dead session, so
+  // concurrent/nested reports of the SAME auth loss (e.g. the teardown's own
+  // fire-and-forget KILL QUERY/export-cancel calls independently discovering
+  // the same expired-and-unrefreshable token) can't re-run the teardown,
+  // re-render login, or re-attempt `refresh()` against the token endpoint.
+  // Reset only by a genuine fresh sign-in (`setTokens`/`connectBasic`), so a
+  // LATER, distinct auth-loss event in the new session is handled normally.
+  let signedOutHandled = false;
   const basicCreds = (): string | null => ss.getItem('ch_basic_auth');
   const basicUser = (): string => ss.getItem('ch_basic_user') || '';
   const originHost = (o: string): string => { try { return new URL(o).host; } catch { return ''; } };
@@ -182,6 +190,9 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection
     // tokens. (The refresh path also lands here; they're already gone → no-op.)
     ss.removeItem('oauth_verifier');
     ss.removeItem('oauth_state');
+    // #502: a fresh/refreshed token means this is a live session again — a
+    // LATER auth loss is a distinct event `onSignedOut` must handle again.
+    signedOutHandled = false;
   }
   function clearTokens(): void {
     token = null;
@@ -244,7 +255,14 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection
     if (!token) return null;
     if (!isTokenExpired(token)) return token;
     if (await refresh()) return token;
-    clearTokens();
+    // #502: deliberately NOT `clearTokens()` here. Every caller of `getToken`
+    // already does `if (!(await getToken())) { <its own onSignedOut/
+    // onAuthFailed call> }` — that path's authenticated teardown (KILL QUERY,
+    // export cancellation) needs the still-current `token`/`authMode` this
+    // closure holds to actually reach the server; clearing it here, before
+    // any of those callers ever get a chance to run, was making every one of
+    // them silently no-op on an already-missing token. `onSignedOut` clears
+    // once its teardown has run.
     return null;
   }
 
@@ -278,9 +296,56 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection
     authHeader,
     // detail is set when CH rejects a *valid* login (authorization denial); the
     // no-arg calls (no token / expired + refresh failed) fall back to expiry.
+    // #502: the shell-owned teardown `deps.onAuthLost` runs (aborting the
+    // workbench/graph/export requests and KILLing their server-side queries)
+    // must authenticate its own fire-and-forget calls with the credential
+    // this dying session held — even one `getToken()`'s 60s skew calls
+    // "expiring soon", or one already past real expiry: a best-effort KILL
+    // QUERY costs nothing extra either way, and it's likely still accepted.
+    // The plain `ctx.getToken()` can't be reused as-is for this: it treats a
+    // failed *proactive* refresh as terminal and returns null regardless of
+    // whether the retained token is still good, so every nested
+    // `authedFetch` inside the teardown would see `!token` and bail before
+    // ever reaching the server. `chCtx.getToken` is frozen to resolve this
+    // exact retained token — computed and captured BEFORE the teardown
+    // runs — for the synchronous duration of `deps.onAuthLost`, then
+    // restored. This works because `authedFetch` reads+calls `ctx.getToken`
+    // synchronously, before its own first `await` — every nested call
+    // captures the frozen value before this function's `finally` below ever
+    // runs (fire-and-forget calls only kick off their promises here; they
+    // don't settle until well after this function returns).
+    //
+    // `ctx.authHeader`/`ctx.refresh`, in contrast, are read by `authedFetch`
+    // only in the continuation AFTER its own `await ctx.getToken()` — i.e.
+    // in a later microtask (or later still, `ctx.refresh` only fires on an
+    // actual 401 response, after a real network round-trip) — by which
+    // point this function has already returned and restored/cleared
+    // everything. Freezing them here would be a no-op that only creates a
+    // false sense of protection, so this deliberately does NOT try: for a
+    // Basic-mode session specifically, this means a stray retry can still
+    // build the wrong Authorization scheme once `authMode` has been reset
+    // (tracked separately as `inbox` #520, not fixed here — a real fix needs
+    // the fire-and-forget calls to be awaited, or their credentials captured
+    // and threaded through explicitly, before this function ever restores or
+    // clears anything).
+    //
+    // `signedOutHandled` guards re-entrancy: the teardown's own frozen calls,
+    // if they still fail, report auth loss again but that's a no-op (no
+    // second render/teardown) until a fresh sign-in (`setTokens`/
+    // `connectBasic`) resets it. The `finally` guarantees credentials are
+    // still cleared even if the injected `onAuthLost` throws.
     onSignedOut: (detail?: string) => {
-      clearTokens();
-      deps.onAuthLost(detail || 'Your session expired — please sign in again.');
+      if (signedOutHandled) return;
+      signedOutHandled = true;
+      const frozenToken = authMode === 'basic' ? basicCreds() : token;
+      const realGetToken = chCtx.getToken;
+      chCtx.getToken = async () => frozenToken;
+      try {
+        deps.onAuthLost(detail || 'Your session expired — please sign in again.');
+      } finally {
+        chCtx.getToken = realGetToken;
+        clearTokens();
+      }
     },
   };
 
@@ -326,6 +391,8 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection
     ss.setItem('ch_basic_user', user);
     ss.setItem('ch_basic_origin', target);
     chCtx.origin = target;
+    // #502: a fresh Basic session is live again — see `setTokens`'s own reset.
+    signedOutHandled = false;
   }
 
   // --- dashboard (#149 D1) -------------------------------------------------

src/ui/app.ts

@@ -455,6 +455,28 @@ export function createApp(env: CreateAppEnv = {}): App {
     disposeShell();
     renderLogin(app as App & { root: Element }, msg);
   };
+  // #502: cancel/tear down every in-flight authenticated operation BEFORE
+  // credentials are cleared and login renders — required for both an
+  // explicit sign-out AND an involuntary auth loss (expired/invalid token,
+  // failed refresh, or a first-contact auth failure via `onAuthLost` below),
+  // so a mid-flight query/export/lineage stream can never land (or repaint)
+  // after the login screen is showing; invalidating the catalog means a
+  // later sign-in (possibly a different server) never sees stale
+  // schema/reference caches. Idempotent: several concurrent requests may
+  // each report auth loss. The workbench session stays reusable after
+  // destroy() — the next renderApp re-attaches its shell effects.
+  const teardownAuthenticatedSession = (): void => {
+    workbench.destroy();
+    // Plain abort (no clearResult settle) — the login render replaces the
+    // whole DOM next, so settling the visible result would be a wasted paint.
+    graph.cancel();
+    exportService.cancelExport();
+    exportService.cancelExportScript();
+    catalog.invalidate();
+    // #313: pane content must never survive a connection change — closed
+    // alongside the catalog reset, before the login screen renders.
+    closeDocPane(app);
+  };
   // The auth + config + ClickHouse connection lifecycle (#276 Phase 2) — OAuth
   // PKCE login/refresh, Basic probing, and IdP config resolution live in
   // `application/connection-session.ts`,
@@ -464,7 +486,12 @@ export function createApp(env: CreateAppEnv = {}): App {
   const conn = createConnectionSession({
     fetch: fetchFn, storage: ss, location: loc, crypto: cryptoObj,
     queryJson: ch.queryJson,
-    onAuthLost: (detail) => renderLoginApp(detail),
+    // #502: onAuthLost is the involuntary counterpart of signOut() below —
+    // same mandatory teardown ordering, before login replaces the DOM.
+    onAuthLost: (detail) => {
+      teardownAuthenticatedSession();
+      renderLoginApp(detail);
+    },
   });
   app.conn = conn;
   // THE single live ClickHouse context — owned by the session, aliased locally
@@ -478,28 +505,14 @@ export function createApp(env: CreateAppEnv = {}): App {
   // comment) — no flat `App` delegates (#276 Phase 5 deleted them).
   // `showLogin`/`signOut` stay app.ts-owned: they compose rendering, not
   // pure forwards.
-  // Sign-out is the one real end-of-session event in a single-route tab —
-  // the first production wiring of the sessions' teardown surfaces (#276
-  // Phase 5). Order matters: cancel/tear down every in-flight operation
-  // BEFORE clearing credentials and rendering login, so a mid-flight
-  // query/export/lineage stream can never land (or repaint) after the login
-  // screen is showing; invalidate the catalog so a later sign-in (possibly a
-  // different server) never sees stale schema/reference caches. The
-  // workbench session stays reusable after destroy(): the next renderApp
-  // re-attaches its shell effects.
+  // Sign-out is the one real explicit end-of-session event in a
+  // single-route tab (#276 Phase 5); `onAuthLost` above is its involuntary
+  // counterpart, sharing the same `teardownAuthenticatedSession()` (#502) —
+  // both must tear down before clearing credentials and rendering login.
   app.signOut = () => {
     app.closeShortcutDialog();
     resetShortcutChord(app);
-    workbench.destroy();
-    // Plain abort (no clearResult settle) — the login render replaces the
-    // whole DOM next, so settling the visible result would be a wasted paint.
-    graph.cancel();
-    exportService.cancelExport();
-    exportService.cancelExportScript();
-    catalog.invalidate();
-    // #313: pane content must never survive a connection change — closed
-    // alongside the catalog reset, before the login screen renders.
-    closeDocPane(app);
+    teardownAuthenticatedSession();
     conn.signOut();
     // #425: the Dashboard teardown, the surface-generation bump and the
     // main-surface reset live in `renderLoginApp` — the one place that knows the

src/ui/results.ts

@@ -161,7 +161,7 @@ export interface ResultsApp {
   now(): number;
   elapsedMs(): number;
   wallNow(): number;
-  conn: Pick<ConnectionSession, 'ensureFreshToken'>;
+  conn: Pick<ConnectionSession, 'ensureFreshToken'> & { chCtx: Pick<ConnectionSession['chCtx'], 'onSignedOut'> };
   /** The shared request/stream/normalize service (#276 Phase 1) — this module
    *  only ever needs `executeRead` (the detached Data view's own re-run). */
   exec: Pick<QueryExecutionService, 'executeRead'>;
@@ -1008,6 +1008,11 @@ export function expandDataPane(app: ResultsApp, r: QueryResult): DetachedView {
         if (refreshBtn) refreshBtn.disabled = true;
         if (!(await app.conn.ensureFreshToken())) {
           if (myGen === gen && !closed) settle('Not signed in');
+          // #522: unlike a dashboard tile's own preflight (`onAuthFailed`),
+          // this path never reported the auth loss at all — dead credentials
+          // would otherwise linger un-torn-down until some other action
+          // happened to rediscover the same dead token.
+          app.conn.chCtx.onSignedOut();
           return;
         }
         const execution = panelExecution(savedPanel, mergedSourceSql(src, source.sql), {