fix(db): bound the write-lock wait by the worst case it can really produce

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

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."
     )

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
     )