acetheking987/bsky-social-app
1
0
Fork 0
Code Issues Actions Packages Projects Releases 1 Wiki Activity

Warn after committed session persistence failures

Browse Source
This commit is contained in:
Eric Bailey
2026-08-31 18:29:51 -05:00
parent 653c7113dc
commit 8c70d3af1c
4 changed files with 156 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/current-review.md
+5 -7
View File
@@ -91,6 +91,8 @@ The pattern existed on `main`, where the resume factory was its main rejection s
**Severity: Low**
**Status: Resolved.** Login, account creation, and partial session metadata refresh now use a success-with-warning policy after reducer commit. Persistence rejection is converted into a `logger.warn` with `safeMessage`, while the method resolves and continues its success path. Focused tests cover all three operations.
`login`, `createAccount`, and `partialRefreshSession` synchronously commit reducer state and then await persistence:
- `index.tsx:353-370`: login.
@@ -103,10 +105,7 @@ On `main`, persistence was fire-and-forget. Logout and removal currently make th
This requires genuinely broken storage, such as quota exhaustion or a browser security error.
**Recommendation:** Choose and document one policy:
1. swallow and log, matching logout/removal; or
2. deliberately reject, with callers treating "session established but durability failed" as success with a warning rather than a failed login.
**Resolution:** Swallow and warn after the reducer commit. This preserves the successful in-memory operation without misreporting a completed login or account creation as failed; the warning records that durability was lost.
## Intended but consequential behavior
@@ -156,11 +155,10 @@ Add a `refreshSession` test with a rejecting `writeSession` to pin the intended
## Remaining recommended order
1. Decide and document the B4 persistence-failure policy for login-shaped methods.
2. Add a `refreshSession` test with rejecting `writeSession`.
1. Add a `refreshSession` test with rejecting `writeSession`.
## Verdict
The core async design is sound. Serialization through `PasswordSession.#sessionPromise`, the per-bundle error channel, arm/kill lifecycle, reducer identity guards, and internally owned persistence lock compose correctly in the reviewed interleavings.
The genuine state-corruption hole from B1 is now closed. B2 is accepted, documented, instrumented, and covered as a conditional-merge tradeoff. B3 is closed; B4 remains as the final lower-severity error-policy issue.
The genuine state-corruption hole from B1 is now closed. B2 is accepted, documented, instrumented, and covered as a conditional-merge tradeoff. B3 and B4 are closed. The remaining work is a focused test pinning explicit refresh behavior when `writeSession` rejects.
plans/versioned-localstorage-sessions.md
+2
View File
@@ -238,6 +238,8 @@ Tab A action: broadcast update notification
Broadcasting after a failed write would tell Tab B to reread localStorage while it still contains `(7, A)`, spreading the stale generation instead of the new one.
Login, account creation, and partial session metadata refresh use a success-with-warning policy once their reducer state has committed. If persistence then fails, the method logs a warning with the error and resolves successfully instead of reporting that the already-completed login or account creation failed. The in-memory session remains usable, but may not survive a reload. The underlying write still throws, and the persisted cache and other tabs are not updated.
### Edge case: a failed write leaves a missing lineage link
If the process survives a failed write, its live `PasswordSession` may advance while authoritative storage remains behind:
src/state/session/__tests__/provider-abort-test.tsx
+93 -14
View File
@@ -1,6 +1,9 @@
import {beforeEach, describe, expect, it, jest} from '@jest/globals'
import {act, render} from '@testing-library/react-native'
let mockWriteSessionError: Error | undefined
const mockAnalyticsLoggerWarn = jest.fn()
/*
* The provider pulls the whole app shell in through `#/state/util` and the
* account factories. These mocks cut the tree back to the session lifecycle
@@ -16,7 +19,9 @@ jest.mock('#/state/persisted', () => {
get: () => defaults.session,
readLatest: () => defaults.session,
writeSession: ({nextSession}: {nextSession: typeof defaults.session}) =>
Promise.resolve(nextSession),
mockWriteSessionError
? Promise.reject(mockWriteSessionError)
: Promise.resolve(nextSession),
onUpdate: () => () => {},
}
})
@@ -26,7 +31,14 @@ jest.mock('#/components/dialogs/Context', () => ({
}))
jest.mock('#/analytics', () => ({
AnalyticsContext: ({children}: {children: React.ReactNode}) => children,
useAnalyticsBase: () => ({metric() {}, logger: {debug() {}, error() {}}}),
useAnalyticsBase: () => ({
metric() {},
logger: {
debug() {},
error() {},
warn: (...args: unknown[]) => mockAnalyticsLoggerWarn(...args),
},
}),
utils: {accountToSessionMetadata: () => ({}), useMeta: () => undefined},
}))
jest.mock('#/state/shell/onboarding', () => ({
@@ -81,6 +93,14 @@ function renderProvider(): SessionApiContext {
return api
}
beforeEach(() => {
mockLogin.mockReset()
mockCreateAccount.mockReset()
mockDisposeBundle.mockReset()
mockWriteSessionError = undefined
mockAnalyticsLoggerWarn.mockReset()
})
/*
* Every factory returns an ARMED bundle, so a call whose result is thrown away
* because a newer call superseded it must dispose that bundle. Leaving it armed
@@ -88,18 +108,6 @@ function renderProvider(): SessionApiContext {
* for an account the app is no longer tracking.
*/
describe('superseded session tasks dispose their bundle', () => {
/*
* Without this, a recorded call from an earlier test satisfies a later
* assertion. The tagged bundles below are the other half of that guard: two
* `{}` literals are structurally equal, so `toHaveBeenCalledWith` could not
* tell one test's bundle from the other's even within a cleared mock.
*/
beforeEach(() => {
mockLogin.mockReset()
mockCreateAccount.mockReset()
mockDisposeBundle.mockReset()
})
it('disposes the bundle of an aborted login', async () => {
const bundle = {tag: 'login-bundle'} as never
let resolveLogin!: (value: unknown) => void
@@ -145,3 +153,74 @@ describe('superseded session tasks dispose their bundle', () => {
expect(mockDisposeBundle).toHaveBeenCalledWith(bundle)
})
})
describe('persistence failures after an in-memory commit', () => {
const account = {
did: 'did:plc:example',
handle: 'alice.test',
service: 'https://bsky.social/',
accessJwt: 'access-jwt',
refreshJwt: 'refresh-jwt',
}
it('treats login as successful and logs a warning', async () => {
const error = new Error('storage failed')
mockLogin.mockResolvedValueOnce({bundle: {}, account})
mockWriteSessionError = error
const api = renderProvider()
await act(async () => {
await api.login({} as never, 'LoginForm')
})
expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith(
'Logged in but session persistence failed',
{safeMessage: error},
)
})
it('treats account creation as successful and logs a warning', async () => {
const error = new Error('storage failed')
mockCreateAccount.mockResolvedValueOnce({bundle: {}, account})
mockWriteSessionError = error
const api = renderProvider()
await act(async () => {
await api.createAccount({} as never, {} as never)
})
expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith(
'Account created but session persistence failed',
{safeMessage: error},
)
})
it('keeps refreshed metadata and logs a warning', async () => {
const error = new Error('storage failed')
const bundle = {
pdsClient: {
call: () =>
Promise.resolve({
did: account.did,
emailConfirmed: true,
emailAuthFactor: true,
}),
},
}
mockLogin.mockResolvedValueOnce({bundle, account})
const api = renderProvider()
await act(async () => {
await api.login({} as never, 'LoginForm')
})
mockWriteSessionError = error
await act(async () => {
await api.partialRefreshSession()
})
expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith(
'Session metadata updated but persistence failed',
{safeMessage: error},
)
})
})
src/state/session/index.tsx
+56 -38
View File
@@ -353,20 +353,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
disposeBundle(bundle)
return
}
await store.dispatch(
{
type: 'switched-to-account',
newBundle: bundle,
newAccount: account,
},
[
await store
.dispatch(
{
type: 'login',
accountDid: account.did,
resultRefreshJwt: account.refreshJwt,
type: 'switched-to-account',
newBundle: bundle,
newAccount: account,
},
],
)
[
{
type: 'login',
accountDid: account.did,
resultRefreshJwt: account.refreshJwt,
},
],
)
.catch(error => {
ax.logger.warn('Account created but session persistence failed', {
safeMessage: error,
})
})
ax.metric('account:create:success', metrics, {
session: utils.accountToSessionMetadata(account),
})
@@ -393,20 +399,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
disposeBundle(bundle)
return
}
await store.dispatch(
{
type: 'switched-to-account',
newBundle: bundle,
newAccount: account,
},
[
await store
.dispatch(
{
type: 'login',
accountDid: account.did,
resultRefreshJwt: account.refreshJwt,
type: 'switched-to-account',
newBundle: bundle,
newAccount: account,
},
],
)
[
{
type: 'login',
accountDid: account.did,
resultRefreshJwt: account.refreshJwt,
},
],
)
.catch(error => {
ax.logger.warn('Logged in but session persistence failed', {
safeMessage: error,
})
})
ax.metric(
'account:loggedIn',
{logContext, withPassword: true},
@@ -615,20 +627,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
/* getSession targets the PDS; only the persisted account fields are patched. */
const data = await bundle.pdsClient.call(com.atproto.server.getSession, {})
if (signal.aborted) return
await store.dispatch({
type: 'partial-refresh-session',
/*
* Read the did off the response rather than the session: the bundle may
* have been disposed while the request was in flight, and the live
* getters throw in that state.
*/
accountDid: data.did,
patch: {
emailConfirmed: data.emailConfirmed,
emailAuthFactor: data.emailAuthFactor,
},
})
}, [store, cancelPendingTask])
await store
.dispatch({
type: 'partial-refresh-session',
/*
* Read the did off the response rather than the session: the bundle may
* have been disposed while the request was in flight, and the live
* getters throw in that state.
*/
accountDid: data.did,
patch: {
emailConfirmed: data.emailConfirmed,
emailAuthFactor: data.emailAuthFactor,
},
})
.catch(error => {
ax.logger.warn('Session metadata updated but persistence failed', {
safeMessage: error,
})
})
}, [store, cancelPendingTask, ax])
/**
* Rotate the session's tokens and hand back the resulting account snapshot.