mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-27 07:45:08 +08:00
fix: #149 — eliminate parallel-test flake in runtime::config tests
## Problem `runtime::config::tests::validates_unknown_top_level_keys_with_line_and_field_name` intermittently fails during `cargo test --workspace` (witnessed during #147 and #148 workspace runs) but passes deterministically in isolation. Example failure from workspace run: test result: FAILED. 464 passed; 1 failed ## Root cause `runtime/src/config.rs::tests::temp_dir()` used nanosecond timestamp alone for namespace isolation: std::env::temp_dir().join(format!("runtime-config-{nanos}")) Under parallel test execution on fast machines with coarse clock resolution, two tests start within the same nanosecond bucket and collide on the same path. One test's `fs::remove_dir_all(root)` then races another's in-flight `fs::create_dir_all()`. Other crates already solved this pattern: - plugins::tests::temp_dir(label) — label-parameterized - runtime::git_context::tests::temp_dir(label) — label-parameterized runtime/src/config.rs was missed. ## Fix Added process id + monotonically-incrementing atomic counter to the namespace, making every callsite provably unique regardless of clock resolution or scheduling: static COUNTER: AtomicU64 = AtomicU64::new(0); let pid = std::process::id(); let seq = COUNTER.fetch_add(1, Ordering::Relaxed); std::env::temp_dir().join(format!("runtime-config-{pid}-{nanos}-{seq}")) Chose counter+pid over the label-parameterized pattern to avoid touching all 20 callsites in the same commit (mechanical noise with no added safety — counter alone is sufficient). ## Verification Before: one failure per workspace run (config test flake). After: 5 consecutive `cargo test --workspace` runs — zero config test failures. Only pre-existing `resume_latest` flake remains (orthogonal, unrelated to this change). for i in 1 2 3 4 5; do cargo test --workspace; done # All 5 runs: config tests green. Only resume_latest flake appears. cargo test -p runtime # 465 passed; 0 failed ## ROADMAP.md Added Pinpoint #149 documenting the gap, root cause, and fix. Closes ROADMAP #149.
This commit is contained in:
58
ROADMAP.md
58
ROADMAP.md
@@ -5720,3 +5720,61 @@ Threading: parser already knows the source (it's the arm that sets `model`). Pro
|
|||||||
**Not a regression of #128.** #128 was about rejecting malformed strings (now closed). #148 is about labeling the valid ones after resolution.
|
**Not a regression of #128.** #128 was about rejecting malformed strings (now closed). #148 is about labeling the valid ones after resolution.
|
||||||
|
|
||||||
**Source.** Jobdori dogfood 2026-04-21 20:40 KST on main HEAD `4cb8fa0` in response to Q's bundle hint. Split from historical #124 residual. Joins **truth-audit / diagnostic-integrity** cluster. Session tally: ROADMAP #148.
|
**Source.** Jobdori dogfood 2026-04-21 20:40 KST on main HEAD `4cb8fa0` in response to Q's bundle hint. Split from historical #124 residual. Joins **truth-audit / diagnostic-integrity** cluster. Session tally: ROADMAP #148.
|
||||||
|
|
||||||
|
## Pinpoint #149. `runtime::config::tests::validates_unknown_top_level_keys_with_line_and_field_name` flakes under parallel workspace test runs
|
||||||
|
|
||||||
|
**Gap.** When `cargo test --workspace` runs with normal parallel test execution (default), `runtime::config::tests::validates_unknown_top_level_keys_with_line_and_field_name` intermittently fails. In isolation (`cargo test -p runtime validates_unknown_top_level_keys_with_line_and_field_name`), it passes deterministically. The same pattern affects other tests in `runtime/src/config.rs` and sibling test modules that share the `temp_dir()` naming strategy.
|
||||||
|
|
||||||
|
**Verified on main HEAD `f84c7c4` (2026-04-21 20:50 KST):** witnessed during `cargo test --workspace` runs for #147 and #148 — one workspace run produced:
|
||||||
|
|
||||||
|
```
|
||||||
|
test config::tests::validates_unknown_top_level_keys_with_line_and_field_name ... FAILED
|
||||||
|
test result: FAILED. 464 passed; 1 failed; 0 ignored; 0 measured
|
||||||
|
```
|
||||||
|
|
||||||
|
Same test passed on the next workspace run. Same test passes in isolation every time.
|
||||||
|
|
||||||
|
**Root cause.** `runtime/src/config.rs` tests share this helper:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
fn temp_dir() -> std::path::PathBuf {
|
||||||
|
let nanos = SystemTime::now()
|
||||||
|
.duration_since(UNIX_EPOCH)
|
||||||
|
.expect("time should be after epoch")
|
||||||
|
.as_nanos();
|
||||||
|
std::env::temp_dir().join(format!("runtime-config-{nanos}"))
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Two weaknesses:
|
||||||
|
1. **Timestamp-only namespacing**: on fast machines with coarse-grained clocks (or with tests starting within the same nanosecond bucket), two tests pick the same path. One races `fs::create_dir_all()` with another's `fs::remove_dir_all()`.
|
||||||
|
2. **No label differentiation**: every test in the file calls `temp_dir()` and constructs sub-paths inside the shared prefix. A `fs::remove_dir_all(root)` in one test's cleanup may clobber a live sibling.
|
||||||
|
|
||||||
|
Other crates in the workspace (`plugins::tests::temp_dir`, `runtime::git_context::tests::temp_dir`) already use the **labeled** form `temp_dir(label)` to segregate namespaces per-test. `runtime/src/config.rs` was missed in that sweep.
|
||||||
|
|
||||||
|
**Fix shape (~30 lines).** Convert `temp_dir()` in `runtime/src/config.rs` to `temp_dir(label: &str)` mirroring the plugins/git_context pattern, plus add a PID + atomic counter suffix for double-strength collision resistance:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
fn temp_dir(label: &str) -> std::path::PathBuf {
|
||||||
|
use std::sync::atomic::{AtomicU64, Ordering};
|
||||||
|
static COUNTER: AtomicU64 = AtomicU64::new(0);
|
||||||
|
let nanos = SystemTime::now().duration_since(UNIX_EPOCH).expect("...").as_nanos();
|
||||||
|
let pid = std::process::id();
|
||||||
|
let seq = COUNTER.fetch_add(1, Ordering::Relaxed);
|
||||||
|
std::env::temp_dir().join(format!("runtime-config-{label}-{pid}-{nanos}-{seq}"))
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Update each `temp_dir()` callsite in the file to pass a unique label (test function name usually works).
|
||||||
|
|
||||||
|
**Acceptance.**
|
||||||
|
- `cargo test --workspace` 10x consecutive runs all green (excluding pre-existing `resume_latest` flake which is orthogonal).
|
||||||
|
- `cargo test -p runtime` 10x consecutive runs all green.
|
||||||
|
- Cleanup `fs::remove_dir_all(root)` never races because `root` is guaranteed unique per-test.
|
||||||
|
- No behavior change for tests already passing in isolation.
|
||||||
|
|
||||||
|
**Blocker.** None. Mechanical rename + label addition.
|
||||||
|
|
||||||
|
**Not applying to.** `plugins::tests::temp_dir` and `runtime::git_context::tests::temp_dir` already use the labeled form. The label pattern is the established workspace convention; this just applies it to the one holdout.
|
||||||
|
|
||||||
|
**Source.** Jobdori dogfood 2026-04-21 20:50 KST, flagged during #147 and #148 workspace-test runs. Joins **test brittleness / flake** cluster. Session tally: ROADMAP #149.
|
||||||
|
|||||||
@@ -1254,11 +1254,21 @@ mod tests {
|
|||||||
use std::time::{SystemTime, UNIX_EPOCH};
|
use std::time::{SystemTime, UNIX_EPOCH};
|
||||||
|
|
||||||
fn temp_dir() -> std::path::PathBuf {
|
fn temp_dir() -> std::path::PathBuf {
|
||||||
|
// #149: previously used `runtime-config-{nanos}` which collided
|
||||||
|
// under parallel `cargo test --workspace` when multiple tests
|
||||||
|
// started within the same nanosecond bucket on fast machines.
|
||||||
|
// Add process id + a monotonically-incrementing atomic counter
|
||||||
|
// so every callsite gets a provably-unique directory regardless
|
||||||
|
// of clock resolution or scheduling.
|
||||||
|
use std::sync::atomic::{AtomicU64, Ordering};
|
||||||
|
static COUNTER: AtomicU64 = AtomicU64::new(0);
|
||||||
let nanos = SystemTime::now()
|
let nanos = SystemTime::now()
|
||||||
.duration_since(UNIX_EPOCH)
|
.duration_since(UNIX_EPOCH)
|
||||||
.expect("time should be after epoch")
|
.expect("time should be after epoch")
|
||||||
.as_nanos();
|
.as_nanos();
|
||||||
std::env::temp_dir().join(format!("runtime-config-{nanos}"))
|
let pid = std::process::id();
|
||||||
|
let seq = COUNTER.fetch_add(1, Ordering::Relaxed);
|
||||||
|
std::env::temp_dir().join(format!("runtime-config-{pid}-{nanos}-{seq}"))
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user