From 6aea717ade638e3b47176f35be5708b8d92adf8d Mon Sep 17 00:00:00 2001
From: Daniel Sutton <dcsu@pm.me>
Date: Fri, 21 Aug 2026 20:14:07 +0100
Subject: [PATCH] fix(run-engine): tolerance-band expiry assertion, honest
 orphan comment, drop dead ?? on Lua reply

---
 .../waitpointCoordinator/storeCoordinator.test.ts     | 11 +++++++++--
 .../engine/waitpointCoordinator/storeCoordinator.ts   | 11 +++++++----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
index b6aabb690..23dca517a 100644
--- a/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
+++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
@@ -689,8 +689,15 @@ describe("createWithIdempotencyKey", () => {
       });
 
       const ttl = await probe.pttl(`wp:idem:{${ENV_ID}}:key-1`);
-      expect(ttl).toBeGreaterThan(0);
-      expect(ttl).toBeLessThanOrEqual(60_000);
+      // Wide band, deliberately: the deadline is computed from the test process's clock
+      // and applied as an absolute PEXPIREAT, while PTTL is computed against the Redis
+      // server's own clock. A few ms of disagreement between those two clocks is normal
+      // and shows up as overshoot on this read, not as a bug in the reservation. The
+      // band still catches every failure worth catching — wrong units, no expiry
+      // applied, a negative TTL — without re-asserting that two independent clocks
+      // agree to the millisecond.
+      expect(ttl).toBeGreaterThan(55_000);
+      expect(ttl).toBeLessThanOrEqual(65_000);
     } finally {
       probe.disconnect();
       await store.quit();
diff --git a/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts b/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
index d2c3f0b50..0ef39b522 100644
--- a/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
+++ b/internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
@@ -268,9 +268,12 @@ export class WaitpointStoreCoordinator {
    * forever, because an idempotency key commonly carries no expiry to clear it.
    *
    * Create-first inverts the failure: a crash leaves an orphan record that nothing ever
-   * referenced, because its id is random and unpublished. No caller hangs, and the orphan
-   * is reaped by the store's own garbage collection rather than by an expiry, which
-   * pending keys never carry.
+   * referenced, because its id is random and unpublished. No caller hangs, but nothing
+   * currently reclaims that record either: the backstop collector the wider plan
+   * describes is keyed off a run's status, and this orphan has no owning run, so that
+   * collector never sees it. The record is harmless — inert, unreferenced, never
+   * returned to anyone — but it is a real leak until a later ticket adds a reaper for
+   * standalone idempotency-keyed orphans specifically.
    */
   async createWithIdempotencyKey(args: {
     record: WaitpointRecordInput;
@@ -294,7 +297,7 @@ export class WaitpointStoreCoordinator {
       return { waitpointId: args.record.id, created: true };
     }
 
-    const winner = reply[1] ?? args.record.id;
+    const winner = reply[1];
     if (winner !== args.record.id) {
       // Safe to discard: this id is random and was never handed to any caller, so no
       // watcher can reference it. Both keys share the record's tag.