Read the conversation that asked before taking a lock on the one being answered in

[kevin9327]

What this changes

server/src/agents/handoff-delivery.ts takes the run lock on the conversation the answer lands in,
and then reads the history of the conversation that asked — a different thread, which the lock
never protected:

• :212 const held = await lock.acquire({ threadId: where.threadId, … })
• :248 await history({ threadId: work.threadId, actorId: work.actorId })
• :265 try { … } finally { clearInterval(heartbeat); await lock.release(…) }

The read at :248 is the one await in the delivery that sits between the acquire and the finally
that gives the lock back.

historyOrEmpty (server/src/copilot.ts:953-963) answers a missing thread with nothing and rethrows
everything else, deliberately — "A 500 from the platform means an outage or a bad key". So one 500,
one expired key, one dropped connection, and the delivery throws with the lock still held:

1. The hop rejects and goes back on the queue with a 60 s delay
   (handoff-runner.ts:393-399), which is correct.
2. The lock on where.threadId — the addressed Bot's own conversation with that person — is never
   released and never renewed (the heartbeat is only armed at :261, after the read). It sits until
   the platform's TTL, THREAD_LOCK_TTL_SECONDS = 120 (copilot.ts:993). For those two minutes the
   person cannot start a run in that conversation.
3. The retry a minute later collides with the lock the hop is still holding itself, gets null
   from acquire, throws "is busy with another run" and spends one of its five attempts on it.

The module already argues this case for the other direction. The finally at :348-359 says:

Given back whatever happened. Left held, the conversation is unusable by anybody until the lock
expires: the person cannot ask a follow-up and the next hop is refused, which turns one failed
delivery into a conversation that has stopped working.

That is exactly what a failed history read produces — it just happens one line above where the
finally starts.

The fix

Hoist the read above the acquire. It is a read of another conversation, so the lock was doing nothing
for it, and moving it earlier also shortens the window the lock is held on the happy path. Nothing
else moves: the ask is still appended with the platform's own run id, which is only known after the
lock.

Where it runs

• New state that outlives a request? None. One local moved earlier in the same function.
• What happens on the second replica? Better, and this is the point. The lock is the
  platform's, shared by every replica; a replica that leaked one made the conversation unusable
  from all of them for the TTL, and the hop's own retry — which any replica may claim — walked
  into it. After this, a failed read leaves nothing behind for another replica to trip over.
• Anything serialised? Unchanged. The lock is still taken NX through the platform, and is
  still the thing that serialises runs in a conversation. What changed is that it is no longer
  held across an operation that can throw outside its finally.
• Anything fanned out to a browser? No.
• New listener, port, or schedule? None.

Boundary and audit

• Every acting call still goes through the gateway: unchanged.
• New refusals and new failures each write a row. No new outcome — the hop already rejects and is
  retried, and agent.handoff_retried / agent.handoff_failed are written by the runner exactly
  as before. This changes only what is left behind on the way out.
• Nothing new is trusted from the client.

Changelog

• CHANGELOG.md, under Unreleased.

Proof

server/tests/agent-handoff-delivery.test.ts gains a historyError option to the existing stub
harness and one case: a history read that throws, asserting the whole lock sequence is empty. It is
red on main and green here.

Against main's handoff-delivery.ts with the new test in place:

143 |     expect(lockCalls).toEqual([]);
error: expect(received).toEqual(expected)
- []
+ [ "acquire" ]
(fail) turning a hop into a turn > a history the platform will not hand back leaves no lock behind
 17 pass, 1 fail

With the change:

bun test server/tests/agent-handoff-delivery.test.ts server/tests/agent-handoff-runner.test.ts \
         server/tests/agent-handoff.test.ts server/tests/agent-handoff-tool.test.ts
                       -> 60 pass, 0 fail (113 expect() calls)
bun run format:check   -> Checked 487 files. No fixes applied.
bun run lint           -> Checked 490 files. No fixes applied.
bun run typecheck      -> app / server / worker all exit 0

The sequence is asserted rather than the absence of a release, because a delivery that acquired and
then released would also be correct and the assertion should say which of the two happened.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.