fix(claude-channel): benign dedup for shared-agent reply race #1309
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { test } from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import { isBenignReplyDedup } from "./collavre-client.ts"; | ||
|
|
||
| test("a sibling session's completed-task conflict is benign", () => { | ||
| // The dispatch fanned out to two sessions sharing one agent; the other won the | ||
| // atomic claim and posted. Nothing of ours is lost by staying quiet. | ||
| assert.equal( | ||
| isBenignReplyDedup( | ||
| JSON.stringify({ error: "Task already completed or not delegated", reason: "already_completed" }), | ||
| ), | ||
| true, | ||
| ); | ||
| }); | ||
|
|
||
| test("a cancelled/failed/recovered task conflict is NOT benign", () => { | ||
| // Nobody answered this dispatch — the reply in hand is the only copy of it. | ||
| assert.equal( | ||
| isBenignReplyDedup( | ||
| JSON.stringify({ error: "Task already completed or not delegated", reason: "not_delegated" }), | ||
| ), | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| test("a task claimed but not yet answered is NOT benign", () => { | ||
| // The server flips the task to done before the reply comment is saved. A 409 | ||
| // from inside that window has no answer behind it — and the window can end in | ||
| // a rollback — so this reply is still the only copy. | ||
| assert.equal( | ||
| isBenignReplyDedup( | ||
| JSON.stringify({ | ||
| error: "Task already completed or not delegated", | ||
| reason: "claimed_without_reply", | ||
| }), | ||
| ), | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| test("a legacy server that sends no reason is NOT benign", () => { | ||
| // Back-compat runs the safe way: surface the conflict, as this client did | ||
| // before dedup existed, rather than silently dropping a possibly-valid reply. | ||
| assert.equal( | ||
| isBenignReplyDedup(JSON.stringify({ error: "Task already completed or not delegated" })), | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| test("a non-JSON body is NOT benign", () => { | ||
| assert.equal(isBenignReplyDedup("<html>502 Bad Gateway</html>"), false); | ||
| assert.equal(isBenignReplyDedup(""), false); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,28 @@ export interface RegisterBody { | |
| name: string; | ||
| } | ||
|
|
||
| // The server's machine-readable marker for the ONE benign reply conflict: the | ||
| // delegated task was already completed, so the dispatch this reply answers has | ||
| // an answer. See TaskClaimService::CONFLICT_ALREADY_COMPLETED. | ||
| const BENIGN_REPLY_CONFLICT_REASON = "already_completed"; | ||
|
|
||
| /** | ||
| * Whether a 409 body from /agent/reply is the sibling-session dedup and may be | ||
| * suppressed. Anything else — a cancelled/failed/recovered task, an id that is | ||
| * not ours, an unparseable body, or an older server that sends no reason at all | ||
| * — is NOT: the reply in hand is the only copy of that answer, and suppressing | ||
| * it drops it with no trace. Unmarked conflicts therefore fall back to being | ||
| * raised, which is what this client did before dedup existed. | ||
| */ | ||
| export function isBenignReplyDedup(body: string): boolean { | ||
| try { | ||
| const parsed = JSON.parse(body) as { reason?: unknown }; | ||
| return parsed?.reason === BENIGN_REPLY_CONFLICT_REASON; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| export function buildRegisterBody(params: RegisterParams): RegisterBody { | ||
| const body: RegisterBody = { | ||
| agent_name: params.agentName, | ||
|
|
@@ -73,7 +95,7 @@ export class CollavreClient { | |
| topicId: number, | ||
| text: string, | ||
| taskId: number, | ||
| ): Promise<{ handled: true; comment_id: number } | { handled: false; reason: string }> { | ||
| const body: Record<string, unknown> = { topic_id: topicId, text, task_id: taskId }; | ||
|
|
||
| const res = await fetch(`${this.baseUrl}/api/v1/agent/reply`, { | ||
|
|
@@ -85,12 +107,34 @@ export class CollavreClient { | |
| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| // 409 Conflict has two causes and only one of them is benign. | ||
| // | ||
| // Benign: multiple Claude Code sessions in the SAME working directory share | ||
| // one Collavre agent; a work-topic dispatch fans out to all of them and each | ||
| // session's turn calls reply with the same task_id. The server's atomic task | ||
| // claim lets exactly one win — the others are refused. The message was | ||
| // already delivered by a sibling, so this is success-with-nothing-to-do. | ||
| // | ||
| // Not benign: the dispatch left `delegated` WITHOUT being answered — an | ||
| // offline session cancelled it, it failed, or stuck-task recovery flipped it | ||
| // while this turn was still composing. The claim fails the same way, but no | ||
| // sibling posted anything, so treating it as dedup drops the user's answer | ||
| // silently. Only the server's explicit already_completed marker suppresses. | ||
| if (res.status === 409) { | ||
| const respBody = await res.text(); | ||
| if (!isBenignReplyDedup(respBody)) { | ||
| throw new Error(`Reply failed (409): ${respBody}`); | ||
| } | ||
| return { handled: false, reason: respBody }; | ||
|
Comment on lines
+123
to
+128
There was a problem hiding this comment.
When the original dispatch is cancelled or failed instead of answered (for example, offline-session cancellation or stuck-task recovery flips a delegated task while Claude is still composing), Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Confirmed and fixed in 96bb8ac. The claim fails identically in both cases, which is exactly why the client could not tell them apart: The server now names the cause rather than leaving the client to guess:
The client suppresses on the marker only.
Tests pin both directions. Ruby: an answered dispatch replied to twice returns
|
||
| } | ||
|
|
||
| if (!res.ok) { | ||
| const respBody = await res.text(); | ||
| throw new Error(`Reply failed (${res.status}): ${respBody}`); | ||
| } | ||
|
|
||
| const json = (await res.json()) as { comment_id: number }; | ||
| return { handled: true, comment_id: json.comment_id }; | ||
| } | ||
|
|
||
| // Post an out-of-band informational comment to a topic WITHOUT completing a | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When two same-directory Claude sessions reply at nearly the same time, the loser can reach this check after the winner's
claimhas committedstatus = donebut beforefinalizehas linked the reply comment. In that normal sibling-race windowreply_comment.present?is still false, so the server returnsclaimed_without_reply; the client then throws instead of treating the race as benign, leaving the original 409-as-user-visible-error behavior timing-dependent even though the winning reply is about to land. Consider waiting/reloading for finalization (or otherwise making claim+link atomic) before deciding this is a hard conflict.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in 97e3bda. You are right that the previous round made the loser's outcome timing-dependent, and the direction of that dependence is the bad one: the sibling race is the common case, and it lands in the done-without-reply window every single time.
#claimcommitsdone, then the controller runscomment.save— validations, callbacks, broadcasts — and only then does#finalizereachcomment.update_column(:task_id, ...). Every winning claim passes through claimed-without-reply on its way to answered, so deciding on the first read was reporting "your answer may be the only copy" for what is almost always a duplicate that is milliseconds from landing.The predicate is unchanged — only an actually linked comment is ever called benign, and nothing is inferred from status or elapsed time. What changed is that finalization gets a bounded chance to produce the proof before the answer is fixed:
Monotonic clock so a wall-clock adjustment cannot stretch or collapse the deadline mid-wait. The reads are
uncached— that one is load-bearing rather than defensive:ActiveRecord::QueryCachewraps the request, so the identicalSELECT 1 FROM comments WHERE task_id = ?would be served from the first (empty) result for the entire wait and the loop could never observe the winner's commit.Only the window pays for it. A non-
donetask returnsnot_delegatedbefore the wait is reached, and a reply that was already linked when the request arrived answers on the first read — both pinned by tests that assert the elapsed time stays under the grace period, so a later edit cannot quietly put the common paths behind a sleep.I did not take the make-claim-and-link-atomic option, though it is the tighter one on paper. Doing it means holding the
SELECT FOR UPDATEacrosscomment.save, which is exactly what the current shape avoids: the long comment above#claimexists because that save firesafter_update_commitwork —TriggerLoopCheckJob, the stop-button broadcast — and running it inside the claim transaction puts those side effects before the commit they depend on.update_allis there specifically to keep them out. Moving the claim after the save instead reopens the duplicate-comment race the claim was written to close. Waiting buys the same observable behaviour for the loser (it blocks either way, on a lock or on a poll) without moving side effects inside a transaction.What survives past the deadline is the case you and I both want surfaced: a claim whose worker died mid-save. It reports
claimed_without_reply500ms later than before, and nothing suppresses it.Four tests in a new
task_claim_service_test.rb, and reverting the wait to a single read fails two of them: a link that lands between the first and second read now reads asalready_completed(with an assertion that it re-read at all, so a first-read decision fails even if it guesses right), and the never-finalized claim asserts both that the window was actually given and that it is bounded.task_claim_service_test4 runs / 22 assertions / 0 failures;agents_controller_test75 runs / 344 assertions / 0 failures; full pre-push suite 2946 runs / 9267 assertions / 0 failures; rubocop clean on both files.