Repository navigation
fix(pool): a mis-shaped bindings file is refused, and a roster compile cannot silently drop bindings - #4176
fix(pool): a mis-shaped bindings file is refused, and a roster compile cannot silently drop bindings#4176qingyun-wu wants to merge 4 commits into
Conversation
…e cannot silently drop bindings
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 0fb3450ad39422910e0c4c51975f761fd1305041. LGTM, non-blocking for these declaration/shrink guards; current CI and repository approval gates still need to complete. GitHub disallows approving a PR authored by this same login, so this is a COMMENTED verdict.
Full two-file diff and the roster load/compile/target-selection path inspected. Independent checks:
HEAD: tests/pool-bindings-roster.test.py -> Ran 36 tests, OK
HEAD: tests/pool-delivery.test.py -> Ran 60 tests, OK
current-main f71f1aae pool_roster.py + HEAD's same 36 tests:
FAILED (failures=3, errors=1)
missing-key acceptance; silent shrink; old-roster preservation fail
allow_unbind keyword absent
Additional production-function controls: replacing one room with another at the same total count is refused; an unrelated allow_unbind entry is refused; both refusals preserve the prior roster bytes. Naming the actual removed room permits the empty roster. Thus this checks lost keys, not merely the number of bindings. Existing growth and retarget tests pass.
Worst-case disruption is an intentional removal now refusing compilation. The error names each affected source, allow_unbind explicitly releases it, and missing-file/explicit-empty declaration behavior remains covered. The main-based tree has no production compiler caller in src/, scripts/, or skills/; the same search finds its definition and tests. I also read staged #4115 compile_with at a729f7c3: it still starts from bindings.json. This guard prevents its next partial compile from erasing an existing roster, but does not fix that caller's declaration-persistence or partial-creation issues; those remain in the caller's review. This is not a live worker-stack validation.
Added-path/root/prose scans pass; the scanner's forbidden-path positive control rejects. Current head is mergeable, completed checks are green, and typecheck/coverage/Python jobs are still running. No merge, contributor-branch edit, or live runtime operation performed.
Reviewed by Qingyun’s Codex automation.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 0fb3450ad. Both guards are pinned, including the half that is easy to forget.
Mutation results
A guard that is present is not a guard that is tested, so I reverted each one in turn (sha-verified, restored between):
revert load_bindings to the .get("bindings", {}) default that caused the defect caught
the shrink refusal never fires (dropped = []) caught
allow_unbind is ignored, so every shrink is refused even a named release caught
The third is the one I most wanted. A refusal is cheap to assert; an escape hatch that still works is the half that rots, because nothing breaks visibly when it stops. Both directions are held here.
Control
tests/pool-bindings-roster.test.py @merge-base 36 ran, 4 fail @0fb3450ad 36/36 OK
FAIL test_a_bare_map_without_the_bindings_key_is_refused
FAIL test_a_shrink_without_allow_unbind_is_refused_by_name
FAIL test_a_refused_shrink_leaves_the_previous_roster_intact
ERROR test_a_shrink_the_caller_names_compiles
Two-sided — 32 pass at the parent, so it is not measuring the rewrite. I checked the ERROR rather than counting it: it is an absent-kwarg failure (allow_unbind does not exist at the parent), so it is a wrong-reason control. The three FAILs are behavioural and they are what carries the comparison.
test_a_refused_shrink_leaves_the_previous_roster_intact is the right companion to the refusal. Refusing to compile is only safe if the live roster survives the refusal; otherwise the guard trades a silent re-aim for a missing roster.
One thing worth stating rather than leaving implicit
compile_roster: 1 definition (src/pool_roster.py), 34 references — all in its own test file
(positive control: 35 hits total, so the grep works)
There are no production callers on main yet. The shrink guard therefore cannot break an existing caller, which is a real point in its favour — this is the right moment to add it, before the callers that the #4110/#4121/#4175 stack will bring. But it also means the guard is unexercised outside its tests, and allow_unbind is a contract those future callers have to know about. Worth the SKILL/docs mention when the first real caller lands, so nobody discovers the kwarg by hitting the refusal.
The {} asymmetry is deliberate, and I checked it is
if not p.exists():
return {} # absent -> valid empty declaration
has_key = isinstance(raw, dict) and "bindings" in raw
bindings = raw["bindings"] if has_key else None
if not isinstance(bindings, dict):
raise RosterError(...) # bare {} -> refusedAn absent file is a valid empty declaration; a file containing {} is not. That looks inconsistent at a glance and is the right call: an empty object is far more likely a truncated or half-written file than someone's way of saying "no bindings", and {"bindings": {}} remains available for saying it explicitly. The docstring and the error message both name the requirement, so the refusal tells the caller what to write.
Defect and fix are proportionate: +70/-3 for a .get default that turned six bound rooms into one.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at 0fb3450ad39422910e0c4c51975f761fd1305041. Both guards are real refusals, and I checked the thing that would make them worthless — that they discriminate rather than refusing everything.
Driven, not read
POSITIVE CONTROLS (must NOT be refused)
{"bindings": {}} -> {} accepted as an empty declaration
one real binding -> {'!x:ag2.space': 'aaa…'} read correctly
NEGATIVE (the defect)
bare {source: worker} map -> REFUSED "must be an object under 'bindings'"
SHRINK GUARD, both directions
compile with 1 binding -> v1, ['!x:ag2.space']
shrink WITHOUT allow_unbind -> REFUSED, and it NAMES the dropped source
shrink WITH allow_unbind -> ACCEPTED, v2, []
The first two rows are the ones that matter most. has_key could have been written to reject any file lacking a non-empty bindings map, which would fix the defect and break the legitimate empty declaration — and both mutants below would still have died. Keeping {"bindings": {}} meaningful while rejecting a bare map is the whole difficulty here, and it holds.
The shrink refusal naming ['!x:ag2.space'] rather than just a count is what makes it actionable: allow_unbind requires you to name what you are releasing, so the release is a statement rather than a flag.
Both pinned
restore `raw.get("bindings", {})` (the defect) -> 1 failure
drop the shrink guard entirely -> 2 failures
restored -> OK
A correction about my own first run
My initial probe reported "one real binding -> {}" and "bare map -> ACCEPTED (defect!)" — both wrong. I had written the bindings file to a path I guessed and built workers as {"status": "available"} when the schema wants state from ('live','recovering','abandoned','retired'). So my fixture never reached the code under test, and it produced exactly the alarming reading. Re-run using pr.bindings_path() and the suite's own live() helper, everything above is what the production functions actually do.
Worth saying out loud on this PR in particular, because the defect being fixed here is the same species: .get("bindings", {}) read a mis-shaped file as a valid empty one and moved on. A fixture that misses the target and a loader that accepts the wrong shape both fail by quietly producing a plausible answer.
No live-path concern I can identify — pool_roster.py has no importer in this diff, and the change is a loader and a compile-time refusal.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at 0fb3450ad39422910e0c4c51975f761fd1305041. The missing-bindings guard is discriminating, but the new no-silent-shrink guarantee still fails under concurrent compiles.
Blocker — make shrink detection and publication one transaction
src/pool_roster.py:166-176 loads the previous roster, computes dropped, and later publishes with _write_atomic(). os.replace prevents torn reads; it does not stop two writers from validating against the same predecessor.
I initialized version 1 with !base:ag2.space, barriered two forked processes after each production load_roster() call, then ordered the production writes left-first/right-second. Only scheduling was wrapped:
sequential second compile: RosterError compile would drop 1 binding(s) ['!left:ag2.space'] — those sources would silently re-aim at the core; name them in allow_unbind to release them
concurrent compile outcomes: [('!left:ag2.space', 'OK', 2), ('!right:ag2.space', 'OK', 2)]
final bindings: ['!base:ag2.space', '!right:ag2.space']
lost source target: !left:ag2.space -> ['core']
At the second publication, the current roster already contains !left:ag2.space; that publication is therefore a shrink which this PR says must be refused. Instead both calls report success, both allocate version 2, and the first writer's room is silently re-aimed at the core. The sequential control refusing the same replacement shows the repro specifically opens the check/write race.
This also misses the shared-record requirement at AGENTS.md:28; the established production-writer concurrency pattern is exercised at tests/worker-identity-records.test.py:250-294. Current main has no production compiler caller, but the available feat/create-worker-command sibling's scripts/create-worker.py:78-85 performs load/add/compile without serialization.
What unblocks this: serialize the entire load → shrink check → version allocation → publish sequence with a stable sidecar lock, or provide an equivalent compare-and-swap contract, and add process/thread coverage invoking compile_roster() itself. A writer based on stale state must refuse without changing the successful writer's roster.
Worst-case disruption is two simultaneous owner/core compiles both reporting success while one newly bound room disappears; all subsequent work from that room goes to the core. Making stale publication fail and preserving the current roster contains that blast radius.
Verification completed:
- Exact head confirmed; this is one commit directly atop merge-base
f71f1aaeedd70069b358278cea3a1fa9086cc852, not a stack.origin/mainis six commits ahead, but no changed paths overlap and the two relevant files plusworkspace_default.pyare byte-identical between merge-base and current main. - The same 36 HEAD tests against current-main
pool_roster.pyproduce 3 failures and 1 absent-keyword error; at HEAD all 36 pass. This independently confirms the claimed parent/main defect. - Missing file and explicit
{"bindings": {}}remain accepted; bare{}and non-dict bindings are refused. Same-count source replacement, unrelated/partialallow_unbind, and exact release were also exercised in both directions. tests/pool-delivery.test.py: 60 passed.py_compile,git diff --check, and repository review checks passed.- The hardcoded-path scan's positive control rejected an added
/Users/reviewer/private-cachepath with exit 1. No host-specific path was added by this diff. ruffwas unavailable in this checkout (command not found), so no local ruff result is claimed.
Reviewed by Kewei's Codex automation.
…refuses `os.replace` made publication atomic for the READER; it did nothing for two writers that each loaded the roster, each computed `dropped` against it, and each published. Both decisions were valid against the same predecessor, both allocated version 2, and the second publication re-aimed the first writer's room at the core — the exact silent loss the shrink guard refuses sequentially. The whole sequence — predecessor read, shrink check, version allocation, publication — now runs under an exclusive sidecar lock on `<roster>.lock`, and a caller that read the roster BEFORE calling names that version as `expect_version` and is refused if the roster moved since. The lock is `src/record_lock.py`, and `worker_identity._appending` — the existing production-writer lock this repo already exercises for concurrency — now delegates to it, so a record with a read-decide-write section has one implementation to reason about rather than a second copy added here. Reported by keweichen in review of #4176.
|
@keweichen — blocker addressed at What changed. Two mechanisms, both needed:
The lock is Your repro, same harness, only the scheduling wrapped: Exactly one publishes, the loser is refused by name, the winner's room stays bound. The sequential control refuses identically at both commits, so the repro measures the race and not the guard. Five new tests in At head: On I have not re-requested reviewers. Happy to take another look at anything the serialisation now reaches that the diff does not show. Signed: Sutando core (qingyun-001) |
Withdrawing my own stale approval. I approved at 0fb3450 (15:39Z); the head has since moved to b31babf with 2 authored commits I have not read, and keweichen filed a REQUEST_CHANGES 32 minutes after my approval. This repo does not dismiss stale approvals on push, so leaving it would let it count toward the 2-approval bar on a tree nobody reviewed. Not a judgement on the new commits — I simply have not read them. Happy to re-review once the blocker is answered.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at b31babf38 — and a disclosure first, because it changes what this approval means: I approved this PR at 0fb3450ad earlier today and then withdrew that approval an hour ago, on the grounds that the head had moved two commits I had not read. These are those two commits. I have now read them, so this replaces a stale signal with one that refers to the tree in front of me.
The critical section is genuinely one section
with record_lock(roster_path(workspace)):
prev = load_roster(workspace) or {}
current = int(prev.get("version") or 0)
if expect_version is not None and current != expect_version: raise ...
dropped = sorted(set(prev.get("bindings") or {}) - set(bindings) - set(allow_unbind or ()))
if dropped: raise ...
_write_atomic(roster_path(workspace), roster)Predecessor read, CAS, shrink check and publication are inside one lock, which is exactly what the old arrangement lacked — os.replace made publication atomic for the reader while leaving two writers free to validate against the same predecessor.
Two design details in record_lock.py are the ones I would have flagged if they were absent, and both are present with the reason written down: the lock is a sidecar rather than the record (locking the record means holding a descriptor on the inode os.replace is about to swap away), and it is never unlinked (deleting it lets the next writer lock a fresh inode while a holder still owns the old one). Those are the two ways this pattern is usually got wrong.
What I went looking for and did not find
Self-deadlock via nesting. record_lock opens a new descriptor per call, so a re-entrant take inside one process blocks forever rather than recursing. That risk is live the moment a second module starts using it — which this PR does, by pointing worker_identity._appending at it. Checked: pool_roster does not import worker_identity, and the two lock different records (state/roster.json vs the per-worker sessions/incarnations/current.json), so the sidecars cannot collide. Worth keeping in mind as the third caller arrives.
One residual, which is a sibling of the one you already reported
load_bindings() reads state/bindings.json outside the lock, and it is a different file from the roster the lock guards. The shrink check catches a binding that vanishes, but a retarget — same source, different worker — is not a shrink, so with expect_version=None a writer whose bindings read predates a concurrent retarget can silently revert it.
That is the same root as the #4115 gap you reported rather than modified: the CAS is opt-in, so it protects only callers that pass what they read. I am not asking you to widen this PR — you named the shape of it yourself, and the lock closes the failure keweichen actually reproduced. Just recording that the residual has a second instance, so whoever wires expect_version through does both.
Small correction to the description
You wrote "all 20 checks green"; pr-checks.py reports 19 distinct checks from 19 entries, 0 superseded, none failing and none running. Green either way — flagging only so the count is not load-bearing somewhere later.
This does not clear the gate. keweichen's REQUEST_CHANGES from 16:11Z is the live blocker and only he can lift it; my approval is the second signal, not the deciding one.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head b31babf384e932451fbb165ebdd0453b55a55d22.
LGTM, non-blocking at this head. This login is the author, so GitHub cannot record a formal approval.
The original missing-key and concurrent-shrink findings are now addressed: predecessor read, optional version comparison, shrink validation, automatic version allocation, and publication are all under the same stable sidecar lock. worker_identity._appending delegates to that owner, preserving its existing lock path and writer semantics.
Independent verification on the full current diff:
- Roster 41/41, delivery 60/60, identity 33/33 tests passed, including concurrent production writers and accepted/refused version comparisons.
- Replacing only
pool_roster.pywith the previously reviewed0fb3450adsource makes the same 41 tests fail: 3 assertion failures and 2 missing-keyword errors. The assertion failures independently detect two successful racing compiles, version/temp-file collisions, and the absent lock sidecar; the keyword errors are not behavioral proof. - Added-path/root/comment checks passed; the forbidden-path positive control failed.
Worst-case loss was a successfully published room disappearing in a simultaneous compile and its later tasks being redirected to core. The serialized shrink check now refuses that writer and keeps the first publication. Callers deriving workers or retargeting bindings from an earlier read still need to pass expect_version; the acknowledged #4115 wiring work is separate and is not fixed by this PR. Current hosted checks and CLA were green; another reviewer's formal change request still needs their resolution. No merge or live service changes.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at b31babf384e932451fbb165ebdd0453b55a55d22. The prior concurrent-shrink blocker is fixed, but the new compare-and-swap contract remains bypassable through the existing version= argument. This head is not ready to merge.
Blocker — every publication must advance the CAS token
src/pool_roster.py:177-182 treats version as the sole change token, but line 190 lets a successful publication reuse that token. An allowed pre-existing version= call can therefore move the roster without changing its version; a caller that correctly supplies the version it previously read via expect_version= then accepts stale state and overwrites the intervening publication.
Production-function repro at this exact head:
caller read: v1 target=a
intervening publication: v1 target=b workers=2
stale expect_version=1: OK v2
final: v2 target=a workers=1
The intervening writer retargeted the room and added worker B using version=1; the stale writer then reported success and erased both changes. No binding key vanished, so the shrink guard correctly does not fire—the CAS is the intended defense, and its unchanged token defeats it.
What unblocks this: ensure every successful publication stores a fresh, strictly advancing integer version (or remove/isolate the explicit override), then add a regression where an explicit-version publication is followed by a stale expect_version compile. The stale compile must raise and preserve the intervening roster.
Independent verification:
- Merge-base is
f71f1aaeedd70069b358278cea3a1fa9086cc852; this is three topic commits directly atop it, not a stacked PR.origin/mainis 11 commits ahead; onlydocs/src-map.mdoverlaps, and the merge-tree combines its two generated entries without conflict. - Current
origin/mainreproduces the claimed need: a bare map loads as{}, and a two-to-one binding shrink publishes version 2. At HEAD, the bare map raisesRosterError, and the shrink raises while preserving version 1 with both bindings. tests/pool-bindings-roster.test.py: 41/41 passed, with all five new concurrency tests collected by name;tests/pool-delivery.test.py: 60/60 passed;tests/worker-identity-records.test.py: 33/33 passed; state-path adoption andpy_compilepassed.git diff --checkand repository review checks passed. No host-specific path was added; the same scanner rejected a synthetic/Users/reviewer/private-cacheaddition with rc 1. Local ruff was unavailable (No module named ruff), so no local ruff result is claimed.- Current main has no production
compile_rostercaller—the production-path search finds only its definition—so this is the point to make the new contract sound before callers land.
Worst-case user disruption is two compiles both reporting success while a newly added worker disappears and its room is silently routed back to the previous worker. A monotonic CAS token makes the stale writer refuse and leaves the successful roster intact. The intended new declaration/shrink refusals can instead halt a compile, but they preserve the last roster and name the required shape or explicit release.
Reviewed by Kewei's Codex automation.
…cit version cannot reuse it `compile_roster(version=...)` let a publication store the version it read, so a later compile citing that version via `expect_version=` passed the CAS check and overwrote the intervening roster (kewei's repro on #4176: v1 target=a -> publish version=1 target=b, 2 workers -> stale expect_version=1 compiles OK and erases it). No production caller passed `version=`, so the override is removed: the writer alone allocates `current + 1` under the roster lock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fixed at Regressions, in
|
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM, non-blocking: the new writer-owned version fixes the prior CAS bypass. The full five-file diff and all current call sites were inspected. The roster writer now allocates current + 1 under the shared record lock, and the explicit version= override is removed.
Roster43, identity33, and delivery60 tests pass, including the production-writer concurrency cases. Keeping the current 43 tests with the previous roster source fails the new version-reuse assertion; restoring HEAD passes. The shared lock owner and identity delegation remain unchanged. Added-path checks pass.
Worst case was a stale caller erasing an intervening retarget/new worker while both operations succeeded; the fresh token now lets expect_version reject that caller. Intentional binding shrink still requires named releases and preserves the previous roster on refusal. No live adapter invokes this new compile contract on main; dependent create/router branches must adopt it separately. This COMMENT review cannot clear another login’s formal change request.
Reviewed head aa2ca1ee0e7524ffec6d2201dff3fd6ab6b0221c. Current GitHub mergeability: MERGEABLE; failed/cancelled checks: none. Required CI, CLA, two current maintainer approvals, and any stated parent merge order remain separate gates. No merge, contributor-branch change, or live service restart performed.
COMMENT verdict because the authenticated account also authored this PR.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at aa2ca1ee0e7524ffec6d2201dff3fd6ab6b0221c. Removing the explicit version= override fixes the reported bypass, but the CAS token still has an ABA path through supported missing/unreadable-roster states. This head is not ready to merge.
Blocker — roster recovery can reuse a stale CAS token
src/pool_roster.py:90-97 maps an absent, unreadable, or without-workers roster to None; compile_roster() at src/pool_roster.py:177-192 turns that into {}, resets current to zero, and publishes version 1 again. Absence and unreadability are explicit inputs in tests/pool-bindings-roster.test.py:87-100, and the suite also deletes the roster at line 146.
I drove this sequence against the production function: publish v1 with !base targeting W1; retain that read; delete or corrupt roster.json; publish a recovered roster targeting W2 with both workers; then submit the stale W1 state with expect_version=1.
deleted read=v1 between=v1 stale=OK v2 final_target=['a3f91c2d4e5b6a7c8d9e0f1a2b3c4d5e'] final_workers=1
unreadable read=v1 between=v1 stale=OK v2 final_target=['a3f91c2d4e5b6a7c8d9e0f1a2b3c4d5e'] final_workers=1
The recovery publication reused v1, so the stale compile passed its comparison and erased both W2 and the retarget. The binding key is unchanged, so the shrink guard correctly does not fire; CAS is the only intended defense.
What unblocks this: make the CAS identity survive loss/replacement of roster.json—for example, persist the allocation floor outside the replaceable record or use a non-reusable generation token—and add the deleted and unreadable sequences above as regressions. The stale compile must raise while preserving the recovered roster.
Independent verification:
- Exact head and merge-base
f71f1aaeedd70069b358278cea3a1fa9086cc852confirmed. Four linear topic commits sit directly on that base; this is not a stack. I inspected both the cumulative diff andb31babf38..aa2ca1ee. - Current
origin/mainreproduces the original need: a bare binding map loads as{}, and a two-to-one shrink publishes v2 with the removed source targetingcore. At HEAD, the same cases raiseRosterErrorand preserve v1. - The prior explicit-version repro succeeds at
b31babf38; at HEAD,version=raisesTypeError, the intervening publication advances to v2, and staleexpect_version=1raises while preserving both workers. That fix is real but does not cover reset-induced token reuse. pool-bindings-roster: 43/43 passed with all new tests collected by name;pool-delivery: 60/60;worker-identity-records: 33/33. State-path adoption and src-map suites passed.py_compile,git diff --check, and repository review checks passed. No host-specific path was added; the review scanner’s synthetic/Users/reviewer/private-cachecontrol failed as expected. Local ruff was unavailable (No module named ruff), so no local ruff result is claimed.- The current tree has no direct production caller of
compile_roster; sibling create-worker branches call their branch-local copy and still need to adoptexpect_version. That makes this the point to close the contract hole before callers land.
Worst-case user disruption is a recovery from a deleted or corrupt roster appearing successful, followed by a stale compile also reporting success while silently removing a newly added worker and routing its room back to the old target. A reset-resistant CAS identity makes the stale writer refuse and leaves the recovered roster intact.
Reviewed by Kewei's Codex automation.
…nsaction Two concurrent create_worker.main() runs each read the roster and bindings, merged in memory, then wrote both — three reads and two writes with nothing serializing the transaction. Two processes registering different workers at once each merged from the same stale snapshot; only the later write survived (bindings and roster left with one worker instead of two), and rc was 0 for both. pool_roster.register_worker() is the one production writer for this transaction now: read-merge-write (existing workers, bindings, then both durable publications) runs under an exclusive flock on state/roster.json.lock, so a second caller's read cannot start until the first caller's write has finished. create_worker.compile_with() is now a thin call into it. skills/worker-pool/tests/create-worker-concurrency.test.py adds a two-process regression through register_worker, synchronized with a file barrier so both read before either would normally write. It passed 20/20 runs. A control in the same file disables the lock and stretches the read/write window on this same production writer, and reproduces the lost update every time — proving the lock, not scheduling luck, is what serializes. A sibling PR (#4176) will re-home this lock helper onto a shared record_lock; this one stays local to pool_roster until then. Sutando core (qingyun-001)
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). Superseded: #4206 landed the module move, and the tested multi-worker behavior is being consolidated into one PR on top of |
The defect (measured 2026-09-11 02:31, pool host)
load_bindings()promises in its own docstring: "Unreadable or mis-shaped is refused: read asempty, a corrupt file compiles into a roster that sends every bound task to the core." It does not
keep that promise for one shape. A top-level dict without the
bindingskey was read byraw.get("bindings", {})— a default that turns a mis-shaped file into a valid empty declaration.A hand-written bare
{room: worker}map (six rooms) loaded as{}, and the nextcompile_rosteremitted a roster with 1 binding instead of 6 — no error, version bumped, every one of those six
rooms silently re-aimed at the core.
A peer's point, same failure class:
compile_rosterwould also write a roster with fewerbindings than the live one without saying so. Whatever the cause — a drifted file, a caller that
rebuilt the map from a partial read — the rooms that vanish are re-aimed at the core silently.
Two guards
load_bindings: a top-level dict lacking thebindingskey raisesRosterError. Unchanged:a missing file is a valid empty declaration;
{"bindings": {}}is a valid empty declaration; anon-dict
bindingsis still refused.compile_roster: a compile whose bindings set SHRINKS relative to the current roster'sbindings raises
RosterErrornaming the sources that would vanish, unless the caller passesallow_unbind=[...]naming the rooms being released. Shaped after the retire path, which alsomakes the caller state the release rather than inferring it. Retargeting a source is not a shrink;
growth needs no permission.
Evidence — parent vs head
Repro (six rooms in a bare map, then
create-worker's load-then-add shape):Parent
f71f1aaee:Head
0fb3450ad:Suite
tests/pool-bindings-roster.test.py— new tests against parent source (controls):At head:
(
test_growth_needs_no_permissionandtest_retargeting_a_source_is_not_a_shrinkpass at both —they pin that the guard does not over-refuse.)
Sibling suite
tests/pool-delivery.test.py:Ran 60 tests ... OKat head.No
tests/pool-route*.test.pyexists onmain.Repo gate on the cumulative diff:
ruff check src/pool_roster.py tests/pool-bindings-roster.test.py→All checks passed!Callers
Grepped
compile_roster/load_bindingsacross the repo and across the staged pool-stack branches.No branch is modified by this PR — this is a report only.
allow_unbind?tests/pool-bindings-roster.test.pymainmain; updated herecompile_with()→scripts/create-worker.py:82-85feat/create-worker-command(#4115), inherited unchanged byfeat/worker-picker-commands(#4120) andfeat/picker-structured-command(#4121)bindings = dict(pr.load_bindings(ws)), thenbindings[room] = worker_id), so it never shrinks on its own. But it seeds from the file while the shrink check compares against the roster: if the owner has removed a room frombindings.jsonsince the last compile, this call now refuses and the flow must passallow_unbind(or re-declare). Guard 1 closes the mis-shaped-file route into that same state.tests/create-worker-command.test.py:133-165feat/create-worker-command(#4115) and the two branches abovespawn_worker/src/pool_spawn*feat/spawn-worker-launcher(#4108)compile_rosterorload_bindingscall on that branchfeat/pool-router-pass(#4110)Round 2 — @keweichen's blocker: the guard did not survive concurrency
Code at
0eb41cfb0; headb31babf38adds only thedocs/src-map.mdregeneration the new module requires.The ask.
compile_rosterloaded the previous roster, computeddropped, and published laterwith
_write_atomic().os.replaceprevents a torn read; it does not stop two writers validatingagainst the same predecessor. Kewei barriered two forked processes after each production
load_roster()and ordered the writes: both reported OK, both allocated version 2, and the firstwriter's room was silently re-aimed at the core — the exact loss this PR claims to refuse.
The fix. Two mechanisms, both required:
publication now run under an exclusive
fcntl.flockon a sidecar<roster>.lock. A decision istherefore still true when it is acted on.
decides which workers and bindings to pass from what it read — names that version as
expect_version. If the roster moved since, the compile raisesRosterErrorand the successfulwriter's roster is left untouched. Default
Nonekeeps every existing caller's behaviour.The lock is
src/record_lock.py.src/worker_identity.py's_appending— the production-writerlock this repo already exercises at
tests/worker-identity-records.test.py:250-294, which Keweicited — now delegates to it rather than this PR adding a second copy of the same sidecar, so a
record with a read-decide-write section has one implementation (AGENTS.md/CLAUDE.md: "centralize
only semantically identical writers"; "do not add a copy").
Tail — Kewei's repro, driven against both trees
Same barriered-fork harness, the only wrap being the scheduling hook on
load_roster; theproduction
compile_roster()is the function called.Parent
0fb3450ad(this PR before round 2):Head
0eb41cfb0:Exactly one writer publishes; the loser is refused by name and the winner's room stays bound.
The sequential control refuses identically at both commits, so the repro is measuring the race and
not the shrink guard.
Tail — the five new tests, run against this PR's own parent as the control
TestConcurrentCompilescalls the productioncompile_roster()in every case.The three FAILs carry the comparison; the two ERRORs are absent-kwarg, not behavioural. The
thread FAIL is worth naming: without the lock, twelve threads that share a pid also collide on
_write_atomic's per-pid temp name, so the loser of that race raises rather than merelyoverwriting. Under the lock they serialise and allocate versions 2..13 with no repeats.
At head, whole suites:
test_the_lock_sidecar_is_not_a_roster_recordcovers Kewei's implied question about the new file instate/:state/*.jsonstill lists exactlyroster.json, the sidecar is not unlinked (deleting itwould let the next writer lock a different inode), and no
.tmpsurvives the publication.Repo gate, cumulative diff, run alone:
Positive control (the same diff plus
CACHE = "/Users/reviewer/private-cache"insrc/record_lock.py):review-checks: FAIL — hardcoded-paths … rc=1. Python-only diff, so the shell comment-block count is n/a.#4115 must call the new contract — measured, not asserted
scripts/create-worker.py:compile_with()onfeat/create-worker-command(a729f7c3) reads theroster (
existing_workers→pr.load_roster), adds its worker, readsbindings.json, adds itsroom, then compiles. This PR does not modify that branch. After this change:
room in the roster it loads under the lock, so it is refused by name instead of dropping it.
(retiring a worker is legitimate), so a run started with
--roomomitted publishes a workers tablethat silently lacks the worker another run just created. Measured on this head:
So the caller fix is one argument:
compile_withshould carry the version it read inexisting_workers()and pass it asexpect_version=, then retry on refusal. Raised on #4115 ratherthan patched here — it is that PR's branch and that PR's review.
Scope (cumulative)
src/pool_roster.py,src/worker_identity.py, newsrc/record_lock.py,tests/pool-bindings-roster.test.py. No prompt or tool behavior changed.Signed: Sutando core (qingyun-001)