Skip to content

fix(lib): say what actually blocks disconnecting a workspace - #1790

Merged
joepio merged 5 commits into
developfrom
claude/disconnect-drive-guard
Sep 28, 2026
Merged

joepio merged 5 commits into
developfrom
claude/disconnect-drive-guard

Conversation

@joepio

@joepio joepio commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Before: turning workspace sync off on the Sync page could fail with "Open this drive with local storage available before disconnecting", whatever was actually wrong. All three preconditions shared that one sentence, and two of them have nothing to do with local storage: a signed-out user, and a client that holds no live connection to the server, were both told to do something they had already done. The third case was not even a real refusal. The local database attaches a few hundred milliseconds after a page load, and again after every sign-in, so a click inside that window was rejected although the database was on its way. And when a socket existed but was not open, the guard let it through and the attempt died on the raw WebSocket is not open.

After: each precondition says what it is.

  • Not signed in: "Sign in before disconnecting this workspace."
  • No local database: "Open this drive with local storage available before disconnecting." (unchanged, and now only for this case)
  • No open connection: "Connect to a server before disconnecting this workspace."

A click during the database's attach window waits for the attach instead of being refused, and a socket that exists but is not open now produces the connection message rather than WebSocket is not open.

The same change also settles the spelling of the local-only workspace registry. localOnlyDrives was written raw and read raw in some places and through normalizeSubject in others, so a trailing slash or a legacy did:ad: identifier could store a key another reader never found again, and a resource inside a disconnected workspace could still try to reach the server. Every write and every read now goes through normalizeSubject: registerLocalOnlyDrive, unregisterLocalOnlyDrive, the rehydrate from localStorage, isLocalOnlyDrive, and both fallbacks in isLocalOnlySubject (the resource's raw drive propval and the driveOf parent chain).

How: Store.makeDriveLocal checks the agent, then awaits waitForClientDb() and db.waitForInit(), then resolves a socket the way promoteLocalDrive already does (getDefaultWebSocket() falling back to getWebSocketForSubject), requiring readyState === WebSocket.OPEN. The subject handed to rbsrItems, verifyLocalDriveCopy and unsubscribeFromDrive stays the caller's spelling on purpose: the server echoes back the subjects it was asked about, and the drive-wide SUB was sent under getDrive()'s own unnormalized spelling. Normalization is confined to the registry. browser/lib/src/make-drive-local.test.ts covers the three messages, the attach-window wait, both registry spellings, the legacy did:ad: round trip and the legacy propval.

Tested: browser/lib 996 passed and 1 skipped; browser/data-browser including vaultAutoBackup.test.ts (38 passed), whose two failures on the first push are what exposed the half-normalized registry; pnpm run -r lint and tsc --noEmit clean.

No screenshots: this changes error copy inside the Sync page's existing error banner, with no layout or component change. The three strings are quoted above in full.

Requested by Joep

Turning workspace sync off answered all three of its preconditions with
"Open this drive with local storage available before disconnecting". Two
of them have nothing to do with local storage, so someone signed out, or
on a server this client holds no live connection to, was told to do
something they had already done.

Each precondition now names itself. The local database is waited for
rather than refused: it attaches a few hundred milliseconds after a page
load and again after every sign-in, and a click inside that window was
rejected outright. A websocket that exists but is not open is reported as
such, instead of letting the inventory request fail on the raw
"WebSocket is not open".

Also normalize the local-only drive set on every write and on rehydrate.
Every reader looks through normalizeSubject, so a trailing slash or a
legacy did:ad: spelling could store a key nothing found again, and a
resource inside a disconnected workspace could still try the server.
build.rs stages the plugin assets there before embedding them, exactly as it
does with the already-ignored server/assets_tmp, so a built checkout reports
the staging directory as untracked.

joepio commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

The red Main check here is not a test failure: it is run 4617, which I cancelled myself (conclusion cancelled) because it was dispatched against c22a75b, one commit before the current head. Run 4618 is queued on 1aeed34 and is the one to read.

Feature branches do not trigger main.yml on push, so both runs are manual workflow_dispatch.


Generated by Claude Code

joepio commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

CI on 1aeed34 (run 4618) failed on one test, and it is not this PR's:

FAIL  src/real-account-migration.test.ts > migrating a real pre-DID account > adopts the account origin and nothing else
Error: Test timed out in 5000ms.

Test Files 1 failed | 122 passed | 1 skipped, Tests 1 failed | 994 passed | 1 skipped. A timeout, not an assertion.

Why it is not the diff:

  • That test runs in 282 ms locally (npx vitest run src/real-account-migration.test.ts), against the default 5000 ms budget. browser/lib/vitest.config.ts sets no testTimeout.
  • It exercises adoptLegacyDriveList over a 56-entry legacy drive list. It never registers a local-only drive, and isLocalOnlySubject returns on localOnlyDrives.size === 0 before reaching anything this PR changed.
  • The other thing this PR changed on a hot path is the localOnlyDrives rehydrate in the constructor, which is behind typeof window !== 'undefined'. These tests run in vitest's default node environment, so that branch does not execute at all.
  • The full browser/lib suite passes here: 995 passed, 1 skipped, and pnpm run -r lint and tsc --noEmit are clean.

The run landed while three other pipelines were in flight on the same host (runs 4619 to 4621), which is the load this suite's 5 s default does not survive. Raising that budget, or stopping the four runner processes from overlapping, is a separate change and not something I will fold into this PR.

Re-running the failed jobs once, which is the single re-run this failure gets.


Generated by Claude Code

…ywhere

Normalizing only the writes broke the readers that did a raw `has`:
`isLocalOnlyDrive('did:ad:drive:test')` missed a drive registered under
that same spelling, because the set now held `atomic:drive:test`. That is
how vaultAutoBackup's two failures came about: a restored drive was not
recognised as local, and a drive switch dropped a pending backup.

`isLocalOnlyDrive` and the `driveOf` fallback in `isLocalOnlySubject` now
normalize like the rest. Covered by a test registering and reading back
the legacy spelling.

joepio commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

Attempt 2 got past the @tomic/lib timeout (that one was load, as above) and surfaced a real bug of mine in data-browser:

FAIL src/helpers/managed/vaultAutoBackup.test.ts > restoreFromVault > only a confirmed node absence makes a restored drive local (NotFound)
FAIL src/helpers/managed/vaultAutoBackup.test.ts > watchForVaultBackups > retains pending backups when switching drives

Cause: I normalized the writes into localOnlyDrives but isLocalOnlyDrive was a raw this.localOnlyDrives.has(drive). So registerLocalOnlyDrive('did:ad:drive:test') stored atomic:drive:test, and reading it back with the same did:ad: string returned false. A restored drive was therefore not recognised as local, and a drive switch dropped a pending backup. My claim in the PR body that every reader normalized was wrong for that method.

Fixed in 2a8efee: isLocalOnlyDrive and the driveOf fallback in isLocalOnlySubject normalize like everything else, with a test that registers and reads back the legacy spelling.

Verified locally before pushing: the two failures reproduce with the old raw has (2 failed | 36 passed in that file) and pass with the fix (38 passed). browser/lib 996 passed, 1 skipped. pnpm run -r lint and tsc --noEmit clean.

One unrelated local failure worth flagging for anyone else running this suite outside CI: src/chunks/RTE/loro-typing-history.test.ts fails two cases on a clean develop checkout here too, with my changes stashed. It passes in CI, so it looks environment-dependent rather than a code problem.

Dispatching CI on the new head.


Generated by Claude Code

@joepio
joepio marked this pull request as ready for review September 26, 2026 17:03
makeDriveLocal keeps both sides: the named preconditions and the
attach-window wait from this branch, then develop's refused-drive path,
which now runs after the database is known to be attached. rbsrItems is
driveInventory on develop. The refused-drive test's 'hosted drive' case
has no socket, so it now expects the connection message.

joepio commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

Main (Mancave) / CI on d5ebb3d (run 4684) is red, but no test failed. The Mancave runner stopped logging at 18:54 UTC while the four e2e shards were still running. The job was then closed as failed at 19:07, with the Dagger step still in_progress and no error in the log. Rust passed 1099/1099. This points to runner loss rather than anything in this PR. The run is still busy with Downstream, so it can't be re-run in place. I've dispatched a fresh main.yml on this branch, and the PR merges once that run is green on d5ebb3d.


Generated by Claude Code

joepio commented Sep 27, 2026

Copy link
Copy Markdown
Member Author

Run 4711 (Main (Mancave) / CI, head d5ebb3d) failed at the Docker Hub login, one second into the job, before any code from this PR ran. The error was Unable to locate executable file: docker on actions-runner-2, which means Docker is missing again on that Mancave runner. The problem is the machine, not this PR, and there is nothing to port. I'll dispatch the run again once Docker is back on Mancave.


Generated by Claude Code

Only browser/CHANGELOG.md conflicted; both sides' entries are kept.
@joepio
joepio merged commit 58408ac into develop Sep 28, 2026
8 checks passed
@joepio
joepio deleted the claude/disconnect-drive-guard branch September 28, 2026 06:30
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