Let a session read claim state instead of probing for it by claiming (#707) - #744
Merged
Conversation
test/mix/tasks/loopctl_enrich_search_events_test.exs asserts that the enrichment task will not answer another machine's row from this machine's transcripts. It built that row with a literal client_host of Marks-Mac-mini.local, which is a real host in the fleet. So the test passes everywhere except on the machine it names. Run it on mac-mini and "another machine" IS this machine: the row is legitimately this host's, the task correctly enriches it, and the assertion inverts. The very next test in the file is the positive case built on local_host/0, so the two were asserting opposite outcomes for the same host - which is only invisible while nobody runs the suite there. It is a full stop, not a flake: every commit on that machine fails the gate, on a test that has nothing to do with the change being committed. Derive the foreign host from local_host/0 instead - "not-" <> local_host() <> ".invalid" - so it is guaranteed to differ on every machine, and the .invalid suffix (RFC 2606) means it can never name a real host in any fleet. Verified by running the file on mac-mini, where it previously failed: 11 tests, 0 failures.
…707) Until now the only way to learn whether a handoff ref was claimed was to attempt the claim and read the result: 201 meant free, 409 already_claimed meant taken. That probe is destructive on this deployment. channel_claim is idempotent for the owning AGENT - re-claiming your own active ref returns the existing claim rather than a 409 - and every session on the fleet authenticates as one agent_id. So a probe issued while a PEER SESSION holds the ref hands back that peer's claim as though it were the prober's own, and the release that tidies the probe up DELETES it. The peer keeps working a handoff the bus has already reopened, and a second machine picks it up. Issue #707 recorded that sequence: a claim vanished well inside its stated lease while another session was most of the way through the work. GET /api/v1/channel/claims is the read that makes the probe unnecessary. It writes nothing and answers the same question, with an optional ref for the point lookup a session about to claim actually wants - an empty claims array means that ref is free right now. The listed set is the handoffs EXCLUSION, mirrored deliberately. A row is active when it is DONE (terminal) or still inside its lease, byte-for-byte the condition active_claim_subquery/1 uses to hide a handoff from directed_handoffs_page/3. A released claim and a lease that expired without completion are both absent, exactly as both reopen the handoff. That coupling is the point: this read has to answer "why is that handoff missing from my handoffs list", and a predicate that merely resembled the exclusion would answer a subtly different question and send the reader back to probing. A test asserts the two sets against each other on the same rows, so they cannot drift. Also documents the scope that made the hazard possible, rather than silently changing it. Claims are owned by (tenant, project, AGENT, ref) with no session dimension, so two sessions on one key can done or release each other's claims and the server cannot tell them apart. That is now stated in the controller moduledoc and in the channel_claim / channel_release tool descriptions, alongside the note that the abandoned-lease sweep, not the release path, is what protects a claim whose session died. Same posture ChannelLockController already documents for soft-locks. Tightening it to a session dimension would strand any session that restarts mid-work behind its own lease, which is a worse failure than the one being fixed. The read gets its own rate-limit bucket rather than a share of the claim write cap: an endpoint that exists to replace a probe must never be able to rate-limit its caller out of claiming, which is what a shared counter would do and would push the caller straight back to probing. Oracle posture matches the sibling reads - a foreign, nonexistent or malformed project_id is an empty page, never a 404. While verifying this, two of the three findings in #707 did not survive and are recorded on the issue: lease_seconds does not reproduce (probed live: 60 requested, 60 honoured, and the plumbing predates the report by a month), and the suggested mechanism for the invisible read is disproven by the query, which uses NOT EXISTS and so cannot filter out a never-claimed handoff. MCP server 2.77.0 adds channel_claims.
…ould do The read listed the handoffs EXCLUSION (done or unexpired), but the unique slot that refuses a claim is held by any unswept row. An expired-but-unswept claim was therefore absent from the read, whose empty page is documented as "this ref is free", while the very next claim answered 409 already_claimed, whose documented meaning is "taken, move on" - so a session abandoned a handoff for up to one sweeper interval. The read now lists every row that still holds the slot and flags the two lifecycle states on each row, so "absent" and "claimable" are one set again. Renamed to claims_page/3 (and clamp_claims_limit/default_claims_limit) because "active" already means the handoffs predicate in this module. Also in this round: - A NUL byte in the ref query param reached Postgres and 500'd (22021). A blank, non-binary or NUL-carrying ref now yields an empty page instead of silently falling through to the UNFILTERED query, which handed a caller asking about one ref some other ref's claim. - OPEN claims are ordered ahead of terminal ones. DONE claims are retained seven days, so a newest-first page could drop a live claim to make room for a week-old finished one - reporting a held ref as free. - A missing or non-UUID project_id is a 422 rather than an empty page. The MCP client only sends project_id when it is set, and an empty page here means "claimable"; a foreign well-formed id keeps the oracle-safe empty page, now recorded as an ownership_rejected security event like the write path. - The read cap is clamped to a quarter of the per-key pipeline budget, so polling can no longer 429 the caller out of claiming - the guarantee the plug comment claimed and a dedicated bucket alone does not provide. - Corrected the docstring that asserted a US-40.D3 membership gate on the read. The read is tenant-scoped and NOT membership-gated, uniform with GET /channel/locks, and now says so where ChannelLockController does.
…he write twins The round-1 commit made the read agree with the write on the expired-unswept row, then overstated the result as an equivalence with the claim outcome. It is not one, and the gaps are exactly where #707 lives: - Listed does NOT imply refused. claim/5 returns the OWNING agent's still-open row idempotently, which on a fleet sharing one agent_id is the common case, and re-claiming a listed ref to check IS the destructive probe. Absent does NOT imply claimable either: a superseded ref, an exhausted per-agent claim budget and a non-member caller are all refused with no row listed. Restated as "a row holds this ref's slot" in coordination.ex, the controller docstring, the MCP tool description and mcp-server README, with the exceptions named where the claim is. - Only OPEN rows were protected from page truncation, so an expired-unswept row -- which still holds the slot -- was evicted by a newer retained DONE row and the ref read back as free. DONE now sorts LAST, behind open and expired-unswept alike. - The NUL-byte guard landed on the read only. claim/5 compares the ref against channel_posts.key, and done/5 and release/5 against channel_claims.ref, both before any changeset runs, so the same input escaped as a Postgrex 22021 500. One definition of a well-formed ref (Coordination.valid_ref?/1, mirroring the claim changeset's trim/NUL/byte-cap rules) now serves the read filter, the owner-claim fetch, and the controller; claim/5 applies its changeset before it queries. - A malformed ref got the empty page the same commit ruled unacceptable for a malformed project_id, for the same reason -- now a 422, and the MCP client sends a blank ref through instead of silently widening the point lookup into a whole-channel list. - The read rate cap was clamped to a quarter of the pipeline budget with no floor, so below a 240/min budget it bound TIGHTER than the claim write cap and 429'd the caller out of the very read that replaces the probe. Floored at claim_limit/1, and the two comments that claimed the opposite corrected. - channel_claim's tool text no longer says a 409 means move on unconditionally: an expired lease awaiting the sweeper should be retried shortly. - Stale post-rename comments: the controller moduledoc bullet still said ACTIVE claims, and resolve_claim_collision still said discovery treats expired leases as inactive.
An expired lease is a fact about the CLAIM; whether the handoff is back in GET /channel/handoffs is a fact about the POST. The round-1 wording conflated them and told the agent to retry a ref whose post was superseded, quarantined or past its 30-day TTL -- none of which reappear, so the advised retry spins and then lands on a refusal. Says what expired actually means in coordination.ex, the controller docstring, the claim_json comment, the MCP tool description and the mcp-server README, and points the reader at channel_handoffs for the other half.
…ue (#707) The review of the claim read routed this to a follow-up because FallbackController was outside the reviewed diff. It is not a follow-up: this PR is what created the contradiction, and an existing Node test pinned the old wording, so CI on this branch was red until it was resolved. POST /channel/claims answered 409 already_claimed for four unrelated situations, and that code renders "Do not retry the same ref; move on to other work." Two of the four are not that: - a lease that EXPIRED without completion still holds the unique slot, so the insert fails - but nobody is working it and ChannelClaimSweeper is about to reap it. The ref is seconds from being free and the caller was told to abandon it. - a SUPERSEDED ref is held by nobody at all. The caller should claim the successor handoff, and instead recorded the ref as taken. The third, an exhausted per-agent claim budget, is a limit on the CALLER that says nothing about the ref - a peer can take that very ref, which the new controller test asserts directly. So the four now carry four codes, all still HTTP 409: already_claimed (a peer's live claim, or your own completed one - move on), claim_lease_expired (retry THIS ref shortly, with an advisory retry-after of 60s), ref_superseded (claim the successor), and claim_budget_exhausted (finish or release one of yours). claim_lease_expired is the one that was actually costing work, and it is the one this branch made visible: claims_page/3 LISTS that same row and flags it expired, so the read and the write were telling a caller opposite stories about one ref. A test now asserts both halves against the same row so they cannot drift apart again. The controller lists the three new reasons explicitly rather than adding a catch-all, so a future fifth cause is a compile-time CaseClauseError instead of a silent 500. The channel_claim tool description stops apologising that the body "cannot see which of those you hit" and names the four codes; its Node test now pins that split rather than the single move-on message it used to require. CHANGELOG records the code change as operator-facing, since a client branching on already_claimed for the superseded or budget cases needs updating.
…ctl into feat/channel-claims-read-2 # Conflicts: # CHANGELOG.md
mkreyman
added a commit
that referenced
this pull request
Aug 22, 2026
…dly (#746) GET /api/v1/routes advertises itself as what an agent calls to discover the main endpoints before guessing. Every one of the 15 Epic 39/40 coordination-bus routes was absent from it - channel posts, handoffs, claims and advisory file locks, the whole surface. That omission is legal by the module's own contract (a curated index, not the API surface, which is the OpenAPI spec) and it is still the wrong call here, for the same reason the admin routes earn coverage. This is the mechanism a session is TOLD to reach for when it hands work to another machine. An agent that cannot find it does not degrade politely to the OpenAPI spec - it does the work twice, or drops it. That is precisely the failure the bus exists to prevent, so hiding the bus from discovery undoes the feature rather than inconveniencing it. Found while verifying #744 against production: the deployed router serves GET /api/v1/channel/claims and the curated index listed no channel route at all, so the read I had just shipped to make claim state discoverable was itself undiscoverable. A coverage test now pins the set the way the /admin one does, matching on (method, path) rather than path alone so a route that gains a verb is caught too. It earned its keep on the first run: it found two more I had missed by hand, the operator quarantine pair (GET /channel/posts/quarantined, POST /channel/posts/:id/release). Like the /admin test it asserts its own filter matched something, so a future change to route shapes cannot quietly turn it into a test of nothing. Descriptions carry the hazards an agent needs at the point of choosing, not just the route: that channel post bodies are untrusted data authored by peers, that a soft lock is advisory and blocks nobody, that a release is scoped to your AGENT and not your session, and that the claim 409 must be branched on by error.code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #707 (the half of it that survived triage — see below).
The defect
Until now the only way to learn whether a handoff ref was claimed was to attempt the
claim and read the result:
201meant free,409 already_claimedmeant taken. Thatprobe is destructive on this deployment.
channel_claimis idempotent for the owning agent — re-claiming your own active refreturns the existing claim rather than a 409 — and every session on the fleet
authenticates as one
agent_id. So a probe issued while a peer session holds the refhands back that peer's claim as though it were the prober's own, and the
releasethattidies the probe up deletes it. The peer keeps working a handoff the bus has already
reopened, and a second machine picks it up.
That is not hypothetical. #707 recorded the sequence: a claim vanished well inside its
stated lease while another session was most of the way through the work, and two machines
ended up on the same task.
The fix
GET /api/v1/channel/claims— a read that writes nothing and answers what the callerneeds before claiming, with an optional
reffor the point lookup.It lists every UNSWEPT claim row, not the "active" ones. The
(tenant_id, project_id, ref)unique index does not care about lifecycle: a DONE claim,an OPEN one, and one whose lease expired before the sweeper reaped it all refuse a fresh
claim identically. Each row carries
doneandexpiredflags so the caller can alsotell what state it is in.
Page ordering is three-tier — open, then expired-unswept, then done — because DONE
claims are retained for days and a plain newest-first page would drop a live claim to
make room for a week-old finished one, reporting a held ref as free.
A malformed
ref(blank,?ref[]=, or a NUL byte — valid UTF-8 that Postgresrefuses to compare against
text, previously a 500) yields an empty page and never fallsthrough to the unfiltered query, which would hand a caller asking about one ref some
other ref's claim.
Coordination.valid_ref?/1is now the single ref-shape rule, mirroringChannelClaim.create_changeset/2, and the write twins (fetch_owned_claim/4,claim/5)use it too.
The scope that made the hazard possible is documented, not silently changed. Claims
are owned by
(tenant, project, AGENT, ref)with no session dimension, so two sessions onone key can
doneorreleaseeach other's claims and the server cannot tell them apart.That now appears in the controller moduledoc and the
channel_claim/channel_releasetool descriptions, with the note that the abandoned-lease sweep — not the release path —
protects a claim whose session died. Tightening ownership to a session dimension would
strand any session that restarts mid-work behind its own lease: a worse failure than the
one being fixed.
What the listing does NOT promise. An earlier draft claimed a ref is listed exactly
when a fresh claim would be refused. Review showed that is false in both directions — the
owning agent's own open row is returned idempotently rather than refused, and a superseded
ref, an exhausted per-agent claim budget, and a non-member caller are all refused with no
row listed. The wording is corrected in all four places it had been copied to.
The read is tenant-scoped and not membership-gated, uniform with
GET /channel/locks;the US-40.D3 membership gate is on the write path. A foreign or nonexistent project still
gets the oracle-safe empty page, now recorded as an
ownership_rejectedsecurity eventlike the write path. A missing or non-UUID
project_idis a 422 rather than an empty pagethat the contract reads as "claimable".
Triage: two of #707's three findings did not survive
Both recorded on the issue with the evidence.
lease_secondsignored — does not reproduce. Probed live against production: 60requested, 60 honoured to the microsecond. The plumbing has been intact since channel_claims table — exactly-once handoff claim (INSERT-to-claim, 409 already_claimed) #447, a
month before the report, and
@default_lease_secondshas never been 86_400.directed_handoffs_page/3correlates claims with NOT EXISTS, so an unclaimedhandoff is precisely the row that passes.
Verification
wf_4a844eca-d81: 2 rounds, 8 reviewer lenses, 15 confirmedfindings, 16 fixed, 0 invalid deferrals.
publish.inSync: true.--strictclean, dialyzer clean.Out-of-scope finding — owed, not deferred
One confirmed finding falls outside this diff and gets its own follow-up branch:
FallbackController's 409already_claimedbody says "do not retry, move on", which iswrong for the expired-but-unswept case this PR makes visible. The fix is distinct error
codes in
resolve_claim_collision/4for its four causes (expired-unswept, superseded ref,exhausted claim budget, caller's own DONE claim) plus matching
FallbackControllerclauses. The in-scope half — the
channel_claimtool description no longer telling anagent a 409 unconditionally means "move on" — is in this PR.
MCP server 2.77.0 adds
channel_claims.