Skip to content

fix(cli): invalidate the local data-plane room on doc unload [risk:high] - #4

Merged
zxch3n merged 5 commits into
mainfrom
fix/data-plane-invalidate-doc-room-on-unload
Aug 7, 2026
Merged

zxch3n merged 5 commits into
mainfrom
fix/data-plane-invalidate-doc-room-on-unload

Conversation

@zxch3n

@zxch3n zxch3n commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Risk: 🔴 high | Confidence: medium — forced 🔴 by CRDT/sync. Every mechanism here is now covered by a test verified by mutation, but the end-to-end symptom has not been reproduced in a running app, and residual holes found by review are documented below rather than fixed.

What was broken

repo.unloadDoc(docId) evicts the doc from loro-repo's instance cache, so the next
openPersistedDoc returns a different LoroDoc. LocalLoroDataPlaneServer resolves a
room's doc once (buildDocRoom) and holds that object while the room has subscribers.
A renderer with the session open keeps the room alive, so after an unload the room keeps
importing/exporting an orphan: renderer uploads never reach the CLI, CLI writes are never
pushed, the room still reports joined, and the 60s idle watchdog cannot notice because relay
pings are answered in parseLine before the room message chain. Re-joining does not repair
it either — ensureRoom returns the cached entry.

Session GC evicts a session idle for 20 min (cleanSessionForGC -> cleanSessionDoc ->
SessionDocument.destroy() -> repo.unloadDoc). On 2026-08-07 a session was GC'd at 06:19:55
and the user's next two turns (06:22:58, 06:25:03) never synced to the CLI. TurnHistoryGate
held the whole reply for its full 20s bound and then flushed 61 and 198 ACP updates in a single
batch each — nothing on screen for 20s, then the entire reply at once. The cloud transport
normally masks this; it was failing in the same window.

The change

  • LocalLoroDataPlaneServer.invalidateDocRoom(docId) — publishes a terminal room status,
    unsubscribes the stale doc, drops the room so the next join rebuilds it against the fresh
    instance, and bumps a per-room-key generation so a build already inside ensureRoom discards
    itself instead of installing the evicted instance.
  • LoroDocumentManager.unloadDocRoom(docId) — the single entry point. SessionDocument
    receives it by injection and no longer reaches for repo.unloadDoc, so the unload and the
    invalidation cannot be separated.

Update 2 — recovery is now room-scoped (09562db)

LocalLoroTransportAdapter did not act on a server-pushed terminal room status, so recovery ran
entirely through the workspace reconnect loop. That made one dead room cost a workspace-wide
reconcile — release every idle document store, rejoin every local room, and charge a backoff step
forgiven only after 30s of health. A session being GC'd is routine, so repeated invalidations
would ratchet local reconnect latency workspace-wide (the post-sleep reconnect-storm shape). The
adapter now re-joins that one room itself; the status stays terminal so the loop remains a
backstop.

The F9 tests no longer call adapter.reconnect() by hand — they assert recovery happens with
nobody driving it. Removing the self-repair fails 2 of 3 component tests and 2 F9 cases.

Two test-quality findings from the same review are also fixed: the single-entry-point guard had a
tautological line-number assertion, and a load-bearing comment in the CLI integration test
described a wait that does not actually cover what it claimed.

Update 1 — the post-install window (d3b13a0)

The re-review found the generation guard closed only half the race, and that the residual
half fails worse than the original bug: an invalidation landing between rooms.set and the
awaiting caller resuming hands back a dead entry, handleJoin registers the peer on it,
buildJoinReplyFrames finds no room and emits nothing — the client parks on connecting
forever, which is not terminal, so the reconnect loop never fires. handleUpdate had the same
window (import into the evicted doc on an unsubscribed entry). Fixed by re-reading this.rooms
in one synchronous block with the use, in both handlers — a helper does not work, because
returning from it is itself an await that reopens the window.

Also reversed the teardown order in SessionDocument.destroy: release the repo room before
evicting the doc, since loro-repo binds the instance into a room's transport attachment at attach
time and unloadDoc does not detach rooms. Free — the preceding setStatus writes the meta room,
not this doc's room — and it matches what the renderer already does in store-ref-tracker.ts.

