Skip to content

test(TC-1317): read-audit review follow-ups (F1-F4) - #361

Merged
samgbafa merged 2 commits into
mainfrom
skgbafa/tc-1317-read-audit-followups
Oct 19, 2026
Merged

samgbafa merged 2 commits into
mainfrom
skgbafa/tc-1317-read-audit-followups

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Follow-ups from the TC-1317 review of #350 (Linear TC-1317 comment cdeb26c7). Stacked on #350; merge after it. No production behaviour changes except two new Prometheus series.

F1. Denials stay denials on every reclassified route. reclassified_auth_routes_answer_busy_and_denials_stay_denials now sends the stranger's kv/get to /signed/kv and to /encryption/networks (audience: the node), not only to /invoke, while the read audit is held. Each must answer 401/403. Mutation check: making either route's _ fallback answer 503 read_audit_busy now fails the test (/signed/kv: a denial must not turn into busy: 503 …, and the same for /encryption/networks). Before this change the full node lib passed under both mutants.

F2. Dead-drain visibility.

  • New Prometheus series: tinycloud_read_audit_stopped_total (reads that failed because the drain was gone) and the gauge tinycloud_read_audit_drain_stopped (1 once the drain is gone). Both come from the existing ReadAuditHealth.stopped / drain_stopped fields, updated on each sampler tick in observe_read_audit.
  • New test tc1317_a_dead_drain_is_counted_and_reported. On a pipeline whose drain is gone, the watchdog reports a stalled drain on its first look, before any read. Reads answer Stopped and are counted (stopped = 2, busy = 0). The warning is rate-limited and repeats after the interval while the drain stays dead. Mutation check: dropping drain_stopped from the watchdog's dead-drain condition fails the test.

F3. Wording. Since #350, a batch's in-flight time includes the SQLite writer-lock wait. So Read audit batch slow and its max_commit_ms measure that wait as well as the commit, on SQLite only. The CHANGELOG and the SLOW_BATCH / slow_batches doc comments now say so. Field names are unchanged.

F4. The deadline is reported against the read audit, and the reply carries Retry-After.

  • a_deadline_before_the_audit_wait_answers_request_timeout attaches the Retry-After fairing and asserts 503, Retry-After: 1 and the request_timeout body.
  • The stage is checked in core by tc1317_a_read_waiting_on_its_audit_is_in_the_read_audit_stage. A read runs under a RequestCancel context while the drain is held. The test asserts context.stage() == ReadAudit while the read waits, and Other after it commits. A deadline that cuts the wait logs that same stage.
  • Mutation check: setting the stage to Other in enqueue fails the core test.
  • Review B1 (TC-1317 comment 0728254b): the first version captured the log line instead. That depended on test order, because tracing-core caches one call-site interest per process, and the cache is computed from whichever thread's dispatcher reaches the call site first. Log capture has been removed.

Verification (local, native PG 18, blackhole)

  • cargo fmt --check.
  • Clippy -Dwarnings, workspace-wide and with duckdb for node and core.
  • core lib with duckdb: 537 passed; core default: no failures.
  • node lib: 542 passed. The reviewer's repros pass: a_deadline_before_the_audit_wait invoke_deadline --test-threads 8 10/10 (2 tests per run), and routes::tests --test-threads 64 3/3 (111 tests per run).
  • CI on bd46936: rust 39147605767 and logship 39147608247 are green.
  • core read_audit request_cancel --test-threads 64: 10/10. core lib: 492 passed in 3 runs. One earlier run failed wal_reader_progresses_while_audit_waits_for_writer (TC-273, with a 100 ms read timeout) at load average 62.
  • The three node duckdb CI targets pass.
  • Smoke: observe_read_audit with stopped = 3, drain_stopped = true exports tinycloud_read_audit_stopped_total 3 and tinycloud_read_audit_drain_stopped 1.

Not included: N5 (re-pin log-shipper/canonical.json after #147 and #154 land).

- F1: the stranger's read is denied (401/403) on /signed/kv and
  /encryption/networks while the read audit is held, not only on /invoke.
- F2: export tinycloud_read_audit_stopped_total and the
  tinycloud_read_audit_drain_stopped gauge; test that a dead drain is
  counted and reported as a stalled drain.
- F3: the slow-batch time includes the SQLite writer-lock wait; say so.
- F4: the deadline test checks Retry-After: 1 and stage=read_audit.
@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-1317 (review of head fd8c7854cbea462f70ff921959700fdf6a11db48, on #350 a811986). One blocker: F4's log assertion depends on test order (it fails deterministically with routes::tests --test-threads 64); a verified fix is in the comment. F1-F3 approved. Comment: https://linear.app/tinycloud-labs/issue/TC-1317#comment-0728254b

The route test's log capture depended on test order: tracing-core caches
one call-site interest per process, computed from whichever thread's
dispatcher reaches the call site first. Assert RequestCancel::stage() in
a core test instead; the route test keeps the 503, Retry-After: 1 and the
request_timeout body.
@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-1317 (re-review of head bd46936bc77d439301aaa34dd40ec8e8ec7dd145, on #350 a811986). B1 resolved; F1-F3 unchanged. Comment: https://linear.app/tinycloud-labs/issue/TC-1317#comment-536d402e

@samgbafa
samgbafa changed the base branch from skgbafa/tc-1317-read-audit-bound to main October 19, 2026 11:12
@samgbafa
samgbafa merged commit 9ac91bc into main Oct 19, 2026
17 checks passed
@samgbafa samgbafa mentioned this pull request Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant