fix(coding): preserve renderer recovery state
This commit is contained in:
@@ -102,6 +102,12 @@
|
||||
discard, per-target sequence/generation-gap recovery, load single-flight,
|
||||
and connection-generation protection. Reconnect never replays a prompt and
|
||||
an obsolete connection flight cannot clear a newer one.
|
||||
- Closed the planner review's two P1 recovery windows: patches arriving while
|
||||
a target GET is in flight are buffered only for that invalidated Conversation
|
||||
and replayed in generation/sequence order after a valid Snapshot; unresolved
|
||||
pending/accepted/uncertain prompt requests re-overlay their local optimistic
|
||||
nodes after snapshot-first reconnect until a durable `clientRequestId`
|
||||
reconciliation arrives. Neither mechanism replays a mutation.
|
||||
- Added optimistic user nodes keyed by `clientRequestId`. Durable upsert keeps
|
||||
the optimistic UI node id; definite rejection restores an untouched draft
|
||||
and marks the node failed; uncertain delivery restores the draft while
|
||||
@@ -122,7 +128,9 @@
|
||||
## Verification
|
||||
|
||||
- `pnpm exec vitest run tests/unit/coding-conversations-facade.test.ts tests/unit/coding-conversations-store.test.tsx`:
|
||||
2 files / 12 tests passed.
|
||||
initial implementation 2 files / 12 tests passed; after review fixes, 2 files
|
||||
/ 13 tests passed, including delayed gap GET + concurrent patch replay and
|
||||
reconnect Snapshot optimistic-node continuity.
|
||||
- `pnpm exec vitest run` for the two PI-110 tests plus PI-010 contracts,
|
||||
coding core routes, event projector, Conversation runtime, product tools,
|
||||
and subagent suites: 8 files / 75 tests passed.
|
||||
@@ -140,6 +148,11 @@
|
||||
- Real Provider remains Explicitly Waived / Accepted Risk with
|
||||
`realTurnVerified=false`. macOS x64/arm64 remains deferred to mandatory
|
||||
PI-150. Neither is recorded as Pass.
|
||||
- Planner review of `a072257...36626c8`: Standards PASS / 0 findings; Spec
|
||||
Needs Fix with two P1 findings for recovery-window patch loss and reconnect
|
||||
Snapshot optimistic-node loss. Both independent reproductions were accepted
|
||||
and fixed locally with the focused regressions above; fixed-range re-review
|
||||
remains required before PI-110 can be marked Done.
|
||||
|
||||
## Follow-ups
|
||||
|
||||
|
||||
@@ -216,6 +216,27 @@ function withRejectedNode(
|
||||
});
|
||||
}
|
||||
|
||||
function withUnreconciledOptimisticNodes(
|
||||
reducer: ConversationReducerState,
|
||||
requests: Record<string, CodingPromptRequestState>,
|
||||
): ConversationReducerState {
|
||||
const snapshot = reducer.snapshot;
|
||||
if (!snapshot) return reducer;
|
||||
const existingRequestIds = requestIdsInSnapshot(snapshot);
|
||||
const nodes = Object.values(requests).flatMap((request) => (
|
||||
request.nodeId
|
||||
&& request.status !== 'rejected'
|
||||
&& !existingRequestIds.has(request.clientRequestId)
|
||||
? [optimisticNode(request.nodeId, request.clientRequestId, request.submittedDraft)]
|
||||
: []
|
||||
));
|
||||
if (nodes.length === 0) return reducer;
|
||||
return replaceConversationSnapshot(reducer, {
|
||||
...snapshot,
|
||||
nodes: [...snapshot.nodes, ...nodes],
|
||||
});
|
||||
}
|
||||
|
||||
function snapshotIsOlder(
|
||||
current: ConversationSnapshot | null,
|
||||
incoming: ConversationSnapshot,
|
||||
@@ -241,11 +262,33 @@ export function createCodingConversationStore(
|
||||
): StoreApi<CodingConversationStoreState> {
|
||||
const deps = { ...defaultDependencies(), ...dependencies };
|
||||
const snapshotLoads = new Map<string, Promise<ConversationSnapshot>>();
|
||||
const recoveryPatches = new Map<string, CodingConversationPatchEvent[]>();
|
||||
let eventSource: EventSource | null = null;
|
||||
let connectFlight: Promise<void> | null = null;
|
||||
let connectionGeneration = 0;
|
||||
let store!: StoreApi<CodingConversationStoreState>;
|
||||
|
||||
return createStore<CodingConversationStoreState>((set, get) => ({
|
||||
const bufferRecoveryPatch = (event: CodingConversationPatchEvent) => {
|
||||
const buffered = recoveryPatches.get(event.conversationId) ?? [];
|
||||
if (buffered.some((candidate) => (
|
||||
candidate.workerGeneration === event.workerGeneration && candidate.seq === event.seq
|
||||
))) return;
|
||||
recoveryPatches.set(
|
||||
event.conversationId,
|
||||
[...buffered, event].sort((left, right) => (
|
||||
left.workerGeneration - right.workerGeneration || left.seq - right.seq
|
||||
)),
|
||||
);
|
||||
};
|
||||
|
||||
const replayRecoveryPatches = (conversationId: string) => {
|
||||
const buffered = recoveryPatches.get(conversationId);
|
||||
if (!buffered?.length) return;
|
||||
recoveryPatches.delete(conversationId);
|
||||
for (const event of buffered) store.getState().applyPatchEvent(event);
|
||||
};
|
||||
|
||||
store = createStore<CodingConversationStoreState>((set, get) => ({
|
||||
selectedConversationId: null,
|
||||
entriesByConversationId: {},
|
||||
summariesByConversationId: {},
|
||||
@@ -316,6 +359,8 @@ export function createCodingConversationStore(
|
||||
})
|
||||
.finally(() => {
|
||||
if (snapshotLoads.get(conversationId) === flight) snapshotLoads.delete(conversationId);
|
||||
const entry = get().entriesByConversationId[conversationId];
|
||||
if (!entry?.reducer.invalidation) replayRecoveryPatches(conversationId);
|
||||
});
|
||||
snapshotLoads.set(conversationId, flight);
|
||||
return flight;
|
||||
@@ -556,7 +601,14 @@ export function createCodingConversationStore(
|
||||
if (!current.reducer.invalidation && snapshotIsOlder(current.reducer.snapshot, snapshot)) {
|
||||
return state;
|
||||
}
|
||||
const reducer = replaceConversationSnapshot(current.reducer, snapshot);
|
||||
const requests = withoutReconciledRequests(
|
||||
state.requestsByConversationId[event.conversationId],
|
||||
snapshot,
|
||||
);
|
||||
const reducer = withUnreconciledOptimisticNodes(
|
||||
replaceConversationSnapshot(current.reducer, snapshot),
|
||||
requests,
|
||||
);
|
||||
const entry: CodingConversationEntry = {
|
||||
...current,
|
||||
reducer,
|
||||
@@ -572,17 +624,25 @@ export function createCodingConversationStore(
|
||||
summariesByConversationId: summariesWithEntry(state.summariesByConversationId, entry),
|
||||
requestsByConversationId: {
|
||||
...state.requestsByConversationId,
|
||||
[event.conversationId]: withoutReconciledRequests(
|
||||
state.requestsByConversationId[event.conversationId],
|
||||
snapshot,
|
||||
),
|
||||
[event.conversationId]: requests,
|
||||
},
|
||||
};
|
||||
});
|
||||
if (!snapshotLoads.has(event.conversationId)) {
|
||||
replayRecoveryPatches(event.conversationId);
|
||||
}
|
||||
},
|
||||
|
||||
applyPatchEvent(event) {
|
||||
if (event.type !== 'patch') return;
|
||||
const currentBeforePatch = get().entriesByConversationId[event.conversationId];
|
||||
if (currentBeforePatch?.reducer.invalidation) {
|
||||
bufferRecoveryPatch(event);
|
||||
if (!snapshotLoads.has(event.conversationId)) {
|
||||
void get().loadSnapshot(event.conversationId, true).catch(() => undefined);
|
||||
}
|
||||
return;
|
||||
}
|
||||
let recover = false;
|
||||
set((state) => {
|
||||
const current = state.entriesByConversationId[event.conversationId] ?? emptyEntry();
|
||||
@@ -626,6 +686,7 @@ export function createCodingConversationStore(
|
||||
}
|
||||
},
|
||||
}));
|
||||
return store;
|
||||
}
|
||||
|
||||
export const codingConversationStore = createCodingConversationStore();
|
||||
|
||||
@@ -194,6 +194,46 @@ describe('coding Conversation store', () => {
|
||||
expect(store.getState().entriesByConversationId['conversation-b'].reducer.invalidation).toBeNull();
|
||||
});
|
||||
|
||||
it('replays target patches that arrive while a gap snapshot is loading', async () => {
|
||||
const recovery = deferred<ConversationSnapshot>();
|
||||
const getSnapshot = vi.fn(() => recovery.promise);
|
||||
const store = createCodingConversationStore({
|
||||
getSnapshot,
|
||||
openEvents: vi.fn(),
|
||||
submitPrompt: vi.fn(),
|
||||
createId: ids(),
|
||||
});
|
||||
store.getState().applySnapshotEvent(snapshotEvent(snapshot('conversation-a')));
|
||||
|
||||
store.getState().applyPatchEvent(patchEvent('conversation-a', 2, {
|
||||
op: 'run.state',
|
||||
run: { status: 'running', runId: 'run-gap' },
|
||||
}));
|
||||
store.getState().applyPatchEvent(patchEvent('conversation-a', 3, {
|
||||
op: 'run.state',
|
||||
run: {
|
||||
status: 'error',
|
||||
runId: 'run-gap',
|
||||
terminalReason: 'failed',
|
||||
},
|
||||
}));
|
||||
recovery.resolve({
|
||||
...snapshot('conversation-a', 1, 2),
|
||||
run: { status: 'running', runId: 'run-gap' },
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(selectCodingConversationSnapshot('conversation-a')(store.getState()))
|
||||
.toMatchObject({
|
||||
cursor: { workerGeneration: 1, seq: 3 },
|
||||
run: { status: 'error', runId: 'run-gap', terminalReason: 'failed' },
|
||||
});
|
||||
});
|
||||
expect(getSnapshot).toHaveBeenCalledTimes(1);
|
||||
expect(store.getState().entriesByConversationId['conversation-a'].reducer.invalidation)
|
||||
.toBeNull();
|
||||
});
|
||||
|
||||
it('uses native EventSource reconnect without reopening or replaying a mutation', async () => {
|
||||
const source = new FakeEventSource();
|
||||
const openEvents = vi.fn(async () => source as unknown as EventSource);
|
||||
@@ -276,6 +316,9 @@ describe('coding Conversation store', () => {
|
||||
await submission;
|
||||
expect(store.getState().requestsByConversationId['conversation-a']['request-1'].status)
|
||||
.toBe('accepted');
|
||||
store.getState().applySnapshotEvent(snapshotEvent(snapshot('conversation-a')));
|
||||
expect(selectCodingConversationSnapshot('conversation-a')(store.getState())?.nodes[0])
|
||||
.toMatchObject({ id: 'node-1', clientRequestId: 'request-1', status: 'optimistic' });
|
||||
store.getState().applyPatchEvent(patchEvent('conversation-a', 1, {
|
||||
op: 'message.upsert',
|
||||
node: {
|
||||
@@ -350,6 +393,7 @@ describe('coding Conversation store', () => {
|
||||
})).rejects.toThrow('Delivery is uncertain');
|
||||
source.fail();
|
||||
source.open();
|
||||
source.emit('snapshot', snapshotEvent(snapshot('conversation-a')));
|
||||
|
||||
expect(store.getState().draftsByConversationId['conversation-a'].text).toBe('Do not replay');
|
||||
expect(store.getState().requestsByConversationId['conversation-a']['request-1'])
|
||||
|
||||
Reference in New Issue
Block a user