fix(viewer): tear down the room model when an abandoned join returns (#3016) - #3048
Conversation
…struct (#3016) The recipient join registers `room:<id>` as a real model and installs the teardown that removes it only after `await reconstruct()` returns. The abandoned-join guard sits below that assignment and returned without running it, leaving the model in `models` — and the doc listener attached — until the next `stopCollab`. The guard now runs the teardown this join installed. It runs the join's own closure rather than the module-level slot: a newer join may already own that slot, and running its teardown would drop the room model of the session the user is actually in. Publishing into the slot is likewise now conditional on this join still being the live one, which closes the mirror-image leak — a stale continuation used to overwrite a newer join's teardown, so the newer room's model was never removed on the next Leave.
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
… on CI's node `collabSlice.leave-after-reconstruct.test.ts` timed out on `Viewer tests (shard 3)` — 60s on the first case, then the 120s file cap — while passing locally. The cause is not the diff under test: the loader hook that parks the recipient reconstruct never applied. `module.registerHooks` (synchronous, in-thread) landed in Node 22.15.0 and tsx feature-detects it. On a newer 22 the `@/lib/collab/geometry-sync` alias is resolved to a `file://` URL by that sync path BEFORE this async `register()` hook is consulted, so `specifier === TARGET` misses, `hydrateGeometryFromRoom` is never wrapped, `__collabHydrateGated` never fires, and every case in the file waits forever at `await atGate`. `collab-session-race-hook.mjs` already documents this exact trap; this hook was written against the specifier and reproduced the bug. Reproduced rather than inferred: on 22.13.1 the file passes, on 22.23.2 it fails 3 of 3 with the CI signature byte for byte — the same `annotation sync setup failed` at collabSlice.ts:1229 (the live path, i.e. the abandoned-join guard was never reached), the same `Cannot unregister unknown model: room:room-3016` from the next case running the stale module-level teardown slot, the same 60s and 120s timeouts. After the fix: 20 of 20 on 22.23.2 and 20 of 20 on 22.13.1. The gate is still doing its job rather than passing vacuously: with `collabSlice.ts` reverted to main and the fixed hook, 22.23.2 reports two assertion failures, not timeouts.
|
Two different things were red here. One is your flake; one was genuinely ours, and it was not a flake at all — it fails 0/3 deterministically on the Node CI actually runs. Fixed and pushed Shard 1 — yours
Shard 3 — ours, and worth reading
Root cause is not the teardown logic and not concurrency. The test parks the recipient reconstruct with a My machine runs 22.13.1. CI pins Verified rather than inferred: downloaded 22.23.2 and ran the file unchanged. Fails 3/3 with your CI signature byte for byte — the same
Concurrency was never it: 8/8 green locally at concurrency 4 under ten CPU hogs. The fix is one file, Not vacuous either way: with Full shards on 22.23.2 at concurrency 1: shard 1 1328 pass / 0 fail, shard 3 1278 pass / 0 fail / 2 skipped. A correction on my own reasoning, and a suggestionI had been treating the shard number as identifying: "this is shard 3, so your cleared shard-1 flake cannot explain it." That is invalid — sharding is The suggestion: the repo's sibling hook |
2 participants
Closes #3016. The abandoned-join guard returned without running or nulling
recipientLiveTeardown, so the reconstructedroom:<id>model outlived the session the user left — until the nextstopCollabhappened to clear it.RED:
The guard now runs
ownLiveTeardown— this join's own closure, held in astartCollab-scoped local — beforesession.dispose().A second leak, found while fixing the first
My initial shape (run own teardown, clear the slot if it is ours) still failed the two-join case. Instrumentation showed why: a stale continuation resumes after a newer join has published its teardown, and the unconditional assignment overwrote it — so the newer room's model was never removed on the next Leave.
That is the mirror image of the reported leak, it is pre-existing, and nothing covered it. Publishing into the module-level slot is now conditional on this join still being the live one.
Both checks read
collabRoomIdagainst this join'sroomId— the same granularity as every other re-check instartCollab— so neither distinguishes a rejoin of the same room. Left as-is deliberately and stated in the changeset rather than silently narrowed.Reverse direction checked
A model the session did not create (
local-file) survives the leave. And after the two-join race, the live room's model survives and its teardown is still armed — leaving removes it. Both matter: a fix that removed too much would look identical in the reported test.2 guards, 2 mutants, 2 killed: guard does not run the teardown → tests 1 and 3 fail; unconditional publish → test 3 fails.
New tests 3 pass;
collabSlice.leave-during-join-race,entry-race,gates,session-failure16 pass, 0 fail.tsc --noEmitclean, oxlint clean.Test infrastructure:
src/test/collab-hydrate-gate-hook.mjs, a per-call loader gate modelled on the existingcollab-session-race-hook.mjs. It is the only way to park a join between the model registration and the guard — every earlier await is followed by acollabRoomIdre-check that returns before the model exists, so no other seam can reach the window.Deliberately not touched
The issue notes two other homes not cleared on leave: the
?room=&t=URL params, andShareDialog.tsx:174minting a new room while open. Both left alone — they are separate surfaces from the store teardown, the issue frames them as "worth recording", and the URL one changes reload behaviour I have no test seam for here.🤖 Generated with Claude Code