From 666cb1a0c9b40d3df1975686efe291daa5bec5c8 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Tue, 4 Aug 2026 16:00:57 +0200 Subject: [PATCH] Retire in-code comments that outlived what they described (B-69) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - app/tx/reconcile.py called the payout retry "a future payout-retry routine — still an open gap". It shipped as B-26: clearing payout_txid leaves the round in exactly the state _retry_payout_if_due picks up, so an abandoned payout rebuilds itself and the log line next to it is an alert, not the recovery path. Reading it the old way, an operator would go hand-fix a round the scheduler was already retrying. - app/db/base.py sized the SQLite busy timeout against "five concurrent background tasks" and then listed only the non-listener ones; the lifespan starts six. - The third item (app/auth/routes.py citing B-31 where it meant B-33) was already correct in the tree; the test pins it so it stays that way. tests/unit/test_code_comments.py derives the task count from the lifespan's own create_task calls rather than restating it, so the comment fails the next time a task is added or removed instead of quietly going stale again. Co-Authored-By: Claude Opus 5 (1M context) --- BUGS.md | 7 ----- CLAUDE.md | 4 +-- README.md | 2 +- app/db/base.py | 5 ++-- app/tx/reconcile.py | 6 ++-- tests/unit/test_code_comments.py | 47 ++++++++++++++++++++++++++++++++ 6 files changed, 57 insertions(+), 14 deletions(-) create mode 100644 tests/unit/test_code_comments.py diff --git a/BUGS.md b/BUGS.md index b418e47..e4ea343 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,13 +40,6 @@ remains the last prerequisite for running unattended. ## Low — documentation and consistency drift -### B-69 — stale in-code comments - -- `app/tx/reconcile.py:206` calls the payout retry "a future payout-retry routine — - still an open gap"; it exists (B-26). -- `app/db/base.py:9` says "five concurrent background tasks"; there are six. -- `app/auth/routes.py:31` cites B-31 where it means B-33 (see B-58). - ### B-70 — the flowchart's WITHDRAW precondition is not implemented as written `flowchart/platform-overview.mmd:39` (node E1) states a withdrawal cannot happen diff --git a/CLAUDE.md b/CLAUDE.md index 8dbfe87..96fb66d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 345 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 349 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 345 tests +python -m pytest # all 349 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` diff --git a/README.md b/README.md index dbe5720..78486e3 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ python -m pytest # all tests python -m pytest tests/unit/test_hd.py # one file ``` -345 unit tests cover HD derivation, PSBT building, the Electrum client, bets, +349 unit tests cover HD derivation, PSBT building, the Electrum client, bets, deposits, withdrawals, the round/draw engine, RBF fee-bumping, admin config, the pending-inclusive balance calculation, and the SSE push channel. No automated integration tests against a live Electrum connection — mainnet diff --git a/app/db/base.py b/app/db/base.py index cbe56bc..fa0dbaa 100644 --- a/app/db/base.py +++ b/app/db/base.py @@ -6,8 +6,9 @@ from app.config import settings # How long a writer waits for a lock held by another writer before SQLite raises # "database is locked" (B-39). A few seconds is enough to ride out this app's own -# five concurrent background tasks (scheduler, confirmation poller, RBF bumper, -# two reconcilers) plus HTTP handlers briefly overlapping a write. +# six concurrent background tasks (Electrum listener, scheduler, confirmation +# poller, RBF bumper, two reconcilers) plus HTTP handlers briefly overlapping a +# write. _SQLITE_BUSY_TIMEOUT_MS = 5000 diff --git a/app/tx/reconcile.py b/app/tx/reconcile.py index fcf4776..2382252 100644 --- a/app/tx/reconcile.py +++ b/app/tx/reconcile.py @@ -202,8 +202,10 @@ async def _abandon(session: AsyncSession, row: PendingTransaction, reason: str) elif row.kind == "payout": # Payout funds come from the pool address, which isn't tracked in # utxo_events, so there's nothing to release. Clearing payout_txid leaves the - # round in "paying_out" with no tx attached, which is the state an operator - # (or a future payout-retry routine — still an open gap) can act on. + # round in "paying_out" with no tx attached, which is exactly the state the + # scheduler's payout retry picks up (B-26: `_retry_payout_if_due`, one attempt + # per 60s), so an abandoned payout is rebuilt on its own rather than waiting + # for an operator — the log line below is the alert, not the recovery path. round_ = await session.get(Round, row.round_id) if row.round_id else None if round_ is not None and round_.status == "paying_out": round_.payout_txid = None diff --git a/tests/unit/test_code_comments.py b/tests/unit/test_code_comments.py new file mode 100644 index 0000000..9e91a86 --- /dev/null +++ b/tests/unit/test_code_comments.py @@ -0,0 +1,47 @@ +"""B-69: in-code comments that describe the rest of the system must stay true. + +Three had rotted: the reconciler still called the payout retry "a future +payout-retry routine — still an open gap" long after B-26 shipped it, +app/db/base.py sized the SQLite busy timeout against "five" background tasks +when there are six, and app/auth/routes.py cited the wrong B-nn. A comment is +invisible to every other test in the suite, so the claims are pinned here. +""" + +import re +from pathlib import Path + + +_ROOT = Path(__file__).resolve().parents[2] + + +def _read(relative: str) -> str: + return (_ROOT / relative).read_text(encoding="utf-8") + + +def test_background_task_count_in_the_busy_timeout_comment_is_right(): + started = len(re.findall(r"asyncio\.create_task\(", _read("app/main.py"))) + assert started == 6, "the lifespan's task count changed — update app/db/base.py's comment" + + comment = _read("app/db/base.py").split("_SQLITE_BUSY_TIMEOUT_MS", 1)[0] + match = re.search(r"(\w+) concurrent background tasks", comment) + assert match, "app/db/base.py no longer explains what the busy timeout is sized for" + words = {"four": 4, "five": 5, "six": 6, "seven": 7, "eight": 8} + assert words.get(match.group(1)) == started + + +def test_the_reconciler_does_not_call_the_payout_retry_an_open_gap(): + source = _read("app/tx/reconcile.py") + assert "still an open gap" not in source + # It exists (B-26) and is what actually recovers an abandoned payout, so the + # comment must point at it rather than at an operator. + assert "B-26" in source + + +def test_the_payout_retry_the_comment_points_at_still_exists(): + assert "_retry_payout_if_due" in _read("app/rounds/scheduler.py") + + +def test_the_login_throttle_comment_cites_its_own_finding(): + limiters = _read("app/auth/routes.py").split("_rate_limiters", 1)[1][:1500] + assert "B-33" in limiters + assert "B-31" not in limiters # B-31 is the resubscribe fan-out, a different fix