fix(orchestrator): give the dispatch lease back when a release is dead-lettered

src/orchestrator/factory.test.ts

@@ -31890,6 +31890,314 @@ describe('completion release retry budget (#379)', () => {
   })
 })
 
+/**
+ * The dead-letter that keeps its lease (the AR-1540 livelock).
+ *
+ * Production 0.1.79 — which already contains #379 — dispatched nothing for
+ * three days. Four issues, including a canary filed purely to test dispatch,
+ * were all simultaneously stuck on:
+ *
+ *   [factory] durable dispatch is leased by another publisher; waiting for
+ *   lease release {"issue":"1540","leaseRemainingMs":189205,"retryMs":1000}
+ *
+ * while the lease holder logged, continuously:
+ *
+ *   [factory] failed to release ar-1540-impl-relay during completion
+ *     RelayError: Agent "ar-1540-impl-relay" has no live host node
+ *     code transport_error, statusCode 503, rawCode agent_host_unavailable
+ *
+ * #379's bound DOES fire on this shape — a 503 `agent_host_unavailable` is not
+ * `isAgentAlreadyGoneOnRelease`, so it lands in `failed[]` exactly like the
+ * plain `Error` the #379 suite throws, and `#chargeReleaseAttempt` runs. What
+ * #379 did not do is give the lease back. `#releaseDeadLetteredSlot` hands back
+ * the BATCH slot and leaves `#dispatchLifecycleEpochs` holding the key, so the
+ * 60 s `#renewDispatchLifecycles` interval re-stamps a fresh 5-minute lease on
+ * a work unit this process has permanently sworn off driving — for the life of
+ * the process. `leaseRemainingMs` of ~189 s on a 300 s TTL is that renewal,
+ * observed 111 s in.
+ *
+ * The result is strictly worse than the spin #379 replaced: the spin was loud
+ * and local, whereas a retained lease on a non-terminal `releasing` row is
+ * silent and blocks every other publisher — including a restart of this one —
+ * from ever claiming the key again.
+ *
+ * The invariant under test: a work unit whose agents are gone must not be able
+ * to hold the dispatch lease indefinitely.
+ */
+class HostUnavailableReleaseFleetClient extends RemoteLifecycleFleetClient {
+  readonly releaseAttempts: Array<{ name: string; reason?: string }> = []
+
+  /** The verbatim production error shape, fields and all. */
+  static hostUnavailable(name: string): Error {
+    return Object.assign(
+      new Error(`Agent "${name}" has no live host node; cannot dispatch release`),
+      {
+        name: 'RelayError',
+        code: 'transport_error',
+        retryable: true,
+        statusCode: 503,
+        status: 503,
+        rawCode: 'agent_host_unavailable',
+      },
+    )
+  }
+
+  override async release(name: string, reason?: string): Promise<void> {
+    this.releaseAttempts.push({ name, reason })
+    if (reason === 'issue-done') throw HostUnavailableReleaseFleetClient.hostUnavailable(name)
+    await super.release(name, reason)
+  }
+}
+
+describe('a dead-lettered release must not keep the durable dispatch lease', () => {
+  const RETRY_MS = 5
+  // Test-only renewal cadence. The TTL the renewer stamps is the production
+  // one; only how often it runs moves, because a renewer that never fires
+  // cannot be the subject of an assertion about renewal.
+  const RENEW_MS = 10
+  const WORKSPACE = 'factory-test'
+
+  /** The stuck `releasing` row, found by issue rather than by re-deriving its key. */
+  const stuckLifecycle = async (stateStore: InMemoryStateStore, issueKey: string) => {
+    const entries = await stateStore.listDispatchLifecycles(WORKSPACE)
+    const found = [...entries].find(([, lifecycle]) => lifecycle.issue.key === issueKey)
+    expect(found).toBeDefined()
+    return found!
+  }
+
+  /**
+   * True once no live lease stands on the key. Asserting on this rather than on
+   * a counter is what keeps the test a statement about the INVARIANT — the key
+   * is claimable — instead of a statement about which line of the fix ran. It
+   * is also read-only, so polling it cannot itself take the lease that the
+   * subsequent claim is trying to prove is available.
+   */
+  const leaseIsFree = async (stateStore: InMemoryStateStore, issueKey: string) => {
+    const [, lifecycle] = await stuckLifecycle(stateStore, issueKey)
+    return lifecycle.lease === undefined || lifecycle.lease.leaseUntilMs <= Date.now()
+  }
+
+  it('frees the lease once the release budget is exhausted, so another publisher can claim the key', async () => {
+    const mount = new FakeMountClient({ [issuePath(75)]: issueFile(75) })
+    const fleet = new HostUnavailableReleaseFleetClient()
+    const stateStore = new InMemoryStateStore({ batchSize: 2 })
+    const errors: unknown[][] = []
+    const factory = createFactory(config(), {
+      mount,
+      fleet,
+      stateStore,
+      triage: new StaticTriage(),
+      dispatchLifecycleRetryMs: RETRY_MS,
+      logger: { warn: () => undefined, error: (...args: unknown[]) => errors.push(args) },
+    })
+
+    await factory.runOnce()
+    fleet.emitAgentExit('ar-75-impl-pear', 'issue-done')
+
+    // STEP 1, stated as an assertion rather than an assumption: the 503 DOES
+    // reach `#chargeReleaseAttempt`. If this line ever fails, the bug is
+    // upstream of the lease and #379 does not fix the case it was written for.
+    await vi.waitFor(
+      () => expect(factory.status().counters.dispatchLifecycleReleaseAbandoned).toBe(1),
+      { timeout: 10_000, interval: 5 },
+    )
+    expect(errors.map(([message]) => message)).toContain(
+      '[factory] release retries exhausted; abandoning cleanup for this work unit',
+    )
+
+    // The row is deliberately left non-terminal so a successor re-drives it.
+    // That is only a recovery story if a successor can actually claim it.
+    expect((await stuckLifecycle(stateStore, 'AR-75'))[1].phase).toBe('releasing')
+
+    // THE PRODUCTION SYMPTOM. On origin/main this wait can only end in its own
+    // deadline: the renewal interval re-stamps a full TTL on the key forever,
+    // and the waiter logs "leased by another publisher" at 1 Hz until somebody
+    // notices three days later.
+    await vi.waitFor(
+      async () => expect(await leaseIsFree(stateStore, 'AR-75')).toBe(true),
+      { timeout: 10_000, interval: 5 },
+    )
+
+    const [key, lifecycle] = await stuckLifecycle(stateStore, 'AR-75')
+    const successor = await stateStore.claimDispatchLifecycle(
+      WORKSPACE,
+      key,
+      lifecycle,
+      'successor-publisher',
+      Date.now(),
+      60_000,
+    )
+    expect(successor.acquired).toBe(true)
+    // And the row it inherits is still the un-finished cleanup, not a lie about
+    // the work being done.
+    expect(successor.lifecycle.phase).toBe('releasing')
+  }, 20_000)
+
+  /**
+   * Freeing the lease once is not enough on its own, and this test has to be
+   * able to prove it (#391 review, P2).
+   *
+   * The first version of this test claimed the key for a successor and then
+   * asserted `lease.owner === 'successor-publisher'` after a fixed 200 ms
+   * sleep. That assertion was VACUOUS. `renewDispatchLifecycle` is owner+epoch
+   * fenced, so once a successor holds the row the abandoner cannot take it back
+   * whether or not its epoch was dropped — and 200 ms never reaches the 60 s
+   * `DISPATCH_LIFECYCLE_RENEW_MS`, so the renewer never ran at all. It passed
+   * with the bug present. That is the third test in this file shown to be green
+   * for the wrong reason, so this one is built the other way round.
+   *
+   * The property that actually matters: `#renewDispatchLifecycles` walks
+   * `#dispatchLifecycleEpochs` and re-stamps a full TTL on every key it finds.
+   * Relinquishing the durable lease while leaving the epoch cached therefore
+   * buys nothing — the livelock returns on the next renewal tick — because
+   * relinquishment leaves `owner` and `epoch` in place and only drops
+   * `leaseUntilMs`.
+   *
+   * `renewDispatchLifecycle` now fences on expiry as well as owner and epoch
+   * (#391 review, P2), which is what makes a relinquished lease unrenewable
+   * however the handback and an in-flight renewal race. This test predates that
+   * fence and still passes with it: it asserts the observable property — the
+   * lease is never restored — rather than any particular mechanism, so it holds
+   * whichever layer is doing the work, and goes red if either is removed.
+   *
+   * So: run the renewer for real, at a test-only interval, and assert the
+   * relinquished lease is never restored. Ablating just the epoch drop from
+   * `#relinquishDispatchLifecycleLease` — keeping the durable release — turns
+   * this red, which is what makes it a test of the handback rather than of the
+   * owner+epoch fencing.
+   */
+  it('leaves nothing for the renewal interval to re-stamp on the abandoned key', async () => {
+    const mount = new FakeMountClient({ [issuePath(76)]: issueFile(76) })
+    const fleet = new HostUnavailableReleaseFleetClient()
+    const stateStore = new InMemoryStateStore({ batchSize: 2 })
+    const factory = createFactory(config(), {
+      mount,
+      fleet,
+      stateStore,
+      triage: new StaticTriage(),
+      dispatchLifecycleRetryMs: RETRY_MS,
+      // The renewer is the mechanism under test, so it has to actually run.
+      // Only the interval moves; the TTL it stamps is the real one.
+      dispatchLifecycleRenewMs: RENEW_MS,
+      logger: { warn: () => undefined, error: () => undefined },
+    })
+
+    await factory.runOnce()
+    fleet.emitAgentExit('ar-76-impl-pear', 'issue-done')
+    await vi.waitFor(
+      () => expect(factory.status().counters.dispatchLifecycleReleaseAbandoned).toBe(1),
+      { timeout: 10_000, interval: 5 },
+    )
+    await vi.waitFor(
+      async () => expect(await leaseIsFree(stateStore, 'AR-76')).toBe(true),
+      { timeout: 10_000, interval: 5 },
+    )
+
+    // THE ASSERTION. Sample across many renewal intervals — a single sample
+    // cannot tell a lease that is gone from one that is about to come back.
+    const deadline = Date.now() + RENEW_MS * 25
+    while (Date.now() < deadline) {
+      expect(await leaseIsFree(stateStore, 'AR-76')).toBe(true)
+      await new Promise((resolve) => setTimeout(resolve, RENEW_MS / 2))
+    }
+
+    // There was a `dispatchLifecycleLeasesLost` assertion here, justified as
+    // "a retained epoch would have driven a renewal, been refused, and counted
+    // a lost lease". That reasoning was wrong and the assertion detected
+    // nothing (#391 review, P3): renewal fences on owner and epoch, and in the
+    // bug shape both still match, so the renewal SUCCEEDS and no lease is ever
+    // counted lost. The counter stayed undefined either way. The sampling above
+    // is the whole of the detection, and the ablation table on this describe is
+    // what demonstrates that rather than asserting it.
+
+    // The row is still claimable by a successor at the end of all that, which
+    // is the point of relinquishing it in the first place.
+    const [key, lifecycle] = await stuckLifecycle(stateStore, 'AR-76')
+    const successor = await stateStore.claimDispatchLifecycle(
+      WORKSPACE,
+      key,
+      lifecycle,
+      'successor-publisher',
+      Date.now(),
+      60_000,
+    )
+    expect(successor.acquired).toBe(true)
+  }, 20_000)
+
+  /**
+   * P1 from the same review, and the sharpest thing in it: the first version of
+   * this fix relinquished the lease AFTER `#writeInFlightRegistry()`, on the
+   * happy path only. A rejecting registry write skipped the handback entirely
+   * and left the abandoned key renewing its lease forever, announced by nothing
+   * louder than a `warn` — the identical shape of the bug being fixed (#379
+   * freed the batch slot but not the lease, on the failure path), reproduced
+   * one level up in the fix for it.
+   *
+   * The registry write is the failure injected here because it is the one the
+   * reviewer named, but the guarantee is structural: the handback is in a
+   * `finally` spanning every await in the cleanup, so `#batch()` rejecting
+   * would be caught by this too.
+   */
+  it('still hands the lease back when the in-flight registry write fails during cleanup', async () => {
+    const mount = new FakeMountClient({ [issuePath(77)]: issueFile(77) })
+    const fleet = new HostUnavailableReleaseFleetClient()
+    const stateStore = new InMemoryStateStore({ batchSize: 2 })
+    const warnings: unknown[][] = []
+    const factory = createFactory(config(), {
+      mount,
+      fleet,
+      stateStore,
+      triage: new StaticTriage(),
+      dispatchLifecycleRetryMs: RETRY_MS,
+      dispatchLifecycleRenewMs: RENEW_MS,
+      logger: { warn: (...args: unknown[]) => warnings.push(args), error: () => undefined },
+    })
+
+    await factory.runOnce()
+
+    // Fail the registry write ONLY inside dead-letter cleanup. Failing
+    // `listFailureHandoffs` is how the rejection is injected — it is awaited
+    // inside `#writeInFlightRegistry`, so this rejects that method the way a
+    // failing durable write would — and the abandoned counter is the gate,
+    // because it is incremented before cleanup is armed and after the release
+    // loop's own registry writes.
+    //
+    // The gate matters: failing every write instead makes
+    // `#writeInFlightRegistry` throw out of `#finishDurableRelease` before the
+    // `failed.length > 0` branch, so nothing is ever charged and the
+    // dead-letter never fires. That version of this test failed for a reason
+    // unrelated to the lease, which is exactly the trap this review is about.
+    const handoffsFrom = stateStore.listFailureHandoffs.bind(stateStore)
+    stateStore.listFailureHandoffs = async (...args: Parameters<typeof handoffsFrom>) => {
+      if (factory.status().counters.dispatchLifecycleReleaseAbandoned) {
+        throw new Error('in-flight registry write failed')
+      }
+      return await handoffsFrom(...args)
+    }
+
+    fleet.emitAgentExit('ar-77-impl-pear', 'issue-done')
+    await vi.waitFor(
+      () => expect(factory.status().counters.dispatchLifecycleReleaseAbandoned).toBe(1),
+      { timeout: 10_000, interval: 5 },
+    )
+
+    // The registry write really did fail — otherwise this test proves nothing
+    // beyond what the happy-path one already proves.
+    await vi.waitFor(
+      () => expect(warnings.map(([message]) => message)).toContain(
+        '[factory] dead-lettered release could not free its batch slot',
+      ),
+      { timeout: 10_000, interval: 5 },
+    )
+
+    // ...and the lease came back anyway.
+    await vi.waitFor(
+      async () => expect(await leaseIsFree(stateStore, 'AR-77')).toBe(true),
+      { timeout: 10_000, interval: 5 },
+    )
+  }, 20_000)
+})
+
 /**
  * Differential equivalence between the pull-index fast path and the record walk.
  *