Skip to content

Read audit: bound the batch commit and discard a stalled connection - #448

Merged
samgbafa merged 4 commits into
mainfrom
tc-2035-read-audit-timeout
Oct 26, 2026
Merged

samgbafa merged 4 commits into
mainfrom
tc-2035-read-audit-timeout

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

A read-audit batch commit now has a client-side bound of 10 s (below the 20 s read wait). A commit that does not finish in that time is failed as the existing retryable unavailable_retryable (503) class, the drain continues with the next batch, and, through the bounded release ping below, the connection the commit held is closed and its pool slot freed instead of being held.

What changes

  • read_audit.rs: commit_batch wraps the batch commit (connection acquire included) in tokio::time::timeout; the failed-batch path is shared with other batch failures. A timed-out batch is not requeued; its reads already got the retryable error. No read's data is returned without a committed record. Abandoned records queued behind the batch are skipped when the next batch forms, as before.
  • vendor/sqlx-core (documented in PATCH.md): the ping that tests a released pool connection is bounded (named const, 5 s, per-pool release_ping_timeout option). sqlx returns a dropped statement's connection to the pool through that ping, and the ping waits for the abandoned statement's reply, so a stalled connection, its release task and its pool slot were held with no bound. Past the bound the connection is closed hard (itself bounded) and its permit released; the pool opens a replacement. This is pool-wide: any released connection whose ping does not answer in 5 s is closed instead of held. db_pool.rs and Cargo.lock are unchanged.
  • Observability: the timeout is logged as a warning with error_class = "read_audit_batch_timeout" (batch size, elapsed ms, timeout ms; counts and durations only) and counted in tinycloud_read_audit_batch_timeout_total. The aggregate Read audit batch failed warning also carries this class. The timeout warning is logged with its own error_class; log-shipper classification follows.
  • The stalled-drain warning (batch in flight over 30 s) fires only if the 10 s bound itself fails to end a batch, for example a wait on the SQLite writer lock, which the bound does not cover.
  • The 10 s bound is a constant, overridable only in tests.

Idempotence: the batch is one INSERT ... ON CONFLICT DO NOTHING statement on keys fixed by the batch, so running it again after it committed writes nothing. If a timed-out commit did land on the server, its rows are extra audit rows and a client retry of the same record leaves no duplicates and no gaps.

Tests (new only)

  • a commit that never returns: the batch's read gets the retryable error within the bound and a later read commits;
  • records queued behind the hung batch are skipped after the timeout;
  • a TCP server that completes startup and never answers backs a one-connection PostgreSQL pool: the batch fails within the commit bound, then the socket is closed and the pool permit freed within the commit bound plus the ping bound, the next acquire opens a new connection, and the drain keeps going;
  • a commit that landed but whose result never arrived, then the same records re-enqueued: identical rows, no duplicates, no gaps (SQLite, and postgres_read_audit_timed_out_commit_is_idempotent_on_retry on the PostgreSQL lane).

Details: TC-2035

dryrun added 3 commits October 25, 2026 21:19
A batch commit now has a 10 s bound (below the 20 s read wait). When it
does not finish, the batch is failed as the existing retryable
unavailable_retryable (503) class, the drain continues with the next batch,
and the connection the commit held is closed and its pool slot freed
instead of being released to the pool (sqlx pings a released connection,
and the ping waits for the abandoned statement's reply).

RecoveringConnection::execute_idempotent, whose only caller is the
read-audit batch, acquires the PostgreSQL pool connection itself and
detaches it if its future is dropped before the result arrives. Retry on
a lost connection is unchanged.

The timeout is logged as a warning with error_class
read_audit_batch_timeout (batch size, elapsed and timeout in ms) and
counted in tinycloud_read_audit_batch_timeout_total.

Details: TC-2035
…itly

Revert RecoveringConnection::execute_idempotent to its original form (the
explicit acquire bypassed sea-orm's metric callback). The held pool slot is
fixed at its root in the vendored sqlx-core: the release ping is bounded by
a named timeout (5 s, per-pool option) and the connection is closed hard,
releasing its permit, when it does not answer.

Details: TC-2035
@samgbafa
samgbafa marked this pull request as ready for review October 26, 2026 10:19
@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-2035 (reviewed exact head 52bbee1)
Reviewed: the commit bound wraps the whole batch commit; on timeout every waiter of that batch gets the retryable unavailable answer (never a confirmation), the warn line and counter carry counts and durations only, the drain continues; the release-ping bound in the vendored pool is a named constant with a per-pool override and a PATCH.md entry; the existing statement-count test passes unedited. CI green on this head (13 pass, 0 fail). Behind main 0 at review time; it needs one more merge from main after the next release lands. A security read of the vendored pool change (Opus) follows after release.

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-2035 (reviewed exact head f4a23eb)
This head is a plain merge of main e47537d (release 1.25.3) into the approved head 52bbee1 (CI green there); the non-CHANGELOG patch-id is unchanged (384f969b...), behind main 0.

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