fix(auth): await refreshed credential handoff (ENG-2116) - #806
Conversation
Refs: ENG-2116
Refs: ENG-2116
Refs: ENG-2116
The 25s hold outlived the renderer's stranded-reservation reap, which aborts at ~13-20s and swallows the AbortError, losing the message with the thinking placeholder still on screen. Bound the barrier under that floor so a slow wake fails visibly instead of silently; the retry loop underneath still converges. Release the barrier on syncUsableMindsCredential rather than on the PUT landing alone: sleep during the loopback call leaves the abort timer on a clock that does not advance, so a push can complete on wake carrying a JWT that expired hours earlier. Reset the gate on sign-out instead of latching it shut, drain a pre-sleep exchange the way refreshAfterOrgSwitch does, back off both retry loops, and gate /responses/answer, which also puts LLM work on the wire.
PR environment is up ·
|
| Console | https://pr-cowork-806.dev.mindshub.ai |
| Auth API | https://auth-pr-cowork-806.dev.mindshub.ai |
| Cowork Web | https://cowork-pr-cowork-806.dev.mindshub.ai |
| Keycloak Admin | https://auth-pr-cowork-806.dev.mindshub.ai/auth/admin/ |
| Inference API | https://api-pr-cowork-806.dev.mindshub.ai/v1 |
| Cowork Server API | https://cowork-pr-cowork-806.dev.mindshub.ai/api |
| Namespace | pr-cowork-806 |
Every service is in this namespace. The ones this PR does not build run the staging image unless you link them with Deploys: in the PR body.
Updated on every push to this PR.
GitHub Actions
Inactive
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core authentication/refresh and request-interception behavior in the Electron main process, which is high-impact and warrants final human review despite strong automated coverage.
Pull request overview
Implements a hotfix to ensure refreshed MindsHub JWTs are not considered “successful” until they are handed off to the running sidecar, and adds a bounded “resume gate” that can temporarily hold response-creation requests after wake so long-running turns don’t run on an expired credential.
Changes:
- Adds a resume credential gate with a strict 12s ceiling and explicit reset semantics for logout.
- Serializes credential selection + sidecar handoff, and makes token refresh await “usable + landed” credential delivery (with handoff-only retry on refusal).
- Introduces a narrow request gate in the Electron loopback interceptor to defer only response-creation POSTs when the sidecar reports the runtime credential is required.
File summaries
| File | Description |
|---|---|
| src/main/minds-resume-gate.ts | New bounded gate primitive for post-resume credential readiness. |
| src/main/minds-resume-gate.test.ts | Unit tests covering gate lifecycle, timeout behavior, and reset/unblock semantics. |
| src/main/minds-response-request-gate.ts | New logic to identify and gate response-creation loopback requests during resume refresh. |
| src/main/minds-response-request-gate.test.ts | Tests for request classification, fail-closed health probing, and timing bounds. |
| src/main/minds-credential.ts | Serializes sidecar handoffs; adds “usable” selection semantics and exposes BYOK detection. |
| src/main/minds-credential.test.ts | Tests for serialization ordering, usable/landed quadrants, and gate interactions. |
| src/main/minds-auth.ts | Refresh now awaits sidecar acceptance; adds handoff-only retry, backoff strategy, and logout fencing. |
| src/main/minds-auth.refresh.test.ts | Extensive refresh/resume/logout/timeout regression tests for the new contract. |
| src/main/minds-auth.orgs.test.ts | Updates org-selection tests to include sidecar routes under the new handoff behavior. |
| src/main/index.ts | Wires resume refresh on OS resume; adds loopback request gating; strengthens logout fencing/reset. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alecantu7
left a comment
alecantu7
left a comment
There was a problem hiding this comment.
Review — de8be78b (hotfix to main)
I reviewed and approved the staging counterpart (#778 @ 6d789c04), including the wake barrier and the sign-out fence. This carries that content unchanged.
Verified identical to the approved version
Diffing the ENG-2116 files between 6d789c04 and this head — minds-auth.ts, minds-credential.ts, minds-resume-gate.ts, minds-response-request-gate.ts, index.ts — returns empty. Byte-identical. So everything I verified there still holds without re-deriving it: the usable && landed conjunct and its quadrant test, the wake barrier with no constructible wedge, the expiry path cancelling rather than sending a stale JWT, and the sign-out fence's try/finally depth counter plus the unconditional resetMindsResumeCredentialGate().
The larger diff against the approved head is main-vs-staging drift (the ENG-2044 artifact-download surface and related work that exists on staging and not on main), not content this PR introduces.
Carried forward, unchanged
The one item I could not settle on #778 applies here identically, and now against production: the 12s barrier is justified as clearing a ~13s reap, but reservationReleaseDecision uses unseenThreshold = 4 with minMissSpacingMs = 4000 and the tally is driven by the 5s interval and an off-cycle onFocus handler — so focus polls at the 4s floor give 0/4/8/12, exactly equal to the bound. Not blocking and not a claim; the arithmetic is solid and the reachability is what I could not close. Also unchanged: the commit rewrote the old absolute assertions into self-referential MINDS_RESUME_READY_TIMEOUT_MS - 3_000 forms, so reverting the constant to 25_000 leaves the suite green.
Merge position
This is last. Against a sidecar without cowork-server#444, minds_runtime_credential_required is absent → null → the gate goes conservative. That is the correct fail-safe, but its cost lands on users rather than CI, so the order matters more here than the green ticks suggest.
Two things that apply to the whole hotfix set
Six open PRs now exist for ENG-2116 — anton#421 / cowork-server#428 / cowork#778 against staging (all three approved), and anton#429 / cowork-server#444 / this set against main. Both tracks carry the same change. Whichever ships, the other wants closing or rebasing deliberately, and per the repo convention main → staging gets merged back by hand — nothing does it automatically, so staging will otherwise sit behind production carrying an approved duplicate of what already shipped.
main is production. These merge straight to it, so the usual staging soak does not happen. That raises the cost of the pin step below from "housekeeping" to "the thing that breaks the build".
Verdict: COMMENT. No new findings. Not an approval.
Three defects from self-review, all in behavior this branch introduces. Sign-out bumps the cancellation epoch before getRevokeToken joins an in-flight refresh, so that exchange returned superseded ahead of saveTokens. Keycloak had already rotated the refresh token and invalidated the one we sent, so endKeycloakSession posted a token the IdP rejects: the SSO session survived sign-out and the next sign-in silently reused the same account with no picker. The window is structural, not rare, because the scheduled refresh fires exactly while getRevokeToken's cache guard fails. Persist ahead of the fence; only the write escapes it. The server-start hook pushed the credential without settling the resume barrier or cancelling the retry ladder, alone among the landing paths. A sidecar restarting during a post-wake retry came back holding a usable credential while turns were still held their full bound and cancelled. Boot refreshes tokens before it starts a sidecar, so the hand-over warning and its retry timer fired on every launch of a signed-in install and buried the real failures the log exists to surface. Only a sidecar that exists can refuse, so ask minds-credential whether one is reachable. Tests cover all three plus the two resume-gate guards nothing pinned; each fails against its own mutant. The README now documents the wake barrier, which is user-visible and was described nowhere. Refs: ENG-2116
GitHub Actions
Failure
alecantu7
left a comment
alecantu7
left a comment
There was a problem hiding this comment.
Approving
Re-verified against this head — the branch moved after my last review, and 76b401e4 is not a trivial delta, so I reviewed it rather than diffing it.
The new commit fixes two real bugs
A stranded rotated refresh token. The previous fence returned before saveTokens when sign-out cancelled mid-exchange — but Keycloak had already rotated and invalidated the token we sent, so endKeycloakSession was handed one the IdP rejects: the SSO session survived sign-out and the next sign-in silently reused the same account with no picker. Moving the persist ahead of the fence is right, and the comment explains why only the write escapes.
A barrier left armed by a restarted sidecar. The server-start hook pushed the credential but never settled the resume gate or cancelled the retry ladder, so a sidecar restarting during a post-wake retry came back holding a usable credential while /api/v1/responses kept being held its full bound and cancelled. handOffMindsCredentialToStartedSidecar closes that.
What I went after and could not make stick
Moving saveTokens ahead of the cancellation fence looked like it should let a late response resurrect tokens after clearTokens() — getRevokeToken is a 5s Promise.race, so the refresh can outlive it, and saveTokens has no guard of its own and calls broadcastAuthChanged(true). The index.ts comment claiming the post-lookup fence prevents a late write is, strictly, no longer the mechanism that does it.
But the behaviour is still safe: doRefreshTokens checks getTokenStoreVersion() !== tokenStoreVersion before saveTokens, and clearTokens() bumps that version — so a late response after sign-out returns superseded and never writes. Only a rotation where the store is otherwise untouched escapes, which is exactly the stated intent. Recording the reasoning because the index.ts comment now points at the wrong guard, and the next reader will check it.
Everything from the earlier rounds still stands: the usable && landed quadrant, the wake barrier with no constructible wedge, the expiry path cancelling rather than sending a stale JWT, and the sign-out fence's try/finally depth counter. Main project 975 passed / 66 files at this head.
Carried forward, unchanged
The 12s barrier arithmetic I could not close on #778 applies identically here, now against production: unseenThreshold = 4 with minMissSpacingMs = 4000, and focus polls at the 4s floor give 0/4/8/12 — exactly the bound. Not blocking, not a claim. Same for the self-referential timeout assertions: reverting the constant to 25_000 leaves the suite green.
The pin sequence is a precondition on merge, not on this approval. cowork-server#444 pins anton to a rev on an unmerged branch, and its own comment says so. Order: merge anton#429 → re-pin #444 to the resulting anton main commit (or branch = "main") → regenerate the lockfile → merge #444 → merge cowork#806. The image installs via uv sync --frozen, so a rev on an unmerged branch reaching a cowork-server main build is a broken image, not a stale one.\n\nBoth tracks are still open. anton#421 / cowork-server#428 / cowork#778 target staging and are also approved. Whichever ships, the other wants closing or rebasing deliberately — main → staging is a manual merge here.
Approved on Alejandro's call. Merging is his, and this one is last.
User story
As a signed-in Cowork user
I want an active turn to adopt refreshed MindsHub credentials and recover from one isolated authentication refusal
So that long-running work is not discarded or blamed on my account
Hotfix scope
This is #778 replayed onto
mainso the fix can ship without waiting for the 431-commitstagingbacklog. All five commits are byte-identical to the ones onfix/eng-2116-refresh-active-jwt, confirmed withgit range-diff; nothing else fromstagingcame along, and there is no extra commit here. #778 stays open againststaging, and the backmerge resolves it.Why this matters
The desktop can refresh a short-lived MindsHub JWT while the running sidecar still holds the old value. After sleep or a long turn, the next model call can therefore fail as if the user's account were disconnected even though the desktop session is healthy.
Acceptance criteria
How to test
Notes for the reviewer
Refresh and delivery are separate outcomes.
okmeans the new token was both stored and accepted by the sidecar. A sidecar refusal returnshandoff_pendingand schedules a handoff-only retry, so Keycloak is not rotated twice.The wake barrier is bounded by the renderer, not by the refresh.
reconcileInFlightreaps a reservation the server never saw after four unseen polls, at roughly 13 seconds, and_streamResponseswallows the resultingAbortError. A hold that outlives that loses the message with the thinking placeholder still on screen, so the barrier gives up first and the request fails visibly instead.The wake barrier is narrow. It intercepts only same-port
POST /api/v1/responses[/]while a near-expiry resume refresh is active. The sidecar health flag decides whether the resolved required roles need the runtime Minds credential; an explicitfalsebypasses the barrier, while an unknown or failed probe remains fail-closed.Cancellation is independent of retry sequencing. Logout advances a cancellation epoch checked after every asynchronous selection, refresh, handoff, and repair step. Ordinary retry completion does not invalidate a concurrent or newer refresh.
Verified locally, against
mainRun in a worktree cut from
origin/main(25db478f) with these five commits replayed on top.npm test(vitest run)npm run test:coverage, which is what CI actually runsminds-credential.tsholds its pinned 100% statements and 100% branches after a 117-line changeminds-authexports and both new modules resolve at HEAD, and the clean typecheck covers the renamed-export class of breakage this replay was most at risk forgit range-diffagainst #77876b401e4)Ships with
Deploys: mindsdb/cowork-server#444
Merge order: Anton #429 first, then cowork-server #444, then this desktop PR.
Deployment: cowork-server is the sole preview/deploy anchor.