From 37223100134f9b86657dcf5a06be5fe3ff320d80 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Wed, 15 Apr 2026 12:57:15 +0800 Subject: [PATCH] fix(tests): #34 race-free parallel cli tests via ctor + #[serial] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tests in \`crates/agentkeys-cli/tests/cli_tests.rs\` mutated process-global env vars (\`HOME\`, \`AGENTKEYS_SESSION_STORE\`) mid-test. Under the default parallel test runner this raced — one test's \`set_var\` could stomp on another test's observed value. Our CI hid this by passing \`--test-threads=1\` everywhere. Drops that requirement. **\`AGENTKEYS_SESSION_STORE=file\` — \`#[ctor]\` pre-main hook** Every test wants this set so \`session_store::save_session\` uses the file path instead of the OS keychain. A single \`#[ctor::ctor] fn\` now runs during dylib init, before any test thread starts. No race window where one test could hit the real keychain while another is still inside a \`set_var\` call. **\`HOME\` — \`#[serial]\` on the 4 tests that mutate it** The self-revoke / by-wallet revoke tests point HOME at their own tempdir to sandbox \`session_store::fallback_path()\`. Each picks a different tempdir. Annotated with \`serial_test::serial\` so they don't race each other — non-HOME tests still run fully parallel. **New dev-deps** - \`serial_test = \"3\"\` - \`ctor = \"0.2\"\` Test: \`cargo test -p agentkeys-cli\` → 37 passed in 0.85s, no \`--test-threads=1\` required. Closes #34. --- Cargo.lock | 66 ++++++++++++++++++++++++- crates/agentkeys-cli/Cargo.toml | 7 +++ crates/agentkeys-cli/tests/cli_tests.rs | 27 ++++++---- 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49cf2ed..c090fa2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -24,11 +24,13 @@ dependencies = [ "assert_cmd", "axum", "clap", + "ctor", "predicates", "reqwest", "rusqlite", "serde", "serde_json", + "serial_test", "tempfile", "tokio", ] @@ -726,6 +728,16 @@ dependencies = [ "typenum", ] +[[package]] +name = "ctor" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a2785755761f3ddc1492979ce1e48d2c00d09311c39e4466429188f3dd6501" +dependencies = [ + "quote", + "syn 2.0.117", +] + [[package]] name = "curve25519-dalek" version = "4.1.3" @@ -1011,6 +1023,17 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +[[package]] +name = "futures-executor" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baf29c38818342a3b26b5b923639e7b1f4a61fc5e76102d4b1981c6dc7a7579d" +dependencies = [ + "futures-core", + "futures-task", + "futures-util", +] + [[package]] name = "futures-io" version = "0.3.32" @@ -2250,7 +2273,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -2311,6 +2334,15 @@ version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f" +[[package]] +name = "scc" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46e6f046b7fef48e2660c57ed794263155d713de679057f2d0c169bfc6e756cc" +dependencies = [ + "sdd", +] + [[package]] name = "schannel" version = "0.1.29" @@ -2326,6 +2358,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "sdd" +version = "3.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" + [[package]] name = "secret-service" version = "3.1.0" @@ -2464,6 +2502,32 @@ dependencies = [ "serde", ] +[[package]] +name = "serial_test" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "911bd979bf1070a3f3aa7b691a3b3e9968f339ceeec89e08c280a8a22207a32f" +dependencies = [ + "futures-executor", + "futures-util", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0a7d91949b85b0d2fb687445e448b40d322b6b3e4af6b44a29b21d9a5f33e6d9" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] + [[package]] name = "sha1" version = "0.10.6" diff --git a/crates/agentkeys-cli/Cargo.toml b/crates/agentkeys-cli/Cargo.toml index a797554..fab669c 100644 --- a/crates/agentkeys-cli/Cargo.toml +++ b/crates/agentkeys-cli/Cargo.toml @@ -32,3 +32,10 @@ axum = { version = "0.7", features = ["json"] } rusqlite = { version = "0.31", features = ["bundled"] } serde_json = { workspace = true } tempfile = "3" +# Serialize tests that mutate process-global env (HOME). See issue #34 +# and the comment on `init_test_env` in `tests/cli_tests.rs`. +serial_test = "3" +# Run `AGENTKEYS_SESSION_STORE=file` as a `#[ctor]` pre-main hook so every +# test binary starts with the env var set — no race window where an early +# test hits the real keychain before the OnceLock runs (issue #34). +ctor = "0.2" diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index 626c940..aec2569 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -8,14 +8,25 @@ use agentkeys_cli::session_store; use agentkeys_core::backend::CredentialBackend; use agentkeys_mock_server::test_client::InProcessBackend; use agentkeys_types::Session; +use serial_test::serial; fn create_test_backend() -> Arc { Arc::new(InProcessBackend::new()) } +/// Set `AGENTKEYS_SESSION_STORE=file` BEFORE the test binary's main runs. +/// `#[ctor::ctor]` runs this function during dylib/init, well before any +/// test thread starts, so no race window exists where one test could hit +/// `cmd_init` → `save_session` → real keychain while another test is +/// still trying to set the env var. This replaces an earlier `OnceLock` +/// approach that still had a narrow startup race (issue #34). +#[ctor::ctor] +fn init_test_env() { + unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } +} + /// Initialize a session via the in-process backend and return both wallet and session. async fn init_session_direct(backend: &Arc) -> (String, Session) { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let ctx = CommandContext::new("unused", false, false) .with_backend(backend.clone() as Arc); let (output, session) = cmd_init(&ctx, Some("test-token-unique".to_string())) @@ -115,8 +126,8 @@ async fn cli_revoke_then_read() { // Test: cmd_revoke_self_clears_local_session #[tokio::test(flavor = "multi_thread")] +#[serial] async fn cmd_revoke_self_clears_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let temp_dir = tempfile::tempdir().unwrap(); let temp_home = temp_dir.path().to_str().unwrap().to_string(); @@ -183,8 +194,8 @@ async fn cmd_revoke_with_agent_calls_revoke_by_wallet() { // be wiped (same as the no-arg self-revoke form), so subsequent commands // don't load a stale revoked token. #[tokio::test(flavor = "multi_thread")] +#[serial] async fn cmd_revoke_with_own_wallet_clears_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let temp_dir = tempfile::tempdir().unwrap(); let temp_home = temp_dir.path().to_str().unwrap().to_string(); @@ -229,8 +240,8 @@ async fn cmd_revoke_with_own_wallet_clears_local_session() { // Counterpart to the above: revoking SOMEONE ELSE's wallet must NOT touch // the caller's local session file. #[tokio::test(flavor = "multi_thread")] +#[serial] async fn cmd_revoke_with_other_wallet_keeps_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let temp_dir = tempfile::tempdir().unwrap(); let temp_home = temp_dir.path().to_str().unwrap().to_string(); @@ -270,8 +281,8 @@ async fn cmd_revoke_with_other_wallet_keeps_local_session() { // Test: cmd_revoke_no_session_errors_cleanly #[tokio::test(flavor = "multi_thread")] +#[serial] async fn cmd_revoke_no_session_errors_cleanly() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let temp_dir = tempfile::tempdir().unwrap(); let temp_home = temp_dir.path().to_str().unwrap().to_string(); @@ -345,7 +356,6 @@ async fn cli_link_alias() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let bare_ctx = CommandContext::new(&base_url, false, false); let (output, session) = cmd_init(&bare_ctx, Some("test-token-unique".to_string())) .await @@ -658,7 +668,6 @@ async fn cmd_store_resolves_alias() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let bare_ctx = CommandContext::new(&base_url, false, false); let (output, session) = cmd_init(&bare_ctx, Some("test-token-alias".to_string())).await.unwrap(); let wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); @@ -693,7 +702,6 @@ async fn cmd_read_unknown_identity_errors_cleanly() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let bare_ctx = CommandContext::new(&base_url, false, false); let (_output, session) = cmd_init(&bare_ctx, Some("test-token-unknown".to_string())).await.unwrap(); @@ -730,12 +738,11 @@ async fn start_scope_test_server() -> (String, String, String) { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } let bare_ctx = CommandContext::new(&base_url, false, false); let (output, _session) = cmd_init(&bare_ctx, Some("scope-test-unique".to_string())) .await .unwrap(); - let master_wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); + let _master_wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); // Create a child session with initial scope [a, b] let http_client = reqwest::Client::new();