fix(attribution): roll back snapshot dedup key on sync appendRecord failure

1UMh (Copilot): appendRecord can throw synchronously before returning
a promise — e.g. when ensureConversationFile() rethrows a non-EEXIST
writeFileSync error. The async .catch() handler attached to the
promise never runs in that case, so the optimistic dedup-key set
sticks on a write that never landed and permanently suppresses
identical retries. Roll back lastAttributionSnapshotJson in the outer
catch too. Regression test forces writeFileSync to throw EACCES on
the first invocation, then asserts the second identical snapshot
attempt fires a fresh write rather than getting deduped.
This commit is contained in:
wenshao 2026-05-06 10:53:43 +08:00
parent a53c7508d2
commit 715c258fb2
2 changed files with 46 additions and 3 deletions

View file

@ -505,6 +505,37 @@ describe('ChatRecordingService', () => {
afterFailure + 1,
);
});
// appendRecord can throw SYNCHRONOUSLY before returning a promise
// (e.g. ensureConversationFile fails because the conversation
// file can't be created). Without rollback in the outer catch,
// the dedup key stays set on a write that never happened, so
// all future identical snapshots get suppressed.
it('should retry an identical snapshot after a synchronous failure', async () => {
// First call: force writeFileSync (used by ensureConversationFile
// to wx-create the JSONL file) to throw a non-EEXIST error.
// ensureConversationFile rethrows that, which propagates through
// appendRecord SYNCHRONOUSLY before any promise is returned.
const writeFileSpy = vi.spyOn(fs, 'writeFileSync');
writeFileSpy.mockImplementationOnce(() => {
const e = new Error(
'EACCES: permission denied',
) as NodeJS.ErrnoException;
e.code = 'EACCES';
throw e;
});
chatRecordingService.recordAttributionSnapshot(baseSnapshot);
await chatRecordingService.flush();
// Sync failure: writeLine never reached.
expect(vi.mocked(jsonl.writeLine)).not.toHaveBeenCalled();
// Identical snapshot on retry: dedup key should have been
// rolled back so this fires a fresh write.
chatRecordingService.recordAttributionSnapshot(baseSnapshot);
await chatRecordingService.flush();
expect(vi.mocked(jsonl.writeLine)).toHaveBeenCalledTimes(1);
});
});
// Note: Session management tests (listSessions, loadSession, deleteSession, etc.)

View file

@ -1007,8 +1007,9 @@ export class ChatRecordingService {
* than being permanently suppressed.
*/
recordAttributionSnapshot(snapshot: AttributionSnapshot): void {
let json: string | undefined;
try {
const json = JSON.stringify(snapshot);
json = JSON.stringify(snapshot);
if (json === this.lastAttributionSnapshotJson) {
return;
}
@ -1021,13 +1022,24 @@ export class ChatRecordingService {
this.lastAttributionSnapshotJson = json;
this.appendRecord(record).catch(() => {
// Write failed — only roll back if the key still belongs to
// our snapshot (a later distinct write may have overwritten it).
// Async write failed — only roll back if the key still
// belongs to our snapshot (a later distinct write may have
// overwritten it).
if (this.lastAttributionSnapshotJson === json) {
this.lastAttributionSnapshotJson = undefined;
}
});
} catch (error) {
// appendRecord (and createBaseRecord/JSON.stringify) can throw
// synchronously — e.g. ensureConversationFile() fails because
// the project temp dir isn't writable. The .catch() handler
// attached to the promise never runs in that case, so we'd
// otherwise leave the dedup key set without a write ever
// having landed and permanently suppress identical retries.
// Roll back here too.
if (json !== undefined && this.lastAttributionSnapshotJson === json) {
this.lastAttributionSnapshotJson = undefined;
}
debugLogger.error('Error saving attribution snapshot:', error);
}
}