From 8bd9a5ff141d11628e7ae3b1f16036d058f82cfb Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Fri, 8 May 2026 20:19:42 +0200 Subject: [PATCH] tests: matrix harness uses with_defaults() workload, not from_env() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 CI failure: Test Workspace and server-aws both red on `concurrent_branch_ops_morphological_matrix` cell b ("merge × merge: same-target-distinct-sources") — second merge returned 429 instead of 200. The matrix passes locally. Root cause: cargo test runs tests in parallel by default. The admission test `ingest_per_actor_admission_cap_returns_429` is wrapped with `#[serial]` and an EnvGuard that sets `OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1` for its duration. Process-wide env vars are visible to concurrently-running tests; the matrix's `Harness::new()` called `AppState::open()` which delegates to `WorkloadController::from_env()`, picking up cap=1 if it ran while the admission test held the EnvGuard. With cap=1 + 2 concurrent merges in cell b, one merge waits behind merge_exclusive while the other is admitted; the waiter holds its admission permit, but a fresh actor permit is needed when admission is per-actor — the second merge's permit acquisition fails because the first hasn't released yet, and 429 fires. Fix (correct by design, AGENTS.md rule 9): the matrix harness builds the WorkloadController explicitly via `WorkloadController::with_defaults()` and passes it to `AppState::new_with_workload`, the constructor added in commit 22d76db. Closes the bug class "tests pick up another concurrent test's env override at construction time" — the matrix is now insulated from any env-var manipulation in the rest of the test suite. Verified locally: with `OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1` set in the environment, the matrix passes (it ignores env entirely now). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/tests/server.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 598bc95..8db2d08 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -2436,9 +2436,27 @@ mod matrix { pub async fn new() -> Self { let temp = init_loaded_repo().await; let repo = repo_path(temp.path()); - let state = AppState::open(repo.to_string_lossy().to_string()) - .await - .unwrap(); + // Build the WorkloadController explicitly with defaults rather + // than letting `AppState::open` call + // `WorkloadController::from_env()`. The admission-gate test + // (`ingest_per_actor_admission_cap_returns_429`) sets + // OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1 inside an EnvGuard while + // it runs. Process-wide env vars are visible to + // concurrently-running tests; if a matrix cell reads env at + // AppState construction time during that window it picks up + // cap=1 and the second concurrent merge in cell b surfaces + // 429 instead of the expected 200. Constructing the + // controller here with explicit defaults makes cells + // independent of any env mutation other tests perform. + let db = Omnigraph::open(repo.to_str().unwrap()).await.unwrap(); + let workload = + omnigraph_server::workload::WorkloadController::with_defaults(); + let state = AppState::new_with_workload( + repo.to_string_lossy().to_string(), + db, + Vec::new(), + workload, + ); let app = build_app(state); Self { _temp: temp,