Skip to content

test(cli,db): freeze the assertion test clock and give pool release tests a test-lane acquire budget - #9442

Merged
matt2e merged 2 commits into
mainfrom
mtoohey/flake-rust-test-clock-and-pool
Nov 14, 2026
Merged

matt2e merged 2 commits into
mainfrom
mtoohey/flake-rust-test-clock-and-pool

Conversation

@matt2e

@matt2e matt2e commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Two Rust tests failed on loaded CI runners because their timing budget came from the wall clock or from a production timeout, not from the behavior they assert. Each fix changes only test code or cfg(test) code.

Commits (cherry-picked unchanged from a local flake-fix series):

  • test(cli): freeze the test clock in the assertion refresh tests
  • test(db): give the pool release Postgres tests a test-lane acquire budget

Root cause and fix per test

federated_identity::tests::short_and_long_assertions_refresh_at_the_earlier_point and its twin in desktop_provider (crates/buzz-cli): failed in CI with assertion left == right failed: lifetime 300, left 2, right 1, at federated_identity.rs:634.

  • CI sighting, 11 November 2026, "Rust / Unit Tests", run 43122921918 attempt 1 (on fix(relay): treat empty filter sets as match-nothing in SQL #9342; the rerun passed):
  • Root cause: under cfg(test), cached_assertion::unix_now() was the wall clock plus a thread-local skew. The mock stamps exp = unix_now() + lifetime, then CachedAssertion::get computes refresh_at(unix_now(), exp) after the HTTP round trip, and the next call checks unix_now() < refresh. The test needed those reads to agree to the second. When the wall clock crossed a second boundary between them, the refresh point moved 1 s earlier, advance(refresh - 1) landed exactly on it, and the assertion was minted one step early.
  • Fix: a thread-local FROZEN base beside SKEW, and test_clock::freeze(at) to pin it. Under cfg(test), unix_now() returns FROZEN.unwrap_or(real_now) + SKEW, so advance and its existing callers in client.rs behave as before. Both tests freeze the clock at the top of their loop body. The mock adapter runs on the same current-thread runtime, so exp, refresh_at and the check share one "now". Production code paths are unchanged.
  • Verification: each test run 1,440 times, 24 in parallel, before and after. federated_identity: 4 failures before, 0 after. desktop_provider: 1 failure before, 0 after.
  • Not seen in CI: the desktop_provider twin. It passed in the job above and is fixed here because it has the same race and failed locally.

runtime::postgres_tests::pool_release_postgres_tests::silence_after_release_hook_does_not_strand_sqlx_ping (crates/buzz-db): panicked at pool_release_postgres_tests.rs:204:10 with called `Result::unwrap()` on an `Err` value: PoolTimedOut after 0.85 s.

  • CI sighting, 10 November 2026, "PostgreSQL Domain / PostgreSQL Tests", run 42851548618 attempt 1 (on buzz-db: pin explicit FK-child cleanup and same-workflow fire links #8317; the rerun passed):
  • Root cause: the test built its pool from the production Db::read_pool_options, whose 150 ms READER_ACQUIRE_TIMEOUT also bounded connect_with's eager first dial (and fresh_query's). On a busy runner the cold dial through PausingProxy, SCRAM and the reader after_connect hook overran it, so the test panicked at the initial connect_with(...).unwrap() before the release-hook recovery path it asserts ever ran. Production is not exposed: the relay dials the reader pool lazily, so that budget only governs request-time acquires, which fall back to the writer by design.
  • Fix: a 30 s TEST_LANE_ACQUIRE_TIMEOUT on both pool builders in this file: the reader test's PoolOptions, and writer_pool's DbConfig, whose eagerly dialed 3 s default is the same exposure test: fix timing-dependent unit tests in buzz-cli, buzz-agent and buzz-admin #9273 fixed for buzz-admin. test_before_acquire(false), after_connect and after_release stay as in production, and recovery still completes at sqlx's 5 s return deadline inside the test's 10 s await_capacity budget. No assertions change.
  • Verification, against a local Postgres: 10 rounds of pool_release_postgres_tests --ignored --test-threads=6 per binary. Before, 2 of 10 rounds failed at :204:10 with PoolTimedOut, the CI signature. After, 0 of 10. Under 28 CPU busy loops, with before and after runs interleaved: 6 of 10 failed before, 0 of 10 after.
  • Not done here: reader_pool_postgres_tests.rs already carries its own 5 s test budget on read_pool_options for the same reason. This PR does not unify the two constants.

One sighting of each failure was found in the CI runs from 7 to 12 November 2026 that were searched.

Related issue

N/A (flake fixes). No existing issue was found, and no open PR touches these tests or the four files changed here.

Testing

Human testing is still required. This PR does not carry buzz-review-completed; the author has not yet run these tests themselves on this branch.

The repetition counts above were measured when the fixes were written, before this branch was split out. They were not repeated on this branch.

Local checks on this branch

The branch is one commit behind main; that commit changes only mobile and docs files, none of them touched here.

Ran and passed at push time, on a checkout of this branch's head (42b3faccc): the organization push check, branch-skew, push-head-scope, file-size-check and git-lfs.

Did not run:

  • The rust-tests and desktop-tauri-checks pre-push lanes. They were excluded from the push (LEFTHOOK_EXCLUDE) because the machine ran out of disk part-way through them on an earlier attempt. That attempt did not complete, so it proves nothing either way about this branch.
  • The desktop and mobile pre-push lanes (desktop-check, desktop-typecheck, desktop-test, mobile-checks). This diff touches no desktop or mobile file.
  • cargo fmt --check, cargo clippy, just ci, and the PostgreSQL lane.

CI's Unit Tests and PostgreSQL Tests jobs are the gate for this branch. A green run does not prove the flakes are gone: both failures need a loaded runner.

🤖 Generated with Claude Code

matt2e and others added 2 commits November 13, 2026 13:07
`cached_assertion::unix_now()` under `cfg(test)` was the wall clock plus a
thread-local skew, so `short_and_long_assertions_refresh_at_the_earlier_point`
(in both `federated_identity` and `desktop_provider`) needed two reads of it
to agree to the second: the mock stamps `exp = unix_now() + lifetime`, then
`CachedAssertion::get` computes `refresh_at(unix_now(), exp)` after the HTTP
round trip and the next call checks `unix_now() < refresh`. When the wall
clock crossed a second boundary in between, the refresh point moved 1 s
earlier and `advance(refresh - 1)` landed exactly on it, re-minting one step
early (`left: 2, right: 1`).

Add a thread-local `FROZEN` base beside `SKEW` and a `test_clock::freeze(at)`
that pins it; `unix_now()` under `cfg(test)` returns
`FROZEN.unwrap_or(real_now) + SKEW`, so `advance` and its existing callers in
`client.rs` are unchanged. Both tests freeze the clock at the top of their
loop body; the mock adapter is spawned on the same current-thread runtime, so
`exp`, `refresh_at`, and the check share one "now".

Verified by running each test 1,440 times (24 in parallel) before and after:
federated_identity 4 -> 0 failures, desktop_provider 1 -> 0 failures.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Signed-off-by: mtoohey <[email protected]>
Signed-off-by: Matt Toohey <[email protected]>
…dget

`silence_after_release_hook_does_not_strand_sqlx_ping` built its pool from
the production `Db::read_pool_options`, whose 150 ms `READER_ACQUIRE_TIMEOUT`
also bounded `connect_with`'s eager first dial (and `fresh_query`'s). On a
busy runner the cold dial through `PausingProxy`, SCRAM, and the reader
`after_connect` hook overran it, and the test panicked at the initial
`connect_with(...).unwrap()` with `PoolTimedOut` before the release-hook
recovery path it asserts ever ran. Production is unaffected: the relay
dials the reader pool lazily, so that budget only governs request-time
acquires that fall back to the writer by design.

