fix(db): bound the write-lock wait by the worst case it can really produce - #123
Merged
Conversation
…oduce The budget test failed on hosted macOS in the post-merge run of 55658f9: a permanently held lock cost 2.05s against a declared 2.0s budget. The measurement was right and the stated bound was wrong. The helper starts its last attempt strictly before BUDGET - CEILING, so the total is over the budget by however long that one attempt runs. An attempt costs the busy timeout plus whatever SQLite's busy handler overshoots it by, and a loaded runner overshoots more, so "total is at most the budget" holds only while an attempt stays inside the ceiling. It is not the figure to build a release gate on. State the bound that always holds instead: at most the budget plus one attempt, which is BUDGET + CEILING. recorder.py now computes that figure and checks it, rather than the budget alone, against its readiness deadline: 22s against 30s. The test asserts the same figure, and still fails when the helper gives up early, which is the half of the contract a count of attempts cannot express. No change to the retry behaviour itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Follow-up to #122, from its own post-merge run.
What failed
test-macoson55658f9a:Why
The measurement was right and the stated bound was wrong.
_write_with_lock_retrystarts its last attempt strictly beforeBUDGET - CEILING, so the total runs over the budget by however long that one attempt takes. An attempt costs the busy timeout plus whatever SQLite's busy handler overshoots it by, and a loaded runner overshoots more. "Total is at most the budget" therefore holds only while an attempt stays inside the ceiling — which is not a figure to build a release gate on.The change
State the bound that always holds: at most the budget plus one attempt,
BUDGET + CEILING= 22s.recorder.pynow computes that figure and checks it, not the budget alone, against the 30s readiness deadline.No change to the retry behaviour itself — this corrects what is claimed and checked about it.
The test still fails when the helper gives up early, which is the half of the contract a count of attempts cannot express. Verified again with only the old retry policy restored: both new regression tests fail, the other 13 pass.
Ran the corrected test 12 times under load average 8: passed every time, 2.3–3.1s.
Full suite: 721 passed, 33 skipped.
ruffclean.Also seen in that run, and not this
test-windowsfailed ontest_highlevel.py::TestRecorder::test_stop_during_incomplete_startup_returns_promptly,assert (387.781 - 386.765) < 1— 1.016s against a hard 1s wall-clock bound.That test monkeypatches
recorder_module.recordaway entirely, and the finalize call added in #122 sits behind a readiness guard that never fires there, so nothing in #122 executes on that path. The test deliberately spends 0.5s in_join_tasks(timeout=0.5)and then allows 1.0s total for thread joins, control-state writes and socket teardown on a shared Windows runner. It has no failure history in the visible runs. Flagging it rather than touching it.