Deferred Allocation/Deletion Formal Modeling and Bug Fixes - #474
Open
lightsighter wants to merge 1 commit into
Open
Deferred Allocation/Deletion Formal Modeling and Bug Fixes#474lightsighter wants to merge 1 commit into
lightsighter wants to merge 1 commit into
Conversation
…fixing associated bugs discovered by the model
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
==========================================
+ Coverage 30.41% 30.42% +0.01%
==========================================
Files 199 199
Lines 41282 41337 +55
Branches 14779 14982 +203
==========================================
+ Hits 12554 12575 +21
- Misses 27488 28305 +817
+ Partials 1240 457 -783 ☔ View full report in Codecov by Harness. |
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.
This PR adds a TLA+ model of Realm's deferred instance allocation/deletion logic (LocalManagedMemory), uses it to find and adjudicate several long-standing bugs — including the event-loop deadlock suspected in the original design talk — and fixes them with a design that was model-checked before being ported to C++. The fixes ship with API-level regression tests that provably detect the old bugs: run against main, they reproduce the deadlock and the exact debug assertion the model predicted.
The model (tla/allocation/)
DeferredAlloc.tla models the allocator's three heap states (current/future/release), the pending_allocs/pending_releases queues with their seqid ordering, release reordering (attempt_release_reordering), poisoned-precondition cleanup (remove_pending_release), and event preconditions — transcribed branch-by-branch from mem_impl.cc with line citations (DESIGN.md records the correspondence). The first-fit range allocator is abstracted to a tag→interval map proven equivalent to BasicRangeAllocator's address-ordered first fit. Clients are constrained by the documented contract: topologically sorted requests (no back edges), destroy preconditions incorporating the created event. C++ asserts are modeled as checkable invariants rather than assumptions, so assert-reachable states are found, not pruned. Instance redistricting is deferred to a v2 model (FUTURE-VERIFICATION.md).
./tla/allocation/run.sh runs the local config matrix in seconds-to-minutes and is the standing regression oracle for future mem_impl.cc changes.
Bugs found and fixed (details in tla/allocation/bugs/)
BUG-1 — event-loop deadlock from trigger-time ordering (bugs/BUG-1.md). A deferred create is inserted into the release/alloc total order when its precondition triggers, but its e_created was handed out at request time. A release requested in between may legally depend on that e_created; the future-state rebuild counts its space anyway, so Realm funds the allocation from a release that can only happen after the allocation completes — a permanent, silent hang, after the mapper was told InstanceAllocResult{success=true}. Minimal witness: 2 instances + 1 user event (7-state TLC trace).
Fix: snapshot cur_release_seqid at request time (DeferredCreate::release_seqid_cap); at trigger, admit only against releases at-or-below the cap (canonical-order replay) with a monotone-cap queue guard; a capped miss returns an honest ALLOC_INSTANT_FAILURE instead of hanging.
BUG-6 — stranded ready release; assert(!it->is_ready) at mem_impl.cc:772 reachable (bugs/BUG-6.md). A reordering-failure pushback can leave a ready release queued behind a non-ready one; the oldest-entry drain then empties pending_allocs, and both cleanup paths are skipped precisely because the queue just emptied. Debug builds abort on the next deferral-needing allocation; release builds silently break the documented release = current + ready releases invariant. A composite of this state with a later reordering swap permanently leaks the stranded range while firing its dealloc notify (the instance-recycling double-tracking class from Fix Instance ID Reuse #442) — no poison involved. TLC witnesses: 9 steps / 3 instances, and 12 steps / 5 instances for the leak.
Fix: a shared sweep applies remaining ready releases (redistrict-aware) to current_allocator at every pending_allocs→empty transition, restoring the invariant — the cc:772 assert is now genuinely true.
BUG-5 — trailing allocations dropped by poisoned-release cleanup (bugs/BUG-5.md). remove_pending_release's rebuild replay never revisits pending allocs whose seqid watermark exceeds every surviving release — they are neither failed nor re-funded. Composed with the BUG-1 fix (which makes poisoned releases routine), this strands an admitted allocation forever; TLC found the deadlock the moment the cap fix was modeled without it.
Fix: after the replay walk, continue the same loop over the remaining allocs — re-place into the rebuilt future state or fail them cleanly. The three fixes are intentionally one change: the cap must not land without the trailing replay.
Behavior change
Allocations that previously "succeeded" by being funded from a not-yet-requested-at-create-time deletion now fail honestly at trigger time. Those successes were unsound promises (the hang above); mappers already handle allocation failure. Unconditioned (NO_EVENT) creates — the dominant case — are bit-identical to the old behavior (verified instruction-level in review).
Verification
Not in this PR