test: harden nextest E2E isolation and database bootstrap - #1011
test: harden nextest E2E isolation and database bootstrap#1011hanakannzashi wants to merge 7 commits into
Conversation
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 24m 12s |
Review —
|
| // ACK first so the fixture behaves like a live HTTP/2 peer. | ||
| // In particular, a PING can arrive while the initial request | ||
| // is still active if the test runner is heavily loaded. | ||
| if flags & 0x1 == 0 { |
There was a problem hiding this comment.
The expression flags & 0x1 == 0 relies on Rust's operator precedence where & binds tighter than ==, which is the opposite of C and is a common readability pitfall. While it is functionally correct here, adding parentheses makes the intent unambiguous and avoids confusion for readers familiar with C precedence rules. Clippy's precedence lint also flags this pattern.
Suggestion:
| if flags & 0x1 == 0 { | |
| if (flags & 0x1) == 0 { |
| Duration::from_millis(100), | ||
| Duration::from_secs(2), | ||
| Duration::from_secs(10), | ||
| Duration::from_secs(30), |
There was a problem hiding this comment.
The second duration argument to apply_with was changed from Duration::from_secs(2) to Duration::from_secs(10). This parameter is h2_timeout — the time the client waits for a PING ACK before declaring the connection dead. With a 100ms PING interval and a 10s timeout, the client will now make up to ~100 PING attempts (each silently retransmitting) before timing out. The original 2s timeout (~20 attempts) was already generous for a 100ms interval. While the server fixture now does ACK PINGs so a timeout is unlikely, the 10s value should ideally be closer to the production default (H2_KEEPALIVE_TIMEOUT = 10s) only if the test's PING interval matched production. Consider documenting why the timeout was increased or reducing it to remain proportional to the 100ms test interval, so the test does not mask regressions where the client fails to detect a dead connection promptly.
There was a problem hiding this comment.
Review · Summary
Found two CI-blocking issues in the new E2E bootstrap flow.
Findings: 🟠 Medium 1 · 🟡 Low 1
Code-specific findings are attached to the diff.
Validation
- ✅ E2E test target compilation — The consolidated E2E test target compiles successfully.
- ❌ Bootstrap target lint — Strict Clippy reproduces the ineffective OpenOptions warning in the new bootstrap target.
- ❌ Captured E2E check — The E2E run stopped in its setup script, so the suite did not complete.
Review details
- Run:
16b7155c-33e9-46eb-852d-2e6a5bc1f206 - Attempts: 1
| experimental = ["setup-scripts"] | ||
|
|
||
| [scripts.setup.e2e-db-bootstrap] | ||
| command = ["cargo", "test", "-p", "api", "--test", "e2e_db_bootstrap", "--", "--ignored", "--exact", "bootstrap_e2e_database", "--nocapture"] |
There was a problem hiding this comment.
🟠 Medium · Restore the E2E bootstrap setup script
The captured E2E job aborts with nextest’s setup-script failure status before any E2E test runs. This newly added command is the setup rule for e2e_all, so the required E2E check remains red until the bootstrap invocation succeeds.
| ); | ||
|
|
||
| let mut nextest_env = OpenOptions::new() | ||
| .write(true) |
There was a problem hiding this comment.
🟡 Low · Remove the redundant write mode
.append(true) already enables writing. Strict Clippy denies this ineffective option, causing the required lint job to fail; remove .write(true).
Summary
This PR refactors the database-backed E2E test architecture around
cargo-nextestto reduce intermittent CI failures such as the original failure.The branch is based on
mainat244e0210. The main changes are:This does not enable nextest retries, rerun failed tests, or relax assertions.
What was flaky
The original CI failure occurred in
admin_pricing_changes::test_cancel_pending_change: an API request returned HTTP 500 witherror communicating with server, while rerunning the same workflow passed.The previous architecture amplified transient database failures:
OnceCelldid not provide suite-wide initialization;Deliberate rerun finding
After the first green CI pass, the E2E job was deliberately rerun to test repeatability.
That second attempt caught another real failure:
message_metadata::test_input_message_metadata_size_limittimed out after 120 seconds;UPDATE models ...insetup_qwen_model;max_connectionsincident.This did not prove a PostgreSQL server failure. It showed that the suite-wide model PATCH was unnecessary shared work and a database/connection hotspot. The follow-up removes those writes from ordinary test setup instead of retrying the whole test.
Architecture changes
Once-per-run database bootstrap
e2e_all.OnceCellplus PostgreSQL advisory-lock fallback for directcargo testinvocations.database_encryption_jobsbefore the suite starts.Immutable high-traffic model fixtures
Bounded database pressure and stalls
These connection settings apply only to E2E pools.
Other isolation fixes
Retry policy
There are no per-test, assertion, or nextest reruns.
Two bounded operation-level retries exist and are called out explicitly:
The latter is the only production runtime behavior change. It retries only transaction-conflict, connection, or pool failures, for at most three attempts with 100 ms and 200 ms backoff. The normal success path still performs one attempt, and semantic errors are not retried.
Compatibility and side effects
TEST_DATABASE_NAMEmust identify a dedicated test database.TEST_DATABASE_NAMEalready existed onmain; this PR does not introduce or rename it. Its existing fallback remainsplatform_api_e2e.EST_DATABASE_NAMEis not a recognized variable.cargo-nextest0.9.98 or newer is required.Local validation
Post-follow-up validation against an isolated PostgreSQL 15 instance, including repeated use of the same database:
cargo clippy --all-targets --all-features -- -D warnings;CI observations
The bootstrap initially took 116.646s on a cold runner because setup triggered additional compile/link work. After moving bootstrap into the already-built
e2e_allbinary, observed setup time fell to 1.757s and 1.690s, approximately a 98.5% reduction.On the pre-follow-up head:
Final CI for
63ff757dwas then exercised twice on independent runners and PostgreSQL service containers:Test Suitechecks passed.This is evidence that the reproduced shared-fixture failure mode has been removed; it is not a claim that any distributed CI suite can be made mathematically flake-free.
Remaining boundary
Two independent local nextest runs should not target the same
TEST_DATABASE_NAMEsimultaneously. Normal CI jobs use isolated PostgreSQL service containers, so they do not share that database.