fix(worker): apply a provider mutation transactionally, inside one slot - #253
Merged
Conversation
added 2 commits
August 19, 2026 00:58
First of the changes that let many crewmates, a no-mistakes offload and a crosscheck run at once. It changes no serialization and no state shape: it establishes the property the later ones depend on, while everything is still serialized and a wrong allowlist is cheap to discover. apply_action_result mutates the document it is handed, and several of its paths mutate before they refuse. adopt_cloud_resources writes the whole fifteen-kind resource set onto the worker record and only then checks the identity, so a create whose resources moved leaves the caller holding a worker carrying resources its create never got. Nothing saves that today, because the raise propagates before the next save_state, but nothing structural stops a later handler from saving it either, and a partly applied worker is exactly the record a reset would then act on for a VM that still exists. The apply now runs against a copy and is committed into the caller's object only if it returns. That makes the partly applied image unreachable rather than unlikely. It also has to be scoped, because that is what will make two mutations for different slots safe in flight together. assert_scoped refuses any apply whose diff reaches outside the one compartment a slot owns: its own worker record, which reset removes; the single queue entry that worker owns; the execution keyed by the action's request digest; and the completed-seconds accumulator. That list was enumerated from every assignment, pop and setdefault in apply_action_result rather than assumed, and the test pins both halves: an apply touching another slot, the queue, capacity, the assignment counter or a foreign execution is refused, and the compartment it does own is allowed. Two details worth knowing. The comparison is between two copies rather than the live object and one copy, because make_action aliases live state into the action it mints and deepcopy preserves that sharing, so an aliased original compared with an unaliased copy reports a difference the apply never made. And pending_action is excluded for the same reason: the action IS the queue entry it names, and execute_action owns that key on both sides of the call. The test drives both production call sites, not only the function they should call. Asserting the property of the new function alone left execute_action and replay_pending free to go back to the in-place apply with the suite green, which is how it was written first. Five mutations checked by making the edit and running the suite: either call site reverting to the in-place apply, assert_scoped becoming a no-op, its allowlist widening to capacity and the assignment counter, and dropping the commit back into the caller's object. All five go red. The fixture is proved real by a valid apply landing against it before anything is corrupted, so a fixture the production function would reject fails there rather than making the later assertion vacuous.
…at did nothing An adversarial review ran a mutation I had not: delete the assert_scoped CALL from apply_result_transactionally, leaving the function itself intact. The suite stayed green. The three test blocks proved transactionality, which comes from copy-and-commit rather than from the guard, and proved assert_scoped correct as a function called directly, with nothing connecting the two. The guard is the entire reason this change exists, so that was the hole worth finding. A stubbed apply that writes outside its slot now proves the call site refuses, and asserts it refused for the right reason rather than merely refusing. The same review disproved this change's own stated rationale for taking two copies. Three mutations show it: with the pending-action exclusion in place, comparing the live object against one copy behaves identically to comparing two copies, and without the exclusion both spellings fail with the same message. The exclusion is the whole fix, because the apply writes only into the copy, so the original is unchanged at comparison time either way. The second copy is gone and the comment says what actually makes the comparison meaningful. An explanation the follow-on changes will be read against is worth more than the copy was. Also from that review: a refusal here leaves the pending action durable, and every later reconcile replays it and re-refuses, which the file's own comment elsewhere calls taking the whole fleet's convergence with it. There is no subcommand to clear one, so both refusal messages now say the pending action will replay until it is resolved. And a test assertion that read `removed and before` was correct only because the dict was non-empty; it is now just `before`. Six mutations checked by making the edit and running the suite, including the reviewer's: deleting the assert_scoped call, either call site reverting to the in-place apply, assert_scoped becoming a no-op, dropping the caller-owned exclusion, and dropping the commit back into the caller's object. All six go red.
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.
First of the changes for C2 (many crewmates, a no-mistakes offload and a crosscheck running at once without the controller serializing them). It changes no serialization and no state shape: it establishes the property the later ones depend on, while everything is still serialized and a wrong allowlist is cheap to discover.
The defect
apply_action_resultmutates the document it is handed, and several paths mutate before they refuse.adopt_cloud_resourceswrites the whole fifteen-kind resource set onto the worker record and only then checks the identity, so a create whose resources moved leaves the caller holding a worker carrying resources its create never got.Nothing saves that today, because the raise propagates before the next
save_state. But nothing structural stops a later handler from saving it, and a partly applied worker is exactly the record aresetwould then act on for a VM that still exists.The change
The apply runs against a copy and is committed into the caller's object only if it returns. The partly applied image becomes unreachable rather than unlikely.
assert_scopedrefuses any apply whose diff reaches outside the one compartment a slot owns:resetremoves)That list was enumerated from every assignment,
popandsetdefaultinapply_action_resultrather than assumed. It is what will make two mutations for different slots safe in flight together. It is coarse on purpose: per container, not per action type, because a per-type contract would have to change every time a type does, which is how an allowlist becomes a wedge on the money path.The aliasing, corrected
An earlier revision of this PR claimed the fix for a false-positive diff was comparing two copies rather than the live object against one. That was wrong, and an adversarial review disproved it with three mutations: with
pending_actionexcluded, one copy behaves identically to two; without the exclusion, both spellings fail with the same message.The exclusion is the whole fix.
make_actionaliases live state into the action it mints (action["request"]is the queue entry) anddeepcopypreserves that sharing, so applying into the copy also changes the copy's own record of the pending action while the original's stays put. The apply writes only into the copy, so the original is unchanged at comparison time either way. The second copy is gone.Cost of a refusal
A refusal here leaves
pending_actiondurable, and every laterreconcile --applyreplays it and re-refuses. The file's own comment elsewhere calls that taking the whole fleet's convergence with it, and there is no subcommand to clear a wedged pending action. Both refusal messages now say so. This is not reachable from current code — the allowlist was re-derived independently and matches — but it is the real cost of getting the allowlist wrong, and the message should say it rather than the PR body claiming it is cheap.Mutations
The test drives the production call sites, not only the functions they should call. Two rounds of that were needed: the first version left
execute_actionandreplay_pendingfree to revert with the suite green, and the second still left theassert_scopedcall deletable with the suite green, which a review found and I had not.assert_scopedcall is deleted fromapply_result_transactionallyexecute_actionreverts to the in-place applyreplay_pendingreverts to the in-place applyassert_scopedbecomes a no-opCALLER_OWNED_KEYSis emptiedThe fixture is proved real by a valid apply landing against it before anything is corrupted, so a fixture the production function would reject fails there rather than making the later assertion vacuous. The out-of-scope case asserts the refusal names
capacity_reservations, so it is refused for the right reason rather than merely refused.tests/fm-worker-lifecycle.test.shpasses 10/10.bin/fm-behavior-shards.sh --check 8passes.shellcheck --norc -xclean.What comes next, and not here
Per the design study: the
pending_actionsmap with a revision CAS and a load fence (still fully serialized), then the lock discipline as one PR. The study also found that splitting the capacity commands would let two concurrent reserves admit against a budget that fits one, becausemerged_specialized_reservationsskips locals whose status is notreserved; those three commands stay fully locked and that split is explicitly out of the program.