Skip to content

fix(test): close remaining non-hermetic OODA decide-cap tests under SIMARD_SCALING=auto (#4571) - #4575

Merged
rysweet merged 2 commits into
mainfrom
feat/issue-4571-diagnose-and-fix-the-deterministic-unit-test-failu
Jul 24, 2026
Merged

fix(test): close remaining non-hermetic OODA decide-cap tests under SIMARD_SCALING=auto (#4571)#4575
rysweet merged 2 commits into
mainfrom
feat/issue-4571-diagnose-and-fix-the-deterministic-unit-test-failu

Conversation

@rysweet

@rysweet rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

Complements #4529 in fully de-flaking the self-deploy unit-test gate, which runs the whole cargo test suite with SIMARD_SCALING=auto set in the deploy environment (src/self_relaunch/gates.rs::run_unit_test_gate).

Background / root cause

OodaConfig::default() reads SIMARD_SCALING; when auto, it installs an AIMD scaler seeded from the ambient max. Tests that build a config via ..OodaConfig::default() while overriding max_concurrent_actions inadvertently inherit that wide scaler, and decide honors scaler.adjust() over the overridden field — so cap assertions fail deterministically. The gate's truncate_output(stderr, 200) only captured cargo's opening Running unittests src/lib.rs ⠋ line, which mis-pointed diagnosis at a lib test; the real failures were in the integration suite.

#4529 already fixed tests/ooda.rs, tests/recipe_ooda_step_parity.rs, and the adaptive_scaling test via scaler: None. This PR (rebased on that work) closes the two remaining gaps:

Changes (net vs main — 2 files)

  1. tests/ooda_daemon.rsrun_ooda_daemon_with_session_uses_session_for_advance_goal used the same non-hermetic max_concurrent_actions: 2, ..Default::default() pattern (untouched by fix(meeting-backend): make resolve_agent_workdir tests checkout-independent (#4505) #4529). Pins scaler: None so the per-cycle cap is env-independent, preventing over-dispatch and future flakiness under SIMARD_SCALING=auto.

  2. tests/adaptive_scaling.rsscaler_current_max_can_override_config was fixed in fix(meeting-backend): make resolve_agent_workdir tests checkout-independent (#4505) #4529 with scaler: None, but that makes the test — named for scaler override — no longer exercise any override (no scaler present). This restores its named purpose: it now installs a deterministically-pinned scaler (floor == ceiling == 2, so adjust() is always ≤ 2 under any pressure) and sets a higher config max (8), proving the scaler overrides max_concurrent_actions (issue Raise Simard's per-OODA-cycle goal-coverage parallelism ceiling from the current ~6 to 24, and make it env-configurable (it is an arbitrary low cap). CONTEXT: src/ooda_loop/cycle.rs:316 computes `cov #2935) — deterministically.

No production code changes. Test-only, additive, non-breaking.

Verification

  • Both changed tests pass under hostile env SIMARD_SCALING=auto SIMARD_MAX_CONCURRENT_ACTIONS=10 (post-rebase).
  • adaptive_scaling override test: 5/5 repeated runs green.
  • cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and pre-push suite (478 tests, 0 failed) all green.

Refs #4571, #4529

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…uto (#4571)

The self-deploy `unit-test` gate runs the full `cargo test` suite with
`SIMARD_SCALING=auto` set in the deploy environment. Four integration
tests built their `OodaConfig` via `..OodaConfig::default()` while
overriding `max_concurrent_actions`. Because `OodaConfig::default()`
reads `SIMARD_SCALING` and, when `auto`, installs an AIMD scaler seeded
from the *ambient* max, the struct-update leaked a wide scaler that
`decide` honors instead of the overridden field — so the cap assertions
saw more actions than configured and failed deterministically, red-caring
the deploy gate for ~7 hours (target a0173ba).

The gate's `truncate_output(stderr, 200)` only captured cargo's opening
"Running unittests src/lib.rs" progress line, which mis-pointed diagnosis
at a lib test; the real failures were in the integration suite.

Fix (mirrors the issue #2732 hermetic pattern already applied to the lib
`decide` tests, which this change extends to the integration tests that
were missed):
- decide-cap tests (`ooda.rs`, `recipe_ooda_step_parity.rs`,
  `ooda_daemon.rs`) pin `scaler: None` so the explicit
  `max_concurrent_actions` is the sole cap, independent of host env.
- `adaptive_scaling::scaler_current_max_can_override_config` now installs
  a deterministically-pinned scaler (floor == ceiling == 2) and sets a
  *higher* config max, so it actually proves the scaler overrides the
  config value — deterministically under any system pressure.

No production code changes; `decide` behavior is correct. Test-only,
additive, non-breaking. Verified: all four tests pass under
`SIMARD_SCALING=auto SIMARD_MAX_CONCURRENT_ACTIONS=10`; full suite shows
0 failures; fmt + clippy clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rysweet
rysweet force-pushed the feat/issue-4571-diagnose-and-fix-the-deterministic-unit-test-failu branch from d1c8739 to f1475a6 Compare July 24, 2026 20:50
@rysweet rysweet changed the title fix(test): make OODA decide-cap tests hermetic under SIMARD_SCALING=auto (#4571) fix(test): close remaining non-hermetic OODA decide-cap tests under SIMARD_SCALING=auto (#4571) Jul 24, 2026
…loy gate

The `unit-test` deploy gate failed deterministically under full-suite load
on `install::paths::tests::install_lock_is_exclusive_per_simard_home`,
red-caring self-deploy (8-commit drift). The captured gate output truncated
at the spinner frame "⠋ Drop t...", pointing at a Drop/teardown test.

Root cause: `InstallLock` released its `LOCK_EX | LOCK_NB` advisory lock only
implicitly, when the backing `File` closed. `flock` locks live on the open
file description, so any sibling test that `fork`/`exec`s while the guard is
held inherits the same locked description. The kernel keeps the lock until
*every* inherited descriptor is closed, which outlasts the guard by the
child's brief fork→exec window. Under `cargo test` CPU saturation that window
overlapped the test's post-drop re-acquire, which then got EWOULDBLOCK — a
deterministic failure at load, invisible in isolation.

Fix: add `impl Drop for InstallLock` (unix) that issues an explicit
`flock(fd, LOCK_UN)` before the file closes. `LOCK_UN` drops the single shared
lock on the open file description immediately, regardless of any inherited
descriptors in concurrently forked children, so a subsequent acquire never
races the exec window. Additive, non-breaking; production release semantics
are strictly more correct (explicit unlock, then close).

Verified: `cargo test --lib` passes 0-failure across 3 consecutive full-suite
runs under load; the target test previously failed deterministically.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rysweet

rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Added: fix the genuine lib-unittest failure behind the ⠋ Drop t... truncation

While the PR body assumed the gate's Running unittests src/lib.rs ⠋ truncation mis-pointed diagnosis at a lib test, reproduction under full-suite load shows there was a real, deterministic cargo test --lib failure — and the truncated Drop t... suffix named it exactly:

thread 'install::paths::tests::install_lock_is_exclusive_per_simard_home' panicked at src/install/paths.rs:342:
lock should be available after guard drop: ... could not be acquired: Resource temporarily unavailable (os error 11)

Root cause

InstallLock released its flock(LOCK_EX|LOCK_NB) guard only implicitly, when the backing File closed. flock locks live on the open file description, so any sibling test that fork/execs while the guard is held inherits the same locked description. The kernel keeps the lock until every inherited descriptor closes — which outlasts the guard by the child's brief fork→exec window (O_CLOEXEC only fires at exec). Under cargo test CPU saturation that window overlapped the test's post-drop re-acquire, yielding a deterministic EWOULDBLOCK. Passes in isolation, fails under load — matching the gate's behavior. The existing serial(install) contract only serializes install tests against each other, so it can't cover a leak from any other forking test in the 9k-test binary.

Fix (production, 1 file)

Add impl Drop for InstallLock (unix) that issues an explicit flock(fd, LOCK_UN) before the file closes. LOCK_UN drops the single shared lock on the open file description immediately, regardless of inherited descriptors in concurrently forked children. Additive, non-breaking; release semantics are strictly more correct (explicit unlock, then close).

Verification

  • cargo test --lib: 0 failures across 3 consecutive full-suite runs under load (target test previously failed deterministically at load).
  • cargo fmt --check, cargo clippy --release -D warnings, and the pre-push gates (clippy --all-targets --all-features --locked -D warnings + release test subset) all green.

Together with the OODA decide-cap test fixes above, this closes both the lib-unittest and integration-suite paths of the #4571 deploy-gate failure.

@github-actions

Copy link
Copy Markdown

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module Lines Covered Coverage
Total 196518 165239 84.1%

Coverage data from CI run. Test files matching tests?/ are excluded from line counts.

@rysweet rysweet added the simard-autonomous Simard-authored PR eligible for gated autonomous self-merge label Jul 24, 2026
@rysweet
rysweet merged commit 24d05c0 into main Jul 24, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

simard-autonomous Simard-authored PR eligible for gated autonomous self-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant