diff --git a/CHANGELOG.md b/CHANGELOG.md index c851fcd3..8581c0d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A hop that cannot read the conversation no longer locks one on its way out + +Delivering a hop took the run lock on the conversation the answer lands in, and only then read the +conversation that asked. A platform that answers that read with anything other than a missing thread +— an outage, a bad key, a dropped connection — threw from between the lock and the `finally` that +gives it back, so the lock was left held and unrenewed. For the next couple of minutes the person +could not start a run in that conversation, and the hop's own retry collided with the lock it was +still holding itself and spent one of its attempts reporting the conversation as busy. The read now +happens before the lock is taken, which it never needed: it is a different conversation. + ### A Bot's shell can no longer reach the embedded database without a password In the all-in-one image the cluster was `trust`-auth on loopback, and the Bot's shell runs in the diff --git a/server/src/agents/handoff-delivery.ts b/server/src/agents/handoff-delivery.ts index fdf07502..b671d2b1 100644 --- a/server/src/agents/handoff-delivery.ts +++ b/server/src/agents/handoff-delivery.ts @@ -202,6 +202,23 @@ export function createHandoffDelivery(options: { ? { threadId: work.answerIn } : await answerIn({ actorId: work.actorId, botId: work.toBotId }); + /* + * The conversation that ASKED, read BEFORE the lock is taken. + * + * The addressed Bot is joining something already in progress and has to have read it; its own + * conversation is new and empty, and reading that would tell it nothing. This is a different + * thread from the one about to be locked, so the lock never protected this read — and holding + * one across it means a read that throws leaks it. `historyOrEmpty` answers a missing thread + * with nothing and rethrows everything else on purpose ("a 500 from the platform means an + * outage or a bad key"), so one 500 left the addressed Bot's conversation locked, unrenewed and + * unreleasable until the platform's own TTL expired. For those two minutes the person could not + * start a run there, and the hop's own retry a minute later collided with the lock it was still + * holding itself, reported the conversation as busy, and spent one of its five attempts on it. + */ + const prior = conversationOnly( + await history({ threadId: work.threadId, actorId: work.actorId }), + ); + /* * The conversation's lock, before a single event is streamed. * @@ -239,14 +256,7 @@ export function createHandoffDelivery(options: { * displayed the question directly above the answer. */ const asked = [ - /* - * The conversation that ASKED, not the one it is answering in. The addressed Bot is joining - * something already in progress and has to have read it; its own conversation is new and - * empty, and reading that would tell it nothing. - */ - ...conversationOnly( - await history({ threadId: work.threadId, actorId: work.actorId }), - ), + ...prior, { id: `handoff-${runId}`, role: "user", content: message }, ]; agent.threadId = where.threadId; diff --git a/server/tests/agent-handoff-delivery.test.ts b/server/tests/agent-handoff-delivery.test.ts index 7252c6d7..cdfaa67f 100644 --- a/server/tests/agent-handoff-delivery.test.ts +++ b/server/tests/agent-handoff-delivery.test.ts @@ -44,7 +44,17 @@ function delivery( events: BaseEvent[], agent: AbstractAgent | null = stubAgent(), lockHeld = true, - options: { history?: readonly unknown[]; deadlineMs?: number } = {}, + options: { + history?: readonly unknown[]; + deadlineMs?: number; + /** + * The platform refusing to hand back the asking conversation. + * + * `historyOrEmpty` answers a missing thread with nothing and rethrows everything else, on + * purpose: a 500 is an outage or a bad key, not an empty conversation. + */ + historyError?: Error; + } = {}, ) { const requests: Array<{ threadId: string; @@ -62,7 +72,10 @@ function delivery( ? {} : { deadlineMs: options.deadlineMs }), agentFor: async () => agent, - history: async () => options.history ?? PRIOR, + history: async () => { + if (options.historyError) throw options.historyError; + return options.history ?? PRIOR; + }, newRunId: () => "run-2", answerIn: async () => ({ threadId: "answer-thread" }), lock: { @@ -99,6 +112,37 @@ function delivery( } describe("turning a hop into a turn", () => { + /* + * A hop that cannot read the conversation it was asked in has to fail without taking anything with + * it. The lock is on the conversation being ANSWERED in, the read is of the one that ASKED, so the + * lock never protected this read — and holding one across it means the failure costs a person the + * use of a conversation they are not even part of, for as long as the platform's own TTL. + */ + test("a history the platform will not hand back leaves no lock behind", async () => { + const { delivery: deliver, lockCalls } = delivery( + FINISHED, + stubAgent(), + true, + { historyError: new Error("the platform answered 500") }, + ); + + await expect( + deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "signed", + }), + ).rejects.toThrow("the platform answered 500"); + + /* + * Nothing was taken, so nothing was left held. Asserting the whole sequence rather than the + * absence of a release: a delivery that acquired and then released would also be correct, and + * this says which of the two happened. + */ + expect(lockCalls).toEqual([]); + }); + test("the addressed Bot reads the conversation before the ask", async () => { const { delivery: deliver, requests } = delivery(FINISHED);