Set a 30 s test-lane acquire timeout on both pool builders in this file:
the reader test's `PoolOptions`, and `writer_pool`'s `DbConfig`, whose
eagerly dialed 3 s default is the same exposure #9273 fixed for
buzz-admin. `test_before_acquire(false)`, `after_connect`, and
`after_release` stay as in production, and recovery still completes at
sqlx's 5 s return deadline inside the 10 s `await_capacity` budget.

Verified by repetition against a local Postgres, 10 rounds of
`pool_release_postgres_tests --ignored --test-threads=6` per binary:
before, 2/10 rounds failed at `:204:10 PoolTimedOut`; after, 0/10. Under
28 CPU busy-loops, interleaved before/after pairs: before 6/10 failed,
after 0/10.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Signed-off-by: mtoohey <[email protected]>
Signed-off-by: Matt Toohey <[email protected]>
@matt2e
matt2e requested a review from a team as a code owner November 13, 2026 03:57
@matt2e
matt2e deployed to codex-review November 13, 2026 03:57 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 13, 2026
@github-actions

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 3c0adc188206bab8551a7af53dcf6bd54c9d0d09...42b3faccc5feb4b2862fc1c984caf5492db8ef5c
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete findings in the authorized PR range. Changes are confined to test clock control and test database acquisition timeouts; production behavior is unchanged.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and history inspection. Builds and tests were not executed, as instructed.

