From a5b51a178d3d8991423a83baffabaa83943e0308 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 28 Aug 2026 15:04:53 -0400 Subject: [PATCH] fix(db): bound the write-lock wait by the worst case it can really produce The budget test failed on hosted macOS in the post-merge run of 55658f9a: 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 --- openadapt_capture/db/crud.py | 26 ++++++++++++++++---------- openadapt_capture/recorder.py | 12 +++++++++--- tests/test_db_lock_retry.py | 19 ++++++++++++++++--- 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/openadapt_capture/db/crud.py b/openadapt_capture/db/crud.py index df5a98e..135a0f7 100644 --- a/openadapt_capture/db/crud.py +++ b/openadapt_capture/db/crud.py @@ -46,16 +46,21 @@ # deadline when it is imported. SQLITE_WRITE_LOCK_BUDGET_SECONDS = 20.0 -# The most one attempt is allowed to cost. +# The room one more attempt needs before the helper will begin it. # # Every statement of the transaction may wait the connection's busy timeout, -# and SQLite's busy handler overshoots that timeout under contention. The -# helper below refuses to BEGIN an attempt unless this much of the budget -# remains, which is what makes the budget an upper bound on the total wait -# rather than an estimate of it. +# and SQLite's busy handler overshoots that timeout under contention, so an +# attempt costs somewhat more than the timeout it was given. The helper refuses +# to BEGIN an attempt unless this much of the budget remains. +# +# That gives the bound its exact shape. The last attempt starts strictly before +# BUDGET - CEILING, so the total wait is under BUDGET whenever an attempt costs +# at most the ceiling, and under BUDGET + CEILING even when a loaded machine +# makes it cost twice that. recorder.py checks the second, weaker figure +# against its readiness deadline, because that is the one that always holds. # # tests/test_db_lock_retry.py measures a real attempt against a real held lock -# and fails if it costs more than this. +# with the production busy timeout and fails if it costs more than this. SQLITE_LOCK_ATTEMPT_CEILING_SECONDS = 2.0 # Back off between attempts so the writers do not resample the lock in step, @@ -115,10 +120,11 @@ def _write_with_lock_retry( corruption. The retry runs against a clock, not a counter. It re-enters the race for as - long as ``SQLITE_WRITE_LOCK_BUDGET_SECONDS`` allows, and it stops as soon - as too little of that budget remains to finish another attempt. The total - wait is therefore never more than the budget, whatever one attempt costs on - the machine underneath. + long as ``SQLITE_WRITE_LOCK_BUDGET_SECONDS`` allows, and it never begins an + attempt once less than ``SQLITE_LOCK_ATTEMPT_CEILING_SECONDS`` of that + budget remains. The total wait is therefore bounded by the budget plus, at + worst, one attempt that ran long -- never by whatever a count of attempts + happens to cost on the machine underneath. """ deadline = monotonic() + SQLITE_WRITE_LOCK_BUDGET_SECONDS backoff = SQLITE_LOCK_FIRST_RETRY_SECONDS diff --git a/openadapt_capture/recorder.py b/openadapt_capture/recorder.py index 33a6f50..9780bd4 100644 --- a/openadapt_capture/recorder.py +++ b/openadapt_capture/recorder.py @@ -536,10 +536,16 @@ def __bool__(self): # Check the two against each other here rather than trusting a comment beside # either one: whichever a later change moves, the package refuses to import # with a budget that cannot fit. -if crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS >= STARTUP_READY_TIMEOUT_SECONDS: +# +# Use the worst case the retry helper can actually produce, which is its budget +# plus one attempt that ran long, not the budget alone. +SQLITE_WRITE_LOCK_WORST_CASE_SECONDS = ( + crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS + crud.SQLITE_LOCK_ATTEMPT_CEILING_SECONDS +) +if SQLITE_WRITE_LOCK_WORST_CASE_SECONDS >= STARTUP_READY_TIMEOUT_SECONDS: raise RuntimeError( - "The SQLite write-lock budget " - f"({crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS:.1f}s) must leave a writer " + "The worst-case SQLite write-lock wait " + f"({SQLITE_WRITE_LOCK_WORST_CASE_SECONDS:.1f}s) must leave a writer " "time to announce readiness within " f"{STARTUP_READY_TIMEOUT_SECONDS:.1f}s." ) diff --git a/tests/test_db_lock_retry.py b/tests/test_db_lock_retry.py index 774291f..f426da0 100644 --- a/tests/test_db_lock_retry.py +++ b/tests/test_db_lock_retry.py @@ -463,9 +463,17 @@ def test_the_total_wait_never_runs_past_the_declared_budget(tmp_path, monkeypatc session.close() engine.dispose() - assert elapsed <= crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS, ( + # The helper starts its last attempt strictly before BUDGET - CEILING, so + # the total is over the budget only by however long that one attempt ran. + # A loaded runner does make it run long: this measured 2.05s against a 2.0s + # budget on hosted macOS. Bound it by what always holds. + worst_case = ( + crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS + + crud.SQLITE_LOCK_ATTEMPT_CEILING_SECONDS + ) + assert elapsed <= worst_case, ( f"a permanently held lock cost {elapsed:.2f}s, over the declared " - f"{crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS:.1f}s budget" + f"{worst_case:.1f}s worst case" ) # ... and it is spent, not abandoned. The replaced policy stopped after # three attempts, which against this lock is under a tenth of the budget. @@ -482,8 +490,13 @@ def test_the_write_lock_budget_fits_the_readiness_deadline(): ``recorder`` refuses to import when this does not hold, so this test states the same contract where a reader of the database code can see it. """ + assert recorder.SQLITE_WRITE_LOCK_WORST_CASE_SECONDS == ( + crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS + + crud.SQLITE_LOCK_ATTEMPT_CEILING_SECONDS + ) assert ( - crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS < recorder.STARTUP_READY_TIMEOUT_SECONDS + recorder.SQLITE_WRITE_LOCK_WORST_CASE_SECONDS + < recorder.STARTUP_READY_TIMEOUT_SECONDS )