The regression test sweeps the invalidation across fixed microtask depths instead of guessing the
distance, and asserts the invariant that actually matters: a joining peer is never left silent
(it gets either its joined reply or a terminal room-status). Five of eight depths fail without
the fix. Two earlier versions of this test passed with the fix removed and were discarded — one
re-read the doc map after its gate so the race could not manifest, the other fired before the
install and only re-covered the generation path.

Three things the first review corrected, each worth reading

1. The status value is load-bearing. The first version published reconnecting. Only
disconnected/error are terminal in createRoomSyncTracker.needsReconnect(), and that
predicate is what roomSyncRegistry.anyNeedsReconnect() — hence
createLocalReconnectLoop({ hasProblem }) — gates on. reconnecting reads as "already
recovering", so the loop never fired. Inbound sync was unaffected (the next join/update rebuilds
the room), which is exactly why the 20s freeze looked fixed while a renderer that was only
reading a session would have gone permanently stale.

2. ensureRoom had to become generation-aware. invalidateDocRoom only inspected
this.rooms, so a build still resolving its doc was invisible to it: the lookup missed, nothing
was published, and the build then installed a room bound to the just-evicted instance — the
original bug with no recovery signal. Reachable because unloadDocRoom awaits
repo.unloadDoc before invalidating.

3. Invalidation runs after the unload on purpose. Before it, a racing join re-opens the doc
into the repo cache just in time to be stranded again, silently. After it, updates can be lost in
a millisecond-wide window — and those are re-uploaded by the peer's next join reconciliation
(handleJoined -> flushLocal({ force: true })), which is now pinned by a test.

Verification

Every test below was verified by mutation — fix neutered, test observed failing, fix restored:

  • packages/components/tests/local-data-plane-room-invalidation-reconnect.test.ts drives the
    real chain: engine -> room-status frame -> LocalLoroTransportAdapter -> real
    createRoomSyncTracker -> real createRoomSyncRegistry -> the exact predicate the reconnect
    loop gates on. Plus a negative control pinning that reconnecting does not wake it. The
    existing data-plane suite passed with defect 1 present precisely because it calls
    adapter.reconnect() by hand instead of exercising the trigger; that line now says so.
  • apps/cli/tests/loro-doc-unload-data-plane-integration.test.ts exercises the real seam against
    a real LoroRepo with SQLite storage: session GC -> unloadDocRoom -> a renderer-authored
    upload must reach getSessionHistorySnapshot, and the peer must receive a terminal room status.
  • packages/shared/tests/local-loro-transport-bug-repro.test.ts F9: orphaned-room reproduction,
    both-directions recovery, recovery of an upload lost in the eviction window, and the in-flight
    build race.
  • apps/cli/tests/loro-doc-unload-invalidates-data-plane.test.ts — guardrail keeping
    repo.unloadDoc to one call site with invalidateDocRoom on the next line.
  • typecheck clean for @lody/shared, @lody/components, apps/cli; oxlint 0 errors;
    prettier clean. Pre-existing failures on this base, confirmed by re-running with the change
    stashed: local-ipc (1), loro-streams (2), sqlite-repo-store (1), and 26 in
    packages/components (27 without this change — the difference is this PR's own new test).

Known holes NOT fixed here

  • The tracker's transport binding is pinned at join time (create-workspace-runtime.ts,
    acknowledged in a comment there). A room that joined while ownership was unknown (cloud-only
    route) and gained its local member later keeps watching the cloud binding, so it never sees the
    local disconnected. Pre-existing.
  • RoomTransportBinding.onStatusChange does not replay the current status and emit dedupes,
    so a tracker attaching after the emit reads null and markFirstSynced() can then paint it
    synced. The new component test attaches to the adapter subscription, which does replay, so
    it cannot catch this. Raising fidelity means driving
    repo.joinDocRoom(...).subscription('local').
  • Flock rooms have no invalidateFlockRoom. buildFlockRoom caches entry.flock the same
    way. No CLI caller evicts a flock today, but loro-repo's FlockHydrator can dropFlockDoc on a
    remote purge without any app call.
  • repo.unloadDoc does not detach loro-repo's own rooms. doAttachRoomTransport binds the
    doc instance at attach time, so any still-joined room — including the cloud transport — keeps
    syncing the orphan. Same bug one layer lower; not addressed here.

