Repository navigation
test(cli,db): freeze the assertion test clock and give pool release tests a test-lane acquire budget - #9442
Conversation
`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]>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes and approves.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :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.
* 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
…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]>
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 teststest(db): give the pool release Postgres tests a test-lane acquire budgetRoot cause and fix per test
federated_identity::tests::short_and_long_assertions_refresh_at_the_earlier_pointand its twin indesktop_provider(crates/buzz-cli): failed in CI withassertion left == right failed: lifetime 300, left 2, right 1, atfederated_identity.rs:634.cfg(test),cached_assertion::unix_now()was the wall clock plus a thread-local skew. The mock stampsexp = unix_now() + lifetime, thenCachedAssertion::getcomputesrefresh_at(unix_now(), exp)after the HTTP round trip, and the next call checksunix_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.FROZENbase besideSKEW, andtest_clock::freeze(at)to pin it. Undercfg(test),unix_now()returnsFROZEN.unwrap_or(real_now) + SKEW, soadvanceand its existing callers inclient.rsbehave as before. Both tests freeze the clock at the top of their loop body. The mock adapter runs on the same current-thread runtime, soexp,refresh_atand the check share one "now". Production code paths are unchanged.federated_identity: 4 failures before, 0 after.desktop_provider: 1 failure before, 0 after.desktop_providertwin. 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 atpool_release_postgres_tests.rs:204:10withcalled `Result::unwrap()` on an `Err` value: PoolTimedOutafter 0.85 s.Db::read_pool_options, whose 150 msREADER_ACQUIRE_TIMEOUTalso boundedconnect_with's eager first dial (andfresh_query's). On a busy runner the cold dial throughPausingProxy, SCRAM and the readerafter_connecthook overran it, so the test panicked at the initialconnect_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.TEST_LANE_ACQUIRE_TIMEOUTon both pool builders in this file: the reader test'sPoolOptions, andwriter_pool'sDbConfig, 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_connectandafter_releasestay as in production, and recovery still completes at sqlx's 5 s return deadline inside the test's 10 sawait_capacitybudget. No assertions change.pool_release_postgres_tests --ignored --test-threads=6per binary. Before, 2 of 10 rounds failed at:204:10withPoolTimedOut, 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.reader_pool_postgres_tests.rsalready carries its own 5 s test budget onread_pool_optionsfor 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-checkand git-lfs.Did not run:
rust-testsanddesktop-tauri-checkspre-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.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