bug: Fix TOCTOU With Budget Leasing - #87
Open
markovejnovic wants to merge 15 commits into
Open
Conversation
Reserve-before-boot is not yet implemented, so these are RED by design: try_run/3 boots the VM and only then asks the budget for permission, so a concurrent herd can put N VMs on a node budgeted for k. Also records two deferred defects in TODO.txt: the unguarded :erpc.call in Scheduler.run/4, and the uid leak between Users.claim/0 and Users.bind/2. Claude-Session: https://claude.ai/code/session_01LCkuEmceEDTAeewU5Csoup
node_state.ex still described Budget.admit/2 as the authoritative admission check; that function was deleted in favor of Budget.lease/2. Also mark the budget-admission window in TODO.txt as fixed: capacity is now leased before boot instead of after.
TODO.txt still quoted the pre-branch try_run/Scheduler.run shapes (the deleted stop_fun parameter, Budget.Hard.reserve) as if current, and several line citations into scheduler.ex/node.ex/hyper.ex had drifted from the edits those files received in this branch. Corrects the quoted code and citations without changing either entry's open status or findings.
Hyper.Node.start_vm_or_release/3 did not handle the :ignore DynamicSupervisor.start_child returns when Hyper.Node.FireVMM.init/1 declines a boot for lack of a budget lease. The unmatched case clause raised, skipping uid/mutable-layer release and permanently leaking the uid (merge blocker). Also folds in the rest of the final review's fix wave: a budget assertion in the E2E VM lifecycle test, guarding the two republish() calls in Hard that were provably no-ops, a missing bad-duration refusal row in the budget config test, and a TODO.txt entry tracking the now-tractable Hard-restart reconciliation gap.
Closes four residuals from the re-review of the :ignore fix: - Reword the :ignore comment (lib/hyper/node.ex and the new test's moduledoc) to name both causes FireVMM.init/1 can decline for (stale routing registration, not just a missing budget lease), since a reader debugging :not_admitted would otherwise never suspect routing. - FakeMutable now reports its release to the test process instead of silently no-op'ing it, and the test asserts receipt. Verified by mutation: deleting Img.Mutable.release(mutable) from the :ignore arm now fails the test (confirmed, then reverted). - Match the module's `_ = if ..., do: republish()` idiom in the leaser-DOWN path of hard.ex, which had drifted to a bare `if`. - Fix a drifted line reference in TODO.txt (:365 -> :370, following the @doc false/@SPEC lines added ahead of start_vm_or_release/3).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Test Results 3 files - 1 206 suites - 21 1m 14s ⏱️ - 2m 9s For more details on these failures, see this check. Results for commit 7b88918. ± Comparison against base commit 3ac24e1. This pull request removes 195 and adds 35 tests. Note that renamed tests count towards both. |
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
1 participant
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.
There was a bug wherein we acquired budget leases after trying to provision a VM. This has been inverted.