Generated by Codex Security Review |
Requested by: @matt2e |
Workflow run

@kazemek21 kazemek21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kyle reviewed this PR and the Nexus review-tracker notes and approves.

Posted by Codex (gpt-6.1-sol) on behalf of kylez

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 :bot: Jude’s code review agent — APPROVE at 42b3faccc5feb4b2862fc1c984caf5492db8ef5c (live base 3c0adc188206bab8551a7af53dcf6bd54c9d0d09; PR merge-base bf3bdd0eaefe39289bf9620594f254c7f68897f5). Clean exact-head checkout; four changed files in the PR diff. Low runtime risk: all changes are test-only or cfg(test).

Integrated review: The frozen assertion clock in crates/buzz-cli/src/cached_assertion.rs:81–108 is consumed by the two current-thread Tokio tests (federated_identity.rs:620–639, desktop_provider.rs:1698–1735); their spawned mock server tasks share that runtime. A serial libtest probe on the pinned toolchain showed distinct OS test threads with thread-local state reset across tests, addressing the potential cross-test freeze leak. The Postgres fixture override (crates/buzz-db/src/runtime/tests/pool_release_postgres_tests.rs:6–30,195–217) raises only eager test-pool acquisition to 30 s. Production's lazy reader remains at 150 ms (runtime/mod.rs:1027,1083–1136); recovery assertions retain their independent 10 s capacity and 5 s fresh-query deadlines (pool_release_postgres_tests.rs:44–72). No blocking defect found in either independent review lane.

Validation at this head: cargo test -p buzz-cli 690 passed/0 failed; cargo test -p buzz-db 215 passed/0 failed, 761 Postgres-dependent ignored; cargo fmt --all -- --check, just file-size-check, python3 scripts/check-postgres-test-discovery.py crates/buzz-db (126 Rust files), and git diff --check passed. Live exact-head GitHub checks Rust / Unit Tests, Rust / Rust Lint and PostgreSQL Domain / PostgreSQL Tests succeeded. The repository-wide discovery wrapper timed out locally after four fixture validations; it is not counted as passed.

Author action: none for this code review. Verification ownership / residual risk: CI owns the exact-head Postgres lane; local reviewers lacked a running Postgres/Docker and did not reproduce loaded-runner behavior, so one green CI run is not proof of flake elimination. The PR body records human testing outstanding under AGENTS.md; the human author owns that pre-merge step or an explicit permitted skip. No native/user-visible journey is changed. A new head requires fresh review.

