[Tim020]

Summary

GET /api/v1/health now reports real readiness instead of always returning ok, and the Docker image gets a HEALTHCHECK.

Endpoint contract

Unauthenticated; version and status kept; 200 when healthy (unchanged).

Healthy (200):

{"version":"0.35.2","status":"ok","checks":{"database":"ok","storage":"ok"},"uptime_seconds":123}

Unhealthy (503): status: "degraded" with the failing check(s) set to "fail". No exception text, paths or stack traces are ever returned; detail is logged server-side (ERROR for DB, WARNING for storage).

• database: SELECT 1 run in the executor, bounded to 3s so a locked DB cannot hang the probe.
• storage: log, db log, client log (parent dir), draft_script_path and compiled_script_path writable (os.access; a not-yet-created dir is judged by its nearest existing ancestor).
• prepare() is overridden to skip the auth/session lookup, otherwise a broken DB would 500 before the check ran.
• Route is now ignore_logging=True so polling does not spam access logs.
• Dockerfile: HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 using a python urllib one-liner (no curl dependency); non-2xx raises so 503 marks the container unhealthy.

Testing

• pytest: 1160 passed; ruff check/format clean
• Full Playwright (chromium): 225 passed (harness waitForServer uses this endpoint)
• Dockerfile not built locally; syntax reviewed.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

102 tests  ±0   102 ✅ ±0   0s ⏱️ ±0s
  9 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d7a71dd. ± Comparison against base commit 85b1c75.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

227 tests  ±0   227 ✅ ±0   1s ⏱️ -1s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d7a71dd. ± Comparison against base commit 85b1c75.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files  ± 0      1 suites  ±0   1m 24s ⏱️ +7s
1 200 tests +11  1 200 ✅ +11  0 💤 ±0  0 ❌ ±0 
1 223 runs  +11  1 223 ✅ +11  0 💤 ±0  0 ❌ ±0 

Results for commit d7a71dd. ± Comparison against base commit 85b1c75.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

227 tests  ±0   227 ✅ ±0   2m 18s ⏱️ -7s
 14 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d7a71dd. ± Comparison against base commit 85b1c75.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

227 tests  ±0   227 ✅ ±0   2m 24s ⏱️ ±0s
 14 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d7a71dd. ± Comparison against base commit 85b1c75.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 9a5c0a80744832f71f64df9915fe6dc5423f142c

Verdict: changes needed. The design is sound and well-tested overall, but one finding below means the endpoint's own "no exception text, paths or stack traces are ever returned" guarantee doesn't actually hold in the deployed container today.

Previous round follow-up: N/A — this is round 1, no prior review exists on this PR.

SonarQube: Quality Gate passed (source: SonarCloud public API, confirmed fresh against head SHA 9a5c0a8, analysis at 2026-10-22T17:46:37Z). All 5 conditions pass: New Reliability/Security/Maintainability Rating = A, New Duplicated Lines Density = 0.0% (≤3% required), New Security Hotspots Reviewed = 100%. 0 new issues, 0 hotspots, 0 duplications. Nothing required to keep the gate green.

New findings: 1 Critical, 2 Important, 5 Suggestions (by severity below). Two items (docs update, auth-skip edge-case test) couldn't be anchored to a diff line and are listed here instead:

• (Suggestion, not anchored) CLAUDE.md asks for docs/ updates when behaviour changes. The 503/"degraded" contract and the new Docker HEALTHCHECK are operator-facing and aren't mentioned anywhere in docs/.
• (Suggestion, not anchored) No test exercises _setting()'s KeyError branch (a storage setting simply absent) or asserts uptime_seconds >= 0 / increases across requests — both currently correct by inspection, not by test.

Strengths: the failure-path tests (test_health_database_failure_returns_503_without_leaking, test_health_unwritable_storage_returns_503) are genuinely good — they inject real secret-looking strings and assert their absence from the response body, not just status codes. The _writable() ancestor-walk correctly terminates at / and handles not-yet-created directories. prepare() skipping auth was checked against BaseController/BaseAPIController and doesn't skip anything else safety-critical for this endpoint. All 5 documented storage settings are correctly included in the check.

🤖 Generated with Claude Code

[Tim020]

Review round 1 response (2ef3edf)