Reported separately (same bug class, out of scope)

  • getSessionHistorySnapshot builds a temporary SessionDocument for a session that may already
    have a live one; its destroy() unloads the shared doc and strands the live one. Currently
    unreachable — the only caller (session-export) runs one-shot with no live session docs.
  • operation-coordinator.ts (targetSubscriptions) and session-dispatch-watcher.ts
    (watchedSessions) hold mirror.subscribe behind sticky has(sessionId) guards that
    cleanSessionDoc never clears, so after any foreign clean they never re-subscribe to the new
    Mirror. Operation delivery and dispatch reconciliation go silently deaf for that session until
    daemon restart.

Not done

  • The risk:high adversarial pipeline is run. Four independent review passes (correctness/
    races, repo-wide bug-class sweep, an adversarial re-review of the fixes, and a fourth round on
    the result) produced every correction above. Three of those rounds each found a real defect,
    two of them in fixes written to address an earlier finding.
  • Three attempts at the race regression test passed with the fix removed and were discarded
    before landing one that does not. Every test in this PR is mutation-verified. Independent review agents covered
    correctness/races and a repo-wide sweep for this bug class, and their findings produced the
    three corrections above. A re-review of the fixes themselves was still in flight when this body
    was written; anything it surfaces will be pushed as a follow-up commit.
  • No real-app verification. The originating symptom has not been reproduced end to end in a
    running Lody. That is why Confidence is medium, not high.

Note: this repository has no risk:* labels defined, so the tier is carried by the title tag and
this line only.

🤖 Generated with Claude Code

zxch3n and others added 5 commits August 7, 2026 16:34
`repo.unloadDoc(docId)` evicts the doc from loro-repo's instance cache, so the
next `openPersistedDoc` returns a DIFFERENT `LoroDoc`. The local Loro data plane
resolves a room's doc ONCE (`buildDocRoom`) and holds that object for as long as
a renderer stays subscribed, so an unload that is not paired with an
invalidation silently severs renderer<->CLI sync in both directions: renderer
uploads land in the orphan, CLI writes stop being pushed, the room still reports
`joined`, and the 60s idle watchdog cannot notice because relay pings are
answered in `parseLine` before the room message chain. Re-joining does not
repair it either -- `ensureRoom` returns the cached entry.

Observed on 2026-08-07: session GC evicted an idle session (20min timeout) and
the next user turn never reached the CLI's session doc. `TurnHistoryGate` then
held the entire agent reply for its full 20s bound and flushed 61 and 198 ACP
updates in single batches. The cloud transport normally masks this; it happened
to be failing in the same window.

- `LocalLoroDataPlaneServer.invalidateDocRoom(docId)` publishes a non-joined
  room status (before dropping the entry, since `publishRoomStatus` reads
  subscribers), unsubscribes the stale doc, and removes the room so the next
  join rebuilds it against the fresh instance.
- `LoroDocumentManager.unloadDocRoom(docId)` is now the only place the CLI calls
  `repo.unloadDoc`; `SessionDocument` receives it by injection instead of
  reaching for `repo.unloadDoc` itself.
- Invalidation runs AFTER the unload deliberately: doing it before leaves a
  window where a racing join re-opens the doc into the repo cache just in time
  to be stranded again, and that failure is silent. Updates lost in the window
  here are re-uploaded by the peer's next join reconciliation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5[1m]
…ation [risk:high]

Adversarial review of 941af3b found the outbound half of that fix did not work,
plus a race that reproduces the original bug with no recovery signal at all.

1. `invalidateDocRoom` published room status `reconnecting`. Only
   `disconnected`/`error` are terminal in `createRoomSyncTracker.needsReconnect()`,
   and that predicate is what `roomSyncRegistry.anyNeedsReconnect()` — hence
   `createLocalReconnectLoop({ hasProblem })` — gates on. `reconnecting` reads as
   "already recovering", so the loop never fired: after an invalidation the room
   and its subscribers are gone, nothing marks it dirty, and a renderer that was
   only READING the session got no further pushes. Inbound was unaffected (the
   next join/update rebuilds the room), which is why the 20s gate freeze looked
   fixed. Now publishes `disconnected`.

2. `invalidateDocRoom` only inspected `this.rooms`, so a room build still inside
   `ensureRoom` was invisible to it: the lookup missed, nothing was published,
   and the build then installed a room bound to the instance the repo had just
   evicted — the original orphaned-room bug, with no status to trigger a rejoin.
   Reachable because `unloadDocRoom` awaits `repo.unloadDoc` before invalidating.
   `ensureRoom` now captures a per-room-key generation before resolving and
   discards its entry if `invalidateDocRoom` bumped it meanwhile.

3. Queued writer work naming the invalidated room is dropped. A leftover `sync`
   task exports against the REBUILT room and could emit an `update` ahead of its
   `joined` reply, breaking the FIFO ordering `enqueueJoinReply` documents.

Tests, each verified by mutation (fix neutered -> test fails -> restored):
- packages/components/tests/local-data-plane-room-invalidation-reconnect.test.ts
  drives the real chain engine -> room-status frame -> LocalLoroTransportAdapter
  -> createRoomSyncTracker -> createRoomSyncRegistry and asserts the predicate
  the loop gates on, plus a negative control pinning that `reconnecting` does
  not wake it. The existing suite passed with defect 1 present precisely because
  it calls `adapter.reconnect()` by hand instead of exercising the trigger.
- apps/cli/tests/loro-doc-unload-data-plane-integration.test.ts exercises the
  real seam against a real LoroRepo: session GC -> unloadDocRoom -> a renderer
  upload must reach `getSessionHistorySnapshot`, and the peer must be told to
  rejoin with a terminal status.
- Two more F9 cases: recovery of an upload lost in the eviction/invalidation
  window, and the in-flight build race in 2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5[1m]
…on [risk:high]

Adversarial re-review of 045c24e found the generation guard closed only half the
race, and that the residual half fails WORSE than the bug being fixed.

`ensureRoom`'s generation check covers the window before the entry is installed.
If an invalidation lands between `rooms.set` and the awaiting caller resuming,
the caller receives an already-dead entry. `handleJoin` then registers the peer
on it and `buildJoinReplyFrames` finds no room, so nothing is emitted: the client
parks on `connecting` forever, and `connecting` is not terminal, so the
renderer's reconnect loop never fires either. `handleUpdate` has the same window,
where the import writes into the evicted doc on an entry that is no longer
subscribed — dead in both directions, silently.

The check has to sit in one synchronous block with the use. A helper does not
work: returning from it is itself an `await` and reopens the window. So both
handlers now re-read `this.rooms` immediately after `ensureRoom` and retry.

Also reverses the teardown order in `SessionDocument.destroy`: release the repo
room BEFORE evicting the doc. loro-repo binds the `LoroDoc` into a room's
transport attachment once at attach time and `unloadDoc` does not detach rooms,
so unloading first leaves every attached transport — the cloud one included —
syncing the orphan. Rooms are ref-counted, so another holder makes that binding
permanent. Nothing is lost by releasing first: the preceding `setStatus` writes
through `repo.upsertDocMeta`, i.e. the meta room, not this doc's room. The
renderer already does it in this order (`store-ref-tracker.ts`).

The regression test sweeps the invalidation across a fixed range of microtask
depths rather than guessing the distance between install and resumption, and
asserts the real invariant: a joining peer is never left silent — it gets either
its `joined` reply or a terminal `room-status` telling it to re-join. Five of the
eight depths fail without the fix.

Two earlier attempts at this test passed with the fix removed and were discarded:
one re-read the doc map after its gate so the race could not manifest, the other
fired the invalidation before the install and so only re-covered the generation
path already under test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5[1m]
…space-wide [risk:high]

Third finding from the adversarial re-review of 045c24e.

`LocalLoroTransportAdapter` did not act on a server-pushed terminal room status,
so recovery ran entirely through the workspace reconnect loop — the only other
thing that notices, since a terminal status is what its `needsReconnect()` keys
on. That made one dead room cost a workspace-wide reconcile: it releases every
idle document store and rejoins EVERY local room, and charges a backoff step
forgiven only after 30s of health. A session being garbage-collected is routine,
so repeated invalidations ratchet local reconnect latency for the whole
workspace — the same shape as the post-sleep reconnect-storm pathology.

The adapter now re-joins that one room itself when the server pushes
`disconnected`/`error` for it. The status stays terminal so the workspace loop
remains a backstop if self-repair is ever bypassed.

Test changes follow the new model:
- The F9 cases no longer call `adapter.reconnect()` by hand; they assert
  recovery happens with nobody driving it, which is what self-repair means.
