Repository navigation
Conversation
…C harness Integration tests leaked every request's AppState: actix-web recycles test-built HttpRequestInner allocations into a per-request pool that the test path never drains, pinning database pools until process exit. Pooled ODBC connections keep cached prepared statements alive, which deadlocks Oracle's client at process exit. Close every test pool (clearing statement caches first, since close() alone leaves checked-out connections behind) while the test runtime is still alive, including after panics. Consolidate the suite around shared helpers (read_body_string, supports_database, request builders, OIDC login/config builders, multipart/webhook request builders) and share one AppState per test where possible, cutting duplication across oidc/uploads/requests/ data_formats/errors/server_timing/cookies/core. Net negative LOC.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10c33b6131
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Give test requests an Actix service-owned pool so dropping the service releases application state. Centralize request setup with response_for, response_with, and helpers for custom requests and shared state, while keeping standard actix_web tests. Cover state release on success, errors, and unwinding. Validate with the full Rust suite, all-feature Clippy, formatting, and the SQLite ODBC prepared-statement test.
Use create_app inside the shared request helper so existing concise test calls exercise production routes and middleware. Update the prefix redirect assertion to check the actual 308 response and preserved request path.
lovasoa
enabled auto-merge
October 7, 2026 10:25
* refactor: centralize variable lookup and enumeration * Reuse request and immutable-variable fixtures for lookup coverage
lovasoa
added this pull request to the merge queue
Oct 7, 2026
Any commits made after this event will not be merged.
lovasoa
enabled auto-merge
October 7, 2026 11:31
lovasoa
added this pull request to the merge queue
Oct 7, 2026
Any commits made after this event will not be merged.
github-merge-queue
Bot
removed this pull request from the merge queue due to invalid changes in the merge commit
Oct 7, 2026
lovasoa
added this pull request to the merge queue
Oct 7, 2026
Any commits made after this event will not be merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Cleans up the integration test suite while fixing the Oracle CI exit hang at its root cause. Net -39 LOC (696+/735-).
Why the Oracle job hangs (root cause)
TestRequest::to_srv_request()leaks itsData<AppState>: actix-web recycles theHttpRequestInnerallocation into a per-request pool on drop, and that pool is only drained by a real server — never on the test path. The leakedAppStatepins its database pool (and pooled connections) until process exit. Pure actix-web behavior; reproduced standalone (every built request leaks itsapp_data, 10/10).finiSqora → bccFreeProcesswhen statements are outstanding at exit (verified: 1 leaked statement exits, 2 hang) — after all 89 tests pass.pool.close()alone is insufficient: it only closes idle connections, so a still-checked-out connection keeps its cached statements alive (verified: close-only hangs 2/2). The teardown acquires every connection, clears its statement cache, then closes — while the runtime is alive, including after panics.What changed
tests/common: newTestSystemrunner (pool registry + deterministic drain + panic safety + unit test), plus shared helpers (read_body_string,supports_database,make_app_data_with_env, unified request building).AppStaterun onTestSystemand funnel creation through the registry (including OIDC/core directAppState::initsites).test_config(no more hand-written JSON triplicates) + shared login-flow helpers.AppStateper test where trivially safe (fewer pools, fewer inits).--test-threads=2. CONTRIBUTING documents the pattern.Validation
cargo fmt,cargo clippy --all-targets --all-features -- -D warnings: clean.- -test-threads=2): 88 passed with clean exit, 3/3 runs (previously hung every run: all tests passed, then deadlock inbccFreeProcess).Follow-up for upstream: actix-web issue about test-path
HttpRequestPoolrecycling (draft ready); if fixed there, the per-test drain could eventually go away.