• Critical, traceback leak: fixed. Partly stale premise: the base already sets serve_traceback=False app-wide (Security quick wins: show deletion auth, debug default off, request size metrics #1455), but the guarantee should not depend on that. get() now catches storage-check errors (logged with exc_info, reported as storage: fail), and write_error returns a minimal degraded 503 body for any 5xx (4xx keep default handling). Tests force unexpected exceptions with serve_traceback=True and assert no exception text, path or traceback in the body.
• Important, leaked probe thread: fixed. Probes run single-flight on a dedicated 1-worker executor; while a timed-out probe is still in flight, further polls report database: fail without starting another, so at most one thread and connection is ever held. Test covers hang, second poll not starting a probe, and recovery.
• Important, leak test coverage: fixed. All five path settings (log_path, db_log_path, client_log_path, draft_script_path, compiled_script_path) are asserted absent.
• Suggestions: reST docstrings added to _check_database_sync and _setting. The probe is now SELECT 1 FROM sqlite_master LIMIT 1, which reads the DB file header and schema. exc_info=True added to the DB failure log. Redundant noqa: BLE001 removed. The storage test patches _writable instead of the global os.access; a separate test covers _writable itself. New tests cover the _setting KeyError branch and uptime_seconds (>= 0, non-decreasing).
• Dockerfile port: documented the constraint with a comment. The app has no env var for the port, so the check cannot follow --port; override with --health-cmd or compose healthcheck: in that case.
• Docs: operator docs for the 503/HEALTHCHECK contract live in documentation/deployment.md, owned by Add Prometheus/Grafana monitoring overlay and docs #1462, and will be updated there. Not edited in this PR.

Verification: pytest 1170 passed, ruff check and ruff format --check clean. Playwright left to CI.

[Tim020]

Review round 2

Head SHA reviewed: 16cb210a120818b502d6f748ceb0f7e0abd399e2

Verdict: changes needed, but close. The fix commit is thorough and addresses 5 of the 6 round-1 findings cleanly; one fix (the DB-probe single-flight guard) resolves its original problem but introduces a new regression that should be fixed before merge.

Verification performed: checked out the actual PR head commit in an isolated worktree (not just read the diff) and ran it directly — ruff check/ruff format --check clean, all 14 tests in test_health.py pass (10 new), and the full server suite (pytest -q) passes at 1199/1199.

Previous round follow-up:

• 5 verified fixed, replied and resolved: write_error safety-net for uncaught exceptions (thread), missing docstrings (thread), Dockerfile port assumption now documented (thread), missing db_log_path/client_log_path in the leak-check test (thread), os.access patch now scoped to _writable (thread).
• 1 partially fixed, replied, left open: the thread-leak fix for the DB probe (thread) — see New findings below.

SonarQube: Quality Gate passed (source: SonarCloud public API, confirmed fresh against head SHA 16cb210 — project_pull_requests/list shows an exact SHA match, analysis at 2026-10-22T22:32:31Z). All 5 conditions pass: New Reliability/Security/Maintainability Rating = A, New Duplicated Lines Density = 0.0%, New Security Hotspots Reviewed = 100%. 0 new issues, 0 hotspots. Nothing required to keep the gate green.

New findings: 1 Important, folded into the existing thread above rather than posted as a new comment since it's a direct evolution of that same finding (same lines, same method): the single-flight guard added this round (if _probe is not None and not _probe.done(): return False) returns database: "fail" for any overlapping request while a probe is in flight, not just a genuinely hung one. Two ordinary near-simultaneous polls (e.g. the Docker HEALTHCHECK and an external monitor) will intermittently report a healthy database as failed, since the second caller never sees the first probe's actual (successful) result. Traced directly in the diff — not speculative. Suggested fix posted in the thread: have the second caller await the same in-flight future instead of short-circuiting, which keeps the thread-leak cap while fixing the false negative.

Strengths: this is a well-layered fix — defense in depth with per-check try/except plus a generic write_error catch-all, the DB probe now actually touches the SQLite file (sqlite_master instead of bare SELECT 1), and the new tests are precise (forcing serve_traceback=True to prove the leak-proofing actually holds under the real production config, not just the test default).

🤖 Generated with Claude Code

[Tim020]

Round 2 (b5a2ee5)

• Single-flight false negative (Important): fixed. Overlapping polls now share the in-flight probe future (shielded) rather than reporting database: fail; a new probe starts only when none is running, keeping the one-thread cap. Each caller is still bounded by the 3s timeout. Tests cover concurrent polls over a slow healthy probe (all ok, one probe call), a hung probe (all callers fail within the timeout, no second probe), and recovery.
• External contract unchanged. pytest 1200 passed, ruff clean.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 3

Head SHA reviewed: d7a71dd82c9ed2498b1057bc41957f1579b3da6d

Verdict: ready to merge. All findings from rounds 1 and 2 are now fixed and verified; no new issues found.

Verification performed: checked out the actual head commit in the isolated worktree and ran it directly — ruff check/ruff format --check clean, test_health.py run three times in a row (15/15 pass every time, including the two new concurrency tests), and the full server suite (pytest -q) passes at 1200/1200.

Previous round follow-up: the one thread left open after round 2 — the DB-probe single-flight guard reporting a healthy database as failed under overlapping polls — is now fixed in commit b5a2ee5: overlapping callers asyncio.shield and join the same in-flight probe future instead of short-circuiting to failure. Verified both empirically (ran the new tests repeatedly) and by hand-tracing the asyncio semantics: the check-then-submit is atomic (no await in between, so no race between concurrent calls), asyncio.shield is load-bearing rather than cosmetic (without it, one caller's timeout could call .cancel() on a still-pending shared future, cancelling it for every other waiter), and multiple wrap_future calls on the same underlying future correctly deliver the same result/exception to every awaiting caller, not just the first. The one-thread leak cap from round 2 is preserved. Two negligible, non-blocking nits noted in the thread (small callback-object growth during a sustained hang, freed on completion; extra traceback frames in shared-exception log output) — neither worth a fix. All 6 threads from rounds 1-2 are now resolved.

SonarQube: Quality Gate passed (source: SonarCloud check-run output for head SHA d7a71dd). 0 new issues, 0 security hotspots, 0% duplication on new code. Nothing required to keep the gate green.

New findings: none. This round's change was small and scoped (one method + two tests); it was scrutinized directly rather than needing a fresh multi-angle sweep, and nothing new surfaced.

Strengths: the fix is precise — it solves exactly the regression flagged in round 2 without reintroducing the original unbounded thread-leak problem from round 1, and the two new tests (test_health_concurrent_polls_share_slow_healthy_probe, test_health_hung_probe_fails_all_callers_without_new_thread) directly exercise the specific race/sharing behavior being fixed using real concurrent requests via asyncio.gather, not just sequential calls.

🤖 Generated with Claude Code