Repository navigation
fix(desktop): survive losing the auth-token lock mid-write - #6790
Conversation
proper-lockfile keeps an acquired lock fresh on a timer. When another desktop main process reclaims the entry because our refresh fell behind, that timer's stat returns ENOENT and the library's default onCompromised handler rethrows from inside its own setTimeout, where no caller can catch it. Supply a handler so the lost lock is recorded instead. The same event marks the lock released, so the finally-block release() rejects with ERELEASED and replaces the operation's result -- reporting a token that was written as a sign-in that failed to save. Releasing is cleanup and says nothing about whether the write landed, so it no longer overrides the operation's own success or failure. Acquisition failures and write failures still reject, so a token that was not persisted can never look persisted. Refs DESKTOP-11M Claude-Session: https://claude.ai/code/session_01PXMCuUAHrKLAhRWtPF1o7V
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe authentication lock wrapper now logs compromised locks and release failures. Token-saving tests verify that failed lock acquisition produces no token file or ChangesAuthentication lock handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized fix to prevent auth writes from being reported as failed when lock cleanup races occur, with acquisition and write failures still propagated. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…write (#6792) * test(desktop): pin that a reclaimed auth-token lock cannot crash the write Nothing covered the path #6790 fixed. Deleting the onCompromised handler from withAuthLock reintroduced a fatal, uncatchable throw on the sign-in token write and the whole suite stayed green. The refresh proper-lockfile arms after handing us the lock only fires on a timer derived from `stale` -- five seconds at the current setting -- which is too long to wait for in a suite that runs in under a second. Intercept that one scheduling call instead: remove the lock entry the way a second desktop process reclaiming it would, then run the library's own refresh callback. Its stat, its ENOENT handling, its compromise bookkeeping and its onCompromised dispatch are all the real thing; only the five-second delay is skipped, and the library is never mocked. Refs DESKTOP-11M Claude-Session: https://claude.ai/code/session_01H1nPckannUVVytgNdnd3YG * test(desktop): wait for the reported lock loss instead of racing it The compromise is reported from proper-lockfile's own stat callback, which can land after saveToken resolves. Reading the count straight after the write made that assertion pass or fail on scheduling; await the report instead. A loss that is never reported now hangs to the test timeout, which is the failure we want, and removing onCompromised still fails fast with the library's ECOMPROMISED throw. Claude-Session: https://claude.ai/code/session_01YXpYt5vhSMHdqFPUymBdc6 --------- Co-authored-by: Satya Patel <satya@superset.sh>
Problem
DESKTOP-11Mplus roughly 20 sibling groups over 90 days (releases 1.18.3, 1.19.0, 1.20.2, 1.22.0) are all one event in the desktop main process:reported
level=fatal,handled=no,mechanism=generic, with no stack. It splits into a new group per machine because the message embeds the lock entry's absolute path, which contains the account name — so the family is self-multiplying and every new group puts an account name in an issue title.User-visible harm: when this fires, the auth write that was in flight succeeded on disk but is reported to the user as failed.
handleAuthCallbackcatches the rejection, puts the CSRF state back, and shows "Superset could not save your sign-in to …"; thepersistToken/signOutmutations fail the same way in the renderer. The user is told their sign-in did not persist when it did, and is pointed at a file path to go inspect.The app itself does not quit:
main/index.ts:276registers anuncaughtExceptionlistener, and@sentry/electron's handler only shows its error box when fewer than three listeners exist. So apart from the wrong outcome, the crash is invisible — except that it marks the session crashed in release health. Rare (~20 events / 90 days), but it lands on the auth path and the message it produces actively misinforms.Root cause
Not what the ENOENT looks like. Two things had to be ruled out first, both against the installed
proper-lockfile@4.1.2with this call site's exact options:mkdir'sEEXISTand thestatthat checks its age —lib/lockfile.js:56-62retries withstale: 0. Reproduced directly: acquisition resolves, no error escapes. A persistent version of that same failure surfaces asELOCKED, never asENOENT.saveToken/clearToken/saveOrganizationIdsall go throughserializeAuthWrite, whose promise is observed twice (by the caller and by the queue chain), and every caller awaits it —persistToken,persistOrganizationIds,signOut, andhandleAuthCallback, which already wraps the write intry/catch. No handler on the write path could have caught this one.What actually happens: after
lock()succeeds, proper-lockfile keeps the entry fresh on a 5s timer (stale / 2). If a write is still in flight when that tick lands and another writer has meanwhile reclaimed the entry — our refresh fell behind, so our lock looked stale to them — the refreshfs.statreturns ENOENT and the library callsonCompromised. We never supplied one, so the default(err) => { throw err }throws from inside its ownsetTimeoutcallback. Nothing can catch that, and it explains the missing stack: asyncfserrors carry no JS frames. The capturedlevel: 'fatal'/mechanism: { handled: false, type: 'generic' }matches@sentry/electron'sonUncaughtExceptionIntegrationverbatim, which is how we know it arrived as an uncaught exception rather than a rejection.Note
Corrected during triage review, before merge. This paragraph originally claimed that two main processes on one machine are reachable because the app never calls
requestSingleInstanceLock(). That is wrong.main/index.ts:344does take a single-instance lock and callsapp.exit(0)when it loses, so a second copy of the same build cannot race this entry; and dev/worktree setups pointSUPERSET_HOME_DIRat their own directory, so those do not contend either.What the event actually establishes is narrower, and the fix does not depend on the difference: the lock entry was gone when our own refresh timer stat'd it. Anything that removes
~/.superset/auth-token.enc.lockwhile a write is in flight produces exactly this ENOENT, and the event does not record what did. The library throwing from inside its own timer is a defect in how we call it regardless of what removed the entry.proper-lockfileis imported nowhere else in the repo, so this lock has exactly one writer in our code.The same event has a second consequence. Losing the lock marks it released, so the
finally { await release() }rejects withERELEASED— and a rejection thrown from afinallyreplaces the result of thetry. That is the step that turns a token that was written into a sign-in reported as failed.Fix
Two lines in
withAuthLock, both about the same lost-lock event:onCompromised, so the lock library reports the loss to us instead of throwing it into a timer. The write is atomic (temp file +rename), so it stays valid whoever else is writing; there is nothing to abort, and the operation goes on to report its own result.finallyrelease best-effort. Releasing is cleanup — it fails when the lock was already taken from us — and it says nothing about whether the write landed, so it must not override the operation's success or failure.On the two shapes proposed in triage. Neither is available, and the evidence above is why. Shape (i), making acquisition tolerate the vanished-entry race so the retries absorb it, is already true in the library — that path resolves today, so implementing it would be a no-op that leaves the fatal event exactly where it is. Shape (ii), handling the rejection on the write path, cannot apply: this is not a rejection but a synchronous throw from the lock library's own refresh timer, unreachable from any
try/catchwe could add, and the Sentry mechanism confirms it arrived as an uncaught exception. What both shapes were reaching for is the third option — the throw exists only because we never gave the library somewhere to report a compromised lock — so that is what this changes, at the same call site, with the same blast radius. I took the release fix with it rather than separately because it is caused by the identical event and without it the crash is gone while the behaviour is still wrong: the user still gets told a sign-in failed that in fact persisted.What this deliberately does not do: it does not swallow failures. Acquisition failures (
lock()rejects before thetry) and write failures (operation()rejects inside it) both still propagate to the caller unchanged. A token that was not persisted can never come back looking persisted. Timing parameters, encryption, file format, permissions repair, and the quarantine path are untouched, and there is no new Sentry capture — the two warnings useconsole.warn, matching the rest of the file, and reach Sentry as breadcrumbs via the existing console integration.Verification
Negative test, written first (
does not report a saved token when the write lock never frees up): a lock entry with a fresh mtime never looks stale, so acquisition runs out of retries.saveTokenmust still reject, must not emittoken-saved, and must leave no token on disk. It passes on unmodifiedmain, and — the point of it — it fails against a naive swallow-everything version of this change (try { … } catch { return undefined as Result }), together with the two negative tests the file already had:Reproduction of the reported event, before and after. Mirrors
withAuthLockwith the same options, a write still in flight at the 5s refresh tick, and another writer that reclaimed and released the entry:Suite —
bun test src/lib/trpc/routers/auth/, the file that holds the signed-in session:Lint —
bunx biome checkon both changed files:Types —
cd apps/desktop && bun run typecheck:Adjacent, named and left alone
app.on("open-url", async …)(main/index.ts:175) andapp.on("second-instance", async …)(main/index.ts:350) both float the promise their async handler returns. Neither can reject today —processDeepLinkawaitshandleAuthCallback, which already catches — so they are unguarded but not currently reachable, and they are not this bug. Left as-is.Refs DESKTOP-11M
https://claude.ai/code/session_01PXMCuUAHrKLAhRWtPF1o7V
Summary by cubic
Prevent desktop auth writes from surfacing as failures when the lock is reclaimed mid-write. Previously,
proper-lockfile’s refresh could throwENOENTas an uncaught exception andrelease()’sERELEASEDwould overwrite a successful write; now we handle compromised locks and make release best-effort.withAuthLock, passonCompromisedtoproper-lockfileto log and continue; wraprelease()in a.catch()so cleanup failures never replace the operation result.Refs DESKTOP-11M
Written for commit 18fc9b0. Summary will update on new commits.
Summary by CodeRabbit