Bugfix: The client can lose its identity token (and hence, access to user data) during first use. - #5761
Open
krisajenkins wants to merge 1 commit into
Open
Conversation
…user data) during first use.
Imagine this session:
1. User connects to Spacetime for the first time, through a webapp.
2. They get issued an anonymous identity.
3. They do some work.
4. They lose their connection temporarily - from a network glitch, from the
tab being put to sleep in the background, anything that doesn't involve
a page-refresh - then we auto-reconnect.
After reconnecting, they should have access to their work again, right? They
won't, and it's due to an implicit lifecycle problem between connection
builders and connections.
Every shipped template builds the connection once, at module scope, and hands
it to the provider:
```tsx
const connectionBuilder = DbConnection.builder()
.withUri(HOST)
.withDatabaseName(DB_NAME)
.withToken(localStorage.getItem(TOKEN_KEY) || undefined)
.onConnect(onConnect) // writes the issued token to localStorage
.onDisconnect(onDisconnect)
.onConnectError(onConnectError);
createRoot(document.getElementById('root')!).render(
<SpacetimeDBProvider connectionBuilder={connectionBuilder}>
<App />
</SpacetimeDBProvider>
);
```
For a first-time visitor that `withToken(...)` reads an empty `localStorage`,
and the empty token is baked into the builder for good. ConnectionManager
retains that builder and rebuilds from it on every automatic reconnect. The
reconnect therefore goes out anonymously, the server mints a fresh Identity,
and the user silently becomes a stranger to their own data: rows keyed on
`ctx.sender` are still attached to an Identity the client can no longer reach,
and whatever the app does to set a user up runs again from scratch under the
new one.
Nothing errors on either side, and a reload re-runs the module, re-reads the
token and appears to fix it - so this presents as unreproducible flakiness
rather than a bug. It cannot bite a returning user, whose token is already in
storage when the builder is made, which is exactly why it survives ordinary
testing.
The manager already holds the right token in `managed.state.token`, so
`#buildManagedConnection` now re-applies it to the builder before building.
`rebuild()` opts out via `resumeSession: false`: it exists precisely to change
identity, so the replacement builder's token must still win.
- Fix all four rebuild paths at once: the reconnect timer, the resume
listeners, zombie-socket revival, and retain() after a drop.
- Cover the fix with reconnect and liveness regression tests, including
guards that rebuild() and never-connected entries are left alone.
# API and ABI breaking changes
None. `resumeSession` is an option on a private method.
One behaviour change worth calling out: handing `retain()` a *different*
builder carrying a different token no longer changes identity. It previously
did, but only when the swap landed while no connection was live - `retain()`
already ignores a replacement builder outright whenever a connection is live,
so identity depended on socket timing. `rebuild()` remains the supported way
to change identity deliberately.
# Expected complexity level and risk
2. The diff is small and confined to one private method, but it decides which
identity every automatic reconnect presents, so the risk is in the
interactions rather than the code: the reconnect, resume, revive and retain
paths all share that method, and getting the `rebuild()` opt-out wrong would
strand a signed-in user back on their anonymous session.
# Testing
- [x] `pnpm test` in `crates/bindings-typescript`: 300 passed / 29 files.
- [x] `pnpm lint` and `pnpm build:types` clean.
- [x] Reverting only `connection_manager.ts` fails 8 of the 11 new tests; the
other 3 are guards asserting behaviour the fix must not disturb, and pass
either way.
- [ ] Reviewer: on any shipped template with site data cleared, connect,
create state keyed on `ctx.sender`, then force a reconnect - toggle the network
off and on, or leave the tab backgrounded long enough for the socket to drop -
and confirm `ctx.sender` is unchanged. A quick tab switch will not reproduce
it; the connection has to actually go down.
Contributor
|
Hey @krisajenkins! Thanks for the patch, coincidentally I am finalizing a design for built-in automatic reconnection for all client SDKs (TypeScript, Rust, C#, and Unreal), moving it out of the framework-binding layer and into the core DbConnection in TypeScript and building it for the other SDKs. |
Contributor
Author
|
Ah, I see. No worries then. Feel free to use the patch or abandon it, and I look forward to seeing the new design. 😎 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Imagine this session:
tab being put to sleep in the background, anything that doesn't involve
a page-refresh - then we auto-reconnect.
After reconnecting, they should have access to their work again, right? They
won't, and it's due to an implicit lifecycle problem between connection
builders and connections.
Every shipped template builds the connection once, at module scope, and hands
it to the provider:
For a first-time visitor that
withToken(...)reads an emptylocalStorage,and the empty token is baked into the builder for good. ConnectionManager
retains that builder and rebuilds from it on every automatic reconnect. The
reconnect therefore goes out anonymously, the server mints a fresh Identity,
and the user silently becomes a stranger to their own data: rows keyed on
ctx.senderare still attached to an Identity the client can no longer reach,and whatever the app does to set a user up runs again from scratch under the
new one.
Nothing errors on either side, and a reload re-runs the module, re-reads the
token and appears to fix it - so this presents as unreproducible flakiness
rather than a bug. It cannot bite a returning user, whose token is already in
storage when the builder is made, which is exactly why it survives ordinary
testing.
The manager already holds the right token in
managed.state.token, so#buildManagedConnectionnow re-applies it to the builder before building.rebuild()opts out viaresumeSession: false: it exists precisely to changeidentity, so the replacement builder's token must still win.
listeners, zombie-socket revival, and retain() after a drop.
guards that rebuild() and never-connected entries are left alone.
API and ABI breaking changes
None.
resumeSessionis an option on a private method.One behaviour change worth calling out: handing
retain()a differentbuilder carrying a different token no longer changes identity. It previously
did, but only when the swap landed while no connection was live -
retain()already ignores a replacement builder outright whenever a connection is live,
so identity depended on socket timing.
rebuild()remains the supported wayto change identity deliberately.
Expected complexity level and risk
identity every automatic reconnect presents, so the risk is in the
interactions rather than the code: the reconnect, resume, revive and retain
paths all share that method, and getting the
rebuild()opt-out wrong wouldstrand a signed-in user back on their anonymous session.
Testing
pnpm testincrates/bindings-typescript: 300 passed / 29 files.pnpm lintandpnpm build:typesclean.connection_manager.tsfails 8 of the 11 new tests; theother 3 are guards asserting behaviour the fix must not disturb, and pass
either way.
create state keyed on
ctx.sender, then force a reconnect - toggle the networkoff and on, or leave the tab backgrounded long enough for the socket to drop -
and confirm
ctx.senderis unchanged. A quick tab switch will not reproduceit; the connection has to actually go down.
Description of Changes
API and ABI breaking changes
Expected complexity level and risk
Testing