Log rejected refresh generations
This commit is contained in:
+8
-16
@@ -54,7 +54,7 @@ If a replacement bundle has already been built before bailing, dispose it. This
|
||||
|
||||
**Severity: Medium-Low**
|
||||
|
||||
**Status: Accepted and documented.** The missing-lineage-link failure mode and its recovery limits are now explicit in the main plan under “Edge case: a failed write leaves a missing lineage link.” Conditional persistence continues to prefer the authoritative stored generation.
|
||||
**Status: Accepted, documented, and instrumented.** The missing-lineage-link failure mode and its recovery limits are now explicit in the main plan under “Edge case: a failed write leaves a missing lineage link.” Conditional persistence continues to prefer the authoritative stored generation, and logs when a rejected refresh result differs from that stored generation without recording token material.
|
||||
|
||||
On `main`, a failed session write left storage stale, but the next dispatch rewrote the complete session unconditionally. Storage therefore healed on the next refresh. On this branch, the `jti`-chained merge (`session-merge.ts:171-200`) cannot distinguish another tab advancing the chain from this tab losing its own previous chain-link write.
|
||||
|
||||
@@ -67,12 +67,9 @@ Concrete sequence:
|
||||
|
||||
This is not a lock race; it is a false positive in the deliberate conditional merge. The trigger is narrow: a storage-write failure while the app remains alive, followed by enough idle time for the old generation to leave the server grace window.
|
||||
|
||||
**Recommendation:** Accept and document this as a tradeoff unless a reliable distinction can be made. Add a distinctive log when a refresh mutation is dropped and its result `jti` differs from the stored one, since that identifies credentials being discarded.
|
||||
**Decision:** Accept this as a documented tradeoff unless a reliable distinction can be made. A distinctive warning log now records when a refresh result is rejected and differs from the persisted generation; same-generation convergent aliases remain silent. The log includes only credential version and status.
|
||||
|
||||
**Tests:**
|
||||
|
||||
- Unit-test `applySessionUpdate` for a missing intermediate chain-link.
|
||||
- Provider-test the committed-generation mismatch path that rebuilds onto the older generation, so the downgrade remains explicit and visible.
|
||||
**Coverage:** `applySessionUpdate` now has a focused missing-lineage-link test, and the provider suite pins committed-generation mismatch reconciliation.
|
||||
|
||||
### B3 - Cross-tab listener's `void resumeSession(...)` has new unhandled-rejection paths
|
||||
|
||||
@@ -155,19 +152,14 @@ Add a `refreshSession` test with a rejecting `writeSession` to pin the intended
|
||||
- Cross-tab metadata clobbering and the no-Web-Locks lost-update fallback are already documented in `plans/versioned-localstorage-session-tangents.md`.
|
||||
- `createTemporaryClientsAndResume` may rotate a token before logout without persistence hooks; this matches `main`.
|
||||
|
||||
## Recommended order
|
||||
## Remaining recommended order
|
||||
|
||||
1. Fix B1 by rechecking abort and bundle identity after the awaited resume commit.
|
||||
2. Fix B3 by catching and logging the cross-tab listener's `resumeSession` rejection.
|
||||
3. Decide and document the B4 persistence-failure policy for login-shaped methods.
|
||||
4. Add tests for:
|
||||
- resume versus logout while persistence is pending;
|
||||
- `refreshSession` with rejecting `writeSession`;
|
||||
- a dropped refresh mutation followed by committed-generation regression.
|
||||
5. Accept and document B2 unless a safe distinction between cross-tab advancement and a missing local chain-link emerges; add a distinctive dropped-generation log.
|
||||
1. Fix B3 by catching and logging the cross-tab listener's `resumeSession` rejection.
|
||||
2. Decide and document the B4 persistence-failure policy for login-shaped methods.
|
||||
3. 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 one genuine state-corruption hole is B1: `resumeSession` performs unguarded asynchronous continuations after persistence. B2 is a knowable conditional-merge tradeoff; B3 and B4 are lower-severity error-policy issues.
|
||||
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 remain lower-severity error-policy issues.
|
||||
|
||||
@@ -259,6 +259,8 @@ The committed reconciliation then rebuilds the live session from persisted gener
|
||||
|
||||
This is a deliberate safety tradeoff. The conditional merge cannot prove that the missing `A -> B` write belonged to this process; the in-memory `B` could instead be stale credentials from another tab or an older login lineage. Accepting `B -> C` without a persisted `B` would weaken the central invariant and could overwrite a different authoritative session. The implementation therefore prefers the persisted generation even though an isolated failed write can turn a healthy in-memory session into a later logout.
|
||||
|
||||
A rejected refresh result emits a diagnostic log when its result generation also differs from the persisted generation. Expected convergent aliases, where the rejected result already matches persisted storage, do not log. The diagnostic includes only the persisted credential version and status, never JWTs or token `jti` values.
|
||||
|
||||
A retry of the original `A -> B` write before another refresh closes the gap. Native retries the same queued root snapshot once. Web reports the storage failure without updating its persisted cache or broadcasting; automatic refresh paths log the failure, while explicit refresh operations report it to their caller.
|
||||
|
||||
* * *
|
||||
|
||||
@@ -1,4 +1,11 @@
|
||||
import {describe, expect, it} from '@jest/globals'
|
||||
import {beforeEach, describe, expect, it, jest} from '@jest/globals'
|
||||
|
||||
const mockLoggerWarn = jest.fn()
|
||||
jest.mock('#/logger', () => ({
|
||||
logger: {
|
||||
warn: (...args: unknown[]) => mockLoggerWarn(...args),
|
||||
},
|
||||
}))
|
||||
|
||||
import {type PersistedAccount, type Schema} from '../schema'
|
||||
import {
|
||||
@@ -73,6 +80,10 @@ describe('versioned persisted sessions', () => {
|
||||
const refreshBAlias = jwt({jti: 'B', issuedAt: 3})
|
||||
const refreshC = jwt({jti: 'C', issuedAt: 4})
|
||||
|
||||
beforeEach(() => {
|
||||
mockLoggerWarn.mockClear()
|
||||
})
|
||||
|
||||
it('advances the credential version when refresh moves to a new jti', () => {
|
||||
const storedSession = session({account: account({refreshJwt: refreshA})})
|
||||
const next = account({refreshJwt: refreshB})
|
||||
@@ -123,6 +134,7 @@ describe('versioned persisted sessions', () => {
|
||||
expect(
|
||||
getCredentialState({session: result, accountDid: DID}).credentialVersion,
|
||||
).toBe(1)
|
||||
expect(mockLoggerWarn).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not let a stale refresh overwrite a newer generation', () => {
|
||||
@@ -150,6 +162,29 @@ describe('versioned persisted sessions', () => {
|
||||
).toBe(8)
|
||||
})
|
||||
|
||||
it('logs when a missing lineage link rejects a newer refresh result', () => {
|
||||
const storedSession = session({
|
||||
account: account({refreshJwt: refreshA}),
|
||||
})
|
||||
|
||||
const result = update({
|
||||
storedSession,
|
||||
nextAccount: account({refreshJwt: refreshC}),
|
||||
mutation: {
|
||||
type: 'refresh',
|
||||
accountDid: DID,
|
||||
baseRefreshJwt: refreshB,
|
||||
resultRefreshJwt: refreshC,
|
||||
},
|
||||
})
|
||||
|
||||
expect(result.accounts[0].refreshJwt).toBe(refreshA)
|
||||
expect(mockLoggerWarn).toHaveBeenCalledWith(
|
||||
'persisted session: rejected refresh result from a non-authoritative generation',
|
||||
{credentialVersion: 0, status: 'active'},
|
||||
)
|
||||
})
|
||||
|
||||
it('patches metadata without replacing authoritative credentials', () => {
|
||||
const storedSession = session({
|
||||
account: account({refreshJwt: refreshB}),
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import {jwtDecode} from 'jwt-decode'
|
||||
|
||||
import {logger} from '#/logger'
|
||||
import {
|
||||
type PersistedAccount,
|
||||
type PersistedCredentialState,
|
||||
@@ -189,6 +190,15 @@ function applyCredentialMutation({
|
||||
previousState.refreshJti !== baseRefreshJti
|
||||
) {
|
||||
/* The stored generation already advanced or became a tombstone. */
|
||||
if (resultRefreshJti && previousState.refreshJti !== resultRefreshJti) {
|
||||
logger.warn(
|
||||
'persisted session: rejected refresh result from a non-authoritative generation',
|
||||
{
|
||||
credentialVersion: previousState.credentialVersion,
|
||||
status: previousState.status,
|
||||
},
|
||||
)
|
||||
}
|
||||
return
|
||||
}
|
||||
if (!nextAccount || !resultRefreshJti) return
|
||||
|
||||
Reference in New Issue
Block a user