feat(worker): lease one Pi account per placement so concurrent crewmates never collide - #280
Merged
Merged
Conversation
added 5 commits
August 20, 2026 21:25
…tes never collide R5's crewmate placement was single-profile: every cloud spawn recorded the pooled Pi coding-agent home as its account home and staged that whole auth.json onto the worker, so N concurrent crewmates all resolved the pool's first slot. The queue's own duplicate-account screen could not catch it either, because account_binding was a digest of (task, account_home) and was therefore unique per task by construction - the documented "no two active tasks share an account lease" invariant was never actually enforced on this lane. The controller now selects a free profile inside the same lock hold and the same durable write that creates the queue entry, so selection and the lease are one act and the queue entry IS the lease. The unit of exclusion is the upstream account rather than the profile name or the account-home path, since two profiles can be re-logged into one account and what a crewmate contends for belongs to the account. bin/fm-pi-account-home.py projects the chosen profile into a controller-owned account home, and fm-spawn.sh narrows the staged credential to it, so the worker receives exactly one account. Every failure refuses by name and none falls through to a shared or arbitrary profile: unreadable pool, no projectable profile, a home naming no upstream account, and exhaustion (which lists each leased profile and its holder). Mechanics: docs/azure-workers.md "Provider-account placement across the Pi fleet".
… bind it A spawn that gets past the request but cannot bind the account it was handed was leaving the queue entry in place, and that entry IS the lease: the pool would lose one account every time it happened. The entry is still queued at that point, so the spawn now withdraws it and says so, and names the manual withdraw when even that fails. Also stops the refused-request unit from trusting the ambient environment: it now unsets the Azure identity variables explicitly and pins the provider to its case fixture. An operator shell exports the real subscription, tenant, resource group and image, and inherited there that unit's request would be admitted and its reconcile would reach the real Azure adapter.
…unt Pi pool Two fixtures in the monitor suite predate account-exclusive placement. make_lifecycle_task created an EMPTY account directory; placement refuses a directory it cannot identify an upstream account in, so that is no longer a usable account authority. The end-to-end lane's pi-agent-home was a single thin profile, and that lane places a compartment AND a child of that compartment. Against a one-account pool the child now correctly refuses as exhausted, which is the invariant working rather than a regression: the old fixture could only ever have passed while the invariant was unenforced, so it was pinning the bug. Both fixtures were already unrealistic against the real lane, whose account home is the multi-profile Pi coding-agent home. Also states the crash-safety unit's floor in the unit itself: at least one kill landed and at least one placement survived, deliberately the weakest non-vacuous floor rather than a rate, so the pass condition does not depend on machine load.
…e is not Records that the controller is the account lease owner and derives the binding from the profile's upstream account identity rather than its local slot name, and that the slot name appearing in bounded status is not a violation of the raw-identity rule: it is a local pool label, and it never reaches an Azure tag (bindings_for_item, the sole source expected_tags reads, carries only the digest).
Concurrent placements are bounded by min(FM_AZURE_WORKER_MAX, distinct upstream accounts). With eight Pi accounts the ninth placement refuses although MAX_WORKERS is 16, and compartments compete in the same pool: one compartment plus its four children takes five of eight. That is the requirement rather than a regression, since sixteen crewmates never could run on eight accounts without sharing one, but it halves effective author parallelism and an operator should not have to discover it from a refusal.
…tage the pool Two review blockers, both reproduced against the previous head with no mutation. The projected home was keyed on the profile's local SLOT NAME while the lease was keyed on the upstream account, so the two were different functions of the pool and the projection was not injective over live leases. Re-logging one slot into another account gives two placements two correct, distinct bindings that both projected into accounts/<slot-name>, and the second write replaced the credential the first placement's still-live lease pointed at: the queue reported two accounts while the disk held one. The home is now accounts/<account-binding>, so the directory name and the exclusion key are the same string and that state is unrepresentable. A second, defensive refusal declines to project over a home a live queue entry still names. Because the path no longer ends in the slot name, the controller reports account-profile explicitly and the spawn reads that rather than parsing the path. The pooled auth.json also still reached the window between the durable lease and the narrowing, with the tracking monitor already polling and willing to dispatch that directory as --account-dir. It is no longer staged at all: the account directory is written exactly once, by the narrowing step, after the lease exists. settings.json still stages with the payload because it is configuration, not credential material. As defence in depth at the point of use, the cloud monitor re-checks the one-slot shape before dispatching, and does so BEFORE taking the shared O_EXCL dispatch marker, because standing down after taking it would leave both owners believing the other dispatched and nothing ever would. Also: both exhaustion refusals now name the remedy; the compartment monitor fixture's account ids are scoped per task so a future third concurrent task cannot hit phantom exhaustion; and the new window unit writes the killed spawn's output to a file instead of capturing it through a command substitution, which blocked on a pipe held by a surviving descendant and made the unit pass once and then hang.
This was referenced Aug 21, 2026
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
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.
R5's "Work" line: multi-profile account selection for crewmate and worker placement, reusing the projection tool and the account-lease identity already in the worker request path.
Two defects this closes, both security-relevant
Neither is a refactor. Both were live on the cloud placement lane.
1. Every crewmate carried the entire account pool.
bin/fm-spawn.shstaged the WHOLE POOLEDauth.jsonfrom${PI_CODING_AGENT_DIR:-~/.pi/agent}onto each worker's account disk. That is a credential-blast-radius problem quite apart from collisions: one compromised, misbehaving, or merely buggy crewmate held live OAuth credentials for all eight upstream accounts, not the one it was placed on. It also guaranteed the collision, because every worker then resolved the pool's first slot no matter what the controller thought it had assigned.Review round 2 found the first version of this fix incomplete: narrowing AFTER the request left a window in which the pool was staged, the lease was already durable, and the tracking monitor was already polling and willing to dispatch that directory. The pool is now not staged at all (below).
2. The duplicate-account guard was decorative and could never fire.
account_bindingwasdigest({task, account_home})- unique per task BY CONSTRUCTION - soensure_unique_bindings's account clause was structurally incapable of raising on this lane, for any input.docs/azure-workers.mdhas stated "No two active tasks share a VM, account lease, browser profile, or writable task disk" the entire time. The VM, browser-profile and task-disk halves were enforced. The account lease half was not enforced at all, and no test could have caught it, because the guard's own precondition was unreachable.The four design questions
What is the unit of exclusion, and why? The upstream account, identified by
bin/fm-pi-account-home.py'saccount_digest(a truncated SHA-256 of the account id, never token material). Not the profile name and not the account-home path: both are handles, and a re-login can point two slots at one account while a projection can put one account behind two paths. What a concurrent crewmate actually contends for - the rate limit, the ban, the session - belongs to the account. Two profiles resolving to one account are therefore ONE lease, proved by a unit that rewrites both fixture profiles to the sameaccountIdand shows the second placement refuse.How is the lease acquired and released, and what happens on a crash between them? There is no separate lease record. Selection happens inside
command_request, in the samecontroller_lockhold and the samesave_statethat writes the queue entry, so the queue entry is the lease. The held set is derived from the non-complete queue, so there is no window in which an account is held by something the queue does not show, and no second document a crash could leave holding a profile nothing owns. Release is the existing machinery, unchanged: release proof tocomplete,withdraw, orsurrender. A crash before the commit leaves nothing; after it, an ordinary queued entry thatstatuslists andwithdrawclears. The projection written just before the commit can survive with no entry - that directory is inert, not a lease, and the next placement may take it.One new handback path (second commit): a spawn that gets past the request but cannot bind the account it was handed now withdraws its own queued entry rather than leaving the lease held by work that will never run, and names the manual withdraw if even that fails. Without it the pool lost one account every time that happened.
What happens when all profiles are leased? It refuses, by name, listing every leased profile and the task holding it, and creates no queue entry. There is no fallback to a shared or arbitrary profile. Same for an unreadable pool, an empty pool, a pool with no projectable profile, and a single-profile home naming no upstream account.
Deterministic or arbitrary, and does replay reuse? Deterministic: the first free profile in the pool's sorted (lexicographic) name order. Replaying the same task generation reuses the same profile, because the replay path short-circuits on the existing entry before selecting anything. A different generation is a different placement and takes the next free account.
Round-2 review: two blockers, both fixed
Both were reproduced against the previous head with no mutation, so neither was theoretical.
Blocker 1: the projection key and the lease key were two different functions of the pool. The lease keyed on the upstream account (correct), but the projected home keyed on the profile's local SLOT NAME. Those disagree exactly where my own design rationale said they could: an operator re-logging slot
openai-codexfrom one account to another produces two placements with two correct, distinct bindings that both project intoaccounts/openai-codex, so the secondwrite_homereplaces the credential the first placement's still-live lease points at.controller.jsonthen reports two accounts while the disk holds one. I reasoned about re-login for the exclusion unit and not for the projection key.Fixed at the single point that caused it: the home is now
accounts/<account-binding>, so the directory name and the exclusion key are literally the same string and the divergence is unrepresentable. A second, defensive refusal declines to project over an account home a live queue entry still names; it is dead while the keys agree and load-bearing if anyone re-keys later. A consequence the review did not name:spawn_cloud_bind_leased_accountderived the profile name from the path's last component, which is now the binding, so the controller reportsaccount-profileexplicitly and the spawn reads that.On the review's aggravating factor (replay re-stages from
account_homeon every call, so a resumed spawn stages whatever is at that path NOW): I believe the same keying change resolves it rather than it needing a separate fix, and I am stating that as REASONING TO BE VERIFIED rather than as a proven claim, because no test in this PR exercises a replay across a pool mutation. The argument is thatwrite_homeonly ever writesaccounts/<binding>with the credential whose upstream account hashes to that binding, so by construction what sits at that path is what the lease names; a pool change moves the profile to a NEW directory and leaves the old one holding the account the old lease still points at. If a reviewer disagrees, the case to attack is a replay after the leased profile's account changed.Blocker 2: the pooled
auth.jsonstill reached the request-to-narrow window. Staging ran before the lease existed, while the monitor pane was already polling, andbin/fm-spawn-cloud-monitor.shguarded--account-dironly with[ -d ... ]. Killed in that window, all four fixture accounts were staged with the lease already durable.Fixed by removing the window rather than guarding it, which is the stronger of the two directions offered: the payload step no longer copies the pooled
auth.jsonat all, and the account directory is written exactly once, by the narrowing step, after the lease exists.settings.jsonstill stages there because it is configuration, not credential material. The monitor guard was added as well, not instead, because the monitor outlives the spawn and dispatches independently, so the shape belongs at the point of use. One correction to the suggested placement: the guard must sit BEFORE the O_EXCL dispatch claim. Behind it, a refusal leaves the marker taken and both owners stand down forever. Ahead of it, a not-yet-narrowed directory simply does not claim and the next poll retries.Both smaller items are done: the exhaustion refusals now name the remedy, and the monitor fixture's account ids are scoped per task so a future third concurrent task cannot hit phantom exhaustion.
Reuse, not reinvention
bin/fm-pi-account-home.pyis imported as a module and supplies all three things placement needs:read_pool,account_digest, andprepare_root/write_home. Nothing here re-derives an account identity or re-implements how an account home is written.account_binding, the field already in the request path, and the exclusion staysensure_unique_bindings, the screen already there. Only the identity's meaning changed, from "this task's lease of this home" to "this upstream account" - which is what makes the existing screen able to fire at all.authoritative_request_bindingsnow returnsaccount_pool_homeinstead of a per-taskaccount_binding: the task's metadata still proves which pool it may draw from, and the controller decides which profile of that pool, because that decision must exclude every other concurrent placement and no task-local document can see them.Compartment children
Covered by construction, not by a special case. A compartment child goes through the same
command_request, andFM_HOMEstill names the primary's controller for both it and an ordinary crewmate (#278/#279 moved the task home, never the money document), so both draw from the one document under the one lock. A unit places a compartment, its child, and an ordinary crewmate - the last two concurrently - and asserts three distinct accounts.Where the projected homes live
Under
$FM_HOME/state/azure-workers/accounts/<profile>(overridable withFM_PI_ACCOUNT_HOME_ROOT), not the shared~/.local/share/agent-fleet/accounts/piroster. Those homes belong to the crosscheck reviewer lane; a placement rewriting one mid-review would swap a running reviewer's credential underneath it. It also makes the root followFM_HOME, so a fixture home cannot write into a real one.Proof
Every leg ran with an explicit fixture provider, a fixture
FM_HOME, and the operator's ambientFM_*/AZURE*/ARM_*variables scrubbed. Each placement unit additionally shadowsazwith a recording stub that fails, and asserts it was never invoked.az vm listacross the subscription is empty: nothing was created.python3 tests/run-one.py tests/fm-worker-placement.test.sh- 8 units, all green:controller.json, with each leased home verified to hold exactly one slot whoseaccountIdmatches its binding. The assignment order is scrambled relative to task order on every run, which is the race actually happening.ELASTIC WORKER REFUSED: provider-account placement is exhausted: all 3 distinct upstream accounts in <pool>/auth.json are leased (openai-codex -> task-1, openai-codex-2 -> task-2, openai-codex-3 -> task-3); refusing to place task-4 on a shared upstream account, with the queue unmutated.0 < survivors < attempts: the floor is one - at least one kill must have landed inside the window and at least one placement must have survived it, so the unit cannot silently pass with zero kills landing. That is deliberately the weakest non-vacuous floor rather than a rate, so the pass condition does not depend on machine load; in practice roughly half land (10 of 24 on a recent local run). No account is held twice, every projected home is named by a live entry, and withdrawing the survivors returns the whole pool, proved by a fresh placement taking the first account again.statusreporting placements in both renderings.python3 tests/run-one.py tests/fm-spawn-cloud.test.sh- green. The assigned-lane unit asserts the end-to-end claim through the realbin/fm-spawn.sh: the stagedstate/<id>.cloud-account/auth.jsonholds exactly one slot, byte-equal to the leased profile's projected home; the meta recordsworker_account_profile/worker_account_home; those match the queue entry; and the pool it came from held more than one account, so a selection genuinely happened. Units also cover the lease handback and the request-to-narrow window, the latter killing the real spawn inside the window and asserting the lease is durable while nothing pooled is staged:tests/fm-worker-lifecycle.test.sh(36 ok, the documented hermetic baseline),fm-secondmate-cloud-monitor,fm-azure-pilot,fm-worker-authority-secondmate,fm-worker-outcome-transport,fm-worker-supervisor: all green.bin/fm-lint.shclean over all 247 scripts.ADMIT-red mutation
Mutated
placement_account_bindingto fold the task back into the lease identity - the exact pre-change shape, one line, at the single point where the lease identity is defined. It disables selection and the screen together, because both read that one identity. The concurrent unit went RED on an actual collision read back from durable state, not on a message:Reverted; the suite is green again.
Two test-fixture edits, and why they are not papering over failures
A reviewer seeing fixture edits inside a placement PR should look hard at them. Both are in
tests/fm-secondmate-cloud-monitor.test.sh:make_lifecycle_taskcreated an EMPTY account directory. Placement refuses a directory it cannot identify an upstream account in, so that is no longer a usable account authority. The fixture now seeds a real pool.pi-agent-homewas a single thin profile, and that lane places a compartment and a child of that compartment. With one account in the pool the child now correctly refuses as exhausted. That is the new invariant working, not a regression - and it means the old one-profile fixture could only ever have passed while the invariant was unenforced. It was pinning the bug. The pool now holds three accounts.Both fixtures were already unrealistic against the real lane, whose account home is the multi-profile Pi coding-agent home.
Hermeticity fix included
test_cloud_spawn_fails_closed_when_the_lifecycle_refuses_the_requestrelied on the ambient environment NOT carrying Azure identity, and its fixture id iscloud-noenv-c7. An operator shell exports the real subscription, tenant, resource group and image; inherited there, that unit's request is admitted and its reconcile reaches the real Azure adapter - a test that believes it is exercising a fake, creating a billable VM tagged with its own hardcoded fixture id. It now unsets the identity variables explicitly and pins the provider to its case fixture.Operational consequence: the pool is now a concurrency ceiling
Worth a reviewer's attention because it changes fleet behavior, not just correctness. Concurrent placements are bounded by
min(FM_AZURE_WORKER_MAX, distinct upstream accounts in the pool). With the fleet's eight Pi accounts, the ninth concurrent placement refuses even thoughMAX_WORKERSis 16 and quota, budget and capacity would all admit it. Compartments compete in the same pool: one compartment plus its four children takes five of the eight before an ordinary crewmate is placed.This is the requirement rather than a regression - sixteen crewmates never could run on eight accounts without sharing one, they just used to do it silently - but it halves effective author parallelism, and raising the ceiling means adding signed-in profiles on distinct upstream accounts, not turning a knob. Recorded in
docs/azure-workers.mdand in R5's status so an operator does not have to discover it from a refusal.What is NOT proven: the acceptance is NOT met
R5's acceptance sentence is "concurrent crewmates run on distinct pi profiles with no account collision". This PR proves placement up to, and including, the credential the worker is handed. It does not prove the run leg: no part of this touched real Azure compute, so what the guest agent does with that credential is unobserved here. Treat the acceptance as open. That leg needs a live worker under the owner's supervision, and R5's status text in
docs/azure-requirements.mdsays so rather than claiming DONE.Deploy note: in-flight cloud placements need a manual withdraw across this merge
Found in re-review, verified end to end, and NOT otherwise recorded here.
A queue entry minted by base
maincarries the old per-taskaccount_bindingand noaccount_home. Replaying that generation on this head is REFUSED:because
authoritative_request_bindingsnow returnsaccount_pool_homerather than a per-taskaccount_binding. The refusal is loud and fail-closed, the legacy binding blocks no real account(a fresh placement alongside it succeeds), and
withdrawclears it. But any cloud placement that isin flight at deploy time needs a manual
bin/fm-worker-lifecycle.sh withdrawbefore it can bereplayed.
Three non-blocking follow-ups from re-review, recorded so they are not lost
bin/fm-secondmate-cloud-monitor.shdid not receive the point-of-use guard. ForKIND=secondmatethe tracking pane is that script, launched before
spawn_cloud_dispatch, so it shares the samerequest-to-narrow window. It can no longer leak a pooled credential (nothing pooled is staged),
so the residual is availability only: leg 1 can dispatch against a directory holding just
settings.json, burning the exactly-once leg marker on a credential-less dispatch. The samerationale used for the crewmate monitor applies here.
select_placement_accountaborts rather than skipping to the next freebinding, so one corrupt entry blocks the lane even with other accounts free. Dead code while the
keys agree, and its message names no remedy.
account_profileslot name recorded at lease time,which may now name a different upstream account. The CREDENTIAL is correct; only the
worker_account_profilelabel is stale. Observability nit.Correction to a claim made earlier in this PR's discussion: placing the monitor guard behind the
O_EXCL claim is NOT a permanent wedge.
reclaim_stale_dispatchreleases a held marker with an emptyresult after
wall + 300(default 3900s), so it is a roughly 65 minute stall that self-heals. Theshipped ordering (guard before the claim) remains the correct one.