fix(tabularius): a fixed relay session id is frozen by the first client that claims it - #2005
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Multi-agent review roll call (CodeRabbit and Claude review automatically. Reviewers: post substantive findings only. Authors/agents: address every thread, push fixes to this branch, reply and resolve, then re-request review.) |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a conduct-broker registration conflict where a fixed relay session_id (“dispatch-serial-results”) becomes permanently owned by the first registering client if later registrations drift only in client-declared identity fields (e.g., provider_identity). The PR mitigates the freeze by keying the relay session id with a short digest derived from the identity fields that remain unnormalized by register(), and adds regression tests covering both the historical freeze and the keyed behavior.
Changes:
- Add
_relay_identity_key()and use it to produce a keyed relaysession_idduring compatibility ticket submission. - Add regression tests demonstrating the freeze on
provider_identitydrift with a fixed session id, and the absence of conflicts with a keyed session id. - Update test imports to use real broker/store types for the regression scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cli/src/limen/tabularius.py |
Introduces _relay_identity_key() and appends its digest to the relay session id to prevent permanent 409 conflicts on client-declared identity drift. |
cli/tests/test_tabularius.py |
Adds targeted tests reproducing the historical freeze and validating the keyed-session-id behavior against the real ConductBroker. |
| # register() binds these three identity fields from the authenticated principal; every OTHER | ||
| # identity field is client-declared and is compared verbatim against the stored session. | ||
| _RELAY_PRINCIPAL_BOUND_IDENTITY_FIELDS = ("agent", "surface", "session_id") |
| unkeyed = f"{ticket.agent}-{ticket.session_id}" | ||
| session_id = fake.registered[0].session_id | ||
| assert session_id != unkeyed | ||
| assert session_id.startswith(f"{unkeyed}-") | ||
| assert fake.registered[0].identity.session_id == session_id |
| requested_identity = unkeyed_identity.model_copy( | ||
| update={ | ||
| "session_id": _safe_identifier( | ||
| f"{unkeyed_identity.session_id}-{_relay_identity_key(unkeyed_identity)}", | ||
| "tabularius-relay-session", | ||
| ) | ||
| } | ||
| ) |
…atch receipt
Every serial dispatch receipt has 409'd at broker registration since 2026-07-19:
ConductError: conduct broker rejected request (409):
{"detail": "session_id is already registered to another identity"}
`register()` binds agent/surface/session_id from the authenticated principal and then rejects
any re-registration whose WHOLE identity object differs. Post-binding those three are forced
equal, so the only fields that can still differ are `provider_identity` and `native_run_id` —
both client-declared. That check is therefore structurally incapable of catching an authority
mismatch (`session_principals`, two lines later, is what guards authority); it can only ever
fire on cosmetic provider drift. Since every relay call site passes a FIXED session_id literal,
a literal is claimable exactly once and the loser is refused forever.
This is the #1408 relay freeze, recurring on the fields the #1408 fix did not normalize —
`_bind_conductor_identity` carries the comment describing exactly this failure mode. Reproduced
against the real broker with ONE principal throughout:
#1 provider_identity='limen-cli' : OK
#2 same again (idempotent re-register) : OK
#3 provider_identity='limen-cli-2' : CONFLICT
#4 'limen-cli-2' again (permanent?) : CONFLICT
#5 native_run_id set, provider unchanged : CONFLICT
#6 original identity still works : OK
The freeze is per-literal, which is why the symptom looked so specific: writers using their own
literals (`harvest`, `dispatch-async/reserve`, `heal-board/lifecycle-repair`) kept landing
receipts all month — the published projection holds 177 August receipts — while the one path
that records a *dispatch* was silently dead. `logs/throughput-governor.jsonl` then read
`dispatched: 0` on every pass and pinned the jules lane in `bootstrap` at 25/day, making the
requested 100/day structurally unreachable (#1995).
Fix: a refused literal falls back once to an id keyed on exactly the identity fields the keeper
compares but does not normalize.
A FALLBACK, NOT ALWAYS-KEYED — and the first cut of this fix got that wrong. The relay session id
is not internal plumbing: the keeper stamps it into `session_id` on every projection event, so it
is part of the recorded receipt that harvest and 14 tests read. Keying it unconditionally renames
an observable identifier estate-wide to fix a condition that only arises once a literal is already
poisoned (14 tests failed on exactly that). So the healthy path keeps the stable literal and only a
refused one falls back — which also makes the fallback legible: a keyed `session_id` in a
dispatch_log entry IS the signal that its literal is frozen.
Classified on the STATUS, never the prose — ConductError's own documented contract, since three
keepers word this refusal three ways. Both 409 shapes (frozen identity, bound principal) mean one
thing here: that literal is not mine to use. Registering a different id cannot touch the session
the keeper is protecting, and every authority check still applies to whatever this client does
register. A non-conflict failure is re-raised untouched.
Verified live against the poisoned keeper: a real `limen dispatch --agent jules --limit 1 --live`
now gets PAST registration and reaches `submit()`, where it stops on a different and legitimate
409 — `task ORG-artist-organ-face-0705 exact revision moved` — because the local projection has
been frozen at `track.date: 2026-07-26` since the board-publication PR rung was retired (#2001
carries that backlog, #2014 the drift that blocks it). Stacked defects; this is one of four.
4 regression tests: the healthy path keeps the literal; a frozen literal falls back and still
relays; a non-conflict failure is never retried; and the real ConductBroker is driven through the
freeze and then through the keyed id. cli/tests/test_tabularius.py: 34 passed.
Refs #1995
622e4d7 to
ffe64a1
Compare
|
Reworked — the first cut renamed an observable identifier. Keying it unconditionally therefore renamed an estate-wide identifier to fix a condition that only Classification is on the status, never the prose — Re-verified live against the poisoned keeper: 4 regression tests now (healthy literal preserved / frozen literal falls back and still relays / |
The first of the stacked defects behind #1995 / the 19-day jules stall. Supersedes #2003
(withdrawn: cut from a 5-commit-stale base, it silently reverted this file's
TaskAlreadyHomedtolerance work). This branch is rebuilt on current
origin/main;git diff origin/mainremovesnothing but the two lines the fix replaces.
Symptom
Every serial dispatch receipt has 409'd at broker registration since 2026-07-19. The jules
sessions launch for real; the receipt is never recorded, and the traceback lands in stdout the
beat's sensor runner swallows (#1989), so nothing surfaced it for 19 days.
Root cause
register()bindsagent/surface/session_idfrom the authenticated principal, then rejectsany re-registration whose whole identity object differs (
identitiesEqualis astableStringifycompare on the worker;prior.identity != session.identityin the pythonbroker). Post-binding those three fields are forced equal — two come from the principal, one is
the lookup key — so the only fields that can still differ are
provider_identityandnative_run_id, both client-declared.That check is therefore structurally incapable of catching an authority mismatch. Authority is
guarded two lines later by
session_principals(session_id is already bound to another principal). The identity check can only ever fire on cosmetic provider drift — and with thefixed relay id
dispatch-serial-results, the first client to register owns that literal forever.This is the #1408 relay freeze, recurring on the fields the #1408 fix did not normalize —
_bind_conductor_identitycarries the comment describing exactly this failure mode.Reproduced against the real broker with one principal throughout:
provider_identitylimen-clilimen-cli(re-register)limen-cli-2limen-cli-2againlimen-cli+native_run_idlimen-cliWhy the symptom looked so narrow
The freeze is per-session-id-literal. Writers with their own literals (
harvest,dispatch-async/reserve,heal-board/lifecycle-repair, …) kept landing receipts all month — thepublished projection holds 177 August receipts — while the one path that records a dispatch
was dead. So the board kept moving and only the dispatch receipt vanished.
Downstream:
lane_throughput_windowcountsdispatch_logentries withstatus == "dispatched",found none, and
logs/throughput-governor.jsonlrecorded"dispatched": 0, "mode": "bootstrap", "cap": 25on every pass. The lane can never leave bootstrap, so the requested 100/day isstructurally unreachable (#1995).
Fix
Key the relay session id on exactly the identity fields the keeper compares but does not
normalize, so a drift opens a new session instead of conflicting. The digest is computed over
whatever is left unbound, so adding an identity field keys it automatically rather than re-opening
the freeze; and it derives from stable per-build values (never per-process), so the keeper's
session table does not grow without bound.
Verification
A real
limen dispatch --agent jules --limit 1 --liverun against the live keeper now getspast registration and reaches
submit(). It stops there on a different, legitimate 409 —task ORG-artist-organ-face-0705 exact revision moved— because the local projection has beenfrozen at
track.date: 2026-07-26ever since the board-publication PR rung was retired. That isthe second stacked defect and #2001 carries it.
3 regression tests added, incl. one that drives the real
ConductBrokerthrough the freeze andthen through the keyed id.
cli/tests/test_tabularius.py: 32 passed. Ruff clean.Non-deploy:
cli/**is not a deploy-trigger path (the api rail is dormant —GCP_SA_KEYexistsnowhere).
Refs #1995