fix(notifications): preserve in-flight outbox events
Reserve the actively delivered outbox event while the external sender is pending so concurrent persistence cannot evict it through expiry or capacity enforcement. Apply the delivery outcome under the exclusive chain before normal cleanup resumes, preserving retry state, delivery acknowledgements, and cooldown timing without duplicate sends. Add deterministic concurrency regressions for expiry-success and capacity-failure races.
This commit is contained in:
@@ -0,0 +1,68 @@
|
||||
# Notification Outbox In-Flight Fix
|
||||
|
||||
## Status
|
||||
|
||||
Abgeschlossen auf `feature/notification-center-v2`.
|
||||
|
||||
Commit-Betreff: `fix(notifications): preserve in-flight outbox events`
|
||||
|
||||
## Ursache
|
||||
|
||||
`NotificationOutbox.performDrain()` wählte das nächste Event unter der Exclusivkette aus, führte `sendEvent(current)` danach aber außerhalb dieser Kette aus. Während der Send wartete, konnte ein paralleles `enqueue()` über `persist()` und `enforceLimits()` genau dieses Event wegen Ablauf oder Kapazitätsgrenze aus der Queue entfernen. Die anschließende Ergebnisverarbeitung fand die Objekt-ID nicht mehr. Dadurch gingen bei einem Fehler Retry-Zustand und Fehlerzeit verloren; bei einem Erfolg entfielen Erfolgszeit, `onDelivered` und der daran gekoppelte Cooldown.
|
||||
|
||||
## TDD RED
|
||||
|
||||
Command:
|
||||
|
||||
```text
|
||||
node_modules\.bin\vitest.cmd run tests\notification-outbox.test.ts
|
||||
```
|
||||
|
||||
Result: Exit 1. Zwei neue Regressionstests schlugen erwartungsgemäß fehl, 22 bestehende Tests bestanden.
|
||||
|
||||
- Ein langsamer erfolgreicher Send wurde während eines parallelen Enqueue ablaufbedingt entfernt; der Delivery-Ack blieb aus.
|
||||
- Ein langsamer fehlgeschlagener Send wurde während eines parallelen Enqueue durch das 250er-Limit entfernt; der Retry wurde nicht gespeichert.
|
||||
|
||||
## Umsetzung
|
||||
|
||||
- Das exakt ausgewählte Event wird während des externen Sends als In-Flight-Objekt reserviert.
|
||||
- Ablaufbereinigung und Kapazitäts-Eviction überspringen ausschließlich dieses Objekt.
|
||||
- Die Ergebnisverarbeitung findet das reservierte Objekt über Identität, verarbeitet Erfolg oder Fehler genau einmal und löst den Schutz innerhalb derselben Exclusivoperation vor dem abschließenden Persistieren.
|
||||
- Nach einem Erfolg wird das Event entfernt und `onDelivered` mit der tatsächlichen Ergebniszeit ausgeführt.
|
||||
- Nach einem Fehler bleiben Versuchszähler, Retry-Termin und Fehlerzeit erhalten, sofern das Event nach der Ergebniszeit noch gültig ist.
|
||||
|
||||
## GREEN und Verifikation
|
||||
|
||||
Command:
|
||||
|
||||
```text
|
||||
node_modules\.bin\vitest.cmd run tests\notification-outbox.test.ts tests\download-health-monitor.test.ts
|
||||
```
|
||||
|
||||
Result: Exit 0. Zwei Testdateien mit 58 von 58 Tests bestanden.
|
||||
|
||||
Command:
|
||||
|
||||
```text
|
||||
node_modules\.bin\tsc.cmd --noEmit
|
||||
```
|
||||
|
||||
Result: Exit 0, keine Ausgabe.
|
||||
|
||||
Command:
|
||||
|
||||
```text
|
||||
git diff --check -- src/main/notification-outbox.ts tests/notification-outbox.test.ts
|
||||
```
|
||||
|
||||
Result: Exit 0, keine Ausgabe.
|
||||
|
||||
## Geänderte Dateien
|
||||
|
||||
- `src/main/notification-outbox.ts`
|
||||
- `tests/notification-outbox.test.ts`
|
||||
- `.superpowers/sdd/2026-08-22-notification-center-v2/inflight-fix-report.md`
|
||||
|
||||
## Bedenken
|
||||
|
||||
Keine offenen funktionalen Bedenken. Vitest zeigt ausschließlich die bereits vorhandene Deprecation-Warnung für die CJS-Ausgabe der Vite Node API; die Tests selbst sind vollständig grün.
|
||||
@@ -173,6 +173,7 @@ export class NotificationOutbox {
|
||||
private retryTimer: NodeJS.Timeout | null = null;
|
||||
private shutdownRequested = false;
|
||||
private drainOperation: Promise<void> | null = null;
|
||||
private inFlightEvent: NotificationEvent | null = null;
|
||||
|
||||
public constructor(options: NotificationOutboxOptions) {
|
||||
this.filePath = options.filePath;
|
||||
@@ -257,6 +258,7 @@ export class NotificationOutbox {
|
||||
}
|
||||
return null;
|
||||
}
|
||||
this.inFlightEvent = next;
|
||||
return next;
|
||||
});
|
||||
if (!current) {
|
||||
@@ -270,7 +272,8 @@ export class NotificationOutbox {
|
||||
}
|
||||
const outcomeAt = finiteInteger(this.clock(), currentNow);
|
||||
const delivered = await this.runExclusive(async () => {
|
||||
const index = this.events.findIndex((event) => event.id === current.id);
|
||||
const index = this.events.indexOf(current);
|
||||
this.inFlightEvent = null;
|
||||
if (index < 0) {
|
||||
return false;
|
||||
}
|
||||
@@ -346,10 +349,15 @@ export class NotificationOutbox {
|
||||
}
|
||||
|
||||
private enforceLimits(now: number): void {
|
||||
this.events = this.events.filter((event) => event.expiresAt > now);
|
||||
this.events = this.events.filter((event) => event === this.inFlightEvent || event.expiresAt > now);
|
||||
while (this.events.length > MAX_EVENTS) {
|
||||
const successIndex = oldestIndex(this.events, (event) => event.priority === "success");
|
||||
const removeIndex = successIndex >= 0 ? successIndex : oldestIndex(this.events, () => true);
|
||||
const successIndex = oldestIndex(this.events, (event) => event !== this.inFlightEvent && event.priority === "success");
|
||||
const removeIndex = successIndex >= 0
|
||||
? successIndex
|
||||
: oldestIndex(this.events, (event) => event !== this.inFlightEvent);
|
||||
if (removeIndex < 0) {
|
||||
break;
|
||||
}
|
||||
this.events.splice(removeIndex, 1);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -579,6 +579,78 @@ describe("NotificationOutbox", () => {
|
||||
expect(stateBeforeRelease.events.map((queuedEvent) => queuedEvent.id)).toContain("late-digest");
|
||||
});
|
||||
|
||||
it("acknowledges a successful in-flight delivery after a parallel enqueue expires it", async () => {
|
||||
const filePath = createOutboxFile();
|
||||
let now = 1000;
|
||||
let releaseSend = (_sent: boolean) => {};
|
||||
let markSendStarted = () => {};
|
||||
const sendStarted = new Promise<void>((resolve) => { markSendStarted = resolve; });
|
||||
const sendResult = new Promise<boolean>((resolve) => { releaseSend = resolve; });
|
||||
const delivered: Array<{ id: string; deliveredAt: number }> = [];
|
||||
const outbox = new NotificationOutbox({
|
||||
filePath,
|
||||
now: () => now,
|
||||
send: async () => {
|
||||
markSendStarted();
|
||||
return sendResult;
|
||||
},
|
||||
onDelivered: (queuedEvent, deliveredAt) => {
|
||||
delivered.push({ id: queuedEvent.id, deliveredAt });
|
||||
}
|
||||
});
|
||||
await outbox.enqueue(event("expires-in-flight", { expiresAt: 1500 }));
|
||||
const draining = outbox.drain();
|
||||
await sendStarted;
|
||||
|
||||
now = 2000;
|
||||
await outbox.enqueue(event("late", { createdAt: 2000, nextAttemptAt: 3000 }));
|
||||
expect(persisted(filePath).events.map((queuedEvent) => queuedEvent.id)).toContain("expires-in-flight");
|
||||
releaseSend(true);
|
||||
await draining;
|
||||
|
||||
expect(delivered).toEqual([{ id: "expires-in-flight", deliveredAt: 2000 }]);
|
||||
expect(outbox.getStatus()).toEqual({ queued: 1, lastSuccessAt: 2000, lastFailureAt: 0 });
|
||||
expect(persisted(filePath).events.map((queuedEvent) => queuedEvent.id)).toEqual(["late"]);
|
||||
});
|
||||
|
||||
it("keeps a failed in-flight delivery queued for retry during a capacity-enforcing enqueue", async () => {
|
||||
const filePath = createOutboxFile();
|
||||
const queuedEvents = [
|
||||
event("fails-in-flight", { createdAt: 1 }),
|
||||
...Array.from({ length: 249 }, (_, index) => event(`queued-${index}`, { createdAt: index + 2 }))
|
||||
];
|
||||
fs.writeFileSync(filePath, JSON.stringify({
|
||||
version: 1,
|
||||
events: queuedEvents,
|
||||
lastSuccessAt: 0,
|
||||
lastFailureAt: 0
|
||||
}), "utf8");
|
||||
let releaseSend = (_sent: boolean) => {};
|
||||
let markSendStarted = () => {};
|
||||
const sendStarted = new Promise<void>((resolve) => { markSendStarted = resolve; });
|
||||
const sendResult = new Promise<boolean>((resolve) => { releaseSend = resolve; });
|
||||
const outbox = new NotificationOutbox({
|
||||
filePath,
|
||||
now: () => 1000,
|
||||
send: async () => {
|
||||
markSendStarted();
|
||||
return sendResult;
|
||||
}
|
||||
});
|
||||
const draining = outbox.drain();
|
||||
await sendStarted;
|
||||
|
||||
await outbox.enqueue(event("late-capacity", { createdAt: 10000 }));
|
||||
expect(persisted(filePath).events.map((queuedEvent) => queuedEvent.id)).toContain("fails-in-flight");
|
||||
releaseSend(false);
|
||||
await draining;
|
||||
|
||||
const state = persisted(filePath);
|
||||
expect(state.events).toHaveLength(250);
|
||||
expect(state.events[0]).toMatchObject({ id: "fails-in-flight", attempts: 1, nextAttemptAt: 2000 });
|
||||
expect(state.lastFailureAt).toBe(1000);
|
||||
});
|
||||
|
||||
it("acknowledges only successful delivery with its actual completion time", async () => {
|
||||
const filePath = createOutboxFile();
|
||||
let now = 1000;
|
||||
|
||||
Reference in New Issue
Block a user