- The component tests now assert the OUTCOME (room repaired, workspace loop left
  with no standing problem) plus, at the wire, that the published status is
  terminal — the backstop half. Removing the self-repair fails 2 of 3 there and
  2 of the F9 cases.

Also addresses two test-quality findings from the same review:
- The single-entry-point guard compared a line number against a value re-derived
  by the same search — tautological. It now asserts the file, the
  `unloadDoc`/`invalidateDocRoom` adjacency, and the presence of the injected
  unloader.
- A load-bearing comment in the CLI integration test claimed a `waitForRemoteSync`
  covered the manager's join-triggered hydrate. It does not: that hydrate is
  fired by the peer join further down and is a floating promise nothing awaits.
  The comment now says why it settles in time anyway and what would break it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5[1m]
…round 4 [risk:high]

Four findings from the fourth adversarial review pass.

Fixed:

- A terminally-failed room could be resurrected by a server status push.
  `handleServerMessage` applied `message.status` before checking
  `state.terminalError`, downgrading `error` to whatever was pushed and quietly
  re-entering the room into the reconnect loops R4 deliberately excludes it
  from. A terminal room is now left alone; only `subscription.rejoin()` clears it.

- Coverage regression from 09562db. Replacing the tracker assertion with a
  hardcoded status-string check removed the ONLY link in the repo between
  `disconnected` and `createRoomSyncTracker.needsReconnect()`
  (`room-sync-tracker.test.ts` never mentions the status). Dropping
  `disconnected` from `isTerminalRoomStatus` would have failed no test. The
  component test now feeds the captured wire status through a real tracker and
  registry.

- `roomGenerations` was never pruned: `dispose()` cleared rooms, writers and
  receivers but not it, and the counter was bumped even for docs no renderer had
  joined. Bumping now happens only for a room that exists, and `dispose()`
  clears the map.

- `dropQueuedRoomWork` ran before the no-entry early return, discarding queued
  work on a path that publishes nothing.

Plus test-quality fixes: a redundant second `settle()` that read as the
self-repair's settle point, and a comment-skip mismatch in the single-entry-point
guard (the scan skipped `//` and `*`, the re-derivation only `*`).

NOT fixed, deliberately — the review's finding that self-repair is a one-shot
rejoin with no retry if the frame is lost. I implemented the suggested fix
(`sendJoin` reporting whether it sent, restoring the terminal status when it did
not) and then reverted it: the relay drops client frames SILENTLY, so `send`
does not throw and the adapter cannot detect that case at all — the fix would
only have covered the already-offline case, where it makes the status less
honest and would spin the workspace loop while offline. A lost frame recovers on
the next `handleConnectionStatus(true)` edge, which is the recovery contract
every other frame this transport sends already has (F5b).

The related claim that the workspace loop remains a "backstop" is also wrong and
the comments said so: `sendJoin` replaces the terminal status in the same
synchronous block, so the loop never observes it. Comments now state what
actually happens instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5[1m]
@zxch3n
zxch3n merged commit f5a28fd into main Aug 7, 2026
Leeeon233 added a commit that referenced this pull request Aug 26, 2026
* feat(cli): update built-in ACP runtimes [risk:high] (#27)

* feat: update built-in ACP runtimes [risk:high]

Model: GPT-5.6

* chore: refresh public dependency lock [risk:high]

Model: GPT-5.6

* fix: gate provider-neutral goal controls [risk:high]

Validate neutral goal snapshots strictly, normalize limited status for mixed-version history, and keep non-Codex goals read-only until the ACP extension control method is routed through Lody.

Model: GPT-5.6

* fix: separate goal and prompt activity [risk:high]

Treat active goals as persistent session state rather than live turn presence, keep quiescent sessions directly dispatchable, and preserve completion notifications and working indicators around actual prompt activity.

Model: GPT-5.6

* fix: preserve goal activity helper API [risk:high]

Keep a deprecated isSessionGoalWorking alias for external consumers while documenting that it reports persistent goal state rather than live prompt activity.

Model: GPT-5.6

* docs: changelog

* fix(components): close empty session side panel [risk:medium]

Model: gpt-5

---------

Co-authored-by: Zixuan Chen <me@zxch3n.com>
Co-authored-by: Lody Dev <zx@loro.dev>
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.

1 participant