test: pin fail-loud behavior when InternalPage.alloc() raises mid-alloc - #449
Merged
RixinLiu merged 2 commits intoAug 18, 2026
Merged
Conversation
The rollback handler added in ovg-project#430 was narrowed to alloc_page() during review so that InternalPage.alloc()'s "Not enough free blocks" invariant failure stays fail-loud: by the time page.alloc() runs, _pick_avail_page() may already have removed the page from avail_pages while its blocks are not yet in ret_index, so rollback could not restore it. The review asked for a regression test where page.alloc() raises to pin that distinction; this adds one for both call sites (a page picked from avail_pages and a freshly allocated page), asserting the error propagates instead of being downgraded to an allocation miss.
rishabhsinha17
force-pushed
the
test/page-alloc-fail-loud
branch
from
August 15, 2026 18:41
4ab3ad2 to
eb3e7e0
Compare
jeff3071
reviewed
Aug 16, 2026
| # invariant failure must propagate, not degrade into a None miss (#430). | ||
| manager = make_manager(fail_after=1) | ||
| assert manager.alloc(2) == [0, 1] | ||
| manager.avail_pages[0].__class__ = ExplodingPage |
Contributor
There was a problem hiding this comment.
Could we use monkeypatch to make page.alloc() raise instead?
Contributor
Author
There was a problem hiding this comment.
Done in 5e25a3a: both tests now inject the raise with monkeypatch (instance attribute for the avail_pages case, class attribute for the page that does not exist until alloc() creates it) and the ExplodingPage subclass is gone. 8 passed, ruff/isort/mypy clean.
Collaborator
|
Thanks for the follow-up, good to have this |
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
3 participants
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.
Summary
Follow-up to #430: the review there narrowed the rollback handler to
alloc_page()so thatInternalPage.alloc()'s "Not enough free blocks" invariant failure stays fail-loud, and asked for a regression test wherepage.alloc()raises to pin the distinction (#430 (comment)). The handler was narrowed in 79794d9; the test was not added. This adds it.Changes
Two tests in
tests/test_alloc_rollback.py, one perpage.alloc()call site: a page picked fromavail_pages(the case where rollback cannot restore state, since_pick_avail_page()has already removed the page while its blocks are not yet inret_index) and a freshly allocated page. Both assert theRuntimeErrorpropagates out ofalloc()instead of being downgraded to aNonemiss.Validation
python -m pytest tests/test_alloc_rollback.py-> 8 passed. ruff, isort, andmypy --python-version 3.10clean on the file. No manifest change needed: the file is already classifiedcpu.