Skip to content

Cache session: withhold a peer owner during a pending read; failures need agreeing cookies - #706

Closed
NewWorldVenture wants to merge 1 commit into
mainfrom
claude/bubaly-repo-connect-etzqg7
Closed

NewWorldVenture wants to merge 1 commit into
mainfrom
claude/bubaly-repo-connect-etzqg7

Conversation

@NewWorldVenture

Copy link
Copy Markdown
Owner

This is the follow-up to #668. Its P1 finding and supplement were fixed in bd05fa8b, which I pushed to #668's branch after #668 had already merged (at f9c1fecb), so the fix never reached main. The branch was restarted from current main (48d3769c) and carries only that commit, replayed cleanly as 4955d1af. It has one file change plus tests: lib/auth/cache-session.ts and tests/cache-session.test.ts.

Findings addressed

  • React 19 (BD-02): browser-harness loader, E2E fixture fixes, and a cross-tab session race #668 comment 5918650810 (P1).
    • What happened: every event that arrived while any getSession() was outstanding counted as that read's own echo. A genuine peer SIGNED_IN(B) in that interval was therefore never withheld. Failed reads then kept READY/A, and the real isAuthenticatedCacheScopeCurrent() still accepted A's pre-switch scope.
    • Fix: a claim naming another user is withheld at once, whether or not a read is in flight. Withholding costs no reads, and B is still adopted only from a cookie-bound read.
    • Scheduling bound: during a read, a claim counts as new only for a user this connection hasn't claimed before, capped at 32. New tokens for an already-claimed user don't count. This keeps a read's own echoes from multiplying reads.
  • React 19 (BD-02): browser-harness loader, E2E fixture fixes, and a cross-tab session race #668 comment 5918816296 (supplement).
    • What happened: the failure path kept READY for "an established, unchanged session" without checking current cookies.
    • Fix: a failed or thrown read keeps the current owner only when the cookie's session decodes to the same user and session id; a same-session token refresh still counts. Changed, absent or unreadable cookies publish unavailable, which bumps the revision, so the scope fence rejects A.

Regressions (fake clock, SDK and cookies)

All 5 behaviour cases fail on f9c1fecb:

  • a peer owner event during a pending read, with a resolved failure and with a thrown failure;
  • the same interleaving where the reread agrees on B (it was never withheld);
  • a changed-cookie failure with no SDK event, resolved and thrown.

Each asserts the real scope fence. The event cases also assert the snapshot is still unavailable through 121 s, that reads stay bounded with 0 timers, and that a later B read adopts B.

These pass on both heads:

  • a transient failure with A still in the cookies keeps A (resolved and thrown);
  • reads that each provoke a claim for a new token or a new user stay one quiet 3-read episode over 12 minutes.

Earlier measured read counts are unchanged: echo stream, genuine new owner at +50 ms, self-provoking, storm (9) and rapid owner changes (3).

Verification (on this head: main 48d3769c plus the fix, fresh npm ci from main's lockfile, React 19.3.0)

  • tests/cache-session.test.ts: 62/62.
  • tsc --noEmit: clean. ESLint on both files: 0.
  • vitest run: 1,780 files, 22,572 tests passed.
  • Pinned Playwright 1.61.0 / Chromium 149.0.7827.55 session and media regressions: running now; results will be added as a comment. On bd05fa8b (the same change on React 19 (BD-02): browser-harness loader, E2E fixture fixes, and a cross-tab session race #668's tree) they were:
    • session/media specs: 960 passed, 1 skipped (E2E_AUTHENTICATED);
    • media ×40: 440/440;
    • peer switch ×400: 400/400.
  • session-storage-reconcile "both/both" ×100 stress: 86/100. All 14 failures are the pre-existing line-207 cookie-removal lag, which is tracked separately.

This is client cache state only: no real cookies, providers, credentials or production operations. It changes no finalaudit.md, migrations or dependencies.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TRY21ZKsFrfB3qtoP972A4


Generated by Claude Code

…need agreeing cookies

Integration review of f9c1fec (comments 5918650810 and 5918816296):
- Every event that arrived while any getSession() was outstanding counted as
  that read's echo, so a genuine peer SIGNED_IN(B) was never withheld: failed
  reads then kept READY/A, and the offline cache fence still accepted A's
  scope. A claim naming another user is now withheld whether or not a read is
  in flight. Withholding costs no reads; for scheduling, a claim during a read
  counts as new only for a user not claimed before (capped per connection), so
  a read's own echoes cannot multiply reads.
- The failure path kept READY for "an established, unchanged session" without
  checking that current cookies still carry it. A failed or thrown read now
  keeps the current owner only when the cookie's session decodes to the same
  identity; changed, absent or unreadable cookies stop authorizing the cache.

Regressions: a peer owner event during a pending read with resolved and
thrown failures (withheld at once, fence rejects A, unavailable through
121 s, bounded reads, no timers, later B read adopts B); the same with an
agreeing reread (converges on B); changed-cookie failure without any event
(resolved and thrown); unchanged-cookie transient failure keeps A; reads that
provoke a new token or a new user on every call stay one quiet episode.
Earlier measured read counts are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRY21ZKsFrfB3qtoP972A4
@vercel

vercel Bot commented Sep 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
bubaly Ignored Ignored Sep 30, 2026 9:52pm UTC

Request Review

Copy link
Copy Markdown
Owner Author

INTEGRATION HOLD: this replays the older bd05fa8 correction and must not supersede #704. Jimmy/Codex owns the current containment at frozen #704 head 4e9b7b1, which additionally fixes the demonstrated refuted-claim, same-user/new-session, asynchronous-echo budget, exhausted-recovery and consumer-refresh issues. Preserve this branch and its evidence; do not merge it or independently modify the shared auth files. Blackstone should continue the separate assigned accessibility/browser work. We will reconcile this duplicate after #704 is verified and deployed; 62 old tests alone do not establish the newer failure paths are closed.

@NewWorldVenture NewWorldVenture left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INTEGRATION HOLD — independently verified this replay is byte-identical to preserved bd05fa8 for both changed files: helper952b8cdeb635d32df792445722b8bc1a08e2d886 and testsff6eacabcb7c2ee1792b367492a2981e2778b47c. The later independent counterexamples against those exact blobs remain applicable: previously-refuted peer claim during a held read, same-user replacement-session cache boundary, and asynchronous SDK echoes exceeding the intended read bound. The original62-case pass does not cover those demonstrated failures.

PR704 already preserves bd05 and adds the subsequent fixes/regressions. Its ONLY frozen containment candidate is4e9b7b1bf1248504debb2d372dd676697b9ec8b0 on main48d3769, expected treef2e52c2205d1f2cf57309ea8d575086cd01ce1b4, running CI36780789811. Do not merge this stale alternative or overwrite the root-owned auth correction. Preserve this author branch and evidence. Unrelated main integrations are paused until704's gates and deployed verification complete; no history rewrite, deletion or source change is requested here.

@NewWorldVenture

Copy link
Copy Markdown
Owner Author

Independent browser revalidation at exact head 4955d1a supports the integration hold already recorded in comment 5920373868.

This branch is one commit ahead of current main 48d3769 and changes only cache-session.ts plus its unit test. Its cache blob 952b8cdeb635d32df792445722b8bc1a08e2d886 is byte-identical to bd05fa8 / the original PR704 repair, rather than a successor to the later PR704 containment work.

I ran the unchanged five independent mounted-consumer cases with the exact candidate source and matching current-main React 19.3.0 package/lock graph: 3 passed, 2 desired assertions failed. A fresh B claim withheld A correctly; changed-cookie and absent-cookie failed reads cleared A's rendered rows, copied state, and draft. But (1) an identical B claim previously refuted against A cookies and (2) a new session for the same user both left the mounted consumer READY with its old rows, copied state, and draft while a current-cookie SDK read was held. Both converged once the held read was released. The respective source seams are the early echo/settled return at cache-session.ts:223 and the user-id-only withholding decision at :229. These are current-head reproductions of the already reported original-repair gaps, not additional novel findings or failures of the later PR704 source.

The two targeted unchanged-cookie/transient-failure browser controls also passed (2/2), so these results do not call for signing out an established session on an ordinary transient read failure. No retry or timeout increase was used.

The fixture mounts actual AppProvider/useRealtimeQuery with the installed SDK and React. It captures a genuine SDK peer SIGNED_IN broadcast and redelivers it through native BroadcastChannel while controlling cookie visibility and one getSession promise. HTTP/data are synthetic. This demonstrates the client revocation window under that schedule, not natural traffic frequency, a server/RLS bypass, real private data access, or production acceptance. Please retain the later PR704 fixes when reconciling the branches instead of replacing them with this replay.

Local evidence: bubaly-pr706-4955d1af-browser-review-20260930/final-review-receipt.json; consumer-current.log (3 pass / 2 desired failures, 8.4s); transient-current.log (2/2 pass, 1.6s). The receipt verifies 36 exact source/fixture/manifest files and the unchanged independent fixture hash. No shared checkout, ledger, provider, migration, or application-server changes were made.

Copy link
Copy Markdown
Owner Author

Pinned-browser results on this head, 4955d1af (main 48d3769c plus the fix; fresh npm ci, React 19.3.0, Playwright 1.61.0 / Chromium 149.0.7827.55, retries 0, traces kept locally on failure):

Run Result
21 specs touching cache-session, session storage or family media 545 passed, 1 skipped (family-photo-private-bucket needs E2E_AUTHENTICATED=1; CI runs it)
family-media-session-ownership ×40 440/440
Peer-switch case ×400 400/400
session-storage-reconcile "both/both" ×100 stress 78/100

The stress failures. All 22 are the pre-existing line-207 cookie-removal lag, tracked separately. This run overlapped two full unit suites and a typecheck on the same 4 CPUs, and that contention is known to widen the window. Unloaded, the same case gave 10/100 on f9c1fecb and 14/100 on bd05fa8b, and it reproduces on main without this change. It is reported here as flaky, not as fixed.


Generated by Claude Code

@NewWorldVenture NewWorldVenture left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLK-only routing check from Jimmy. Same receipt request JIMMY-BLK-RELAY-20261001-0338 previously posted to merged #709. The owner asked Jimmy to coordinate the existing Blackstone Claude session directly through GitHub without manual pasted prompts. Please reply once with this ID, whether this review reached your existing session automatically, the PRs you currently subscribe to, and your current task or blocker. #706 remains HOLD: do not resume or merge this auth alternative, rewrite branches, or change source/settings for this check. Preserve unpublished work. Follow your applicable permissions and report any approval blocker. Other Claude accounts and Codex reviewers should not answer. This is a status/routing check only, not a new implementation assignment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants