From e1b94f2c44bd6e1b624e097a5434d6216dda23ca Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 26 Aug 2026 14:37:06 +0200 Subject: [PATCH 1/3] fix(orchestrator): give the dispatch lease back when a release is dead-lettered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production 0.1.79 — which already contains #379 — dispatched nothing for three days. Four issues, one of them a canary filed purely to test dispatch, were all simultaneously blocked on: [factory] durable dispatch is leased by another publisher; waiting for lease release {"issue":"1540","leaseRemainingMs":189205,"retryMs":1000} while the holder logged, continuously: RelayError: Agent "ar-1540-impl-relay" has no live host node code transport_error, statusCode 503, rawCode agent_host_unavailable #379's bound is not the thing that failed. A 503 `agent_host_unavailable` is not `isAgentAlreadyGoneOnRelease`, so it lands in `failed[]` exactly like any other release failure, `#chargeReleaseAttempt` runs on every re-arm, and the dead-letter fires on schedule. What #379 did not do is give the LEASE back. `#dispatchLifecycleEpochs` is not merely a cache: `#renewDispatchLifecycles` walks it every 60 s and re-stamps a full 5-minute lease onto every key it finds, unconditionally. `#releaseDeadLetteredSlot` handed back the batch slot and left the key in that map, so a work unit this process had permanently sworn off driving kept a renewed lease for the life of the process. The `releasing` row is retained on purpose so a successor can re-drive the cleanup with a fresh budget — but a successor first has to CLAIM the row, and a lease renewed forever by a process that will never finish is a claim nobody can win. `leaseRemainingMs` of ~189 s against the 300 s TTL is that renewal, observed 111 s in. That is strictly worse than the spin #379 replaced: the spin was loud and process-local, whereas a retained lease on a non-terminal row is silent and blocks every other publisher, a restart of this one included. The fix relinquishes durable ownership wherever the budget declines a re-arm. The epoch is dropped before the durable release so a renewal tick already in flight cannot re-stamp the lease afterwards; if the durable release itself fails, dropping the epoch alone still ends the livelock, because the lease then expires within its TTL instead of never. Relinquishing at the dead-letter alone is not enough. `#driveDispatchLifecycle` re-claims the lease at the top of every drive, before it has read the phase, so anything that drives an already-dead-lettered key — the held-agent-deadline sweep, a registry restore, a takeover — puts the epoch straight back into the renewal map. `#chargeReleaseAttempt` therefore relinquishes on its abandoned early return too, not only on the transition. Deliberately NOT done: reclassifying `agent_host_unavailable` as terminal. `isAgentAlreadyGoneOnRelease` returning true means the release SUCCEEDED and the agent is gone; a 503 does not establish that, and treating it as success would checkpoint `releasedAtMs` and let worktree cleanup run against a worker that may still be alive behind a briefly unreachable host. The distinction between "host briefly down" and "host permanently gone" is not visible in a single response — it is a distinction in time, and the attempt budget is already the thing that measures it. The budget IS the terminal classifier; it just was not wired to release ownership. Tests reproduce the production shape: the verbatim RelayError 503 `agent_host_unavailable` on the remote/durable lifecycle, asserted through the invariant (a successor can claim the key) rather than through a counter, so the test states the property instead of the implementation. Both fail on origin/main by timing out on a lease that never becomes free. Co-Authored-By: Claude Opus 5 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 --- src/orchestrator/factory.test.ts | 193 +++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 91 +++++++++++++-- 2 files changed, 274 insertions(+), 10 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index fade767f..f8104d5b 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -31890,6 +31890,199 @@ 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 { + 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 + 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 at the dead-letter is not enough on its own: the renewal + * interval walks `#dispatchLifecycleEpochs` and re-stamps a full TTL on + * everything it finds. If the abandoned key is still in that map, the lease + * comes straight back and the livelock resumes a minute later — which is + * exactly the shape production was in. This asserts the epoch is dropped, so + * there is nothing left for the renewer to re-stamp. + */ + it('does not let the renewal interval re-stamp 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, + 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 }, + ) + + // A successor takes the key... + 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) + + // ...and the abandoning process must not be able to renew it back out from + // under them. `renewDispatchLifecycle` is owner+epoch fenced, so this is + // really asserting that the abandoner no longer believes it owns the key. + await new Promise((resolve) => setTimeout(resolve, RETRY_MS * 40)) + const after = await stateStore.getDispatchLifecycle(WORKSPACE, key) + expect(after?.lease?.owner).toBe('successor-publisher') + }, 20_000) +}) + /** * Differential equivalence between the pull-index fast path and the record walk. * diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 74479f1c..1a9dcaaa 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -7720,7 +7720,17 @@ export class FactoryLoop implements Factory { * the only thing still running is the spin. */ #chargeReleaseAttempt(record: InFlightIssue, key: string, context: string): boolean { - if (this.#dispatchLifecycleReleaseAbandoned.has(key)) return false + if (this.#dispatchLifecycleReleaseAbandoned.has(key)) { + // Re-entry, and the reason relinquishing the lease once is not enough. + // `#driveDispatchLifecycle` re-claims the lease at the TOP of every + // drive, before it has read the phase, so anything that drives an + // already-dead-lettered key — the held-agent-deadline sweep, a registry + // restore, a takeover — puts the epoch straight back into the renewal + // map and re-arms the livelock this bound just escaped. Whenever the + // budget declines a re-arm, ownership goes back too. + this.#trackDispatchLifecycleDrive(this.#relinquishDispatchLifecycleLease(key, record.issue.key)) + return false + } const attempts = (this.#dispatchLifecycleReleaseAttempts.get(key) ?? 0) + 1 this.#dispatchLifecycleReleaseAttempts.set(key, attempts) if (attempts <= DISPATCH_LIFECYCLE_MAX_RELEASE_ATTEMPTS) return true @@ -7742,18 +7752,71 @@ export class FactoryLoop implements Factory { // spin, it does not declare the work unit clean. durableLifecycleRetained, }) - const drive = this.#releaseDeadLetteredSlot(record, key) - .catch((error) => { - this.#logger.warn?.('[factory] dead-lettered release could not free its batch slot', { - issue: record.issue.key, - error: describeError(error).errorMessage, - }) - }) - .finally(() => this.#dispatchLifecycleDrives.delete(drive)) - this.#dispatchLifecycleDrives.add(drive) + this.#trackDispatchLifecycleDrive( + this.#releaseDeadLetteredSlot(record, key) + .catch((error) => { + this.#logger.warn?.('[factory] dead-lettered release could not free its batch slot', { + issue: record.issue.key, + error: describeError(error).errorMessage, + }) + }), + ) return false } + /** Keeps a lifecycle-side effect awaitable by `stop()` without leaking the set entry. */ + #trackDispatchLifecycleDrive(promise: Promise): void { + const drive = promise.finally(() => this.#dispatchLifecycleDrives.delete(drive)) + this.#dispatchLifecycleDrives.add(drive) + } + + /** + * Stop asserting durable ownership of a work unit this process will not drive + * again. + * + * `#dispatchLifecycleEpochs` is not merely a cache. `#renewDispatchLifecycles` + * walks it every `DISPATCH_LIFECYCLE_RENEW_MS` and re-stamps a full + * `DISPATCH_LIFECYCLE_LEASE_MS` onto every key it finds, unconditionally. + * + * A dead-lettered release deliberately leaves its row in the non-terminal + * `releasing` phase so a successor or a restart can re-drive the cleanup with + * a fresh budget. That is only a recovery path if the successor can CLAIM the + * row — and a lease renewed forever by a process that has permanently given + * up driving it is a claim nobody can ever win. #379 bounded the retry and + * handed back the batch slot, then kept the key locked for the life of the + * process, which is how production reached four issues (a dispatch canary + * among them) all logging `durable dispatch is leased by another publisher` + * at 1 Hz for three days while the holder logged nothing but 503 + * `agent_host_unavailable`. + * + * The epoch is dropped BEFORE the durable release so a renewal tick already + * in flight cannot re-stamp the lease after it has been relinquished. If the + * durable release itself fails, the dropped epoch alone still ends the + * livelock: nothing renews the lease any more, so it expires within + * `DISPATCH_LIFECYCLE_LEASE_MS` instead of never. + */ + async #relinquishDispatchLifecycleLease(key: string, issueKey: string): Promise { + const epoch = this.#dispatchLifecycleEpochs.get(key) + if (epoch === undefined) return + this.#dispatchLifecycleEpochs.delete(key) + try { + await this.#state.releaseDispatchLifecycleLease( + this.#workspaceId, + key, + this.#dispatchLifecycleOwner, + epoch, + ) + this.#increment('dispatchLifecycleLeasesRelinquished') + } catch (error) { + this.#logger.warn?.('[factory] could not relinquish the dispatch lease of an abandoned work unit', { + issue: issueKey, + // The lease still expires on its own now that nothing renews it. + expiresWithinMs: DISPATCH_LIFECYCLE_LEASE_MS, + error: describeError(error).errorMessage, + }) + } + } + /** * Hand back the batch slot of a work unit whose release was dead-lettered. * @@ -7769,12 +7832,20 @@ export class FactoryLoop implements Factory { * local slot does not write a terminal phase. On a local lifecycle there is * no durable record to retain, so the batch record is all there is and * completing it is what keeps capacity honest. + * + * The DURABLE lease has to go back with the slot. Retaining the row for a + * successor while renewing the lease that locks the successor out is not a + * handoff, it is a permanent block on the key — see + * `#relinquishDispatchLifecycleLease`. */ async #releaseDeadLetteredSlot(record: InFlightIssue, key: string): Promise { const batch = await this.#batch() const next = batch.complete(record.issue) this.#uncompensatedDispatchClaims.delete(key) await this.#writeInFlightRegistry() + // Before admitting anything else, and before `dispatch` gets a chance to + // throw: this process is done with the key either way. + await this.#relinquishDispatchLifecycleLease(key, record.issue.key) // A freed slot that nothing is admitted into is only half the repair. if (next && !this.#stopping) await this.dispatch(next.decision, { dryRun: next.dryRun }) } From 3c02a602dd2f6a5e906d62d0a2374918476c76ee Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 26 Aug 2026 14:59:46 +0200 Subject: [PATCH 2/3] fix(orchestrator): make the lease handback unskippable, and prove the renewal test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both cubic-dev-ai threads on #391. P1 — the fix had the hole it was fixing. The handback ran after `#writeInFlightRegistry()`, on the happy path only, so a rejecting registry write skipped it and left the abandoned key renewing its lease forever behind nothing louder than a `warn`. That is the same shape as the defect under repair (#379 freed the batch slot but not the lease, on the failure path), reproduced one level up in its own fix. Cleanup that only runs when the rest of cleanup succeeded is not cleanup. The handback now sits in a `finally` spanning every await in `#releaseDeadLetteredSlot`, so neither `#batch()` nor `#writeInFlightRegistry()` can strand the key. It is safe there because `#relinquishDispatchLifecycleLease` handles its own errors and cannot throw, so it can never mask the failure that brought us in. `#chargeReleaseAttempt` also arms the cleanup drive BEFORE the `logger.error` call, since a caller-supplied logger is the one remaining thing between "this unit is abandoned" and "its lease is handed back" that could throw. P2 — the renewal test was vacuous, and the reviewer was exactly right. It claimed the key for a successor and asserted `lease.owner` after a fixed 200 ms sleep. `renewDispatchLifecycle` is owner+epoch fenced, so that assertion holds whether or not the epoch was dropped, and 200 ms never reaches the 60 s `DISPATCH_LIFECYCLE_RENEW_MS`, so the renewer never ran at all. The red previously reported for it was the OTHER failure mode — the 10 s `leaseIsFree` deadline, the same one the first test already covers — not the property the test named. The renewer is now driven for real through a test-only `dispatchLifecycleRenewMs` port (same precedent as `dispatchLifecycleRetryMs`; only the interval moves, the TTL stamped is the production one). The assertion samples across many renewal intervals, because a single sample cannot tell a lease that is gone from one about to come back, and additionally asserts the abandoner never even ATTEMPTED a renewal on the key. This matters because `renewDispatchLifecycle` fences on owner and epoch but NOT on expiry: a relinquished lease keeps its owner and epoch, so its own former owner can fully resurrect it. Relinquishing the durable lease while leaving the epoch cached therefore buys nothing. Each test is now pinned by ablation rather than by assertion: ablation test1 test2 test3 pre-fix origin/main RED RED RED fix minus the epoch drop grn RED grn fix minus the `finally` grn grn RED And, run in the same file against the same bug (epoch retained), the OLD test PASSES while the new one FAILS — vacuity demonstrated rather than asserted. Co-Authored-By: Claude Opus 5 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 --- src/orchestrator/factory.test.ts | 136 +++++++++++++++++++++++++++---- src/orchestrator/factory.ts | 58 +++++++++---- src/types.ts | 7 ++ 3 files changed, 169 insertions(+), 32 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index f8104d5b..347560ca 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -31951,6 +31951,10 @@ class HostUnavailableReleaseFleetClient extends RemoteLifecycleFleetClient { 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. */ @@ -32030,14 +32034,33 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () }, 20_000) /** - * Freeing the lease at the dead-letter is not enough on its own: the renewal - * interval walks `#dispatchLifecycleEpochs` and re-stamps a full TTL on - * everything it finds. If the abandoned key is still in that map, the lease - * comes straight back and the livelock resumes a minute later — which is - * exactly the shape production was in. This asserts the epoch is dropped, so - * there is nothing left for the renewer to re-stamp. + * 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. + * `renewDispatchLifecycle` fences on owner and epoch but NOT on expiry, so a + * lease that was relinquished — `leaseUntilMs` pushed to the floor while + * `owner` and `epoch` stay put — is fully resurrectable by its own former + * owner. Relinquishing the durable lease while leaving the epoch cached + * therefore buys nothing: the livelock returns on the next renewal tick. + * + * 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 epoch rather than of the + * fencing. */ - it('does not let the renewal interval re-stamp the abandoned key', async () => { + 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 }) @@ -32047,6 +32070,9 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () 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 }, }) @@ -32056,13 +32082,27 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () () => 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 }, ) - // A successor takes the key... + // 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)) + } + + // And the abandoner never so much as ATTEMPTED a renewal on this key: a + // retained epoch would have driven `renewDispatchLifecycle`, been refused, + // and counted a lost lease. Zero attempts is the epoch being gone, stated + // without reference to who won the row. + expect(factory.status().counters.dispatchLifecycleLeasesLost).toBeUndefined() + + // 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, @@ -32073,13 +32113,79 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () 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) => { + 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 abandoning process must not be able to renew it back out from - // under them. `renewDispatchLifecycle` is owner+epoch fenced, so this is - // really asserting that the abandoner no longer believes it owns the key. - await new Promise((resolve) => setTimeout(resolve, RETRY_MS * 40)) - const after = await stateStore.getDispatchLifecycle(WORKSPACE, key) - expect(after?.lease?.owner).toBe('successor-publisher') + // ...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) }) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 1a9dcaaa..3486c30b 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -140,6 +140,7 @@ import { type InFlightIssue, issueKey, type ParkedIssue, + type QueuedIssue, type TrackedAgent, } from './batch-tracker' import { @@ -777,6 +778,7 @@ export class FactoryLoop implements Factory { readonly #babysitterWakeUnreachableRetryMs: number readonly #startupAgentExitDrainTimeoutMs: number readonly #dispatchLifecycleRetryMs: number + readonly #dispatchLifecycleRenewMs: number readonly #state: StateStore readonly #workspaceId: string readonly #relayflows?: FactoryPorts['relayflows'] @@ -1261,6 +1263,7 @@ export class FactoryLoop implements Factory { this.#babysitterWakeUnreachableRetryMs = ports.babysitterWakeUnreachableRetryMs ?? BABYSITTER_WAKE_UNREACHABLE_RETRY_MS this.#startupAgentExitDrainTimeoutMs = ports.startupAgentExitDrainTimeoutMs ?? STARTUP_AGENT_EXIT_DRAIN_TIMEOUT_MS this.#dispatchLifecycleRetryMs = ports.dispatchLifecycleRetryMs ?? DISPATCH_LIFECYCLE_RETRY_MS + this.#dispatchLifecycleRenewMs = ports.dispatchLifecycleRenewMs ?? DISPATCH_LIFECYCLE_RENEW_MS this.#workspaceId = config.workspaceId ?? 'default' this.#relayflows = ports.relayflows this.#worktrees = ports.worktrees @@ -6816,7 +6819,7 @@ export class FactoryLoop implements Factory { if (this.#dispatchLifecycleRenewTimer || this.#dispatchLifecycleEpochs.size === 0) return this.#dispatchLifecycleRenewTimer = setInterval(() => { void this.#renewDispatchLifecycles() - }, DISPATCH_LIFECYCLE_RENEW_MS) + }, this.#dispatchLifecycleRenewMs) this.#dispatchLifecycleRenewTimer.unref?.() } @@ -7738,6 +7741,21 @@ export class FactoryLoop implements Factory { this.#dispatchLifecycleReleaseAttempts.delete(key) this.#increment('dispatchLifecycleReleaseAbandoned') const durableLifecycleRetained = this.#usesDurableDispatchLifecycle() + // Cleanup is armed BEFORE anything that can throw (#391 review, P1). The + // only statements above are set/map writes and a counter, none of which can + // reject; `this.#logger.error` below is caller-supplied and can. Ordering + // the drive first means nothing between "this unit is abandoned" and "its + // lease is handed back" is allowed to fail in a way that skips the handback + // — which is precisely the defect shape this whole method exists to fix. + this.#trackDispatchLifecycleDrive( + this.#releaseDeadLetteredSlot(record, key) + .catch((error) => { + this.#logger.warn?.('[factory] dead-lettered release could not free its batch slot', { + issue: record.issue.key, + error: describeError(error).errorMessage, + }) + }), + ) // `error`, not `warn`. Every previous layer of this failure was invisible // until somebody read stderr by hand; a work unit whose cleanup this // process has permanently given up on is exactly the event that must not @@ -7752,15 +7770,6 @@ export class FactoryLoop implements Factory { // spin, it does not declare the work unit clean. durableLifecycleRetained, }) - this.#trackDispatchLifecycleDrive( - this.#releaseDeadLetteredSlot(record, key) - .catch((error) => { - this.#logger.warn?.('[factory] dead-lettered release could not free its batch slot', { - issue: record.issue.key, - error: describeError(error).errorMessage, - }) - }), - ) return false } @@ -7837,15 +7846,30 @@ export class FactoryLoop implements Factory { * successor while renewing the lease that locks the successor out is not a * handoff, it is a permanent block on the key — see * `#relinquishDispatchLifecycleLease`. + * + * The handback lives in a `finally` for a reason worth stating plainly + * (#391 review, P1). The FIRST version of this fix relinquished after + * `#writeInFlightRegistry()`, on the happy path only — so a rejecting + * registry write would skip it and leave the abandoned key renewing its lease + * forever, with nothing but a `warn` to show for it. That is the identical + * shape of the bug being fixed (#379 freed the slot but not the lease, on the + * failure path), reproduced one level up. Cleanup that only runs when the + * rest of cleanup succeeded is not cleanup. `#batch()` and + * `#writeInFlightRegistry()` can both reject; neither may strand the key. */ async #releaseDeadLetteredSlot(record: InFlightIssue, key: string): Promise { - const batch = await this.#batch() - const next = batch.complete(record.issue) - this.#uncompensatedDispatchClaims.delete(key) - await this.#writeInFlightRegistry() - // Before admitting anything else, and before `dispatch` gets a chance to - // throw: this process is done with the key either way. - await this.#relinquishDispatchLifecycleLease(key, record.issue.key) + let next: QueuedIssue | undefined + try { + const batch = await this.#batch() + next = batch.complete(record.issue) + this.#uncompensatedDispatchClaims.delete(key) + await this.#writeInFlightRegistry() + } finally { + // Unconditional, and safe to put in a `finally` because + // `#relinquishDispatchLifecycleLease` handles its own errors and cannot + // throw — so it can never mask the failure that brought us here. + await this.#relinquishDispatchLifecycleLease(key, record.issue.key) + } // A freed slot that nothing is admitted into is only half the repair. if (next && !this.#stopping) await this.dispatch(next.decision, { dryRun: next.dryRun }) } diff --git a/src/types.ts b/src/types.ts index cc5c60cc..4362117c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -68,6 +68,13 @@ export interface FactoryPorts { * release retry budget without spending ten real seconds waiting for it. */ dispatchLifecycleRetryMs?: number + /** + * Interval at which owned dispatch-lifecycle leases are renewed. Test-only + * override of the built-in 60 s, so a suite can actually observe the renewer + * run instead of asserting around it. A test that never lets this fire cannot + * distinguish a relinquished lease from a retained epoch (#391 review, P2). + */ + dispatchLifecycleRenewMs?: number relayflows?: FactoryRelayflowDispatchPort /** Local CLI checkout isolation. Remote fleet nodes own their own checkout lifecycle. */ worktrees?: AgentWorktreeManager From 1303ceb6d8473f7a96a2e7d27133be558d1ae6dc Mon Sep 17 00:00:00 2001 From: Miya Date: Wed, 26 Aug 2026 15:31:43 +0200 Subject: [PATCH 3/3] fix(state): fence lease renewal on expiry, not just owner and epoch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both cubic-dev-ai threads from the second review of #391. P2 — the handback was still losable to a race, and my own comment claimed otherwise. `#renewDispatchLifecycles` iterates a SNAPSHOT of the owned-epoch map. Dropping the epoch before the durable release only stops a renewal tick that STARTS afterwards; a tick already in flight still carries the key, and `releaseDispatchLifecycleLease` relinquishes by dropping `leaseUntilMs` while leaving `owner` and `epoch` exactly in place. Those are precisely the credentials an owner+epoch-only renewal check accepts, so the in-flight tick restored the lease for a full term and the livelock resumed. `renewDispatchLifecycle` now fences on expiry as well, in both stores: an expired or relinquished lease must be re-CLAIMED, which bumps the epoch and fences out the previous holder, never silently extended back to life. `saveDispatchLifecycle` and `promoteDispatchLifecycle` already fenced this way, so this closes an inconsistency in the StateStore contract rather than adding a new rule — renewal was the one operation that did not check. That fence is what makes the handback safe however the two race. The epoch drop and a new live re-read of the epoch map inside the renewal loop narrow the window ahead of it, but neither closes it alone, and the comment that claimed the ordering was sufficient has been corrected rather than left to mislead the next reader. P3 — the `dispatchLifecycleLeasesLost` assertion was justified by reasoning that contradicted the paragraph above it, and detected nothing. It claimed a retained epoch would drive a renewal, be refused, and count a lost lease; but renewal fenced on owner and epoch, both of which still match in the bug shape, so the renewal SUCCEEDED and no lease was ever counted lost. The counter stayed undefined either way. Removed, with a note saying so, rather than kept behind a corrected comment: the `leaseIsFree` sampling is the whole of the detection and the ablation table is what demonstrates it. New store-level test asserts the fence against BOTH implementations from one script, because a fence that holds in only one of them is not a fence. Co-Authored-By: Claude Opus 5 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 --- src/orchestrator/factory.test.ts | 33 ++++++++++++------- src/orchestrator/factory.ts | 25 ++++++++++++-- src/ports/state.ts | 16 +++++++++ src/state/file-state-store.test.ts | 52 +++++++++++++++++++++++++++++- src/state/file-state-store.ts | 10 +++++- src/state/in-memory-state-store.ts | 10 +++++- 6 files changed, 128 insertions(+), 18 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 347560ca..42d34106 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -32048,17 +32048,23 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () * * The property that actually matters: `#renewDispatchLifecycles` walks * `#dispatchLifecycleEpochs` and re-stamps a full TTL on every key it finds. - * `renewDispatchLifecycle` fences on owner and epoch but NOT on expiry, so a - * lease that was relinquished — `leaseUntilMs` pushed to the floor while - * `owner` and `epoch` stay put — is fully resurrectable by its own former - * owner. Relinquishing the durable lease while leaving the epoch cached - * therefore buys nothing: the livelock returns on the next renewal tick. + * 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 epoch rather than of the - * fencing. + * 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) }) @@ -32095,11 +32101,14 @@ describe('a dead-lettered release must not keep the durable dispatch lease', () await new Promise((resolve) => setTimeout(resolve, RENEW_MS / 2)) } - // And the abandoner never so much as ATTEMPTED a renewal on this key: a - // retained epoch would have driven `renewDispatchLifecycle`, been refused, - // and counted a lost lease. Zero attempts is the epoch being gone, stated - // without reference to who won the row. - expect(factory.status().counters.dispatchLifecycleLeasesLost).toBeUndefined() + // 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. diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 3486c30b..3cb5125e 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -7036,6 +7036,17 @@ export class FactoryLoop implements Factory { async #renewDispatchLifecycles(): Promise { for (const [key, epoch] of [...this.#dispatchLifecycleEpochs]) { + // The snapshot above can outlive the ownership it records: a relinquish + // that runs while this loop is awaiting the store for an earlier key + // leaves a stale entry here, and renewing it would re-block a key this + // process has already handed back (#391 review, P2). Re-read the live map + // rather than trusting the snapshot. + // + // This narrows the window but does not close it on its own — the delete + // can still land after this check and before the renew resolves. Closing + // it is `renewDispatchLifecycle`'s expiry fence, which makes a + // relinquished lease unrenewable no matter how the two race. + if (this.#dispatchLifecycleEpochs.get(key) !== epoch) continue const renewed = await this.#state.renewDispatchLifecycle( this.#workspaceId, key, @@ -7798,9 +7809,17 @@ export class FactoryLoop implements Factory { * at 1 Hz for three days while the holder logged nothing but 503 * `agent_host_unavailable`. * - * The epoch is dropped BEFORE the durable release so a renewal tick already - * in flight cannot re-stamp the lease after it has been relinquished. If the - * durable release itself fails, the dropped epoch alone still ends the + * The epoch is dropped BEFORE the durable release, so a renewal tick that + * STARTS after this point finds nothing to renew. That ordering alone is not + * sufficient and an earlier version of this comment wrongly claimed it was + * (#391 review, P2): `#renewDispatchLifecycles` iterates a snapshot array, so + * a tick already in flight still holds this key and would restore the lease + * for a full term. The guarantee comes from `renewDispatchLifecycle` fencing + * on expiry as well as owner and epoch, which makes a relinquished lease + * unrenewable however the two race; the epoch drop and the loop's live re-read + * narrow the window ahead of it. + * + * If the durable release itself fails, the dropped epoch alone still ends the * livelock: nothing renews the lease any more, so it expires within * `DISPATCH_LIFECYCLE_LEASE_MS` instead of never. */ diff --git a/src/ports/state.ts b/src/ports/state.ts index 96200911..e3590286 100644 --- a/src/ports/state.ts +++ b/src/ports/state.ts @@ -477,6 +477,22 @@ export interface StateStore { nowMs: number, leaseMs: number, ): Promise + /** + * Extend an owned, still-live dispatch-lifecycle lease. + * + * Fenced on owner, epoch AND expiry. The expiry half is not incidental: an + * expired or relinquished lease must be re-CLAIMED, which bumps the epoch and + * fences out the previous holder, never silently extended back to life. Without + * it a lease that was deliberately handed back is fully resurrectable by its + * own former owner, because relinquishment leaves `owner` and `epoch` in place + * and only drops `leaseUntilMs` — so any renewal already in flight, or driven + * from a snapshot taken before the handback, restores it for a full term and + * re-blocks the key (#391 review, P2). + * + * `saveDispatchLifecycle` and `promoteDispatchLifecycle` already fence this + * way; renewal was the one operation that did not, which was an inconsistency + * in this contract rather than a deliberate allowance. + */ renewDispatchLifecycle( workspaceId: string, key: string, diff --git a/src/state/file-state-store.test.ts b/src/state/file-state-store.test.ts index b2a8f188..7e9403e8 100644 --- a/src/state/file-state-store.test.ts +++ b/src/state/file-state-store.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' import { describe, expect, it } from 'vitest' -import type { BabysitterSessionState, ConversationSessionState, DispatchLifecycle, GithubIssueCommentWatchState, WaitingClarification } from '../ports/state' +import type { BabysitterSessionState, ConversationSessionState, DispatchLifecycle, GithubIssueCommentWatchState, StateStore, WaitingClarification } from '../ports/state' import { dispatchIssueIdentity } from '../dispatch/work-unit-identity' import { FileStateStore } from './file-state-store' import { InMemoryStateStore } from './in-memory-state-store' @@ -183,6 +183,56 @@ describe('FileStateStore', () => { } }) + /** + * Renewal must fence on expiry, not only on owner and epoch (#391 review, P2). + * + * `releaseDispatchLifecycleLease` relinquishes by dropping `leaseUntilMs` to + * the floor and leaving `owner` and `epoch` exactly where they were. So if + * renewal checks only owner and epoch, the former owner can extend a lease it + * has already handed back — for a full term — and re-block the key. That is + * not hypothetical: `#renewDispatchLifecycles` iterates a SNAPSHOT of the + * owned-epoch map, so a renewal tick already in flight when the handback runs + * holds precisely the credentials that would pass an owner+epoch-only check. + * + * `saveDispatchLifecycle` and `promoteDispatchLifecycle` already fence this + * way, so this closes an inconsistency in the contract rather than adding a + * new rule. Both stores implement it, so both are asserted here against the + * same script — a fence that holds in only one of them is not a fence. + */ + it.each<[string, (watchStatePath: string) => StateStore]>([ + ['FileStateStore', (watchStatePath) => new FileStateStore({ batchSize: 2, watchStatePath })], + ['InMemoryStateStore', () => new InMemoryStateStore({ batchSize: 2 })], + ])('refuses to renew a relinquished or expired dispatch lifecycle lease (%s)', async (_name, make) => { + const root = await mkdtemp(join(tmpdir(), 'factory-renew-fence-')) + try { + const store = make(join(root, 'state.json')) + const seed = dispatchLifecycle(91) + const key = dispatchIssueIdentity(seed.issue) + + const claim = await store.claimDispatchLifecycle('workspace-1', key, seed, 'owner-a', 1_000, 5_000) + expect(claim).toMatchObject({ acquired: true, lease: { owner: 'owner-a', epoch: 1 } }) + + // Control: a live lease renews normally, so a failure below is the fence + // and not a broken fixture. + expect(await store.renewDispatchLifecycle('workspace-1', key, 'owner-a', 1, 2_000, 5_000)).toBe(true) + + // Hand it back, then try to renew with credentials that are still, by + // owner and epoch, entirely valid. + await store.releaseDispatchLifecycleLease('workspace-1', key, 'owner-a', 1) + expect(await store.renewDispatchLifecycle('workspace-1', key, 'owner-a', 1, 2_100, 5_000)).toBe(false) + + // And the point of all of it: the key is claimable by someone else. + const successor = await store.claimDispatchLifecycle('workspace-1', key, seed, 'owner-b', 2_200, 5_000) + expect(successor).toMatchObject({ acquired: true, lease: { owner: 'owner-b', epoch: 2 } }) + + // A lease that lapsed on its own is the same case: it must be re-claimed, + // which bumps the epoch and fences the old holder, never silently extended. + expect(await store.renewDispatchLifecycle('workspace-1', key, 'owner-b', 2, 99_000, 5_000)).toBe(false) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + it('persists and fences dispatch lifecycle ownership across processes and crash takeover', async () => { const root = await mkdtemp(join(tmpdir(), 'factory-lifecycle-')) try { diff --git a/src/state/file-state-store.ts b/src/state/file-state-store.ts index 1d122df1..7adadf9f 100644 --- a/src/state/file-state-store.ts +++ b/src/state/file-state-store.ts @@ -291,7 +291,15 @@ export class DocumentStateStore extends InMemoryStateStore { return await this.#exclusive(async () => this.#withMutationLock(async () => { const document = await this.#loadFromDisk() const lifecycle = document.workspaces[workspaceId]?.dispatchLifecycles[key] - if (!lifecycle?.lease || lifecycle.lease.owner !== owner || lifecycle.lease.epoch !== epoch) return false + // Expiry is part of the fence. See StateStore#renewDispatchLifecycle: a + // relinquished lease keeps its owner and epoch, so without this a handback + // is undone by any renewal driven from a pre-handback snapshot. + if ( + !lifecycle?.lease || + lifecycle.lease.owner !== owner || + lifecycle.lease.epoch !== epoch || + lifecycle.lease.leaseUntilMs <= nowMs + ) return false lifecycle.lease.leaseUntilMs = nowMs + leaseMs lifecycle.updatedAtMs = nowMs await this.#persist(document) diff --git a/src/state/in-memory-state-store.ts b/src/state/in-memory-state-store.ts index 3b69e87e..3b0a7559 100644 --- a/src/state/in-memory-state-store.ts +++ b/src/state/in-memory-state-store.ts @@ -251,7 +251,15 @@ export class InMemoryStateStore implements StateStore { leaseMs: number, ): Promise { const lifecycle = this.#workspace(workspaceId).dispatchLifecycles.get(key) - if (!lifecycle?.lease || lifecycle.lease.owner !== owner || lifecycle.lease.epoch !== epoch) return false + // Expiry is part of the fence. See StateStore#renewDispatchLifecycle: a + // relinquished lease keeps its owner and epoch, so without this a handback + // is undone by any renewal driven from a pre-handback snapshot. + if ( + !lifecycle?.lease || + lifecycle.lease.owner !== owner || + lifecycle.lease.epoch !== epoch || + lifecycle.lease.leaseUntilMs <= nowMs + ) return false lifecycle.lease.leaseUntilMs = nowMs + leaseMs lifecycle.updatedAtMs = nowMs return true