fix(worker): resolve a staged cloud credential in the home that staged it - #283
Merged
Conversation
ruby-dlee
force-pushed
the
fix/compartment-cloud-account-remover
branch
from
August 21, 2026 03:38
37ce177 to
d9b2220
Compare
…d it A compartment child's task home is the SECONDMATE's home, not the home that owns the elastic-worker controller document. bin/fm-spawn.sh stages the plaintext provider credential at <task home>/state/<id>.cloud-account/auth.json accordingly, but every remover re-derived the primary's state directory, so withdraw and surrender removed a path that never existed and the credential outlived the task in the compartment home with nothing left that would ever take it. That contradicts the docs/azure-workers.md invariant and the ownership bin/fm-cloud-state-lib.sh claims. Removal no longer resolves at its call sites at all. fm-cloud-state-lib.sh owns both the resolution and the removal, so a caller cannot disagree with the remover. The stager records the home it staged into at <controller state>/<id>.cloud-task-home, next to the one document every remover already reads, and fm_cloud_state_remove reads it before it removes anything. A record only redirects into an absolute, traversal-free, non-symlinked directory still carrying the .fm-secondmate-home marker the stager itself required, so no removal path is widened into a directory it does not own, and the record is removed with the state it points at. Lanes covered: withdraw, surrender, teardown (including the reaping loop that ends a cloud-placed child of a secondmate home, which had no removal at all), and the spawn's own rollback when the controller refuses a request after staging. The credential is never relocated to the primary's state: the task-home split stays exactly as it landed. The ordinary crewmate lane has task home equal to FM_HOME, writes no record, and produces a byte-identical set of artifacts, metadata keys and spawn output against the base commit.
…task id Review found the first approach both destructive and incomplete. It recorded the staging home in a file keyed by task id, and task ids are HOME-SCOPED: the same id can be live in a compartment home and in the primary at once. A record left by an earlier compartment child then redirected an unrelated primary task's teardown into the compartment, deleting a LIVE child's credential, payload and returned result while leaving the primary's own credential behind, which is the leak this branch exists to close. The record was reachable because the re-spawn sweep, which is the other place that enumerates per-task cloud state, did not know the file existed. Both halves are addressed at the root. Removers no longer infer a home at all. Each is handed the state directory of the home whose task it is: the spawn's rollback and re-spawn sweep pass the directory they just staged into, teardown passes its own task's home and each secondmate home's own state directory for that home's children, and withdraw and surrender, which must run with FM_HOME on the primary, are told by the controller. The authorized task home is already durable on the queue item and the worker record, so both commands echo it back on an FM-TASK-HOME line beside their receipt. The value the remover uses is the value the spawn passed to --task-home and the controller authorized under its own lock, and no id-keyed redirect exists to be stale. The per-task cloud file set is now enumerated once, in fm-cloud-state-lib.sh. The library, the re-spawn sweep and the spawn rollback used to spell it out separately, which is what let a file be created by one lane and removed by none. The sweep and the rollback share one entry point that removes the task-end set plus the leased-worktree pointer they own, and neither touches the outcome directory. The shipped documentation claimed reader-side symlink and marker-content validation the reader did not perform. There is no reader any more, and the paragraph now states only what the code does.
…e worker The surrender lane emitted no FM-TASK-HOME line at all for a real compartment child, so its removal fell back to the controller's own state directory and left the credential in the compartment home: the exact leak this branch exists to close, reintroduced on one lane by the hardening that closed the reader. parent_task and task_home both land on the QUEUE ITEM at admission; only task_home is copied onto the worker record. The hardened receipt carries the parent so the wrapper can hold the home to the same marker-content check the spawn held FM_SPAWN_TASK_HOME to, and sourcing that from the worker record meant the parent was always absent and the line was never printed. The receipt now reads the queue item, and stays silent when a worker record carries a task_home that disagrees with its own item: the fallback leaves a credential to be found, while following the wrong half of a diverged admission would remove state in a home the task does not live in. The surrender fixture used the convenient shape rather than the real one. It now writes parent_task and task_home where admission writes them, which is what made this reachable in the first place.
…t names Five findings from the second adversarial review. The safety net could not catch the failure it exists to catch. It re-used the directory the removal had just resolved, so when resolution fell back wrongly, which is the entire defect this branch closes, it inspected the controller's own home, found nothing because nothing was ever staged there, and the command exited 0 announcing success. That is why the leak was silent in production rather than loud. The audit's subjects are now fixed independently of the resolution: the home the controller NAMED for this task, read raw and only stat'ed, plus the controller's own state directory. The authorized task home no longer travels on a parsed line inside a mixed stdout stream. It has its own channel, --task-home-out, written by the controller and read by the wrapper as two fixed lines. On a shared stream its safety rested on two unrelated invariants holding forever - that stderr is never folded in, and that no id can contain a space or a newline - and either one relaxing would let another line decide where a removal is aimed. The lease pointer was excluded from the task-end set on the stated grounds that teardown removed it with the task's metadata. Teardown never mentions the name, so it was created by the spawn lane and removed by no task-end lane: the exact class of leak this owner exists to close, asserted otherwise in a comment. It is in the one set now. The enumeration invariant caught one spelling. It now reads bin/*.py and bin/*.mjs as well as bin/*.sh, matches flagless rm, globs over name stems, and find -delete, and its remaining blind spots (a name assembled from a variable or from concatenation) are stated in the test rather than implied away. The teardown reaping guard pinned a line's text and position, which left it green while the behavior was broken two ways: wrapping the call in a never-true condition, and shadowing the owner with a local no-op. It now refuses a redefinition anywhere in the script and requires the call at the same nesting as the child's own metadata removal. It is still a source check, not an execution proof, and the test says so. Also: run_child_lifecycle now prepends the fakebin to PATH like every other helper in that suite.
…alse texts The reaping guard was a source check and it failed open. Wrapping the call in a never-true condition escaped it, because shell does not force re-indentation and the guard compared indentation; so did shadowing the owner in the bash keyword form, and so did re-pointing sub_state at a directory that does not exist, which leaves the call textually perfect and aimed at nothing. The claim that both earlier escapes were caught was false, and so was the stated reason for keeping a source check: five return paths run between the reap and the home's removal, so a home whose children are reaped can survive. It is a run-time seam now. The loop's own text is extracted from the shipped script and executed against stubs, with the owner replaced by a recorder writing OUTSIDE the home, so what happens to the home afterwards is irrelevant. It asserts on a call that happened and on the arguments it happened with, which sees all four escapes. A redefinition at the script's top level is outside the extracted text, so a separate check refuses that in both spellings. The audit hung forever on a FIFO: the reader gated the channel and the audit did not, two functions applying different trust to one input. They share the gate now. The enumeration guard fired on innocent lines - rsync --delete, and the operator advice strings this wrapper prints beside its own refusals. Narrowed, with the true positives re-driven to confirm they still fail it. Three texts asserted things the code does not do, and the operator contract was one of them: docs/azure-workers.md still described the deleted FM-TASK-HOME line, the library called a pure delegate a group selector, and the audit's comment said the named home is never followed when [ -e ] follows symlinks. All three now say what the code does, including the shape that stays silent. Also recorded: withdraw and surrender now remove the lease pointer that the cloud monitor reads, which downgrades a landing to "kept for manual landing" rather than losing commits; and the file channel's own exotic fail path when TMPDIR is unwritable.
… mirrors #284 added a second worker-versus-item divergence check, on role, inside command_execute's controller lock. This branch already had one, on task_home, in the removal receipt. Both read a drifted pair as untrustworthy and both fail closed; they differ only in what closed means for their lane, because execute must not run while a removal that does not happen leaves a credential on disk to be found. Neither can admit a state the other refuses: different fields, different commands, and neither widens what the other allows. Comment only. The receipt writes no controller state and takes no lock, and the one call sitting inside a lock hold is on the idempotent surrender path where write_surrender_output already writes a file at the same point.
ruby-dlee
force-pushed
the
fix/compartment-cloud-account-remover
branch
from
August 21, 2026 05:15
668c7b3 to
4c7a3ae
Compare
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.
The defect, reproduced first
Since the compartment-child task-home split (#278/#279), a secondmate compartment's child stages its plaintext provider credential at
<task home>/state/<id>.cloud-account/auth.json, and its task home is the COMPARTMENT's home.withdrawandsurrenderre-derived the PRIMARY's state directory, because the controller document has exactly one home and every lifecycle command for the child runs withFM_HOMEon the primary. They removed a path that never existed, reported success, and the credential stayed in the compartment home with nothing left that would ever take it.Reproduced before touching any production code, end to end through the real
bin/fm-spawn.shand the realbin/fm-worker-lifecycle.sh:Correction to the brief this branch was written from:
bin/fm-teardown.sh:130is theSTATE=definition, not a remover, and teardown resolves its own task's home correctly. Teardown's real gap was different:cleanup_firstmate_home_children, the loop that reaps a secondmate home's children when the parent goes first, had no cloud-state removal at all.What changed
Removers do not infer a home. Each is handed the state directory of the home whose task it is:
$STATE, the directory they just staged into;withdrawandsurrenderare told by the CONTROLLER. The authorized task home is already durable on the queue item and the worker record, so both commands echo it back on anFM-TASK-HOMEline beside their receipt. The value the remover uses is the value the spawn passed to--task-homeand the controller authorized under its own lock.The credential is NOT relocated to the primary's state. The task-home split stays exactly as #278 landed it.
The per-task cloud file set is now enumerated once, in
bin/fm-cloud-state-lib.sh. The library, the re-spawn sweep and the spawn rollback used to spell it out separately, which is what allowed a file to be created by one lane and removed by none.Why not a per-task record file
The first version of this PR recorded the staging home in
<controller state>/<id>.cloud-task-home. Review showed that is destructive. Task ids are HOME-SCOPED, so the same id can be live in a compartment home and in the primary at once, and a stale record redirected an unrelated primary task's teardown into the compartment. Executed against that implementation, same fixture:and against what is on this branch now:
run_a_removal_never_reaches_a_same_id_task_in_another_homepins this. It deliberately seeds the rejected design's record file, which nothing writes and nothing reads any more, so the case stays red against any remover that resolves a home from the task id.Lanes covered
tests/fm-spawn-cloud.test.shtests/fm-worker-lifecycle.test.shtests/fm-cloud-state.test.shtests/fm-spawn-cloud.test.shThe reaping-loop call is checked by parsing
cleanup_firstmate_home_childrenand requiring the removal to appear before the metadata removal that ends the child. It is not execution-driven, and deliberately so: the loop is followed by the removal of the whole home, which would make any after-the-fact file assertion pass whether or not the call exists. Correctness there follows by composition with the library tests, not by execution.Structural, not a comment
test_compartment_child_staging_and_removal_resolve_the_same_homeruns a real compartment-child spawn, asks the filesystem which home actually holds the credential, and fails if the controller's recorded task home resolves a different one.run_no_second_enumeration_of_the_cloud_file_setfails if anybin/*.shother than the library removes a per-task cloud file by name.Admit-red mutations
fm_worker_receipt_state_dirforced to answer the controller's own state directory (the pre-change behavior). With the audit fixed this is now refused loudly rather than reaching the old assertion text:ELASTIC WORKER REFUSED: surrendered task-1 but its staged provider credential remains, and the withdraw lane reds onnot ok - withdraw left the child's staged provider credential in the compartment home.not ok - the primary's own credential was left behind.AssertionError: surrender left a compartment child's staged provider credential in its task home.Every red is the credential surviving or a live home being destroyed, never a message differing. Each was driven after watching the unmutated head go green first.
Ordinary crewmate lane is unchanged
Base commit versus this branch, identical fixture scaffolding, run tonight:
Scope, since the earlier count was quoted without it: 47 is every entry in the whole task-home snapshot (
checkout-refresh-state,config,data,projects,state,treehouse-pools). Understate/alone it is 30 entries, 31 countingstate/itself.Deliberate behavior change in the rollback
The rollback now also removes
.cloud-execute-dispatchedand the threeworker-*artifacts, because it shares one entry point with the re-spawn sweep. At the rollback point none of them can exist: the sweep at the start of the same spawn removed them and no execute has run. The outcome directory is untouched, and its neighbouring preservation of an unlanded bundle is unchanged and asserted.Rebased onto #280 (
d8154a61)#280 moved the credential write: the account directory is now written once, by the narrowing step, AFTER the lease exists. Consequences re-checked on the rebased head, not assumed:
spawn_cloud_bind_leased_accountfails after a successful request, the spawn callswithdrawitself, throughspawn_cloud_lifecycle, which runs withFM_HOMEon the primary. For a compartment child that withdraw is exactly the lane this PR repairs.<id>.worker-request.outto the re-spawn sweep only, so nothing removed it at the END of a task's life. It is in the single enumeration now, so teardown, withdraw and surrender cover it, and a test asserts that.Shard 7 red on the first rebased head, and why
d9b2220afailedBehavior tests (shard 7/8). It reproduced locally on the first run oftests/fm-worker-lifecycle.test.sh(the suite that shard carries), so it was deterministic, not a flake, and not a #280 interaction.Root cause was mine. Closing F2 meant carrying
parent_taskon theFM-TASK-HOMEline so the reader could apply the stager's own marker-content check.parent_taskandtask_homeboth land on the QUEUE ITEM at admission, but onlytask_homeis copied onto the worker record, and the surrender path sourced the receipt from the worker. The parent was therefore always absent, the line was never printed, and surrender fell back to the controller's own state directory: the leak this branch exists to close, reintroduced on exactly one lane by the hardening that closed the reader.The receipt now reads the queue item, and stays silent when a worker record's
task_homedisagrees with its own item's. The surrender fixture had used a convenient shape rather than the real admission shape; it now writes both fields where admission writes them, which is what made the defect reachable.Re-driven admit-reds after the fix, all red on the credential surviving:
Second review: five findings
The safety net could not catch the failure it exists to catch (blocking). It re-used the directory the removal had just resolved, so when resolution fell back wrongly, which is the entire defect this branch closes, it inspected the controller's own home, found nothing because nothing was ever staged there, and the command exited 0 announcing success. That is why the leak was silent in production rather than loud. Subjects are now fixed independently of the resolution: the home the controller NAMED for this task, read raw and only stat'ed, plus the controller's own state directory. Driven, same F1 mutation both times:
The task home no longer travels on a parsed line in a mixed stdout stream. It has its own channel,
--task-home-out, written by the controller and read as two fixed lines. On a shared stream its safety rested on two unrelated invariants holding forever, that stderr is never folded in and that no id can contain a space or newline; either relaxing would let another line decide where a removal is aimed.The lease pointer was a live instance of this PR's own class.
.cloud-worktreewas excluded from the task-end set on the stated grounds that teardown removed it with the task's metadata.grep -n 'cloud-worktree' bin/fm-teardown.shreturns nothing, so it was created by the spawn lane and removed by no task-end lane. It is in the one set now.The enumeration invariant caught one spelling. It now reads
bin/*.pyandbin/*.mjsas well asbin/*.sh, and catches flaglessrm, globs over name stems, andfind -delete; all three were driven and now fail the guard. It cannot catch a name assembled from a variable or from string concatenation, and the test says so rather than implying otherwise.The teardown reaping guard failed open, and my previous claim about it was false. It was a source check, and three escapes walked through it: the call wrapped in a never-true condition (shell does not force re-indentation, so the indentation comparison held), the owner shadowed in the bash keyword form, and
sub_statere-pointed at a directory that does not exist, which leaves the call textually perfect and aimed at nothing. The stated reason for keeping a source check was also wrong: fivereturn 1paths run between the reap andremove_firstmate_home, so a home whose children are reaped can survive.It is a run-time seam now. The loop's own text is extracted from the shipped script and EXECUTED against stubs, with the owner replaced by a recorder writing OUTSIDE the home, so what happens to the home afterwards is irrelevant. It asserts on a call that happened and on the arguments it happened with. All four escapes driven against it, all four now red. A redefinition at the script's top level is outside the extracted text, so a separate check refuses that in both spellings.
The audit hung forever on a FIFO. The reader gated the channel and the audit did not: two functions applying different trust to one input, which is the seam that produced the original defect. They share the gate now. Driven: pre-fix killed after hanging (exit 137), post-fix returns immediately.
The enumeration guard fired on innocent code.
rsync --deleteand the operator-advice strings this wrapper prints beside its own refusals were all flagged. Narrowed, with the three true positives re-driven to confirm they still fail it.Three texts asserted things the code does not do.
docs/azure-workers.mdstill described the deletedFM-TASK-HOMEline and called a pure delegate a group selector; the audit's comment said the named home is "never followed" when[ -e ]follows symlinks. All three now say what the code does, including the one symlink shape that stays silent (a named home that is itself a symlink to an empty marked decoy). That shape was silent before this PR too, so it is not a regression, and the other two symlink shapes now fail loudly.Accepted consequences, recorded rather than fixed
<id>.cloud-worktree, whichbin/fm-spawn-cloud-monitor.sh:245reads. Both commands end the task, and the monitor treats an unreadable pointer as "the leased worktree is gone" and keeps the bundle for manual landing, so the loss is a reported landing, never lost commits.save_state, leaving the task surrendered with no removal. The wrapper's ownmktempwould have failed first.Notes
azshim intests/andtests/run.shscrubs no ambientFM_*; what makes these lanes safe isFM_WORKER_PROVIDER_COMMANDbeing pinned inrun_child_spawn,stand_up_compartmentandrun_child_lifecycle, so no lane can select the real provider.run_child_lifecyclenow also prepends the fakebin toPATHlike its siblings.