@matt2e
matt2e merged commit 97188e9 into main Nov 14, 2026
90 checks passed
@matt2e
matt2e deleted the mtoohey/flake-rust-test-clock-and-pool branch November 14, 2026 00:45
wpfleger96 added a commit that referenced this pull request Nov 14, 2026
* origin/main: (24 commits)
  docs(readme): point desktop readers at block/buzz-app and describe where each client lives (#9493)
  test(cli,db): freeze the assertion test clock and give pool release tests a test-lane acquire budget (#9442)
  test(desktop): unmount every TerminalSubstrate tree before the window closes (#9440)
  fix(mobile): drop the iOS share extension from the build until it's signed (#9489)
  chore(release): release Buzz Desktop version 0.5.30 (#9491)
  perf(mobile): parse each event's thread reference once (#9460)
  perf(mobile): load only the directory when search opens (#9466)
  perf(mobile): stop rebuilding Home's channel list when a route covers it (#9459)
  perf(mobile): ask typing subscriptions only for the typing window (#9454)
  perf(mobile): skip avatar URL parsing for non-HTTP URLs (#9448)
  perf(mobile): download the identity archive list only when it changed (#9445)
  perf(mobile): don't treat a Control Center pull as a resume (#9443)
  fix(mobile): keep a superseded roster read out of the offline snapshot (#9452)
  perf(mobile): read channel bot roles from the member list (#9451)
  perf(mobile): read Activity once per reconnect (#9455)
  fix(mobile): offer role controls for agents (#9475)
  perf(mobile): stop rebuilding the native nav bar on every scrolled frame (#9458)
  perf(mobile): keep verified-name batches open as people are added (#9450)
  perf(mobile): read only changed custom emoji sets on reconnect (#9446)
  perf(mobile): keep latest-message times on a directory load (#9457)
  ...

Signed-off-by: Will Pfleger <[email protected]>

# Conflicts:
#	crates/buzz-cli/src/desktop_provider.rs
bostonaholic added a commit that referenced this pull request Nov 14, 2026
…eaction-recipients

* origin/main: (111 commits)
  perf(acp): avoid repeated observer batch serialization (#9483)
  docs(readme): point desktop readers at block/buzz-app and describe where each client lives (#9493)
  test(cli,db): freeze the assertion test clock and give pool release tests a test-lane acquire budget (#9442)
  test(desktop): unmount every TerminalSubstrate tree before the window closes (#9440)
  fix(mobile): drop the iOS share extension from the build until it's signed (#9489)
  chore(release): release Buzz Desktop version 0.5.30 (#9491)
  perf(mobile): parse each event's thread reference once (#9460)
  perf(mobile): load only the directory when search opens (#9466)
  perf(mobile): stop rebuilding Home's channel list when a route covers it (#9459)
  perf(mobile): ask typing subscriptions only for the typing window (#9454)
  perf(mobile): skip avatar URL parsing for non-HTTP URLs (#9448)
  perf(mobile): download the identity archive list only when it changed (#9445)
  perf(mobile): don't treat a Control Center pull as a resume (#9443)
  fix(mobile): keep a superseded roster read out of the offline snapshot (#9452)
  perf(mobile): read channel bot roles from the member list (#9451)
  perf(mobile): read Activity once per reconnect (#9455)
  fix(mobile): offer role controls for agents (#9475)
  perf(mobile): stop rebuilding the native nav bar on every scrolled frame (#9458)
  perf(mobile): keep verified-name batches open as people are added (#9450)
  perf(mobile): read only changed custom emoji sets on reconnect (#9446)
  ...

Signed-off-by: Matthew Boston <[email protected]>

This branch was successfully deployed

1 active deployment
codex-review — 42b3facc Deployed Nov 13, 2026 by matt2e via Run Codex Security Review #14822
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants