From 1fe5ccdced841e22bb0f950d5303e03b6929df6f Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Thu, 27 Aug 2026 09:24:06 +0200 Subject: [PATCH] fix(tests): give the `_as` cleanup guards a runtime-local token client The teardown guard already knew it must not reuse the test's HTTP client - its connection-pool tasks belong to the test's runtime - and builds its own ApiService inside the teardown runtime for exactly that reason. What the module doc missed is that there are *two* clients. A DataHubConfig carries one of its own for the token endpoint, and the `_as` guards take a config from the test, so building an ApiService from it replaced the REST client and left the token one pointing at the test's runtime. That deadlocks rather than erroring, because the test's runtime is not busy but *blocked*: drop parks in thread::scope until teardown returns, so the one thread that could drive the connection is waiting for the thread waiting for it. A valid cached token hides it - get_api_token returns without touching the network - so it only surfaces once the token has been invalidated (a 401 clears it) or has expired. That is how one failing ACL test wedged the whole suite for 19 minutes with no output. Measured, driving the same teardown three ways: after the test runtime is dropped, ~6ms; while it is blocked, never returns; while it is blocked but with a runtime-local client, ~74ms. The regression test reproduces the blocked-runtime half faithfully (the drop runs on its own thread while the test blocks waiting for it) so a deadlock fails an assertion in 30s instead of stalling the suite silently. Verified both ways: it fails without the fix and passes with it. Co-Authored-By: Claude Opus 5 (1M context) --- src/tests.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 9 deletions(-) diff --git a/src/tests.rs b/src/tests.rs index e3a04c6..0e399ff 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -125,9 +125,23 @@ pub mod cleanup { //! Crucially, the teardown builds its **own** [`ApiService`](crate::ApiService) //! via [`create_api_service`] *inside* that fresh runtime rather than reusing //! the test's service. The test's `reqwest` client has connection-pool - //! background tasks bound to the test's runtime, which is being torn down - //! during the unwind; driving an HTTP request through it from another - //! runtime hangs or fails silently. A runtime-local client sidesteps that. + //! background tasks bound to the test's runtime; driving an HTTP request + //! through it from another runtime deadlocks. A runtime-local client sidesteps that. + //! + //! **There are two clients, and both have to be runtime-local.** A + //! [`DataHubConfig`] carries one of its own, used only to reach the token endpoint, and + //! the `_as` guards below take a config from the test. Building an `ApiService` from it + //! replaces the *REST* client but not that one, so a teardown that has to mint or refresh + //! a token still reaches through the test's runtime — and hangs. + //! + //! It hangs rather than erroring because the test's runtime is not merely busy, it is + //! *blocked*: `drop` parks in [`std::thread::scope`] until teardown returns, so the one + //! thread that could drive the connection is waiting for the thread that is waiting for + //! it. Measured: teardown after the test runtime is dropped succeeds in ~6ms; the same + //! teardown while it is blocked never returns; with a runtime-local client, ~74ms. + //! + //! A cached, unexpired token needs no network and hides this, which is why it only + //! surfaces when the token was invalidated (a 401 clears it) or has expired. use crate::datahub::DataHubConfig; use crate::events::EventIdCollection; @@ -242,22 +256,33 @@ pub mod cleanup { }) } + /// Re-point a config's token-endpoint client at the runtime that will actually use it. + /// + /// Everything else is shared, the auth state included, so a still-valid token is reused and + /// no redundant round-trip to the identity provider happens. See the module docs for what + /// goes wrong without this. + fn with_runtime_local_client(mut config: DataHubConfig) -> DataHubConfig { + config.http_client = reqwest::Client::new(); + config + } + /// Like [`cleanup_resources`], but deletes as the principal described by `config` /// instead of the `.env` identity. /// /// [`cleanup_resources`] builds its service with [`create_api_service`], which reads /// `.env` — right for the single-identity suites, wrong for a multi-tenant test where /// the data belongs to some other org and the default identity cannot even see it (a - /// cross-tenant delete is a 404, not an error worth reading). The config is cloned into - /// the closure and turned into a service on the teardown runtime, so the runtime-local - /// client property described in the module docs still holds. + /// cross-tenant delete is a 404, not an error worth reading). The config is cloned into the + /// closure and turned into a service on the teardown runtime, and its token-endpoint client + /// is replaced there too — see [`with_runtime_local_client`], without which a teardown that + /// has to refresh a token deadlocks against the blocked test runtime. pub fn cleanup_resources_as(config: DataHubConfig, external_ids: Vec) -> CleanupGuard { CleanupGuard::new(move || { Box::pin(async move { if external_ids.is_empty() { return; } - let api = ApiService::new(config); + let api = ApiService::new(with_runtime_local_client(config)); let ids: Vec = external_ids .iter() .map(|e| IdAndExtId::from_external_id(e)) @@ -280,7 +305,7 @@ pub mod cleanup { if external_ids.is_empty() { return; } - let api = ApiService::new(config); + let api = ApiService::new(with_runtime_local_client(config)); let ids: Vec = external_ids .iter() .map(|e| IdAndExtId::from_external_id(e)) @@ -304,7 +329,7 @@ pub mod cleanup { if external_ids.is_empty() { return; } - let api = ApiService::new(config); + let api = ApiService::new(with_runtime_local_client(config)); let ids: Vec = external_ids .iter() .map(|e| IdAndExtId::from_external_id(e)) @@ -539,6 +564,48 @@ pub mod cleanup { ); } } + #[cfg(test)] + mod tests { + use super::*; + use crate::tests::ids::unique_id; + use std::sync::mpsc; + use std::time::Duration; + + /// A `_as` guard must tear down even when the token cache is empty. + /// + /// This is the shape that deadlocked: a 401 clears the cache (see + /// `DataHubConfig::invalidate_token`), so teardown has to reach the token endpoint, and + /// it did so through the *test's* client while the test's runtime sat blocked waiting for + /// teardown to finish. Neither side could move. + /// + /// The drop runs on its own thread and the test blocks waiting for it, which reproduces + /// the blocked-runtime half faithfully while turning a hang into a failed assertion — + /// a deadlock would otherwise just stall the suite with no output. + #[tokio::test] + #[ignore] + async fn a_guard_tears_down_after_the_token_cache_was_cleared() { + // `create_default` reads process env; only `create_api_service` loads `.env`. + dotenv::dotenv().ok(); + let config = crate::datahub::DataHubConfig::create_default(); + config + .get_api_token() + .await + .expect("the .env principal should mint a token"); + // Exactly what a 401 does to the cache. + config.invalidate_token().await; + + let guard = cleanup_datasets_as(config, vec![unique_id("guard_probe")]); + + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + drop(guard); + let _ = tx.send(()); + }); + rx.recv_timeout(Duration::from_secs(30)) + .expect("teardown deadlocked: it needed a token and reached for the test's client"); + } + } + } #[test] fn test_to_snake_lower_cased_allow_start_with_digits() {