Skip to content

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

Closed
BorisTyshkevich wants to merge 3 commits into
mainfrom
fix/onauthlost-teardown-502
Closed

fix(#502): run mandatory authenticated-session teardown on involuntary auth loss too#521
BorisTyshkevich wants to merge 3 commits into
mainfrom
fix/onauthlost-teardown-502

Conversation

@BorisTyshkevich

@BorisTyshkevich BorisTyshkevich commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

What & why

Closes #502.

app.signOut documents and enforces an ordering invariant: cancel/tear down
every in-flight authenticated operation (Workbench request + server-side
KILL QUERY, schema graph, both export modes, catalog) and close the
documentation pane, all before clearing credentials and rendering the
login screen. The involuntary auth-loss path (onAuthLost, reached via
chCtx.onSignedOut) only rendered login — none of that teardown ran.

This factors the teardown into one idempotent teardownAuthenticatedSession()
in src/ui/app.ts, shared by app.signOut and the onAuthLost callback.

Three rounds of fixes were needed in src/application/connection-session.ts
beyond the app.ts wiring — each caught by an independent review pass, not by
me alone.
Recording the full chain here since two of the three attempts
looked complete and weren't:

  1. Reordering alone wasn't enough. chCtx.onSignedOut calls
    deps.onAuthLost(...) before clearTokens(), not after.

  2. That reorder never even applied to the "failed refresh" trigger
    (one of onAuthLost bypasses mandatory authenticated-session teardown #502's own three listed triggers): getToken() was clearing
    credentials itself, synchronously, before its caller ever invoked
    onSignedOut. Moved that clearing out of getToken() — it's now solely
    onSignedOut's job. This alone still didn't fix the underlying problem
    (see feat(editor): consistent schema gestures, SQL formatter, undo-friendly inserts #3) and opened a reentrancy hole, closed with a signedOutHandled
    latch (first report runs the teardown, concurrent/nested reports no-op,
    reset only by a genuine fresh sign-in).

  3. Removing that early clear still didn't make KILL QUERY reach the
    server.
    getToken() itself returns null whenever its own proactive
    refresh fails, independent of whether it also clears storage — so every
    nested authedFetch inside the teardown still saw !token and bailed
    before ever building a request. Fixed by having onSignedOut temporarily
    freeze chCtx.getToken to resolve the retained credential directly (no
    refresh, no expiry check) for its own synchronous duration, restoring the
    real function afterward. This works only because authedFetch reads and
    calls ctx.getToken synchronously, before its own first await — every
    nested fire-and-forget call captures the frozen value before the
    finally restores it.

    I initially also froze chCtx.authHeader/chCtx.refresh the same way,
    believing it would retroactively close Basic-mode teardown requests (KILL QUERY/export-cancel) get the wrong Authorization scheme after credentials clear #520 (a Basic-mode session's
    authHeader() reading authMode after it's been reset). That was wrong,
    caught by yet another review pass: authedFetch reads authHeader
    (and calls refresh, on an actual 401) only in the continuation after
    its own getToken await — well after onSignedOut has already
    restored/cleared everything — so freezing them was dead code with no
    protective effect. Removed. Basic-mode teardown requests (KILL QUERY/export-cancel) get the wrong Authorization scheme after credentials clear #520 remains genuinely open, not fixed by
    this PR; a real fix needs the fire-and-forget calls awaited (or their
    credentials threaded through explicitly) before onSignedOut restores or
    clears anything — out of scope here.

  4. onSignedOut also wraps deps.onAuthLost(...) in a try/finally so a
    throwing injected callback can't skip credential cleanup.

Fixed for real, not filed as inbox: src/ui/results.ts's detached/
expanded-view refresh preflight called ensureFreshToken() but never
reported the failure to onSignedOut at all — a real regression from step 2
above (getToken()'s own clearTokens() used to at least silently wipe
storage on this path; removing it left credentials lingering indefinitely if
this was a user's only live activity). ResultsApp.conn's type now also
carries chCtx.onSignedOut, and the failure branch calls it. Closes #522.

Filed #520 (inbox) — pre-existing, narrower Basic-mode gap described
above, predates this change and remains unfixed by it.

Tests

  • chCtx.onSignedOut() on a deferred Workbench request aborts its signal and
    kills its server-side query, exactly like signOut(); graph, both export
    modes, the catalog, and the doc pane are torn down the same way.
  • Resolving that request afterward records no History entry, no successful-
    result columns, no bound-parameter recording, and no schema reload.
  • A token that needed (and failed) a proactive refresh still gets its
    KILL QUERY sent using the retained credential, at both the
    connection-session unit level and the full app integration level.
  • A second report for the same dead session is a no-op (latch); a fresh
    sign-in resets it so a later distinct auth loss is handled again.
  • Credentials are still fully cleared even when the injected onAuthLost
    throws.
  • The detached/expanded Data Pane view's own refresh failure now reports
    auth loss too.
  • The existing 401/403-after-authConfirmed "stays a query error" contract
    is untouched (no changes to src/net/ch-client.ts) and still covered by
    its own tests.

Checklist

…y auth loss too

app.signOut cancels every in-flight authenticated operation (Workbench
request + KILL QUERY, schema graph, both export modes, catalog) and
closes the doc pane before clearing credentials and rendering login —
but onAuthLost (expired/invalid token, failed refresh, or a concurrent
first-contact auth failure) only rendered login, skipping all of it.
Factor the teardown into one idempotent teardownAuthenticatedSession()
shared by both paths.

chCtx.onSignedOut now notifies onAuthLost before clearing credentials,
not after: the teardown's fire-and-forget KILL QUERY / export-cancel
requests need getToken() to still see a valid token when they reach it,
or they silently no-op instead of reaching the server.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017E2cHgYTiobxSJLnCxgCPj
BorisTyshkevich and others added 2 commits July 28, 2026 12:00
Independent review found the first pass didn't actually cover one of the
three triggers #502 lists: getToken() cleared credentials itself on a
failed proactive refresh, before its caller (which always routes a null
getToken() into onSignedOut/onAuthFailed) ever got a chance to run the
teardown with a still-usable token. Credential clearing is now solely
onSignedOut's job, run after the teardown.

That change makes the teardown's own fire-and-forget authenticated calls
able to rediscover the same dead token and report auth loss again, so
onSignedOut now latches once it starts handling a dead session (reset
only by a genuine fresh sign-in via setTokens/connectBasic) to stop that
recursing. onSignedOut also now clears credentials in a finally, so a
throwing onAuthLost callback can't skip cleanup.

Filed #522 (inbox) for a third, separately-shaped instance of the same
family of bug the review surfaced: results.ts's detached-view refresh
never routes its own auth failure into onSignedOut at all.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017E2cHgYTiobxSJLnCxgCPj
…m results.ts

Round 2 stopped getToken() from clearing credentials on a failed proactive
refresh, on the theory that this would let the teardown's own authenticated
calls use the retained token. That didn't work: every one of those calls
still goes through the same getToken(), which discards a token whenever its
own refresh attempt fails, regardless of whether the token itself is still
usable — so a nested call during teardown still saw null and authedFetch
still bailed before reaching the server.

chCtx.onSignedOut now freezes chCtx.getToken to resolve the retained token
directly (no refresh, no expiry check) for its own synchronous duration,
restoring it afterward. This works specifically because authedFetch reads
and calls ctx.getToken synchronously, before its own first await — so a
nested fire-and-forget call captures the frozen value before onSignedOut's
finally ever runs.

Also considered, and deliberately NOT done: freezing ctx.authHeader/
ctx.refresh the same way. Both are read by authedFetch only in the
continuation after its own await (refresh, only after an actual 401
response) — well after onSignedOut has already restored/cleared everything
— so freezing them would be dead code with no real effect. That's why a
Basic-mode session's authHeader-scheme bug (#520) genuinely isn't fixed by
this change, despite an earlier belief that it would be.

Separately: src/ui/results.ts's detached/expanded-view refresh preflight
called ensureFreshToken() but never reported the failure to onSignedOut at
all — unlike every other call site in the app. Before round 2, getToken()'s
own clearTokens() call at least silently wiped storage on this path; after
round 2 removed that, credentials would linger uncleared indefinitely if
this was a user's only live activity. Fixed for real (not filed as inbox):
ResultsApp.conn now also carries chCtx.onSignedOut, and rerun() calls it on
failure.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017E2cHgYTiobxSJLnCxgCPj
@BorisTyshkevich

Copy link
Copy Markdown
Collaborator Author

Superseded by merged #524, which carries this teardown/race evidence into the authenticated execution-scope design for #512.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant