Skip to content

test: harden nextest E2E isolation and database bootstrap - #1011

Open
hanakannzashi wants to merge 7 commits into
mainfrom
codex/stabilize-e2e-test-architecture
Open

test: harden nextest E2E isolation and database bootstrap#1011
hanakannzashi wants to merge 7 commits into
mainfrom
codex/stabilize-e2e-test-architecture

Conversation

@hanakannzashi

@hanakannzashi hanakannzashi commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR refactors the database-backed E2E test architecture around cargo-nextest to reduce intermittent CI failures such as the original failure.

The branch is based on main at 244e0210. The main changes are:

  • bootstrap and migrate the dedicated E2E database once per nextest run;
  • eliminate high-contention writes to shared model fixtures;
  • isolate tests that intentionally mutate catalog or database-global state;
  • reduce E2E concurrency and bound database/connection stalls;
  • replace timing, identity, and pagination assumptions with deterministic helpers.

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 with error communicating with server, while rerunning the same workflow passed.

The previous architecture amplified transient database failures:

  • nextest runs tests in separate processes, so a process-local OnceCell did not provide suite-wide initialization;
  • E2E processes repeatedly checked database creation and migrations;
  • each test created a pool while up to 16 database-backed tests ran concurrently;
  • every app instance started database-encryption recovery against the same queue;
  • many tests repeatedly PATCHed the same fixed model rows, pricing, and aliases;
  • some fixtures used fixed identities, timestamps, sleeps, or bounded list sizes that were unsafe on a reused database;
  • a pooled socket hard-closed through Docker's published PostgreSQL port could pass an earlier health check and then fail or stall.

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_limit timed out after 120 seconds;
  • the test had not reached the metadata-size assertion;
  • the trace stopped while preparing the shared UPDATE models ... in setup_qwen_model;
  • PostgreSQL service logs showed no restart, OOM, or max_connections incident.

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

  • Add a nextest setup script for e2e_all.
  • Reuse the already-built E2E binary instead of compiling and linking a separate bootstrap target.
  • Create the database, run migrations, and seed shared fixtures before nextest starts E2E processes.
  • Validate a bootstrap marker derived from the nextest run ID and database target.
  • Retain an in-process OnceCell plus PostgreSQL advisory-lock fallback for direct cargo test invocations.
  • Restore the deterministic mock user and clear abandoned database_encryption_jobs before the suite starts.

Immutable high-traffic model fixtures

  • Seed eight shared baseline models during bootstrap: standard/cache Qwen, privacy filter, GLM, DeepSeek, Qwen Omni, Qwen Image, and Qwen Reranker.
  • Make their setup helpers read-only; ordinary tests no longer PATCH those shared rows or wait a fixed 200 ms after each write.
  • Restore every pricing, context, verification, activity, free-usage, attestation, cache/text-pricing, and alias field on each bootstrap, so a reused test database cannot retain polluted fixture state.
  • Give high-context, alias, alias-consistency, cache-pricing, and admin-update scenarios distinct model names.
  • Register all test-specific model names with the mock inference provider.
  • Remove the model-cost serialization rule once those fixtures become immutable.

Bounded database pressure and stalls

  • Reduce the E2E database test group from 16 concurrent tests to 4.
  • Keep each ordinary test pool capped at four connections.
  • Verify a pooled connection before reuse.
  • Add test-only connection, pool checkout, creation, and recycle timeouts.
  • Enable test-only PostgreSQL statement, lock, and idle-transaction timeouts.
  • Enable TCP keepalive and a bounded TCP user timeout for the CI database path.
  • Continue serializing DDL, encryption, scheduler, and other database-global tests.

These connection settings apply only to E2E pools.

Other isolation fixes

  • Do not start database-encryption recovery workers when constructing E2E routers; production still starts them by default.
  • Use UUID-scoped organizations, workspaces, keys, mutable models, aliases, and provider registrations.
  • Generate UUID-based MockProvider response IDs.
  • Allocate non-overlapping provider-attribution analytics windows.
  • Replace key fixed sleeps with bounded polling.
  • Read complete paginated admin listings instead of assuming the database has fewer than 100 or 500 rows.
  • Make model-list assertions select the arranged fixture rather than whichever row sorts first.
  • Prevent loopback HTTP fixtures from inheriting host proxy settings.
  • Allow 500 ms for output-handle draining while still treating longer leaks as test failures.
  • Update the Make targets, CI comments, and test documentation for nextest.

Retry policy

There are no per-test, assertion, or nextest reruns.

Two bounded operation-level retries exist and are called out explicitly:

  • database bootstrap may retry transient initialization failures up to three times before any E2E test starts;
  • the idempotent organization concurrent-limit read and fixed-value update now use the repository's existing transient database retry policy.

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

  • Production database-encryption recovery remains enabled by default.
  • Production database pool settings are unchanged; the new connection and query timeouts are test-only.
  • Real provider response IDs and production proxy behavior are unchanged.
  • Lower E2E concurrency can increase wall-clock time in exchange for substantially lower connection churn and contention.
  • Bootstrap clears encryption jobs, restores the mock user, and resets shared model fixtures, so TEST_DATABASE_NAME must identify a dedicated test database.
  • TEST_DATABASE_NAME already existed on main; this PR does not introduce or rename it. Its existing fallback remains platform_api_e2e. EST_DATABASE_NAME is not a recognized variable.
  • The configured PostgreSQL user must be allowed to create the dedicated test database.
  • cargo-nextest 0.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:

  • affected shared-fixture tests: 110/110 passed in 21.416s;
  • full E2E suite: 722/722 passed, with 11 configured skips, in 143.066s;
  • reused-database bootstrap: 0.688s;
  • the updated model-list regression test passed independently;
  • unit tests: 1,464/1,464 passed, with 4 configured skips, in 38.924s;
  • cargo clippy --all-targets --all-features -- -D warnings;
  • formatting and diff whitespace checks.

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_all binary, observed setup time fell to 1.757s and 1.690s, approximately a 98.5% reduction.

On the pre-follow-up head:

  • all 722 E2E tests passed in 106.707s on the first CI attempt;
  • lint, unit, integration, cargo-deny, and cargo-audit jobs passed;
  • the deliberate second E2E attempt exposed the shared-model timeout described above.

Final CI for 63ff757d was then exercised twice on independent runners and PostgreSQL service containers:

  • attempt 1: 722/722 passed, 11 skipped; bootstrap 1.786s, nextest 151.305s, E2E job 4m13s;
  • deliberate attempt 2: 722/722 passed, 11 skipped; bootstrap 1.803s, nextest 149.008s, E2E job 3m59s;
  • lint, unit, integration, cargo-deny, cargo-audit, and aggregate Test Suite checks 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_NAME simultaneously. Normal CI jobs use isolated PostgreSQL service containers, so they do not share that database.

@ironloopai

ironloopai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details
  • Run: 16b7155c-33e9-46eb-852d-2e6a5bc1f206
  • Base: main at 244e021
  • Head: codex/stabilize-e2e-test-architecture at 1ec2e17
  • Created: 2026-09-03 10:52 UTC
  • Updated: 2026-09-03 11:16 UTC

Automatic trigger · attempt 1 of 3 · completed in 24m 12s

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review — test: harden nextest E2E isolation and database bootstrap

Solid direction: the shared-fixture races (fixed users/models/aliases, timestamp-hashed mock IDs, fixed sleeps) are the right things to attack, and keeping production defaults intact behind AppBuildOptions is the right seam. from_config_for_test is #[cfg(test)]-gated and build_app_with_config still delegates to the Default options, so no production behavior changes. No new logging touches customer data.

Three things I would fix before merge.


1. The setup script builds a test target that CI's build phase never builds — inside a 90s kill budget

.github/workflows/test.yml:202 runs cargo nextest run --test e2e_all. That target filter is forwarded to cargo test --no-run, so only e2e_all is built. The setup script then shells out to:

command = ["cargo", "test", "-p", "api", "--test", "e2e_db_bootstrap", ...]
slow-timeout = { period = "30s", terminate-after = 3 }   # -> 90s hard kill

which has to compile and link a brand-new test binary against api + services + database + axum/tokio. This repo's own comment says all e2e tests were merged into one binary specifically "for faster linking" — linking one more of these in debug is routinely 30–60s on its own. Worse, -p api narrows workspace member selection relative to the outer whole-workspace resolve, which can change feature unification and trigger a much larger rebuild under a separate fingerprint.

Failure mode: on the first run after merge (and any run where the cache key misses), the setup script is terminated at 90s and the entire e2e job fails before a single test runs. Locally this is masked because make test-integration uses --test '*', which builds both binaries up front.

Suggested fix — build it in the normal build phase and give the script headroom:

- run: cargo nextest run --test e2e_all --test e2e_db_bootstrap

(the bootstrap test is ignored, so nextest lists and skips it) plus something like slow-timeout = { period = "60s", terminate-after = 3 }. Dropping -p api so the inner invocation matches the outer resolve would also help.


2. list_all_admin_models / list_all_pricing_changes can livelock on exactly the databases they exist for

crates/api/tests/common/mod.rs:1237 and crates/api/tests/e2e_all/admin_pricing_changes.rs:79 both demand a globally consistent snapshot: every page must report the same total, and the assembled vector must contain exactly total distinct ids, or the whole read restarts.

With <=1000 rows only one request is issued per attempt, so stable is trivially true. Past 1000 rows — the reused long-lived local database that motivates the helper — each attempt issues >=2 requests, and with 8 concurrent nextest processes upserting models page.total changes between them. stable = false -> retry -> same race -> panic at "admin model listing should stabilize within 5 seconds".

The snapshot is not needed. Every call site is .iter().find(|m| m.model_id == model_name) or .any(...) against a name this test owns, and the two absence assertions (admin_list_models_after_soft_delete, deprecate_hides_old_from_admin_list_by_default) are also sound over a union of pages — a row concurrently added by another process cannot be the deleted model under test. Paging to the end and deduping by id, without the expected_total/stable equality gate, is both shorter and race-free.


3. backend_output_limits: the page size is boundary-tight against an alphabetically-last model

let list_path = format!("/v1/model/list?limit={}", openrouter_rows.len());

crates/api/tests/e2e_all/backend_output_limits.rs:198

The comment says "fetch the complete catalog," but this sets the page size to the exact count observed a moment earlier from a different endpoint. list_models orders by model_name ASC (crates/database/src/repositories/model.rs:1276) and one of the cases is z-ai/glm-5, which sorts at the very end. In practice both endpoints read the same in-process moka snapshot (300s TTL), so this usually holds — but any refresh between the two calls that admits one model sorting before z-ai/glm-5 drops it from the page and find_by_field panics. There is zero margin by construction.

/v1/model/list has no upper bound on limitcrates/api/src/routes/models.rs:65-75 only rejects negatives, and it returns total. Either pass a large constant or read total from the response and re-request; both are strictly safer than the current derivation.


Non-blocking

  • no_proxy() only reaches unit tests. from_config_for_test is cfg(test)-gated, so the services-crate unit tests get it, but the e2e FakeIta fixtures construct their client through the production from_config path and stay proxy-sensitive on a runner with HTTP_PROXY set. The stated motivation is not fully covered.
  • setup_alias_ita_server (crates/api/tests/e2e_all/ita_attestation.rs:215) copies the entire ItaAttestationConfig block from common/ita_server.rs, including a hardcoded max_retries: 0. Worth extracting a shared builder so the two cannot drift.
  • Hardcoded database_encryption_key. test_config() no longer reads DB_ENCRYPTION_KEY. Anyone who previously ran e2e with that variable set has rows in their reused TEST_DATABASE_NAME encrypted under the old key. Worth a README line next to the existing database_encryption_jobs-truncation note.
  • threads-required = "num-test-threads" vs. max-threads = 8. The serialized overrides request more threads than the e2e-db group cap. You report the full suite passing, so nextest is presumably clamping — flagging it so the interaction is deliberate rather than incidental if either number is tuned later.

Not verified by build: no target/ in this checkout, so this is a static read of the diff rather than a compile/run check.

⚠️

@hanakannzashi
hanakannzashi deployed to Cloud API test env September 3, 2026 10:58 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ 2 posted as inline comment(s)
  • 📝 0 posted as summary

// 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

Suggested change
if flags & 0x1 == 0 {
if (flags & 0x1) == 0 {

Comment on lines 240 to 242
Duration::from_millis(100),
Duration::from_secs(2),
Duration::from_secs(10),
Duration::from_secs(30),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread .config/nextest.toml Outdated
experimental = ["setup-scripts"]

[scripts.setup.e2e-db-bootstrap]
command = ["cargo", "test", "-p", "api", "--test", "e2e_db_bootstrap", "--", "--ignored", "--exact", "bootstrap_e2e_database", "--nocapture"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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.

Comment thread crates/api/tests/e2e_db_bootstrap.rs Outdated
);

let mut nextest_env = OpenOptions::new()
.write(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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).

@hanakannzashi
hanakannzashi deployed to Cloud API test env September 3, 2026 12:10 — with GitHub Actions Active
@hanakannzashi
hanakannzashi deployed to Cloud API test env September 3, 2026 12:14 — with GitHub Actions Active
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