diff --git a/.gitignore b/.gitignore index 74365c534..557223cf7 100644 --- a/.gitignore +++ b/.gitignore @@ -38,3 +38,4 @@ scripts/dashboard-audit/out/ pr*-worktree/ *-worktree/ .simard/ +gym_history.db diff --git a/Cargo.lock b/Cargo.lock index 181215c37..2835dd75c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3883,7 +3883,7 @@ dependencies = [ [[package]] name = "simard" -version = "0.35.0" +version = "0.36.0" dependencies = [ "amplihack-agent-eval", "amplihack-memory", diff --git a/Cargo.toml b/Cargo.toml index a807622a7..689564263 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "simard" -version = "0.35.0" +version = "0.36.0" edition = "2024" default-run = "simard" diff --git a/docs/concepts/reconcile-and-self-deploy.md b/docs/concepts/reconcile-and-self-deploy.md index 3bf125a30..0d5e89f38 100644 --- a/docs/concepts/reconcile-and-self-deploy.md +++ b/docs/concepts/reconcile-and-self-deploy.md @@ -215,7 +215,7 @@ of the following hold: | **Memory intact** | cognitive-memory fact count ≥ the pre-deploy count (within tolerance), via the `CognitiveMemoryOps` count API | | **Goal board intact** | the goal board loads and the active-goal count is preserved | | **Brains LLM-backed** | zero `BrainJudgmentRecord.fallback == true` records over a probe cycle (see [parse-failure record](../reference/ooda-brain-parse-failure-record.md)) | -| **No quarantine** | no *fresh* corrupt-store quarantine appeared in the live cognitive-store directory since the deploy window opened (retained historical forensic snapshots are ignored) | +| **No quarantine** | no *fresh*, *unacknowledged* corrupt-store quarantine appeared in the live cognitive-store directory since the deploy window opened — retained historical forensic snapshots and acknowledged artifacts are ignored (see below) | Any single failing probe fails the health check and triggers rollback. The probe output is the same structured JSON whether it is run by the orchestrator or by an @@ -238,6 +238,29 @@ The probe also reports the in-window (`fresh_quarantines`) and retained only retained forensic snapshots (see [`NoQuarantineProbe`](../reference/self-deploy-api.md#noquarantineprobe)). +### Clearing a stuck quarantine + +The window filter alone does not cover one pathological case (#4469): the largest +corrupt store is a *recovery asset* that `simard cleanup` deliberately never +deletes (#2550). If such an asset lands *inside* the deploy window — or a fresh +corruption keeps re-landing on the one artifact protected from deletion — the +probe stays red on exactly the artifact that can never be swept, and self-deploy +freezes commits behind merged `main`. + +The probe is therefore also **acknowledgement-aware**. An operator can +acknowledge a genuinely-stuck quarantine (`simard self-health +--acknowledge-quarantine`) — or, for the protected recovery asset past the +30-day forensic window, the daemon auto-acknowledges it — by writing a durable +`.ack` sidecar next to the artifact. Acknowledgement silences the probe +**without deleting the recovery asset**: an acknowledged quarantine counts as +neither *fresh* nor *retained*, so `all_healthy()` can converge. A *new* +corruption event writes a fresh, unacknowledged artifact and correctly reddens +the probe again, because the marker is keyed to the exact filename. Full +mechanics: +[self-deploy quarantine-acknowledge](../reference/self-deploy-quarantine-acknowledge.md) +and the runbook [Clear a stuck memory +quarantine](../howto/clear-a-stuck-memory-quarantine.md). + ## Why build-from-source, not release-download A merged-but-unreleased commit *cannot* be fetched as a published binary — that diff --git a/docs/howto/clear-a-stuck-memory-quarantine.md b/docs/howto/clear-a-stuck-memory-quarantine.md new file mode 100644 index 000000000..273176cfd --- /dev/null +++ b/docs/howto/clear-a-stuck-memory-quarantine.md @@ -0,0 +1,138 @@ +--- +title: How to clear a stuck memory quarantine +description: Operator runbook for the self-health `no_quarantine` deadlock (#4469) — how to recognize a genuinely-stuck cognitive-memory quarantine that freezes self-deploy, acknowledge it with `simard self-health --acknowledge-quarantine` so the probe clears WITHOUT deleting the #2550 recovery asset, confirm convergence, and reverse the acknowledgement if needed. +last_updated: 2026-07-22 +review_schedule: as-needed +owner: simard +doc_type: howto +status: implemented +related: + - ../reference/self-deploy-quarantine-acknowledge.md + - ../reference/self-deploy-api.md + - ../concepts/reconcile-and-self-deploy.md + - ../howto/verify-and-roll-back-a-self-deploy.md +--- + +# How to clear a stuck memory quarantine + +> **Status: implemented.** `simard self-health --acknowledge-quarantine` writes +> a durable `.ack` sidecar next to each cognitive-memory quarantine artifact so +> the `no_quarantine` probe can clear **without** deleting the artifact. The +> underlying convention is documented in the +> [quarantine-acknowledge reference](../reference/self-deploy-quarantine-acknowledge.md). + +## When to use this + +Use this runbook when self-deploy is frozen **only** because of a quarantine +that can never clear on its own — the deadlock from issue #4469: + +- `simard self-health` reports `[FAIL] no_quarantine quarantined=true`, **and** +- the overseer keeps emitting "DeployDrift — running binary is N commit(s) behind + merged main", **and** +- the offending artifact is the long-lived **recovery asset** (the largest + `cognitive*.corrupt-` file, which `simard cleanup` deliberately never + deletes — see issue #2550). + +If the quarantine is **fresh** (recent corruption you have not yet +investigated), do **not** acknowledge it — investigate the corruption first. The +autonomous auto-ack only ever touches the protected recovery asset once it is +older than the 30-day forensic window; everything else stays red by design. + +## Step 1 — Confirm the deadlock + +```console +$ simard self-health +simard self-health: UNHEALTHY + [ok ] version_advanced running= target= + [ok ] memory_intact live_facts=1206 baseline=n/a + [ok ] goal_board_intact active_goals=5 + [ok ] brains_llm_backed fallback_records=0 + [FAIL] no_quarantine quarantined=true + [ok ] entrypoint_parity path=/home/you/.local/bin/simard version=simard 0.35.0 mismatch=false foreign=false +``` + +Only `no_quarantine` is red, and the artifact is the retained recovery asset. +Inspect what is present under **both** locations the probe scans — the +top-level state root and the live-store subdir `/state/` (where the +de-forked backend drops corrupt snapshots): + +```console +$ ls -1 ~/.simard/ ~/.simard/state/ 2>/dev/null | grep '\.corrupt-' +cognitive.corrupt-20260601T090412Z # large recovery asset — retained by #2550 +``` + +(If `SIMARD_STATE_ROOT` is set, look under that root and its `state/` subdir +instead — the probe, the acknowledge path, and `simard cleanup` all resolve the +same directory set.) + +## Step 2 — Acknowledge the quarantine + +```console +$ simard self-health --acknowledge-quarantine +simard self-health: HEALTHY + [ok ] version_advanced running= target= + [ok ] memory_intact live_facts=1206 baseline=n/a + [ok ] goal_board_intact active_goals=5 + [ok ] brains_llm_backed fallback_records=0 + [ok ] no_quarantine quarantined=false + [ok ] entrypoint_parity path=/home/you/.local/bin/simard version=simard 0.35.0 mismatch=false foreign=false +``` + +This writes an `.ack` sidecar next to each present quarantine artifact — in +both the top-level state root and `/state/` — and re-runs the probe. +The artifact is **not** deleted: + +```console +$ ls -1 ~/.simard/ | grep '\.corrupt-' +cognitive.corrupt-20260601T090412Z # still here — recovery asset retained +cognitive.corrupt-20260601T090412Z.ack # acknowledgement sidecar +``` + +The command is idempotent — running it again is safe and reports the artifacts +as already acknowledged. Exit code is `0` once every probe is healthy. + +## Step 3 — Confirm self-deploy converges + +With `no_quarantine` green, `all_healthy()` reaches `true`, the post-deploy +health check passes, and the next deploy is accepted instead of rolled back: + +```console +$ simard self-deploy +# … canary + gates pass, swap accepted, health check HEALTHY … + +$ simard self-health +simard self-health: HEALTHY +``` + +The recurring "DeployDrift — N commit(s) behind merged main" signal stops once +the running binary advances to merged `main`. + +## Reversing an acknowledgement + +Acknowledgement is reversible. Delete the sidecar to make the probe count the +artifact again: + +```console +$ rm ~/.simard/cognitive.corrupt-20260601T090412Z.ack +$ simard self-health # no_quarantine reddens again +``` + +Deleting the sidecar never affects the quarantine artifact itself. + +## What this does *not* do + +- It does **not** delete the quarantine artifact — the #2550 recovery asset is + retained so you can still salvage records from it. +- It does **not** silence *future* corruption. A new corruption event writes a + new `cognitive.corrupt-` artifact with no sidecar, so `no_quarantine` + reddens again immediately and self-deploy blocks — exactly as intended. +- It does **not** change any other probe or the `self-health` exit-code + convention. + +## See also + +- [Self-deploy quarantine-acknowledge reference](../reference/self-deploy-quarantine-acknowledge.md) + — the `.ack` convention, the `quarantine_ack` API, and the guarded auto-ack. +- [Self-deploy API reference](../reference/self-deploy-api.md#simard-self-health) + — the `simard self-health` subcommand and the six probes. +- [Verify and roll back a self-deploy](../howto/verify-and-roll-back-a-self-deploy.md). diff --git a/docs/reference/overseer-deploy-canary-diagnostics.md b/docs/reference/overseer-deploy-canary-diagnostics.md index 66019df4f..b1dbd6a4c 100644 --- a/docs/reference/overseer-deploy-canary-diagnostics.md +++ b/docs/reference/overseer-deploy-canary-diagnostics.md @@ -173,6 +173,48 @@ downstream sink at once. `refusal_reason` re-applies the same idempotent bound defensively for `CanaryResult`s built by other paths. Truncation never splits a multi-byte character. +### `unit-test` gate `first_failure=` detail (#4470) + +The `failing_detail` surfaced above is only as useful as the underlying +`GateResult.detail`. For the `unit-test` gate — the gate that reddened the +self-deploy canary in the #4470 incident — the raw `cargo test` stderr tail +often does **not** contain the failing test's name near the end, so the bounded +512-byte tail could name no test at all. `run_unit_test_gate` +([`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs)) +therefore **extracts the first failing test path** from the full `cargo test` +output and prepends it to the gate detail as a stable `first_failure=` prefix: + +```text +tests failed (exit 101): first_failure=::::; +``` + +| Field | Meaning | +| --- | --- | +| `first_failure=` | The first test path parsed from a `test ... FAILED` line (or the `failures:` block) in the `cargo test` output. Omitted only when no test name can be parsed (e.g. a link/compile abort with no test lines) — the bounded stderr tail is still included. | +| `` | The existing truncated stderr, unchanged. | + +Extraction rules: + +- **Parsed from the runner output**, not guessed — it reads the `... FAILED` + lines / `failures:` section that `cargo test` emits. The first failing test + wins (deterministic). +- **Bounded** to ≤ 512 bytes total, at a UTF-8 char boundary, consistent with the + `failing_detail` cap above. +- **Sanitized**: CR, LF, and other control characters are stripped from the + parsed test name before it is embedded, so the detail is a single clean line + and cannot forge additional log fields or JSON. The parsed name is treated as + **data, not a format string**. +- **Schema-stable**: `GateResult` keeps its `{ gate, passed, detail }` shape; + only the *content* of `detail` is enriched. `exit 101` (a Rust test-binary + panic/abort) still surfaces as before, now accompanied by the specific test. + +Because the failing gate's `detail` is what `TargetCanaryReport.failing_detail` +copies from, the `first_failure=` prefix rides all the way up to the operator +`deploy_refused` reason, the `overseer::deploy` WARN, and the `failing_detail` +OTel attribute — so a red `unit-test` canary now names the exact test to fix in +one glance. Acting on it is covered in +[STEP 2: acting on the surfaced detail](#step-2-acting-on-the-surfaced-detail). + ### `CanaryResult::refusal_reason` A new inherent method composes the enriched, human-readable refusal string @@ -386,3 +428,6 @@ weakened or disabled to mask a real regression. WARN event and the per-problem detail rows. - [Overseer tick self-healing](./overseer-tick-self-healing.md) — the `is_transient` fail-closed classifier and the SR-1 latch invariant. +- [Self-deploy quarantine-acknowledge](./self-deploy-quarantine-acknowledge.md) + — the paired `no_quarantine` deadlock fix (#4469): the *other* self-deploy + blocker that had to clear alongside the red canary for self-deploy to converge. diff --git a/docs/reference/self-deploy-api.md b/docs/reference/self-deploy-api.md index 6a1a3ad91..09b0a7586 100644 --- a/docs/reference/self-deploy-api.md +++ b/docs/reference/self-deploy-api.md @@ -10,6 +10,7 @@ related: - ../concepts/reconcile-and-self-deploy.md - ../concepts/operational-autonomy-model.md - ./self-deploy-source-prep.md + - ./self-deploy-quarantine-acknowledge.md - ./overseer-operator-notifications.md - ./overseer-tick-details.md - ../safe-self-update.md @@ -334,16 +335,31 @@ internally. There are **six** probes: `version_advanced`, `memory_intact`, [`entrypoint_parity`](#entrypointparityprobe). ```text -simard self-health [--json] [--pre-deploy-facts=N] +simard self-health [--json] [--pre-deploy-facts=N] [--acknowledge-quarantine] --json Emit the SelfHealthReport as JSON (default: human table). --pre-deploy-facts Baseline fact count to compare against (the orchestrator passes the count captured before the swap). When omitted, the "memory intact" probe reports the live count only. + --acknowledge-quarantine + Acknowledge every currently-present cognitive-memory + quarantine artifact under the state root (writing an + `.ack` sidecar next to each) before probing, so a + genuinely-stuck quarantine clears the `no_quarantine` + probe WITHOUT deleting the #2550 recovery asset. + Idempotent. See the quarantine-acknowledge reference. Exit code: 0 when every probe is healthy; non-zero when any probe fails. ``` +The additive `--acknowledge-quarantine` flag resolves the `no_quarantine` +deadlock (#4469) in which the retained #2550 recovery asset keeps the probe red +forever. Acknowledgement silences the probe for a specific, named artifact but +never deletes it, and a *new* corruption event reddens the probe again. The full +`.ack` convention, the `quarantine_ack` module API, the ack-aware +`count_quarantine_files`, and the guarded autonomous auto-ack are specified in +[self-deploy quarantine-acknowledge](./self-deploy-quarantine-acknowledge.md). + ### `self-health` output ```json @@ -420,6 +436,16 @@ both lets operators tell "clean store" (`0` / `0`) apart from "clean since deplo N forensic snapshots retained" (`0` / `N`) directly from the health JSON, without inspecting the store directory by hand. +> **Acknowledgement-aware counting (#4469).** The probe's JSON schema is unchanged, +> but both `count_quarantine_files` and the fresh/retained tally count only +> **unacknowledged** `cognitive*.corrupt-` artifacts: an artifact with a +> sibling `.ack` sidecar — and the `.ack` files themselves — are skipped, counting +> as neither fresh nor retained. This lets a genuinely-stuck quarantine (the +> retained #2550 recovery asset) clear so `all_healthy()` can converge without +> deleting it, while a *new* (unacknowledged) corruption event still reddens the +> probe. See +> [self-deploy quarantine-acknowledge](./self-deploy-quarantine-acknowledge.md). + > **Known limitation — mtime freshness.** Freshness is keyed on filesystem mtime. > Any operation that rewrites the mtime of a *retained* historical snapshot — > a rename, a `.bak` copy that preserves the original name, or a manual `touch` — diff --git a/docs/reference/self-deploy-quarantine-acknowledge.md b/docs/reference/self-deploy-quarantine-acknowledge.md new file mode 100644 index 000000000..b01d26f03 --- /dev/null +++ b/docs/reference/self-deploy-quarantine-acknowledge.md @@ -0,0 +1,363 @@ +--- +title: Self-deploy quarantine-acknowledge reference +description: Reference for the durable quarantine acknowledge/clear path that lets a genuinely-stuck cognitive-memory quarantine reset the self-health `no_quarantine` probe without deleting the #2550 recovery asset — the `.ack` sidecar convention, the `quarantine_ack` module API, the ack-aware `no_quarantine` probe, the `simard self-health --acknowledge-quarantine` operator flag and its guarded autonomous auto-ack, and the `cmd_cleanup::disk` sidecar-sweep behaviour. +last_updated: 2026-07-22 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./self-deploy-api.md + - ./overseer-deploy-canary-diagnostics.md + - ../concepts/reconcile-and-self-deploy.md + - ../howto/verify-and-roll-back-a-self-deploy.md + - ../howto/clear-a-stuck-memory-quarantine.md + - ../../src/self_deploy/quarantine_ack.rs + - ../../src/self_deploy/health.rs + - ../../src/cmd_cleanup/disk.rs + - ../../src/operator_cli/self_health.rs +--- + +# Self-deploy quarantine-acknowledge reference + +> **Status: implemented.** The `.ack` sidecar convention, the +> [`quarantine_ack`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/quarantine_ack.rs) +> module, the ack-aware `no_quarantine` probe in +> [`src/self_deploy/health.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/health.rs), +> the `simard self-health --acknowledge-quarantine` flag in +> [`src/operator_cli/self_health.rs`](https://github.com/rysweet/Simard/blob/main/src/operator_cli/self_health.rs), +> and the sidecar-aware sweep in +> [`src/cmd_cleanup/disk.rs`](https://github.com/rysweet/Simard/blob/main/src/cmd_cleanup/disk.rs) +> live in the tree today. The change is **additive and non-breaking**: the +> `NoQuarantineProbe` JSON schema, the `self-health` exit-code convention, and +> the `remove_old_corrupt_dbs` retention rules are all unchanged. No public +> signature was removed. + +## Why this exists + +The post-deploy [`no_quarantine`](./self-deploy-api.md#self-health-output) probe +fails whenever a `cognitive*.corrupt-` quarantine artifact sits in the +state-root directory. That is correct while corruption is fresh — but it created +a **deadlock** (issue #4469): + +1. When LadybugDB quarantines a corrupt store it leaves a `cognitive.corrupt-` + artifact in `~/.simard`. `no_quarantine` goes red and stays red. +2. The largest *substantial* quarantine is the **#2550 recovery asset** — a + corrupt store a prefix-recovery salvaged real records from — and + `remove_old_corrupt_dbs` deliberately **never** sweeps it, regardless of age. +3. So the one artifact that keeps `no_quarantine` red is exactly the one that is + protected from deletion. The probe can therefore **never** clear on its own, + `all_healthy()` never reaches `true`, and self-deploy freezes with the running + binary stuck commits behind merged `main` (the recurring + "DeployDrift — running binary is N commit(s) behind merged main" signal). + +The fix adds a **durable acknowledge path**: an operator (or, for the protected +recovery asset past the forensic window, the daemon itself) can *acknowledge* a +quarantine so the probe stops counting it — **without deleting the recovery +asset**. Acknowledgement silences the probe; retention is untouched. New, +unacknowledged corruption still reddens the probe immediately, because +acknowledgement is keyed to a specific artifact filename (which embeds the +`.corrupt-` timestamp). + +## The `.ack` sidecar convention + +Acknowledgement is recorded as a small sibling **sidecar file** next to the +quarantine artifact it acknowledges, in the resolved +[`simard_state_root()`](../../src/state_root.rs): + +``` +~/.simard/cognitive.corrupt-20260722T131600Z # the quarantine artifact +~/.simard/cognitive.corrupt-20260722T131600Z.ack # its acknowledgement sidecar +``` + +Properties: + +- **Filename-keyed.** The sidecar name is `.ack`. Because + every quarantine name carries a unique `.corrupt-` infix, an `.ack` only + ever silences the one artifact it names. A *new* corruption event produces a + new `.corrupt-` artifact with no sidecar, so `no_quarantine` re-reddens. +- **Additive, never destructive.** Writing an `.ack` never touches, moves, or + deletes the quarantine artifact. The #2550 recovery asset survives verbatim. +- **Idempotent.** Acknowledging an already-acknowledged artifact is a no-op that + succeeds. Re-running the operator command is always safe. +- **Reversible.** Deleting the `.ack` sidecar restores the pre-ack behaviour: + the artifact is counted again and `no_quarantine` reddens (assuming the + artifact is still present). + +### Sidecar payload + +The sidecar is a small, fixed marker file — the exact bytes +`acknowledged\n`. Presence of the sidecar *is* the acknowledgement; the +convention deliberately stores no structured payload, so there is nothing to +parse, version, or leak. Who acknowledged (operator vs. the guarded +autonomous auto-ack), when, and why are recorded on the **structured +tracing/OTel event** emitted at acknowledgement time, not in the sidecar. The +sidecar exists only to be counted (or skipped) by the probe. + +## `quarantine_ack` module API + +[`src/self_deploy/quarantine_ack.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/quarantine_ack.rs) +is the **single owner** of the `.ack` convention. Both the `no_quarantine` probe +and the operator CLI go through it; no other module constructs `.ack` paths. + +```rust +/// Suffix appended to a quarantine artifact's basename to form its sidecar. +pub const ACK_SUFFIX: &str = ".ack"; + +/// Compute the `.ack` sidecar path for a quarantine artifact living directly +/// under `state_root`. `quarantine_name` MUST be a validated basename that +/// passes the single canonical corrupt-quarantine predicate; separators, +/// `..`, and absolute paths are rejected. Returns `None` for an invalid name. +pub fn ack_marker_path(state_root: &Path, quarantine_name: &str) -> Option; + +/// `true` when `name` is itself an `.ack` sidecar (so scanners can skip it). +pub fn is_ack_marker_name(name: &str) -> bool; + +/// `true` when the quarantine artifact `quarantine_name` under `state_root` has +/// a present regular-file `.ack` sidecar. `false` for an invalid name, a +/// missing sidecar, or a non-regular-file (symlink/dir) at the sidecar path +/// (fail toward "not acknowledged"). +pub fn is_acknowledged(state_root: &Path, quarantine_name: &str) -> bool; + +/// Durably acknowledge the quarantine artifact `quarantine_name` under +/// `state_root` by writing its fixed-marker `.ack` sidecar. Idempotent: +/// acknowledging an already-acked artifact succeeds and leaves a single +/// sidecar. Never touches the artifact. Returns the written sidecar path. +/// +/// Fails closed on a path-safety violation, a symlinked/irregular sidecar +/// target, or a write error. +pub fn acknowledge(state_root: &Path, quarantine_name: &str) -> SimardResult; + +/// List the acknowledgeable `cognitive*.corrupt-*` artifact basenames present +/// directly under `state_root`, excluding `.ack` sidecars and the live store. +/// The operator `--acknowledge-quarantine` path iterates this list, keeping +/// `quarantine_ack` the single owner of "what is an acknowledgeable quarantine". +pub fn present_quarantine_artifacts(state_root: &Path) -> Vec; +``` + +### Path safety + +Every `.ack` path is built by [`ack_marker_path`], which accepts **only** a +basename that passes the shared corrupt-quarantine-name predicate and rejects +anything containing a path separator, a `..` component, or an absolute prefix. +The resolved path is asserted to round-trip through `Path::file_name()` before +any I/O, and writes use `symlink_metadata` (`lstat`) plus `create_new` +(`O_EXCL`) to **refuse** an existing non-regular-file target — a planted +`cognitive.corrupt-X.ack -> /etc/passwd` symlink can never be followed or +overwritten. The state root itself is resolved only via +[`simard_state_root()`](../../src/state_root.rs), which already enforces a +non-empty, absolute, NUL-free path. The sidecar holds a small fixed marker +(`acknowledged\n`), `fsync`-ed on write. + +## Ack-aware `no_quarantine` probe + +The [`no_quarantine`](./self-deploy-api.md#self-health-output) probe in +[`src/self_deploy/health.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/health.rs) +now counts only **unacknowledged** quarantine artifacts. The production scan +`tally_quarantine_files` skips both `.ack` sidecars and any artifact that has a +present `.ack` sidecar, splitting the rest into *fresh* vs. *retained* counts +against the forensic window (only *fresh* fails the probe): + +```rust +fn tally_quarantine_files(dir: &std::path::Path, window_start: DateTime) -> QuarantineTally { + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(_) => return QuarantineTally::default(), + }; + let mut tally = QuarantineTally::default(); + for entry in entries.flatten() { + let file_name = entry.file_name(); + let name = file_name.to_string_lossy(); + if !is_corrupt_quarantine_name(&name) { + continue; + } + // `.ack` sidecars are never quarantines, and an acknowledged artifact + // is "seen" — it counts as neither fresh nor retained. + if quarantine_ack::is_ack_marker_name(&name) + || quarantine_ack::is_acknowledged(dir, &name) + { + continue; + } + let Ok(mtime) = entry.metadata().and_then(|m| m.modified()) else { + continue; + }; + if DateTime::::from(mtime) >= window_start { + tally.fresh += 1; + } else { + tally.retained += 1; + } + } + tally +} +``` + +The test-only `count_quarantine_files` helper delegates to this same scan +(summing `fresh + retained`) so there is a single source of the acknowledgement +logic. + +The `NoQuarantineProbe` **JSON schema is unchanged** — it still serializes as +`{ "healthy": bool, "quarantined": bool }`. `quarantined` is now `false` once +every quarantine artifact is acknowledged, so `no_quarantine.healthy` can reach +`true` and `all_healthy()` can converge. An older orchestrator deserializing the +report sees no new fields. + +> **Single canonical predicate.** `is_corrupt_quarantine_name` is defined **once** +> and shared by both the probe and `cmd_cleanup::disk::remove_old_corrupt_dbs` +> (`cognitive.` / `cognitive_memory.` stem + `.corrupt-` infix). There is no +> second copy to drift: the probe and the sweep agree on what a quarantine is by +> construction. + +## `simard self-health --acknowledge-quarantine` + +The operator surface is an **additive** flag on the existing +[`simard self-health`](./self-deploy-api.md#simard-self-health) subcommand. See +also the how-to: [Clear a stuck memory +quarantine](../howto/clear-a-stuck-memory-quarantine.md). + +```text +simard self-health [--json] [--pre-deploy-facts=N] [--acknowledge-quarantine] + + --acknowledge-quarantine + Acknowledge every currently-present cognitive-memory quarantine + artifact under the state root AND the live-store subdir + `/state/`, writing an `.ack` sidecar next to each + (source: operator). Idempotent. Does NOT delete any artifact — the + #2550 recovery asset is retained. After acknowledging, the probe is + re-run and the (now-cleared) report is printed. + +Exit code: 0 when every probe is healthy; non-zero when any probe fails. +``` + +Behaviour: + +- With `--acknowledge-quarantine`, the command first acknowledges each present + quarantine artifact (via `quarantine_ack::acknowledge`) across the **same + directory set the probe and cleanup sweep scan** — the top-level state root + and `/state/`, single-sourced from + `state_root::quarantine_scan_dirs_under` — then runs the normal probe and + prints the report. Scanning only the top level would silently miss a stuck + quarantine under `state/` (the primary location the de-forked backend drops + corrupt snapshots), leaving the probe red despite a "success" here. Because + acknowledgement is idempotent, running it twice is safe: the second run finds + each sidecar already present and re-writes nothing. +- Without the flag, `self-health` behaviour is exactly as before: it reports the + six probes and exits non-zero if any is unhealthy. Acknowledgement is **never** + implicit for a manual health check. +- The **exit-code convention is unchanged**: `0` iff every probe is healthy + after the (optional) acknowledgement. + +### Guarded autonomous auto-ack + +The auto-ack runs **inside the probe itself** — in `run_self_health_probe` +([`src/self_deploy/health.rs`](https://github.com/rysweet/Simard/blob/main/src/self_deploy/health.rs)), +the function the `no_quarantine` probe already calls — **not** in the operator +CLI, so it also fires for the autonomous orchestrator's post-deploy +`health_check` (which calls `run_self_health_probe` directly and never goes +through `operator_cli::self_health`). + +> **This is defense-in-depth, not the primary convergence mechanism.** The +> `no_quarantine` probe already converges on retained (old) quarantines via the +> **fresh-window semantics**: only a quarantine with mtime at/after the caller's +> `window_start` counts as *fresh*, and only fresh quarantines redden the probe +> (see `no_quarantine_passes_with_only_historical_quarantines_in_state_dir`). +> Under the current callers the window is recent (the orchestrator passes `now`; +> the operator CLI `now - 5m`), so the aged protected asset the auto-ack targets +> — mtime at least `CORRUPT_DB_MAX_AGE_DAYS` old — is already counted `retained`, +> never `fresh`, and the probe already passes on it. The auto-ack adds two +> guarantees on top: it drops the aged protected asset out of the `retained` +> diagnostic, and it keeps the probe green even if a caller were to pass an +> observation window *older* than the forensic age. A genuinely-stuck *fresh* +> quarantine is cleared by the operator's manual `--acknowledge-quarantine`, not +> by this path. + +The auto-ack is narrowly scoped — it fires only for the aged protected asset: + +- **Only** the #2550 **protected recovery asset** is eligible — the single + artifact `remove_old_corrupt_dbs` refuses to sweep: the largest quarantine + whose size is at least `CORRUPT_DB_PROTECT_MIN_BYTES` (1 MB). +- **Only** when it is **older than the forensic window** + (`CORRUPT_DB_MAX_AGE_DAYS`, 30 days) — long past the point an operator would + have acted on it. +- Every other quarantine artifact — anything fresh, anything not the protected + asset — is **never** auto-acked and still reddens the probe. + +> **Single-sourced protected-asset selection.** The "which artifact is the +> protected recovery asset" decision — largest quarantine with size ≥ +> `CORRUPT_DB_PROTECT_MIN_BYTES` — and the `CORRUPT_DB_MAX_AGE_DAYS` age gate are +> the **same** predicate and constants `remove_old_corrupt_dbs` uses in +> [`src/cmd_cleanup/disk.rs`](https://github.com/rysweet/Simard/blob/main/src/cmd_cleanup/disk.rs). +> `health.rs` reuses that selection helper and those constants rather than +> re-deriving "protected" independently, so the probe and the sweep can never +> disagree about which single artifact is protected. (This is separate from the +> corrupt-*name* predicate above; both the name predicate and the +> protected-asset selection are single-sourced.) + +When an auto-ack fires it writes the fixed-marker `.ack` sidecar and emits a +structured OTel event (WARN) recording that the source was the autonomous +daemon, the artifact name, its age, and the reason. There is no +`print!`/`println!` — the record is tracing/OTel only. The auto-ack is +reversible (delete the sidecar) and is logged so an operator can always see +that the daemon cleared the deadlock on its own. + +> **Why this is safe.** Auto-ack silences a probe; it never deletes data and +> never touches a *fresh* quarantine. A genuinely new corruption event produces +> a new, young, unacknowledged artifact that both fails the age gate and lacks a +> sidecar — so it correctly reddens `no_quarantine` and blocks the deploy. + +## Cleanup interaction (`cmd_cleanup::disk`) + +`remove_old_corrupt_dbs` +([`src/cmd_cleanup/disk.rs`](https://github.com/rysweet/Simard/blob/main/src/cmd_cleanup/disk.rs)) +is updated so acknowledgement and reclamation stay consistent: + +- **Scans the resolved state root.** The scan directory is now + [`simard_state_root()`](../../src/state_root.rs) rather than a hardcoded + `$HOME/.simard`, so a `SIMARD_STATE_ROOT` override points the sweep at the + **same** directory the probe scans and acknowledges in. (Previously the two + could diverge under an override.) +- **Skips `.ack` sidecars.** `is_ack_marker_name` sidecars are not quarantine + artifacts and are never counted or reclaimed on their own. +- **Sweeps a sidecar with its artifact.** When a quarantine artifact is reclaimed + (age cap or keep-last-N), its `.ack` sidecar, if any, is removed in the same + pass so no orphaned sidecars accumulate. +- **Preserves the #2550 recovery asset and its marker.** The largest substantial + quarantine is still never swept, and its `.ack` sidecar (from an auto-ack or a + manual ack) is preserved alongside it. Acknowledgement silences the probe; it + does **not** make the recovery asset eligible for deletion. + +## Convergence guarantee + +With the acknowledge path in place, a stuck quarantine no longer freezes +self-deploy: + +1. `no_quarantine` counts only unacknowledged artifacts, so an acknowledged + (or auto-acked protected) quarantine no longer reddens it. +2. `all_healthy()` can reach `true`, the post-deploy health check passes, and the + swapped build is accepted instead of rolled back. +3. Self-deploy converges — the running binary advances to merged `main` and the + recurring "DeployDrift — running binary is N commit(s) behind merged main" + signal stops firing. + +**Autonomy is bounded by the forensic window.** Fully autonomous convergence +happens only *after* the protected recovery asset ages past +`CORRUPT_DB_MAX_AGE_DAYS` (30 days), because the guarded auto-ack refuses to +touch it before then. While the protected asset is still inside that window, an +operator must run `simard self-health --acknowledge-quarantine` to converge — the +daemon deliberately will not silence a recent quarantine on its own. This is the +intended trade-off: the forensic window is preserved for fresh corruption, and +autonomy resumes once it has elapsed. + +A genuinely new corruption still reddens the probe and blocks the deploy, so the +safety property the probe exists to enforce is preserved. + +## See also + +- [Self-deploy API reference](./self-deploy-api.md) — the six probes, the + `simard self-health` subcommand, and `all_healthy()`. +- [Overseer deploy red-canary diagnostics](./overseer-deploy-canary-diagnostics.md) + — the paired `unit-test` gate `first_failure=` detail (#4470) that makes the + *other* self-deploy blocker diagnosable. +- [Reconcile & self-deploy](../concepts/reconcile-and-self-deploy.md) — what + "healthy" means and the end-to-end deploy flow. +- [Clear a stuck memory quarantine](../howto/clear-a-stuck-memory-quarantine.md) + — the operator runbook. diff --git a/docs/testing/deflaking-ooda-meeting-env-races.md b/docs/testing/deflaking-ooda-meeting-env-races.md new file mode 100644 index 000000000..0b65d931b --- /dev/null +++ b/docs/testing/deflaking-ooda-meeting-env-races.md @@ -0,0 +1,304 @@ +--- +title: De-flaking the OODA-config and meeting cost-ledger env races +description: > + How two parallel-`cargo test` flakes are made deterministic: the + OODA-config default race (issue #4433) is closed by giving + `ooda_config_default_values` the `cognitive_memory` serial key and clearing + the concurrency env before it reads `OodaConfig::default()`, mirroring its + already-correct twin, and by extending the `serial_guard` meta-test so the + indirect concurrency-env read can no longer be reintroduced unkeyed. The + meeting cost-ledger flake (issues #4359 / #4355 / #4354) is NOT yet fixed: + the obvious HOME/serial hypothesis is retracted here because it is already + implemented, so this page records a reproduce-before-fix contract, not a + finished patch. +last_updated: 2026-07-23 +review_schedule: when a new env-reading constructor is added to the OODA config path, when the meeting cost-ledger root cause is confirmed, or when serial_test is upgraded +owner: simard +doc_type: reference +related: + - ./hermetic-tests.md + - ./cognitive-memory-serial-isolation.md + - ./deflaking-known-flaky-tests.md + - ./ci-resilient-test-patterns.md + - ./COVERAGE_BASELINE.md +--- + +# De-flaking the OODA-config and meeting cost-ledger env races + +This page is the test-author and reviewer contract for two parallel-`cargo test` +flakes. It is a companion to +[serial(cognitive_memory) test isolation](./cognitive-memory-serial-isolation.md), +which owns the whole-binary env-serialization scheme this work plugs into, and +to [De-flaking the known flaky tests](./deflaking-known-flaky-tests.md), whose +structure it mirrors. + +The two flakes are at very different stages, and this page deliberately keeps +them apart: + +| Race | Flaky test | Status | +| ---- | ---------- | ------ | +| **A — OODA-config default** ([#4433](https://github.com/rysweet/Simard/issues/4433)) | `ooda_loop::tests_types::ooda_config_default_values` | **Root cause confirmed. Fix specified below and ready to build.** | +| **B — meeting cost-ledger** ([#4359](https://github.com/rysweet/Simard/issues/4359) · [#4355](https://github.com/rysweet/Simard/issues/4355) · [#4354](https://github.com/rysweet/Simard/issues/4354)) | `base_type_copilot::tests::meeting_turn_records_full_enriched_prompt_tokens_not_bare_objective` | **Root cause UNCONFIRMED. No fix yet. Reproduce-before-fix contract only.** | + +> **One canonical serial key.** Both races live inside the *same* +> process-global environment. A second, separate serial key would **not** help: +> the race is variable-agnostic — a glibc `setenv` on *any* name can +> `realloc(environ)` and free the array a concurrent `getenv` is mid-read (see +> [the cognitive_memory contract](./cognitive-memory-serial-isolation.md)). The +> only correct outcome is that *every* env mutator and every watched env reader +> in the lib-test binary funnels through the single existing `cognitive_memory` +> serial key. Anything that touches the process environment shares that one key +> or it races. This page adds nothing to the keying scheme; it only brings two +> stragglers into it (Race A) and refuses to guess at a third (Race B). + +--- + +## Race A — OODA-config default (issue #4433): confirmed, ready to build + +### The race + +`ooda_loop::tests_types::ooda_config_default_values` asserts the shipped default +concurrency ceiling: + +```rust +// src/ooda_loop/tests_types.rs — BEFORE (racy) +#[test] +fn ooda_config_default_values() { + let config = OodaConfig::default(); + assert_eq!(config.max_concurrent_actions, 24); // issue #2935 default + // ... +} +``` + +`OodaConfig::default()` reads three process-global variables — +`SIMARD_OODA_MAX_CONCURRENT`, `SIMARD_MAX_CONCURRENT_ACTIONS`, and +`SIMARD_SCALING` (`src/ooda_loop/types.rs`). This test carries **no serial key** +and does **not** clear those variables. A sibling suite in +`src/ooda_loop/types.rs` — `simard_ooda_max_concurrent_overrides_default`, +`max_concurrent_defaults_to_24_when_unset`, and the other `#2935` cases — +`set_var`/`remove_var`s exactly those variables. Those writers *do* carry +`#[serial(cognitive_memory)]`, but because `ooda_config_default_values` does +not, its read of `OodaConfig::default()` can run concurrently with a writer's +`set_var("SIMARD_OODA_MAX_CONCURRENT", "30")` and observe a torn or leaked +value — `max_concurrent_actions` comes back as `30` / `8` / `5` instead of the +default `24`, and the assertion fails intermittently. + +The correct pattern already exists one file over, in the twin +`max_concurrent_defaults_to_24_when_unset` +(`src/ooda_loop/types.rs`): it holds the `cognitive_memory` serial key and +clears the three concurrency variables to a known-clean baseline before reading +`OodaConfig::default()`. + +### The fix (finished shape) + +Bring `ooda_config_default_values` up to the twin's pattern — the serial key +plus an explicit pre-read clear of the concurrency surface, so the read is +never concurrent with a writer and never observes leaked state: + +```rust +// src/ooda_loop/tests_types.rs — AFTER (deterministic) +#[serial_test::serial(cognitive_memory)] +#[test] +fn ooda_config_default_values() { + // Clear the concurrency-env surface OodaConfig::default() reads so the + // assertion sees the shipped default, not a value leaked by a #2935 + // writer. Order-independent; no leakage in either direction. + // SAFETY: serialised via #[serial(cognitive_memory)] — no concurrent env + // mutation can tear this read/clear (see the cognitive_memory contract). + unsafe { + std::env::remove_var("SIMARD_OODA_MAX_CONCURRENT"); + std::env::remove_var("SIMARD_MAX_CONCURRENT_ACTIONS"); + std::env::remove_var("SIMARD_SCALING"); + } + let config = OodaConfig::default(); + assert_eq!(config.max_concurrent_actions, 24); // issue #2935 default + assert!((config.improvement_threshold - 0.02).abs() < f64::EPSILON); + assert_eq!(config.gym_suite_id, "progressive"); +} +``` + +This changes no production behaviour: `OodaConfig::default()` still resolves the +same variables in the same precedence. The suite is only prevented from racing +on them. + +### Guardrail: extend the existing meta-test, do not add a new one + +A regression guard for exactly this class of bug already ships: +`src/test_support/serial_guard.rs` (the `cognitive_memory` contract, issues +[#2360](https://github.com/rysweet/Simard/issues/2360) / +[#2375](https://github.com/rysweet/Simard/issues/2375)). It parses the source +tree with `syn` and fails the build when a `#[test]` touches the watched env +surface without the key. Its detection rule has two arms: + +- **Mutation watch (`EnvWatch::AnyVar`, the shipped default):** any + `set_var` / `remove_var` of *any* variable in a keyless test is an offender. + This already covers every OODA concurrency *writer* — they carry the key, so + they pass; a future keyless writer would be caught automatically. **No change + needed for writers.** +- **Read watch (`READ_WATCHED_VARS` + `ENV_READING_HANDLERS`):** a keyless test + is an offender if it directly `std::env::var`s a watched name + (`SIMARD_STATE_ROOT`, `SIMARD_MEMORY_SOCKET`, `SIMARD_LLM_PROVIDER`, + `SIMARD_MEETINGS_DIR`, `SIMARD_MEETINGS_ROOT`, `SIMARD_HANDOFF_DIR`) or calls + a named env-reading handler. + +The reader in Race A is the gap: `ooda_config_default_values` does not read the +concurrency variables *directly* — it reads them **indirectly** through the +`OodaConfig::default()` constructor. Neither the concurrency variable names nor +the constructor are in the read watch, so the guard cannot currently see this +reader. This is a *documented blind spot*, in the same family as the +false-negatives already recorded in the cognitive_memory contract. + +The guardrail extension therefore adds the **OODA concurrency read surface** to +the existing guard, so a *future* keyless reader is flagged: + +- Register `OodaConfig::default` as an env-reading trigger (analogous to the + existing `ENV_READING_HANDLERS` entries such as `add_goal` / + `write_auto_save`). Because the guard's call collector currently matches a + single path segment (`default`, too generic to watch safely), the trigger + must be recognised by its **fuller `OodaConfig::default` path**, and the + collector extended minimally to record that two-segment call. This keeps the + trigger precise and false-positive-free: only the concurrency-config + constructor is watched, not every `::default()` in the tree. +- Detection rule, stated precisely: *a keyless `#[test]` that (a) mutates any + process env var, (b) directly reads a `READ_WATCHED_VARS` name, (c) calls a + registered env-reading handler, or (d) constructs `OodaConfig::default()`, is + an offender.* Clauses (a)–(c) already exist; (d) is what #4433 adds. +- **Exemptions** use the guard's existing, machine-checked allowlist + (`AuditOptions::allowlist`): a `(test_name, justification)` pair, where an + empty justification is itself an audit failure. There is no new exemption + syntax — genuinely env-free tests that merely *name* a watched symbol are + allowlisted with a written reason, exactly as today. + +The point of the arms above is that the fix is not "one more annotation on one +test"; it closes the *reader shape* so #4433 cannot silently return. + +--- + +## Race B — meeting cost-ledger (issues #4359 / #4355 / #4354): UNCONFIRMED, no fix yet + +> **This section describes work that has NOT been done, because the root cause +> is not yet known.** It intentionally contains no "finished fix" and no +> patched code block. Treat everything below as a contract for *how* to find and +> close the flake, not a description of a closed flake. Writing a fix before the +> reproduction exists is the exact blind-patch failure this page forbids. + +### The retracted hypothesis (do not blind-patch this) + +The obvious theory is: "the meeting cost ledger lives at +`$HOME/.simard/costs/ledger.jsonl`, so a concurrent meeting test on a shared +process-global `HOME` writes into the same ledger; redirect `HOME` to a temp +dir, hold the `cognitive_memory` serial key, and match the entry by session id." + +**That hypothesis is retracted, because it is already fully implemented and the +flake persists.** In +`base_type_copilot::tests::meeting_turn_records_full_enriched_prompt_tokens_not_bare_objective` +today: + +- `HOME` is already redirected to a per-test `tempfile::TempDir`. +- The test already carries `#[serial_test::serial(cognitive_memory)]`. +- The ledger entry is already matched by a **unique session id *and* model** + (`session-…-000000004164`, `copilot-meeting`), so a concurrent meeting test + sharing the temp `HOME` cannot substitute its own entry. +- `HOME` is restored with panic-safe teardown (`catch_unwind` + + `resume_unwind`). + +So HOME isolation, the serial key, and entry disambiguation are **not the +missing fix — they are already present.** Re-adding or re-emphasising them would +be a no-op dressed as a repair. Any PR that "fixes #4359 by isolating HOME / +adding the serial key" should be rejected on sight: read the test first. + +### The actual contract: reproduce, then isolate at source + +The true shared mutable state behind Race B is **not yet identified**. It is +some resource *other than* the already-isolated HOME ledger path — a candidate, +none confirmed, includes: a process-global `static` in the cost-tracking write +path (`crate::cost_tracking`), a shared on-disk path that does *not* derive from +`HOME`, an `ETXTBSY`/"Text file busy" race on the freshly-written `fake_copilot` +binary (the test already retries this, but the retry may be masking or +interacting with the real failure), or a torn env read of a variable the +meeting path consults that is outside the current watch set. **Pick none of +these by inspection.** The mandated order is: + +1. **Reproduce first.** Stand up a deterministic reproduction *before* touching + any production or test code. Run the single test under a stress loop with + thread-count variation and no test caching, e.g.: + + ```bash + for i in $(seq 1 200); do + cargo test --locked --lib \ + base_type_copilot::tests::meeting_turn_records_full_enriched_prompt_tokens_not_bare_objective \ + -- --test-threads=8 --nocapture || { echo "FAILED on iter $i"; break; } + done + ``` + + Vary `--test-threads` (1, 4, 8, 16) and run the *whole* `--lib` binary too + (the race may only appear against concurrent unrelated tests). A fix is not + allowed to proceed until a reproduction is captured. +2. **Identify the true shared resource** from the reproduction — the specific + `static`, path, or env read that two concurrent tests contend on. +3. **Isolate it at source**, preferring the project's established pattern: + thread an explicit, per-test root/handle through the exercised path so it + never resolves the shared resource ambiently (the same shape that closed the + goal-board state-root race in + [De-flaking the known flaky tests](./deflaking-known-flaky-tests.md)). Fall + back to the `cognitive_memory` serial key **only** if the contended resource + is genuinely a process-global env read — and if so, extend + `READ_WATCHED_VARS` / the guard so it is enforced, not just annotated once. +4. **Prove closure** by re-running the same stress loop from step 1 to a clean + pass count (see the verification gate below). + +Until steps 1–4 are done, the correct state of this page's Race B section is +"open, unconfirmed" — and it must stay that way rather than acquire a +speculative fix. + +--- + +## Coverage follow-up (issue #4331): conditional scope + +[Issue #4331](https://github.com/rysweet/Simard/issues/4331) (coverage) is +pulled into this work **only if** it shares a root cause with Race A or the +confirmed Race B cause — for example, if the same OODA concurrency-env reader or +the same cost-ledger resource is what a coverage gap left unguarded. If #4331 is +an independent coverage target, it is explicitly **out of scope here** and stays +on its own track. Do not expand this work to chase it speculatively. + +## Constraints (apply to every change on this page) + +- **Additive only.** No renames of existing tests, helpers, or public symbols; + no signature churn on production APIs. Race A adds an attribute + a pre-read + clear; the guard extension adds one trigger. Race B adds nothing until its + cause is confirmed. +- **No `print!`/`println!` debugging left in tests** — use assertions and + `--nocapture` transiently only. +- **Panic-safe teardown.** Any test that mutates process env or `HOME` must + restore prior state through `catch_unwind` + `resume_unwind` (Race B's test + already models this). +- **`--locked` everywhere.** All build/test invocations pass `--locked` so the + gate matches CI and cannot silently drift `Cargo.lock`. +- **No production behaviour change.** These are test-isolation fixes; the OODA + config resolution and the meeting cost-ledger write path behave identically + in production before and after. + +## Verification gate + +A change on this page is done only when all of the following pass under +`--locked`: + +1. **Race A determinism:** `ooda_config_default_values` passes ≥ 50 consecutive + iterations of the whole lib binary at `--test-threads=8`: + + ```bash + for i in $(seq 1 50); do + cargo test --locked --lib -- --test-threads=8 || { echo "FAILED iter $i"; exit 1; } + done + ``` + +2. **Guard enforcement:** the `serial_guard` meta-test + (`src/test_support/serial_guard.rs`) passes, and — as a red-phase check — + temporarily removing the new key/clear from `ooda_config_default_values` + makes the guard *fail*, proving clause (d) actually catches the reader shape. +3. **Race B (when in scope):** the step-1 stress loop above reaches ≥ 200 + consecutive passes across `--test-threads` ∈ {1, 4, 8, 16}, from a captured + reproduction, before the flake is declared closed. +4. **Docs integrity:** `cargo test --locked --test docs_integrity` is green — + this page's nav entry resolves and it has no dead intra-repo links. diff --git a/mkdocs.yml b/mkdocs.yml index c15fdaf6e..3d18ae13d 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -232,6 +232,7 @@ nav: - Diagnose and Recover Lost Creative-Ideas Goals: howto/diagnose-lost-creative-ideas-goals.md - Clean Fixture Leaks: howto/clean-fixture-leaks.md - Verify and Roll Back a Self-Deploy: howto/verify-and-roll-back-a-self-deploy.md + - Clear a Stuck Memory Quarantine: howto/clear-a-stuck-memory-quarantine.md - Verify and Repair PATH-Entrypoint Parity: howto/verify-path-entrypoint-parity.md - Run Self-Deploy from Any Directory: howto/run-self-deploy-from-any-directory.md - Check for Updates: howto/check-for-updates.md @@ -329,6 +330,7 @@ nav: - amplihack Freshness Gate: reference/amplihack-freshness-gate.md - Multi-Binary Self-Update: reference/multi-binary-self-update.md - Self-Deploy API: reference/self-deploy-api.md + - Self-Deploy Quarantine-Acknowledge: reference/self-deploy-quarantine-acknowledge.md - Self-Deploy Source Prep & Warm Target Dir: reference/self-deploy-source-prep.md - State-Root Resolution: reference/state-root-resolution.md - Operator Read State-Root Contract: reference/operator-read-state-root-contract.md @@ -500,6 +502,7 @@ nav: - Writing Hermetic Tests: testing/hermetic-tests.md - serial(cognitive_memory) Isolation: testing/cognitive-memory-serial-isolation.md - De-Flaking Known Flaky Tests: testing/deflaking-known-flaky-tests.md + - De-Flaking OODA & Meeting Env Races: testing/deflaking-ooda-meeting-env-races.md - Ecosystem & Audits: - Amplihack Ecosystem Map: ecosystem-map.md - amplihack-rs ↔ amplihack Parity Matrix: amplihack-rs-parity.md diff --git a/src/cmd_cleanup/disk.rs b/src/cmd_cleanup/disk.rs index 4beac0d8b..c5aab57ea 100644 --- a/src/cmd_cleanup/disk.rs +++ b/src/cmd_cleanup/disk.rs @@ -394,40 +394,39 @@ pub(crate) fn is_corrupt_quarantine_name(name: &str) -> bool { /// The age cap alone leaves a burst of *young* quarantines untouched for a week /// (this host saw 88 MB / 112 artifacts accumulate); the keep-last-N cap bounds /// that growth immediately while preserving the most recent forensic snapshots. -pub fn remove_old_corrupt_dbs(report: &mut CleanupReport) { - let state_root = crate::state_root::simard_state_root(); - let live_store_dir = crate::state_root::resolve_subdir("state"); - - reclaim_corrupt_dbs_in_dir(&state_root, report); - // `resolve_subdir("state")` is always distinct from the top-level root, but - // guard against an unexpected alias so a directory is never scanned twice. - if live_store_dir != state_root { - reclaim_corrupt_dbs_in_dir(&live_store_dir, report); - } +/// A quarantine candidate discovered under the scan directory: its path, whether +/// it is a directory-backed store, its size in bytes, and its mtime. +struct QuarantineCandidate { + path: PathBuf, + is_dir: bool, + size: u64, + modified: std::time::SystemTime, } -/// Apply the age / keep-last-N / largest-asset quarantine bounds to a single -/// directory `dir`'s listing (non-recursive). Extracted from -/// [`remove_old_corrupt_dbs`] so the identical policy can run independently over -/// each live-store directory (issue #4469). Bounds are computed over `dir`'s own -/// candidate set only — never merged across directories — so each directory -/// keeps its own newest-N and its own largest recovery asset. Absent/unreadable -/// `dir` ⇒ no-op. -fn reclaim_corrupt_dbs_in_dir(dir: &Path, report: &mut CleanupReport) { - let Ok(entries) = std::fs::read_dir(dir) else { - return; +/// Scan `scan_dir` for corrupt-quarantine artifacts, EXCLUDING `.ack` +/// acknowledgement sidecars (#4469). Each candidate carries its size + mtime so +/// callers can apply the age / keep-last-N caps and the #2550 protection over +/// the full set (read_dir order is unspecified). Size is computed once here +/// because the largest-asset guard needs it for every candidate. Unreadable or +/// absent dir ⇒ empty. +/// +/// Note `is_corrupt_quarantine_name` also matches an `.ack` sidecar (it carries +/// the `.corrupt-` infix), so the sidecar exclusion MUST come first. +fn scan_quarantine_candidates(scan_dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(scan_dir) else { + return Vec::new(); }; - let max_age = std::time::Duration::from_secs(CORRUPT_DB_MAX_AGE_DAYS * 24 * 3600); - let now = std::time::SystemTime::now(); - - // Collect every quarantine candidate with its size + mtime so we can apply - // the age cap, the keep-last-N cap, AND the largest-asset protection over the - // full set (read_dir order is unspecified, so we cannot rank by iteration - // order). Size is computed once here rather than lazily at removal time - // because the largest-asset guard below needs it for every candidate. - let mut candidates: Vec<(PathBuf, bool, u64, std::time::SystemTime)> = Vec::new(); + let mut candidates = Vec::new(); for entry in entries.flatten() { - if !is_corrupt_quarantine_name(&entry.file_name().to_string_lossy()) { + // Borrow the lossy name instead of forcing a `String` per entry: most + // entries fail the predicates below and are skipped, so avoid the + // per-entry heap allocation. + let file_name = entry.file_name(); + let name = file_name.to_string_lossy(); + if crate::self_deploy::quarantine_ack::is_ack_marker_name(&name) { + continue; + } + if !is_corrupt_quarantine_name(&name) { continue; } let Ok(meta) = entry.metadata() else { continue }; @@ -442,32 +441,117 @@ fn reclaim_corrupt_dbs_in_dir(dir: &Path, report: &mut CleanupReport) { } else { meta.len() }; - candidates.push((entry.path(), is_dir, size, modified)); + candidates.push(QuarantineCandidate { + path: entry.path(), + is_dir, + size, + modified, + }); } + candidates +} + +/// The #2550 protected recovery asset among `candidates`: the LARGEST quarantine +/// whose size is at least [`CORRUPT_DB_PROTECT_MIN_BYTES`] (ties → newest). This +/// is the single, shared definition of "protected asset" used by BOTH the +/// cleanup sweep and the health-probe guarded auto-ack (#4469), so the two can +/// never disagree about which artifact is protected. +fn select_protected_asset(candidates: &[QuarantineCandidate]) -> Option<&QuarantineCandidate> { + candidates + .iter() + .filter(|c| c.size >= CORRUPT_DB_PROTECT_MIN_BYTES) + .max_by(|a, b| { + a.size + .cmp(&b.size) + .then_with(|| a.modified.cmp(&b.modified)) + }) +} + +/// Basename of the #2550 protected recovery asset directly under `state_root` +/// when it is past the forensic window ([`CORRUPT_DB_MAX_AGE_DAYS`]). +/// +/// This is the largest retained recovery asset that the #2550 rule never sweeps. +/// The guarded defense-in-depth auto-ack in +/// [`crate::self_deploy::health`] acknowledges exactly this artifact (and only +/// once it is aged out of the forensic window) WITHOUT deleting the retained +/// recovery asset — see that module for how this interacts with the probe's +/// fresh-window semantics. Returns `None` when there is no protected asset or it +/// is still inside the window (fresh corruption is never eligible). +pub(crate) fn aged_protected_recovery_asset(state_root: &Path) -> Option { + let candidates = scan_quarantine_candidates(state_root); + let asset = select_protected_asset(&candidates)?; + let max_age = std::time::Duration::from_secs(CORRUPT_DB_MAX_AGE_DAYS * 24 * 3600); + let aged = std::time::SystemTime::now() + .duration_since(asset.modified) + .unwrap_or_default() + >= max_age; + if !aged { + return None; + } + asset + .path + .file_name() + .and_then(|n| n.to_str()) + .map(|s| s.to_string()) +} + +pub fn remove_old_corrupt_dbs(report: &mut CleanupReport) { + // #4469: sweep every directory that can hold corrupt cognitive-memory + // quarantines — the top-level state root (`~/.simard`, the native pre-#2307 + // quarantine location) AND the live-store subdir `/state/`, + // where the de-forked library backend actually drops corrupt snapshots next + // to the live `cognitive` store. Before this the driver only scanned the top + // level, so 62 corrupt artifacts accumulated unbounded under `state/` on the + // live host. The directory set is single-sourced from + // `state_root::quarantine_scan_dirs` (honoring `SIMARD_STATE_ROOT`, deduped) + // — the SAME set the self-health `no_quarantine` probe scans — so the probe + // and the sweep can never disagree on where the quarantines live. The age / + // keep-last-N / largest-asset bounds are applied independently per directory + // via `remove_old_corrupt_dbs_in`. + for dir in crate::state_root::quarantine_scan_dirs() { + remove_old_corrupt_dbs_in(&dir, report); + } +} + +/// Sweep aged / over-count corrupt-quarantine artifacts under `scan_dir`, +/// applying the age cap, the keep-last-N cap, and the #2550 protected-asset +/// guard, and reclaiming each swept artifact's `.ack` sidecar (#4469). +/// +/// Path-injected so the sweep logic is decoupled from state-root resolution: +/// [`remove_old_corrupt_dbs`] passes the resolved [`crate::state_root::simard_state_root`], +/// while tests pass a tempdir directly and never mutate the process-global +/// `SIMARD_STATE_ROOT`/`HOME` env (which parallel tests race on). +pub(crate) fn remove_old_corrupt_dbs_in(scan_dir: &Path, report: &mut CleanupReport) { + // #4469: first reclaim any orphaned `.ack` sidecars (parent quarantine + // already gone), independent of whether there are live quarantine + // candidates below — otherwise a directory with zero remaining quarantines + // would never shed its stale markers. + reclaim_orphaned_ack_sidecars(scan_dir, report); + let mut candidates = scan_quarantine_candidates(scan_dir); + if candidates.is_empty() { + return; + } + let max_age = std::time::Duration::from_secs(CORRUPT_DB_MAX_AGE_DAYS * 24 * 3600); + let now = std::time::SystemTime::now(); // Issue #2550: never sweep the LARGEST *substantial* quarantine — it is the // most likely recovery asset. A corrupt store that a prefix-recovery salvaged // tens of thousands of records from is many megabytes; losing it is exactly // the permanent data-loss this issue exists to prevent, so it is protected - // from BOTH the age cap and the keep-last-N cap. Trivial quarantines (a - // truncated WAL sidecar, an empty rebuilt store) fall below - // `CORRUPT_DB_PROTECT_MIN_BYTES` and are reclaimed normally. Ties break - // toward the newest. - let protected: Option = candidates - .iter() - .filter(|c| c.2 >= CORRUPT_DB_PROTECT_MIN_BYTES) - .max_by(|a, b| a.2.cmp(&b.2).then_with(|| a.3.cmp(&b.3))) - .map(|c| c.0.clone()); + // from BOTH the age cap and the keep-last-N cap. Trivial quarantines fall + // below `CORRUPT_DB_PROTECT_MIN_BYTES` and are reclaimed normally. + let protected: Option = select_protected_asset(&candidates).map(|c| c.path.clone()); // Newest first, so index 0..CORRUPT_DB_KEEP are the survivors of the count cap. - candidates.sort_by_key(|c| std::cmp::Reverse(c.3)); + candidates.sort_by_key(|c| std::cmp::Reverse(c.modified)); - for (rank, (path, is_dir, size, modified)) in candidates.iter().enumerate() { - // The recovery asset is never reclaimed, regardless of age or rank. - if protected.as_deref() == Some(path.as_path()) { + for (rank, cand) in candidates.iter().enumerate() { + // The recovery asset is never reclaimed, regardless of age or rank — and + // neither is its `.ack` sidecar, which stays because the asset stays. + if protected.as_deref() == Some(cand.path.as_path()) { continue; } - let too_old = now.duration_since(*modified).unwrap_or_default() >= max_age; + let too_old = now.duration_since(cand.modified).unwrap_or_default() >= max_age; let beyond_keep = rank >= CORRUPT_DB_KEEP; if !too_old && !beyond_keep { continue; @@ -475,21 +559,133 @@ fn reclaim_corrupt_dbs_in_dir(dir: &Path, report: &mut CleanupReport) { let reason = if too_old { "age" } else { "keep-last-N" }; eprintln!( " Removing corrupt DB {} ({} MB, {reason})", - path.display(), - size / (1024 * 1024) + sanitize_path_for_log(&cand.path), + cand.size / (1024 * 1024) ); - let removed = if *is_dir { - std::fs::remove_dir_all(path) + let removed = if cand.is_dir { + std::fs::remove_dir_all(&cand.path) } else { - std::fs::remove_file(path) + std::fs::remove_file(&cand.path) }; if let Err(e) = removed { - report - .errors - .push(format!("failed to remove {}: {e}", path.display())); + report.errors.push(format!( + "failed to remove {}: {e}", + sanitize_path_for_log(&cand.path) + )); } else { - report.bytes_freed += size; - report.dirs_removed.push(path.clone()); + report.bytes_freed += cand.size; + report.dirs_removed.push(cand.path.clone()); + // #4469: reclaim the acknowledgement sidecar with its quarantine so + // no orphaned `.ack` markers accumulate — regardless of the marker's + // own age (age/keep rules never apply to a marker directly). + reclaim_ack_sidecar(&cand.path, report); + } + } +} + +/// Remove the `.ack` acknowledgement sidecar (#4469) alongside the +/// quarantine it acknowledges, once that quarantine has been swept. Only a +/// durable regular-file sidecar is reclaimed; a non-regular-file at that path +/// (symlink/dir) is left untouched. Missing sidecar ⇒ no-op. +fn reclaim_ack_sidecar(quarantine: &Path, report: &mut CleanupReport) { + let Some(name) = quarantine.file_name().and_then(|n| n.to_str()) else { + return; + }; + let sidecar = quarantine.with_file_name(format!( + "{name}{}", + crate::self_deploy::quarantine_ack::ACK_SUFFIX + )); + match std::fs::symlink_metadata(&sidecar) { + Ok(meta) if meta.file_type().is_file() => { + let size = meta.len(); + match std::fs::remove_file(&sidecar) { + Ok(()) => { + report.bytes_freed += size; + report.dirs_removed.push(sidecar); + } + Err(e) => report.errors.push(format!( + "failed to remove {}: {e}", + sanitize_path_for_log(&sidecar) + )), + } + } + _ => {} + } +} + +/// Maximum byte length of a filesystem path rendered into operator-facing +/// stderr / [`CleanupReport`] output. Generous (well past `PATH_MAX`) so real +/// paths are never truncated; the bound only exists as a belt-and-suspenders +/// cap on a maliciously long untrusted basename. +const PATH_LOG_MAX_BYTES: usize = 4096; + +/// Render a filesystem path for operator-facing stderr / [`CleanupReport`] +/// output with control characters neutralized (#4469 security review, LOW-1). +/// +/// A corrupt-quarantine basename is an untrusted on-disk filename (it only needs +/// the `cognitive*` prefix + `.corrupt-` infix to be swept) that may embed CR/LF +/// or ANSI escape sequences. `Path::display()` emits those verbatim, letting an +/// attacker with write access to the state root forge log lines or inject +/// terminal-control sequences into the operator's console. Route every +/// operator-visible path through the shared `util::log_sanitize` control-char +/// strip — the same escaping the health probe applies via Debug (`?name`). +fn sanitize_path_for_log(path: &Path) -> String { + crate::util::log_sanitize::sanitize_to_single_line( + &path.display().to_string(), + PATH_LOG_MAX_BYTES, + ) +} + +/// Reclaim orphaned `.ack` acknowledgement sidecars (#4469) directly under +/// `scan_dir`: a marker whose parent quarantine artifact no longer exists +/// (operator `rm`, or manual deletion of the #2550 protected asset). +/// +/// [`reclaim_ack_sidecar`] only sheds a sidecar when *this* sweep removes its +/// parent, so a parent deleted out-of-band would otherwise leave the marker as a +/// permanent orphan — 13 bytes each, but unbounded over time. This pass closes +/// that gap. Only a durable regular-file marker with a missing parent is +/// reclaimed; a marker whose parent still exists is left for the normal sweep, +/// and a non-regular-file at the marker path (planted symlink/dir) is never +/// followed or removed. +fn reclaim_orphaned_ack_sidecars(scan_dir: &Path, report: &mut CleanupReport) { + let Ok(entries) = std::fs::read_dir(scan_dir) else { + return; + }; + for entry in entries.flatten() { + let file_name = entry.file_name(); + let name = file_name.to_string_lossy(); + if !crate::self_deploy::quarantine_ack::is_ack_marker_name(&name) { + continue; + } + // `DirEntry::metadata` does not traverse a symlink at the entry, so this + // never follows a planted link — only a genuine regular file is eligible. + match entry.metadata() { + Ok(meta) if meta.file_type().is_file() => {} + _ => continue, + } + // Parent quarantine basename = marker name minus the `.ack` suffix. + let Some(parent_name) = name.strip_suffix(crate::self_deploy::quarantine_ack::ACK_SUFFIX) + else { + continue; + }; + // If the parent quarantine still exists (as anything), keep the marker; + // the normal sweep will reclaim it in lockstep when the parent goes. + if std::fs::symlink_metadata(scan_dir.join(parent_name)).is_ok() { + continue; + } + let marker = entry.path(); + let size = std::fs::symlink_metadata(&marker) + .map(|m| m.len()) + .unwrap_or(0); + match std::fs::remove_file(&marker) { + Ok(()) => { + report.bytes_freed += size; + report.dirs_removed.push(marker); + } + Err(e) => report.errors.push(format!( + "failed to remove {}: {e}", + sanitize_path_for_log(&marker) + )), } } } diff --git a/src/cmd_cleanup/tests.rs b/src/cmd_cleanup/tests.rs index 3f895ac2f..0bdceb659 100644 --- a/src/cmd_cleanup/tests.rs +++ b/src/cmd_cleanup/tests.rs @@ -1,3 +1,4 @@ +use super::disk::remove_old_corrupt_dbs_in; use super::*; /// Set a path's mtime to roughly `days` days in the past (plus an hour of @@ -287,7 +288,6 @@ fn trim_snapshots_keeps_newest_n() { // ── remove_old_corrupt_dbs ── #[test] -#[serial_test::serial(cognitive_memory)] fn corrupt_db_removed_when_older_than_threshold() { let tmp = tempfile::tempdir().unwrap(); let simard = tmp.path().join(".simard"); @@ -307,7 +307,8 @@ fn corrupt_db_removed_when_older_than_threshold() { .unwrap() .set_times(times) .unwrap(); - run_corrupt_cleanup_with_home(tmp.path()); + let mut report = CleanupReport::default(); + remove_old_corrupt_dbs_in(&simard, &mut report); assert!(!old.exists(), "old corrupt DB should be removed"); assert!(young.exists(), "young corrupt DB should survive"); assert!(unrelated.exists(), "non-corrupt DB must never be touched"); @@ -325,7 +326,6 @@ fn corrupt_db_keep_is_sane() { /// quarantines present, only the newest `CORRUPT_DB_KEEP` survive, and the live /// store files are never touched. #[test] -#[serial_test::serial(cognitive_memory)] fn corrupt_db_keep_bounds_quarantine_count() { let tmp = tempfile::tempdir().unwrap(); let simard = tmp.path().join(".simard"); @@ -364,7 +364,8 @@ fn corrupt_db_keep_bounds_quarantine_count() { paths.push(p); } - run_corrupt_cleanup_with_home(tmp.path()); + let mut report = CleanupReport::default(); + remove_old_corrupt_dbs_in(&simard, &mut report); let remaining = paths.iter().filter(|p| p.exists()).count(); assert_eq!( @@ -681,31 +682,26 @@ fn backdate(path: &std::path::Path, days: u64) { f.set_times(times).unwrap(); } -/// Run `remove_old_corrupt_dbs` with `HOME` pointed at `home`, restoring the -/// previous environment afterward. Serialized by the caller's -/// `#[serial(cognitive_memory)]` attribute so the process-wide env mutation -/// cannot race other tests that read the cognitive-memory store. +/// Run `remove_old_corrupt_dbs_in` against `home/.simard` AND its `state/` +/// live-store subdir directly, mirroring the production `remove_old_corrupt_dbs` +/// two-directory sweep (#4469) without mutating any process-global env. /// -/// `remove_old_corrupt_dbs` resolves its target via -/// [`crate::state_root::simard_state_root`], which honors `SIMARD_STATE_ROOT` -/// **before** falling back to `$HOME/.simard`. Other tests in the binary set -/// `SIMARD_STATE_ROOT` and some leak it (never restore it), so this helper must -/// unset it for the duration of the call — otherwise cleanup scans the leaked -/// path instead of `home/.simard` and reclaims nothing. Both env vars are -/// restored to their prior values before returning. +/// Path-injected rather than `HOME`-mutating: the production sweep resolves its +/// scan dirs via `simard_state_root()` / `resolve_subdir("state")`, which read +/// the process-global `SIMARD_STATE_ROOT`/`HOME` env that parallel tests mutate +/// under other serial keys. Driving the injected `remove_old_corrupt_dbs_in` for +/// both the top-level root and its `state/` subdir makes these tests +/// deterministic and free of cross-test env races (#4469 regression fix) while +/// still exercising both live-store directories the production entry point +/// reclaims. fn run_corrupt_cleanup_with_home(home: &std::path::Path) -> CleanupReport { - let old_home = std::env::var_os("HOME"); - let old_state_root = std::env::var_os(crate::state_root::STATE_ROOT_ENV); - // SAFETY: serialized via the caller's #[serial(cognitive_memory)]; both - // vars are restored below before any assertion can unwind. - unsafe { - std::env::set_var("HOME", home); - std::env::remove_var(crate::state_root::STATE_ROOT_ENV); - } let mut report = CleanupReport::default(); - remove_old_corrupt_dbs(&mut report); - restore_env("HOME", old_home); - restore_env(crate::state_root::STATE_ROOT_ENV, old_state_root); + let root = home.join(".simard"); + let state = root.join("state"); + remove_old_corrupt_dbs_in(&root, &mut report); + if state != root { + remove_old_corrupt_dbs_in(&state, &mut report); + } report } @@ -916,6 +912,285 @@ fn corrupt_db_keeps_young_library_quarantine() { assert_eq!(report.bytes_freed, 0); } +// ── #4469: acknowledgement-aware corrupt-DB sweep ── + +/// Run `remove_old_corrupt_dbs` with `SIMARD_STATE_ROOT` pointed at `root`, +/// restoring the previous value afterward. Serialized by the caller's +/// `#[serial(cognitive_memory)]` attribute (SIMARD_STATE_ROOT is on the watched +/// env surface). +fn run_corrupt_cleanup_with_state_root(root: &std::path::Path) -> CleanupReport { + let old = std::env::var_os("SIMARD_STATE_ROOT"); + unsafe { + std::env::set_var("SIMARD_STATE_ROOT", root); + } + let mut report = CleanupReport::default(); + remove_old_corrupt_dbs(&mut report); + match old { + Some(v) => unsafe { std::env::set_var("SIMARD_STATE_ROOT", v) }, + None => unsafe { std::env::remove_var("SIMARD_STATE_ROOT") }, + } + report +} + +/// The sweep must scan the resolved `simard_state_root()` (honoring +/// `SIMARD_STATE_ROOT`), not the hardcoded `$HOME/.simard`. Otherwise the +/// health probe (which already uses `simard_state_root()`) and the cleanup +/// sweep disagree on which directory holds the quarantines, so an acknowledged +/// artifact is never swept from the directory the probe actually scans (#4469). +#[test] +#[serial_test::serial(cognitive_memory)] +fn corrupt_db_sweep_scans_resolved_state_root() { + let root = tempfile::tempdir().unwrap(); + // Quarantine lives DIRECTLY under the resolved state root (not a `.simard` + // subdir) when SIMARD_STATE_ROOT is set. + let aged = root.path().join("cognitive.corrupt-1700000000"); + std::fs::write(&aged, b"corrupt-bytes").unwrap(); + backdate(&aged, CORRUPT_DB_MAX_AGE_DAYS + 1); + + let report = run_corrupt_cleanup_with_state_root(root.path()); + + assert!( + !aged.exists(), + "aged quarantine under SIMARD_STATE_ROOT must be swept (scan must use \ + simard_state_root(), not $HOME/.simard)" + ); + assert!( + report.dirs_removed.iter().any(|p| p == &aged), + "swept quarantine should be reported" + ); +} + +/// An acknowledgement sidecar (`*.ack`) is NOT a corrupt store: it must be +/// excluded from the sweep's candidate scan. While its parent quarantine is +/// still present (here: retained because it is young and below the count cap), +/// the marker is retained too — never counted or reported as a removed +/// "corrupt DB". (Orphan markers whose parent is *gone* are reclaimed; see +/// `corrupt_db_sweep_reclaims_orphaned_ack_sidecar`.) +#[test] +#[serial_test::serial(cognitive_memory)] +fn corrupt_db_sweep_never_treats_ack_marker_as_quarantine() { + let tmp = tempfile::tempdir().unwrap(); + let simard = tmp.path().join(".simard"); + std::fs::create_dir_all(&simard).unwrap(); + + // A live parent quarantine that is retained (young, only candidate → within + // the keep-last-N cap), so its marker must be retained alongside it. + let quarantine = simard.join("cognitive.corrupt-1700000000"); + std::fs::write(&quarantine, b"tiny").unwrap(); + let marker = simard.join("cognitive.corrupt-1700000000.ack"); + std::fs::write(&marker, b"").unwrap(); + backdate(&marker, CORRUPT_DB_MAX_AGE_DAYS + 10); + + let report = run_corrupt_cleanup_with_home(tmp.path()); + + assert!( + quarantine.exists(), + "the young, in-cap parent quarantine must be retained" + ); + assert!( + marker.exists(), + "an `.ack` marker must never be swept as if it were a corrupt store while \ + its parent quarantine is retained" + ); + assert!( + !report.dirs_removed.iter().any(|p| p == &marker), + "an `.ack` marker must never be reported as a removed quarantine" + ); +} + +/// An orphaned `.ack` sidecar — one whose parent quarantine no longer exists +/// (operator `rm`, or manual deletion of the #2550 protected asset) — is +/// reclaimed by the sweep so stale markers cannot accumulate unbounded (#4469). +/// This holds even when the directory has *no* remaining quarantine candidates. +#[test] +#[serial_test::serial(cognitive_memory)] +fn corrupt_db_sweep_reclaims_orphaned_ack_sidecar() { + let tmp = tempfile::tempdir().unwrap(); + let simard = tmp.path().join(".simard"); + std::fs::create_dir_all(&simard).unwrap(); + + // A marker with NO parent quarantine present — a pure orphan. Its own age is + // irrelevant to orphan reclaim, but back-date it to prove age is not the + // trigger. + let marker = simard.join("cognitive.corrupt-1700000000.ack"); + std::fs::write(&marker, b"acknowledged\n").unwrap(); + backdate(&marker, CORRUPT_DB_MAX_AGE_DAYS + 10); + assert!( + !simard.join("cognitive.corrupt-1700000000").exists(), + "precondition: the parent quarantine is absent", + ); + + let report = run_corrupt_cleanup_with_home(tmp.path()); + + assert!( + !marker.exists(), + "an orphaned `.ack` marker (parent gone) must be reclaimed" + ); + assert!( + report.dirs_removed.iter().any(|p| p == &marker), + "the reclaimed orphan marker must be reported in the cleanup report" + ); +} + +/// When a quarantine artifact is swept, its `.ack` sidecar is reclaimed +/// alongside it — regardless of the marker's own mtime — so no orphaned markers +/// accumulate (#4469). +#[test] +#[serial_test::serial(cognitive_memory)] +fn corrupt_db_sweep_removes_sidecar_with_its_quarantine() { + let tmp = tempfile::tempdir().unwrap(); + let simard = tmp.path().join(".simard"); + std::fs::create_dir_all(&simard).unwrap(); + + // An aged, small (below the protection floor) quarantine that WILL be swept. + let quarantine = simard.join("cognitive.corrupt-1700000000"); + std::fs::write(&quarantine, b"tiny").unwrap(); + backdate(&quarantine, CORRUPT_DB_MAX_AGE_DAYS + 5); + + // Its sidecar is FRESH — age/keep rules alone would retain it, leaving an + // orphan. The sweep must remove it because its parent quarantine is removed. + let marker = simard.join("cognitive.corrupt-1700000000.ack"); + std::fs::write(&marker, b"").unwrap(); + + let report = run_corrupt_cleanup_with_home(tmp.path()); + + assert!(!quarantine.exists(), "aged quarantine should be swept"); + assert!( + !marker.exists(), + "the sidecar must be reclaimed with its parent quarantine, even when the \ + marker itself is fresh" + ); + let _ = report; +} + +/// The #2550 protected recovery asset is retained — and so is its `.ack` +/// sidecar. Acknowledging silences the probe without deleting the recovery +/// asset OR orphaning its marker. +#[test] +#[serial_test::serial(cognitive_memory)] +fn corrupt_db_sweep_retains_protected_asset_marker() { + let tmp = tempfile::tempdir().unwrap(); + let simard = tmp.path().join(".simard"); + std::fs::create_dir_all(&simard).unwrap(); + + // The recovery asset: multi-MB, aged — protected from BOTH caps by #2550. + let asset = simard.join("cognitive.corrupt-1700000000"); + std::fs::write( + &asset, + vec![0u8; (CORRUPT_DB_PROTECT_MIN_BYTES + 512) as usize], + ) + .unwrap(); + backdate(&asset, CORRUPT_DB_MAX_AGE_DAYS + 7); + + // Its durable acknowledgement, aged — must survive because the asset survives. + let marker = simard.join("cognitive.corrupt-1700000000.ack"); + std::fs::write(&marker, b"").unwrap(); + backdate(&marker, CORRUPT_DB_MAX_AGE_DAYS + 7); + + let report = run_corrupt_cleanup_with_home(tmp.path()); + + assert!( + asset.exists(), + "protected recovery asset must be retained (#2550)" + ); + assert!( + marker.exists(), + "the protected asset's `.ack` marker must be retained alongside it" + ); + assert!( + !report + .dirs_removed + .iter() + .any(|p| p == &asset || p == &marker), + "neither the protected asset nor its marker should be reported removed" + ); +} + +// ── #4469: aged protected-recovery-asset selection (guarded auto-ack source) ── +// `aged_protected_recovery_asset` is the single-sourced selector the health +// probe's guarded auto-ack uses. It returns ONLY the #2550 protected asset +// (largest quarantine >= CORRUPT_DB_PROTECT_MIN_BYTES) and ONLY once it is past +// the forensic window, so fresh or trivial corruption is never auto-acked. + +#[test] +fn aged_protected_asset_returns_the_aged_recovery_asset() { + let dir = tempfile::tempdir().unwrap(); + let asset = dir.path().join("cognitive.corrupt-1700000000"); + std::fs::write( + &asset, + vec![0u8; (CORRUPT_DB_PROTECT_MIN_BYTES + 512) as usize], + ) + .unwrap(); + backdate(&asset, CORRUPT_DB_MAX_AGE_DAYS + 1); + + assert_eq!( + crate::cmd_cleanup::disk::aged_protected_recovery_asset(dir.path()).as_deref(), + Some("cognitive.corrupt-1700000000"), + "the aged, substantial recovery asset must be selected" + ); +} + +#[test] +fn aged_protected_asset_none_when_inside_forensic_window() { + let dir = tempfile::tempdir().unwrap(); + let asset = dir.path().join("cognitive.corrupt-1700000000"); + std::fs::write( + &asset, + vec![0u8; (CORRUPT_DB_PROTECT_MIN_BYTES + 512) as usize], + ) + .unwrap(); + // Substantial but still fresh — must NOT be eligible. + backdate(&asset, CORRUPT_DB_MAX_AGE_DAYS.saturating_sub(2)); + + assert_eq!( + crate::cmd_cleanup::disk::aged_protected_recovery_asset(dir.path()), + None, + "a fresh protected asset is never auto-ack eligible" + ); +} + +#[test] +fn aged_protected_asset_none_for_sub_floor_quarantine() { + let dir = tempfile::tempdir().unwrap(); + // Aged but below the protection floor — a trivial quarantine, not the asset. + let small = dir.path().join("cognitive.corrupt-1700000000"); + std::fs::write(&small, b"tiny").unwrap(); + backdate(&small, CORRUPT_DB_MAX_AGE_DAYS + 5); + + assert_eq!( + crate::cmd_cleanup::disk::aged_protected_recovery_asset(dir.path()), + None, + "a sub-floor quarantine is never the protected recovery asset" + ); +} + +#[test] +fn aged_protected_asset_picks_largest_and_ignores_ack_sidecar() { + let dir = tempfile::tempdir().unwrap(); + let small = dir.path().join("cognitive.corrupt-1700000000"); + std::fs::write( + &small, + vec![0u8; (CORRUPT_DB_PROTECT_MIN_BYTES + 16) as usize], + ) + .unwrap(); + backdate(&small, CORRUPT_DB_MAX_AGE_DAYS + 3); + let large = dir.path().join("cognitive.corrupt-1700000001"); + std::fs::write( + &large, + vec![0u8; (CORRUPT_DB_PROTECT_MIN_BYTES * 3) as usize], + ) + .unwrap(); + backdate(&large, CORRUPT_DB_MAX_AGE_DAYS + 3); + // A `.ack` sidecar must never be considered a candidate itself. + std::fs::write(dir.path().join("cognitive.corrupt-1700000001.ack"), b"").unwrap(); + + assert_eq!( + crate::cmd_cleanup::disk::aged_protected_recovery_asset(dir.path()).as_deref(), + Some("cognitive.corrupt-1700000001"), + "the LARGEST aged substantial quarantine is the recovery asset" + ); +} + /// Root Cause B (issue #4469): the live cognitive store and its quarantines live /// under `/state/` (e.g. `~/.simard/state/cognitive`), NOT top-level /// `~/.simard`. Before this fix `remove_old_corrupt_dbs` only scanned the diff --git a/src/ooda_loop/decide.rs b/src/ooda_loop/decide.rs index 5cca68f61..1ff70c63b 100644 --- a/src/ooda_loop/decide.rs +++ b/src/ooda_loop/decide.rs @@ -225,6 +225,7 @@ mod tests { ); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_skips_zero_urgency_priorities() { let priorities = vec![ @@ -245,6 +246,7 @@ mod tests { assert_eq!(actions[0].goal_id, Some("g1".to_string())); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_memory_priority_to_consolidate_action() { let priorities = vec![Priority { @@ -259,6 +261,7 @@ mod tests { assert!(actions[0].goal_id.is_none()); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_improvement_priority_to_run_improvement() { let priorities = vec![Priority { @@ -272,6 +275,7 @@ mod tests { assert!(actions[0].goal_id.is_none()); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_regular_goal_to_advance_goal() { let priorities = vec![Priority { @@ -285,6 +289,7 @@ mod tests { assert_eq!(actions[0].goal_id, Some("ship-v1".to_string())); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_empty_priorities_returns_empty() { let config = OodaConfig::default(); @@ -292,6 +297,7 @@ mod tests { assert!(actions.is_empty()); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_preserves_reason_as_description() { let priorities = vec![Priority { @@ -304,6 +310,7 @@ mod tests { assert_eq!(actions[0].description, "important task"); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_extract_ideas_priority() { let priorities = vec![Priority { @@ -318,6 +325,7 @@ mod tests { assert!(actions[0].goal_id.is_none()); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_safe_update_priority() { let priorities = vec![Priority { @@ -337,6 +345,7 @@ mod tests { // a brain error transparently falls back to the deterministic mapping. // ----------------------------------------------------------------------- + #[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_uses_brain_judgment_for_action_kind() { struct AlwaysGymBrain; @@ -361,6 +370,7 @@ mod tests { assert_eq!(actions[0].kind, ActionKind::RunGymEval); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_records_brain_rationale_not_fallback_marker() { // Wiring test: when an LLM-backed brain is provided, the rationale @@ -407,6 +417,7 @@ mod tests { // (2) embed a ParseFailureRecord on the per-cycle BrainJudgmentRecord, // (3) SKIP the priority (no action produced — no fallback). // ----------------------------------------------------------------------- + #[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_skips_priority_on_brain_error() { use crate::error::SimardError; @@ -478,6 +489,7 @@ mod tests { reset_consecutive_count_for_tests(BrainPhase::Decide, goal_id); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_successful_parse_resets_consecutive_counter() { use crate::ooda_brain::BrainPhase; @@ -603,6 +615,7 @@ mod tests { // Issue #2227: eval-watchdog routing and defense-in-depth guard // ----------------------------------------------------------------------- + #[serial_test::serial(cognitive_memory)] #[test] fn decide_maps_eval_watchdog_to_run_gym_eval() { let priorities = vec![Priority { @@ -621,6 +634,7 @@ mod tests { assert!(actions[0].goal_id.is_none()); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_routes_synthetic_deterministically_even_with_llm_brain() { // An LLM brain that always returns AdvanceGoal. For synthetic @@ -666,6 +680,7 @@ mod tests { assert_eq!(actions[1].goal_id, Some("real-goal".to_string())); } + #[serial_test::serial(cognitive_memory)] #[test] fn decide_guard_still_catches_unknown_synthetic_advance_goal() { // The defense-in-depth guard is still needed for edge cases where diff --git a/src/ooda_loop/tests_parse_failure_1890.rs b/src/ooda_loop/tests_parse_failure_1890.rs index 380da06b8..0bbda6d73 100644 --- a/src/ooda_loop/tests_parse_failure_1890.rs +++ b/src/ooda_loop/tests_parse_failure_1890.rs @@ -186,6 +186,7 @@ fn run_isolated(f: impl FnOnce() -> R) -> R { // decide_with_brain — silent-fallback closure // =========================================================================== +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_pushes_parse_failure_record() { // ANTI-REGRESSION (issue #1890): before this PR, decide_with_brain on @@ -212,6 +213,7 @@ fn decide_with_brain_errored_pushes_parse_failure_record() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_parse_failure_carries_error_and_raw_response() { let priorities = one_priority("g1"); @@ -258,6 +260,7 @@ fn decide_with_brain_errored_parse_failure_carries_error_and_raw_response() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_skips_priority() { // Brain error must skip the priority (no action produced). @@ -275,6 +278,7 @@ fn decide_with_brain_errored_skips_priority() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_record_marks_brain_error() { // The judgment record must indicate brain_error (not fallback). @@ -295,6 +299,7 @@ fn decide_with_brain_errored_record_marks_brain_error() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_ok_path_leaves_parse_failure_none() { // Healthy LLM brain returns Ok — no parse failure, no schema churn. @@ -320,6 +325,7 @@ fn decide_with_brain_ok_path_leaves_parse_failure_none() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_record_serializes_parse_failure_to_json() { // End-to-end: the BrainJudgmentRecord MUST serialize the parse_failure @@ -353,6 +359,7 @@ fn decide_with_brain_errored_record_serializes_parse_failure_to_json() { assert!(back.parse_failure.is_some()); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_consecutive_count_increments_per_call() { // Resolution A6: track consecutive failures per (phase, goal_id) so @@ -380,6 +387,7 @@ fn decide_with_brain_errored_consecutive_count_increments_per_call() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_consecutive_count_resets_on_next_successful_parse() { // Three failures, then one Ok — counter MUST reset. @@ -406,6 +414,7 @@ fn decide_with_brain_consecutive_count_resets_on_next_successful_parse() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn decide_with_brain_errored_continues_to_next_priority() { // One failing priority must NOT stop the cycle — subsequent priorities @@ -615,6 +624,7 @@ fn orient_with_brain_errored_consecutive_count_increments_per_call() { ); } +#[serial_test::serial(cognitive_memory)] #[test] fn orient_and_decide_counters_are_independent_for_same_goal() { // Resolution A7: (phase, goal_id) is the counter key. A decide failure diff --git a/src/ooda_loop/tests_types.rs b/src/ooda_loop/tests_types.rs index 5ad505374..ed58a1771 100644 --- a/src/ooda_loop/tests_types.rs +++ b/src/ooda_loop/tests_types.rs @@ -244,8 +244,20 @@ fn action_kind_equality() { // --- OodaConfig --- +#[serial_test::serial(cognitive_memory)] #[test] fn ooda_config_default_values() { + // Race A (issue #4433): OodaConfig::default() reads SIMARD_OODA_MAX_CONCURRENT + // / SIMARD_MAX_CONCURRENT_ACTIONS / SIMARD_SCALING from the process-global + // env. Clear that surface so the assertion observes the shipped default, not + // a value leaked by a #2935 concurrency-env writer. Order-independent. + // SAFETY: serialised via #[serial(cognitive_memory)] — no concurrent env + // mutation can tear this read/clear (see the cognitive_memory contract). + unsafe { + std::env::remove_var("SIMARD_OODA_MAX_CONCURRENT"); + std::env::remove_var("SIMARD_MAX_CONCURRENT_ACTIONS"); + std::env::remove_var("SIMARD_SCALING"); + } let config = OodaConfig::default(); // Issue #2935: the per-OODA-cycle goal-coverage parallelism ceiling was // raised from the arbitrary low default of 5 to 24 (env-configurable via diff --git a/src/operator_cli/self_health.rs b/src/operator_cli/self_health.rs index 46614efdd..01a6e9dcc 100644 --- a/src/operator_cli/self_health.rs +++ b/src/operator_cli/self_health.rs @@ -10,30 +10,43 @@ //! See `docs/reference/self-deploy-api.md#simard-self-health`. use crate::memory_ipc::open_reader_client; +use std::path::Path; pub(super) const SELF_HEALTH_HELP: &str = "\ Simard self-health subcommand -Usage: simard self-health [--json] [--pre-deploy-facts=N] +Usage: simard self-health [--json] [--pre-deploy-facts=N] [--acknowledge-quarantine] - --json Emit the SelfHealthReport as JSON (default: human table). - --pre-deploy-facts=N Baseline cognitive-memory fact count to compare against - (the orchestrator passes the count captured before the - swap). When omitted, the memory probe reports the live - count only. + --json Emit the SelfHealthReport as JSON (default: human table). + --pre-deploy-facts=N Baseline cognitive-memory fact count to compare against + (the orchestrator passes the count captured before the + swap). When omitted, the memory probe reports the live + count only. + --acknowledge-quarantine Acknowledge every present cognitive-memory quarantine + artifact under the state root AND the live-store subdir + `/state/`, writing a durable `.ack` sidecar + next to each so the `no_quarantine` probe stops + counting it (issue #4469). Idempotent and NON-destructive: + no artifact is deleted — the #2550 recovery asset is + retained. Use this to clear a genuinely-stuck quarantine + that freezes self-deploy. The probe is then re-run. Exit code: 0 when every probe is healthy; non-zero when any probe fails. "; -/// Parse `--json` and `--pre-deploy-facts=N` from the remaining args. +/// Parse `--json`, `--pre-deploy-facts=N`, and `--acknowledge-quarantine` from +/// the remaining args. fn parse_flags( args: impl Iterator, -) -> Result<(bool, Option), Box> { +) -> Result<(bool, Option, bool), Box> { let mut json = false; let mut pre_deploy_facts = None; + let mut acknowledge_quarantine = false; for arg in args { if arg == "--json" { json = true; + } else if arg == "--acknowledge-quarantine" { + acknowledge_quarantine = true; } else if let Some(n) = arg.strip_prefix("--pre-deploy-facts=") { pre_deploy_facts = Some( n.parse::() @@ -45,16 +58,64 @@ fn parse_flags( ); } } - Ok((json, pre_deploy_facts)) + Ok((json, pre_deploy_facts, acknowledge_quarantine)) +} + +/// Acknowledge every present cognitive-memory quarantine artifact under +/// `state_root` (issue #4469), writing a durable `.ack` sidecar next to each so +/// the `no_quarantine` probe stops counting it. Idempotent and non-destructive: +/// no artifact is deleted (the #2550 recovery asset is retained). Returns the +/// number of artifacts acknowledged. Best-effort: a per-artifact failure is +/// logged and skipped so one hostile entry cannot block clearing the rest. +/// +/// Scans the SAME directory set the `no_quarantine` probe and the cleanup sweep +/// scan — the top-level state root AND the live-store subdir +/// `/state/` (where the de-forked backend actually drops corrupt +/// snapshots) — single-sourced via +/// [`crate::state_root::quarantine_scan_dirs_under`]. Otherwise this operator +/// remediation would silently miss a stuck quarantine in `state/` (the primary +/// location) that still reddens the probe, leaving self-deploy frozen despite a +/// "success" from this command. +fn acknowledge_all_present_quarantines(state_root: &Path) -> usize { + let mut acknowledged = 0; + for dir in crate::state_root::quarantine_scan_dirs_under(state_root) { + for name in crate::self_deploy::present_quarantine_artifacts(&dir) { + match crate::self_deploy::acknowledge(&dir, &name) { + Ok(_) => acknowledged += 1, + // `?name` (Debug) escapes control chars in the untrusted quarantine + // basename to prevent log-line forgery (#4469 security review). + Err(e) => tracing::warn!( + artifact = ?name, + dir = ?dir, + error = %e, + "self_health.acknowledge_quarantine_failed: skipping one artifact (#4469)" + ), + } + } + } + acknowledged } /// Dispatch `simard self-health`. pub(super) fn dispatch_self_health_command( args: impl Iterator, ) -> Result<(), Box> { - let (json, pre_deploy_facts) = parse_flags(args)?; + let (json, pre_deploy_facts, acknowledge_quarantine) = parse_flags(args)?; let state_root = crate::state_root::simard_state_root(); + + // #4469: acknowledge present quarantines FIRST (writing durable `.ack` + // sidecars) so the re-run probe below sees a cleared `no_quarantine`. The + // artifacts themselves are retained; acknowledgement only silences the probe. + if acknowledge_quarantine { + let n = acknowledge_all_present_quarantines(&state_root); + tracing::info!( + acknowledged = n, + "self_health.acknowledge_quarantine: wrote durable .ack sidecars; \ + artifacts retained (#4469)" + ); + } + let reader = open_reader_client(&state_root)?; // A manual self-health checks THIS running binary against itself, so the @@ -149,17 +210,28 @@ mod tests { #[test] fn parse_flags_defaults() { - let (json, baseline) = parse_flags(Vec::::new().into_iter()).unwrap(); + let (json, baseline, ack) = parse_flags(Vec::::new().into_iter()).unwrap(); assert!(!json); assert_eq!(baseline, None); + assert!(!ack); } #[test] fn parse_flags_json_and_baseline() { let args = vec!["--json".to_string(), "--pre-deploy-facts=1206".to_string()]; - let (json, baseline) = parse_flags(args.into_iter()).unwrap(); + let (json, baseline, ack) = parse_flags(args.into_iter()).unwrap(); assert!(json); assert_eq!(baseline, Some(1206)); + assert!(!ack); + } + + #[test] + fn parse_flags_acknowledge_quarantine() { + let args = vec!["--acknowledge-quarantine".to_string()]; + let (json, baseline, ack) = parse_flags(args.into_iter()).unwrap(); + assert!(!json); + assert_eq!(baseline, None); + assert!(ack, "--acknowledge-quarantine must set the ack flag"); } #[test] @@ -173,4 +245,72 @@ mod tests { let args = vec!["--pre-deploy-facts=notanumber".to_string()]; assert!(parse_flags(args.into_iter()).is_err()); } + + #[test] + fn acknowledge_all_writes_sidecars_and_retains_artifacts() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + // Two quarantines + the live store + an unrelated file. + std::fs::write(root.join("cognitive.corrupt-20260101120000"), b"a").unwrap(); + std::fs::write(root.join("cognitive.wal.corrupt-20260101120000"), b"b").unwrap(); + std::fs::write(root.join("cognitive"), b"live").unwrap(); + std::fs::write(root.join("unrelated.txt"), b"x").unwrap(); + + let n = acknowledge_all_present_quarantines(root); + assert_eq!(n, 2, "both quarantines acknowledged; live store excluded"); + + // Sidecars written; artifacts and the live store retained. + assert!(root.join("cognitive.corrupt-20260101120000.ack").is_file()); + assert!( + root.join("cognitive.wal.corrupt-20260101120000.ack") + .is_file() + ); + assert!(root.join("cognitive.corrupt-20260101120000").is_file()); + assert!(root.join("cognitive").is_file()); + assert!( + !root.join("cognitive.ack").exists(), + "the live store must never be acknowledged" + ); + + // Idempotent: a second pass re-acknowledges without error or accumulation. + assert_eq!(acknowledge_all_present_quarantines(root), 2); + let markers = std::fs::read_dir(root) + .unwrap() + .flatten() + .filter(|e| crate::self_deploy::is_ack_marker_name(&e.file_name().to_string_lossy())) + .count(); + assert_eq!(markers, 2, "exactly one sidecar per quarantine"); + } + + /// #4469 regression: the operator remediation MUST cover the live-store + /// subdir `/state/` too — the primary location the de-forked + /// backend drops corrupt snapshots, which the `no_quarantine` probe and the + /// cleanup sweep both scan. Before the fix this scanned only the top level, + /// so a stuck quarantine under `state/` could never be cleared manually and + /// self-deploy stayed frozen despite a "success" from this command. + #[test] + fn acknowledge_all_covers_state_subdir() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let state = root.join("state"); + std::fs::create_dir_all(&state).unwrap(); + + // One quarantine at the top level, one under state/. + std::fs::write(root.join("cognitive.corrupt-20260101120000"), b"a").unwrap(); + std::fs::write(state.join("cognitive.corrupt-20260202120000"), b"b").unwrap(); + + let n = acknowledge_all_present_quarantines(root); + assert_eq!( + n, 2, + "quarantines under BOTH the state root and /state/ must be acknowledged" + ); + assert!(root.join("cognitive.corrupt-20260101120000.ack").is_file()); + assert!( + state.join("cognitive.corrupt-20260202120000.ack").is_file(), + "the state/ quarantine's sidecar must be written next to it" + ); + // Non-destructive: both artifacts retained. + assert!(root.join("cognitive.corrupt-20260101120000").is_file()); + assert!(state.join("cognitive.corrupt-20260202120000").is_file()); + } } diff --git a/src/operator_commands_ooda/tests/report_tests.rs b/src/operator_commands_ooda/tests/report_tests.rs index e55ed4ce1..3772df22a 100644 --- a/src/operator_commands_ooda/tests/report_tests.rs +++ b/src/operator_commands_ooda/tests/report_tests.rs @@ -4,8 +4,18 @@ use crate::{CognitiveStatistics, GoalProgress}; // --- OodaConfig defaults --- +#[serial_test::serial(cognitive_memory)] #[test] fn ooda_config_default_values() { + // Race A (issue #4433): OodaConfig::default() reads the concurrency env. + // Clear it under the serial key so the default assertion cannot observe a + // value leaked by a #2935 concurrency-env writer. + // SAFETY: serialised via #[serial(cognitive_memory)]. + unsafe { + std::env::remove_var("SIMARD_OODA_MAX_CONCURRENT"); + std::env::remove_var("SIMARD_MAX_CONCURRENT_ACTIONS"); + std::env::remove_var("SIMARD_SCALING"); + } let config = OodaConfig::default(); // Issue #2935: raised from 5 to 24 (env-configurable via SIMARD_OODA_MAX_CONCURRENT). assert_eq!(config.max_concurrent_actions, 24); @@ -157,14 +167,24 @@ fn ooda_state_has_empty_active_goals() { // --- OodaConfig --- +#[serial_test::serial(cognitive_memory)] #[test] fn ooda_config_gym_suite_id_is_progressive() { let config = OodaConfig::default(); assert_eq!(config.gym_suite_id, "progressive"); } +#[serial_test::serial(cognitive_memory)] #[test] fn ooda_config_max_concurrent_defaults_to_24() { + // Race A (issue #4433): clear the concurrency env under the serial key so + // the default assertion cannot observe a leaked #2935 writer value. + // SAFETY: serialised via #[serial(cognitive_memory)]. + unsafe { + std::env::remove_var("SIMARD_OODA_MAX_CONCURRENT"); + std::env::remove_var("SIMARD_MAX_CONCURRENT_ACTIONS"); + std::env::remove_var("SIMARD_SCALING"); + } let config = OodaConfig::default(); // Issue #2935: raised from 5 to 24 (env-configurable via SIMARD_OODA_MAX_CONCURRENT). assert_eq!(config.max_concurrent_actions, 24); diff --git a/src/self_deploy/health.rs b/src/self_deploy/health.rs index a003bcbe0..55e1b3526 100644 --- a/src/self_deploy/health.rs +++ b/src/self_deploy/health.rs @@ -151,12 +151,32 @@ fn commits_compatible(running: &str, target: &str) -> bool { r == t || r.starts_with(&t) || t.starts_with(&r) } -/// `true` for a quarantined corrupt cognitive-memory filename. Mirrors -/// `cmd_cleanup::disk::is_corrupt_quarantine_name`: both backend generations -/// leave a `cognitive*.corrupt-` artifact when a store is quarantined. -fn is_corrupt_quarantine_name(name: &str) -> bool { - (name.starts_with("cognitive.") || name.starts_with("cognitive_memory.")) - && name.contains(".corrupt-") +/// Count quarantined corrupt cognitive-memory artifacts directly under +/// `state_root`, ignoring acknowledgement sidecars. Absent/unreadable dir ⇒ `0` +/// (nothing to quarantine). +/// +/// A quarantine that carries a durable `.ack` sidecar (issue #4469) is treated +/// as "seen" and does NOT count — this is what lets a genuinely-stuck but +/// retained recovery asset clear the probe without deleting it. The `.ack` +/// sidecars themselves are never mistaken for quarantines, and a *fresh* +/// (unacknowledged) corruption event still counts because the marker is keyed +/// to the exact filename. +/// +/// Test-only helper: total count of unacknowledged quarantined corrupt +/// cognitive-memory artifacts directly under `state_root`, regardless of +/// forensic-window age. Delegates to the production [`tally_quarantine_files`] +/// scan (summing *fresh* + *retained*) so the directory-scan and +/// acknowledgement logic lives in exactly one place — the test helper can never +/// drift from the production probe path (#4469 philosophy review S5). Compiled +/// only under `#[cfg(test)]`. +#[cfg(test)] +fn count_quarantine_files(state_root: &std::path::Path) -> u64 { + // Any window start yields the same total: an artifact is either fresh + // (mtime ≥ window) or retained (mtime < window), and this helper wants the + // age-agnostic sum. Acknowledged artifacts and `.ack` sidecars are already + // excluded by the production scan. + let tally = tally_quarantine_files(state_root, Utc::now()); + tally.fresh + tally.retained } /// Tally quarantined corrupt cognitive-memory artifacts directly under `dir`, @@ -171,6 +191,12 @@ fn is_corrupt_quarantine_name(name: &str) -> bool { /// window is genuine post-deploy corruption and is still counted, so the probe /// is not neutered into always passing. /// +/// Acknowledgement-aware (issue #4469): a quarantine carrying a durable `.ack` +/// sidecar is treated as "seen" and counts as neither fresh nor retained, so a +/// genuinely-stuck but acknowledged protected recovery asset can clear the +/// probe without being deleted. The `.ack` sidecar files themselves are never +/// counted as quarantines. +/// /// Absent/unreadable dir ⇒ `(0, 0)` (nothing to quarantine). Entries whose /// mtime cannot be read are skipped entirely (fail-safe: never counted fresh, /// and not surfaced as retained either since freshness is unknowable). @@ -181,7 +207,20 @@ fn tally_quarantine_files(dir: &std::path::Path, window_start: DateTime) -> }; let mut tally = QuarantineTally::default(); for entry in entries.flatten() { - if !is_corrupt_quarantine_name(&entry.file_name().to_string_lossy()) { + // Borrow the lossy name instead of forcing a `String` per entry: most + // entries fail the predicate below and are skipped, so avoid the + // per-entry heap allocation. + let file_name = entry.file_name(); + let name = file_name.to_string_lossy(); + if !crate::cmd_cleanup::is_corrupt_quarantine_name(&name) { + continue; + } + // #4469: `.ack` sidecars are never quarantines, and an acknowledged + // artifact is "seen" — it counts as neither fresh nor retained so a + // stuck protected recovery asset can converge without being deleted. + if crate::self_deploy::quarantine_ack::is_ack_marker_name(&name) + || crate::self_deploy::quarantine_ack::is_acknowledged(dir, &name) + { continue; } let Ok(mtime) = entry.metadata().and_then(|m| m.modified()) else { @@ -205,13 +244,89 @@ struct QuarantineTally { retained: u64, } +/// Guarded autonomous auto-ack (#4469): if the #2550 protected recovery asset +/// under `state_root` is past the forensic window and not already acknowledged, +/// durably acknowledge it WITHOUT deleting the retained asset. Best-effort: +/// emits a structured tracing/OTel WARN and continues on any error (never +/// `print!`). Returns the acknowledged artifact basename when it fired, else +/// `None`. +/// +/// The "protected recovery asset" selection and the forensic-window age gate are +/// single-sourced from [`crate::cmd_cleanup::disk`], so the probe and the cleanup +/// sweep can never disagree about which artifact is protected. Fresh corruption +/// (young, or not the protected asset) is never eligible and still reddens the +/// probe. +/// +/// **What this does and does NOT do.** This is *defense-in-depth*, not the +/// primary convergence mechanism. Under the current callers the observation +/// window is recent ([`run_self_health_probe`] receives `now` from the +/// orchestrator, `now - 5m` from the operator CLI), so the aged protected asset +/// this path targets — mtime at least [`CORRUPT_DB_MAX_AGE_DAYS`] old — is +/// already counted `retained`, never `fresh`, and the `no_quarantine` probe +/// therefore already passes on it via the fresh-window semantics of +/// [`tally_quarantine_files`] (see +/// `no_quarantine_passes_with_only_historical_quarantines_in_state_dir`). The +/// auto-ack adds two guarantees on top: it drops the aged protected asset out of +/// the `retained` diagnostic, and it keeps the probe green even if a caller were +/// to pass an observation window *older* than the forensic age (which would +/// otherwise re-classify the aged asset as `fresh`). The escape hatch for a +/// genuinely-stuck *fresh* quarantine is the manual `--acknowledge-quarantine` +/// operator command, which the auto-ack deliberately does NOT replicate — this +/// path fires unattended and must never silence genuine fresh corruption. +/// +/// Deliberate asymmetry with the manual +/// [`acknowledge`](crate::self_deploy::quarantine_ack::acknowledge) path: the +/// operator `--acknowledge-quarantine` CLI acknowledges *any* present, ackable +/// artifact the operator explicitly chooses, whereas this *automatic* probe path +/// narrows itself to the single aged #2550 protected recovery asset, precisely +/// because it fires unattended and must not silently acknowledge genuine fresh +/// corruption. +fn auto_ack_stuck_recovery_asset(state_root: &std::path::Path) -> Option { + let name = crate::cmd_cleanup::disk::aged_protected_recovery_asset(state_root)?; + if crate::self_deploy::quarantine_ack::is_acknowledged(state_root, &name) { + return None; + } + match crate::self_deploy::quarantine_ack::acknowledge(state_root, &name) { + Ok(marker) => { + // Both `artifact` and `marker` embed the untrusted quarantine + // basename (the marker is `{name}.ack`); an on-disk filename may + // contain a newline (a single `Component::Normal` on Unix), so log + // BOTH via Debug (`?`) — which escapes control chars — to prevent + // log-line forgery under the default non-JSON subscriber (#4469 + // security review, LOW-1/LOW-2). + tracing::warn!( + artifact = ?name, + marker = ?marker, + min_age_days = crate::cmd_cleanup::disk::CORRUPT_DB_MAX_AGE_DAYS, + "self_deploy.quarantine.auto_ack: durably acknowledged aged #2550 \ + protected recovery asset as defense-in-depth (#4469); artifact \ + retained on disk" + ); + Some(name) + } + Err(e) => { + tracing::warn!( + artifact = ?name, + error = %e, + "self_deploy.quarantine.auto_ack_failed: could not acknowledge aged \ + protected recovery asset (#4469)" + ); + None + } + } +} + /// Run the post-deploy probes against the live daemon and assemble a report. /// /// Effectful: reads the running build commit, the live memory fact count, the /// goal board, recent `brain_parse_failure` metrics, and the store quarantine -/// state. Every probe degrades to `healthy: false` on its own error rather than -/// aborting the whole report, so the orchestrator always gets a verdict to act -/// on (and rolls back on any unhealthy probe). +/// state. The `no_quarantine` probe additionally has an intentional *write* +/// side-effect — it may durably write one `.ack` sidecar via the guarded +/// `auto_ack_stuck_recovery_asset` auto-ack (aged #2550 protected recovery +/// asset only; #4469) so the deadlock can self-clear on the unattended +/// post-deploy path. Every probe degrades to `healthy: false` on its own error +/// rather than aborting the whole report, so the orchestrator always gets a +/// verdict to act on (and rolls back on any unhealthy probe). /// /// * `target_commit` — the commit the candidate was built from. /// * `baseline_facts` — pre-deploy memory count (the orchestrator captures it); @@ -280,13 +395,39 @@ pub fn run_self_health_probe( }; // Probe 5: no *fresh* quarantined corrupt cognitive-memory store. Scans the - // live-store directory `/state/` (where LadybugDB drops corrupt - // snapshots next to the live `cognitive` store) — the SAME directory - // `cmd_cleanup::disk` reclaims. Only quarantines at/after the window start - // count, so retained historical forensic snapshots don't fail the probe - // forever, but genuine post-deploy corruption still does (issue #4469). - let live_store_dir = crate::state_root::resolve_subdir("state"); - let quarantine_tally = tally_quarantine_files(&live_store_dir, fallback_window_start); + // SAME directory set the cleanup sweep reclaims — the top-level state root + // AND the live-store subdir `/state/` (where LadybugDB drops + // corrupt snapshots next to the live `cognitive` store) — single-sourced + // from `state_root::quarantine_scan_dirs`, so the probe and + // `cmd_cleanup::disk` can never disagree on where quarantines live. Only + // quarantines at/after the window start count as fresh, so retained + // historical forensic snapshots don't fail the probe forever, but genuine + // post-deploy corruption still does (issue #4469). + // + // #4469: before counting, run the guarded autonomous auto-ack against each + // of those directories as *defense-in-depth*. The `no_quarantine` probe + // already converges on retained (old) quarantines via the fresh-window + // semantics below — only quarantines with mtime at/after `window_start` + // count as fresh. The auto-ack additionally acknowledges the #2550 protected + // recovery asset once it ages past the forensic window (durable `.ack` + // sidecar, WITHOUT deleting it) so it drops out of the `retained` diagnostic + // and the probe stays green even if a caller passes an observation window + // older than the forensic age. It lives on the probe path (not just the + // operator CLI) so it also fires for the orchestrator's unattended + // post-deploy health check. Fresh corruption is never eligible for auto-ack; + // a genuinely-stuck *fresh* quarantine is cleared by the operator's manual + // `--acknowledge-quarantine` command. + let mut quarantine_tally = QuarantineTally::default(); + for dir in crate::state_root::quarantine_scan_dirs() { + // Best-effort guarded auto-ack: the return value (which artifact, if any, + // was acked) is intentionally discarded here — success/failure is logged + // internally via structured tracing/OTel WARN inside the helper, and the + // subsequent tally re-scans the directory to reflect any new `.ack`. + let _ = auto_ack_stuck_recovery_asset(&dir); + let dir_tally = tally_quarantine_files(&dir, fallback_window_start); + quarantine_tally.fresh += dir_tally.fresh; + quarantine_tally.retained += dir_tally.retained; + } let quarantined = quarantine_tally.fresh > 0; let no_quarantine = NoQuarantineProbe { healthy: !quarantined, @@ -580,6 +721,172 @@ mod probe_logic_tests { assert_eq!(tally.retained, 0); } + // ── #4469: acknowledgement-aware quarantine scan ── + // The `no_quarantine` probe must stop failing on a quarantine that carries a + // durable `.ack` sidecar, so a genuinely-stuck (but protected/retained) + // corrupt store can clear without deleting the recovery asset. + + #[test] + fn quarantine_scan_ignores_acknowledged_artifact_and_its_marker() { + let dir = tempdir().unwrap(); + // A quarantined corrupt store that has been durably acknowledged. + std::fs::write(dir.path().join("cognitive.corrupt-20260101"), b"x").unwrap(); + std::fs::write(dir.path().join("cognitive.corrupt-20260101.ack"), b"").unwrap(); + // An acked artifact does not count, and the `.ack` sidecar itself is + // never mistaken for a quarantine. + assert_eq!( + count_quarantine_files(dir.path()), + 0, + "acknowledged quarantine (and its marker) must not fail the probe" + ); + } + + #[test] + fn quarantine_scan_still_flags_fresh_corruption_after_ack() { + let dir = tempdir().unwrap(); + // Old, acknowledged quarantine. + std::fs::write(dir.path().join("cognitive.corrupt-20260101"), b"x").unwrap(); + std::fs::write(dir.path().join("cognitive.corrupt-20260101.ack"), b"").unwrap(); + // A NEW corruption event — filename-keyed markers must not silence it. + std::fs::write(dir.path().join("cognitive.corrupt-20260202"), b"x").unwrap(); + assert_eq!( + count_quarantine_files(dir.path()), + 1, + "fresh corruption must re-fail the probe despite an earlier ack" + ); + } + + #[test] + fn quarantine_scan_end_to_end_ack_clears_probe() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("cognitive.corrupt-20260101120000"), b"x").unwrap(); + assert_eq!( + count_quarantine_files(dir.path()), + 1, + "unacked = quarantined" + ); + + crate::self_deploy::quarantine_ack::acknowledge( + dir.path(), + "cognitive.corrupt-20260101120000", + ) + .expect("acknowledge succeeds"); + assert_eq!( + count_quarantine_files(dir.path()), + 0, + "durable ack clears the no_quarantine probe" + ); + } + + // ── #4469: guarded autonomous auto-ack of the stuck recovery asset ── + // The #2550 protected recovery asset (largest quarantine ≥ 1 MB) is retained + // forever yet keeps `no_quarantine` red, so it can never clear on its own. + // Once it ages past the forensic window the probe auto-acks it — and only it. + + /// Backdate a path's mtime `days` into the past (plus slack). + fn backdate(path: &std::path::Path, days: u64) { + let when = + std::time::SystemTime::now() - std::time::Duration::from_secs(days * 24 * 3600 + 3600); + let times = std::fs::FileTimes::new().set_modified(when); + std::fs::File::options() + .write(true) + .open(path) + .unwrap() + .set_times(times) + .unwrap(); + } + + const PROTECT_MIN: u64 = crate::cmd_cleanup::disk::CORRUPT_DB_PROTECT_MIN_BYTES; + const MAX_AGE_DAYS: u64 = crate::cmd_cleanup::disk::CORRUPT_DB_MAX_AGE_DAYS; + + #[test] + fn auto_ack_clears_aged_protected_recovery_asset_and_retains_it() { + let dir = tempdir().unwrap(); + let asset = dir.path().join("cognitive.corrupt-20260101120000"); + std::fs::write(&asset, vec![0u8; (PROTECT_MIN + 512) as usize]).unwrap(); + backdate(&asset, MAX_AGE_DAYS + 1); + + // Before: the aged protected asset keeps the probe red. + assert_eq!( + count_quarantine_files(dir.path()), + 1, + "unacked = quarantined" + ); + + let acked = auto_ack_stuck_recovery_asset(dir.path()); + assert_eq!( + acked.as_deref(), + Some("cognitive.corrupt-20260101120000"), + "auto-ack must fire for the aged protected asset" + ); + // After: probe clears, artifact retained, sidecar written. + assert_eq!( + count_quarantine_files(dir.path()), + 0, + "auto-ack clears the probe" + ); + assert!(asset.is_file(), "the recovery asset must be retained"); + assert!( + dir.path() + .join("cognitive.corrupt-20260101120000.ack") + .is_file() + ); + } + + #[test] + fn auto_ack_ignores_fresh_protected_asset() { + let dir = tempdir().unwrap(); + // Large enough to be "protected", but INSIDE the forensic window. + let asset = dir.path().join("cognitive.corrupt-20260101120000"); + std::fs::write(&asset, vec![0u8; (PROTECT_MIN + 512) as usize]).unwrap(); + backdate(&asset, MAX_AGE_DAYS.saturating_sub(2)); + + assert_eq!( + auto_ack_stuck_recovery_asset(dir.path()), + None, + "fresh asset not eligible" + ); + assert_eq!( + count_quarantine_files(dir.path()), + 1, + "fresh quarantine still reddens" + ); + } + + #[test] + fn auto_ack_ignores_trivial_aged_quarantine() { + let dir = tempdir().unwrap(); + // Aged, but below the protection floor — not the recovery asset. + let small = dir.path().join("cognitive.corrupt-20260101120000"); + std::fs::write(&small, b"tiny").unwrap(); + backdate(&small, MAX_AGE_DAYS + 5); + + assert_eq!( + auto_ack_stuck_recovery_asset(dir.path()), + None, + "a trivial (sub-floor) quarantine is never auto-acked" + ); + assert_eq!(count_quarantine_files(dir.path()), 1); + } + + #[test] + fn auto_ack_is_idempotent() { + let dir = tempdir().unwrap(); + let asset = dir.path().join("cognitive.corrupt-20260101120000"); + std::fs::write(&asset, vec![0u8; (PROTECT_MIN + 512) as usize]).unwrap(); + backdate(&asset, MAX_AGE_DAYS + 1); + + assert!( + auto_ack_stuck_recovery_asset(dir.path()).is_some(), + "first pass acks" + ); + assert_eq!( + auto_ack_stuck_recovery_asset(dir.path()), + None, + "already acknowledged ⇒ no repeat ack" + ); + } + #[test] fn entrypoint_parity_healthy_on_path_identity_and_version_match() { let probe = evaluate_entrypoint_parity( diff --git a/src/self_deploy/mod.rs b/src/self_deploy/mod.rs index 6153c9697..9f0a3cbe8 100644 --- a/src/self_deploy/mod.rs +++ b/src/self_deploy/mod.rs @@ -26,6 +26,7 @@ pub mod drift; pub mod health; pub mod orchestrator; pub mod orphan; +pub mod quarantine_ack; pub mod requeue; pub mod restart; pub mod source_prep; @@ -43,6 +44,9 @@ pub use orchestrator::{DeploySourceKind, SelfDeployOrchestrator, SelfDeployOutco pub use orphan::{ OrphanEngineer, find_engineer_orphans, match_engineer_orphan, reap_engineer_orphans, }; +pub use quarantine_ack::{ + ack_marker_path, acknowledge, is_ack_marker_name, is_acknowledged, present_quarantine_artifacts, +}; pub use requeue::ProdEngineerRequeue; pub use restart::{DaemonRestarter, FakeRestarter, SystemdOrExecRestarter}; pub use source_prep::{ diff --git a/src/self_deploy/quarantine_ack.rs b/src/self_deploy/quarantine_ack.rs new file mode 100644 index 000000000..bd7cc3908 --- /dev/null +++ b/src/self_deploy/quarantine_ack.rs @@ -0,0 +1,332 @@ +//! Durable quarantine acknowledgement (`.ack` sidecars) — issue #4469. +//! +//! When LadybugDB quarantines a corrupt cognitive-memory store it leaves a +//! `cognitive*.corrupt-` artifact under the state root. The self-health +//! `no_quarantine` probe fails while any such artifact is present, and the +//! #2550 retention rule protects the largest substantial quarantine from the +//! cleanup sweep — so a genuinely-stuck quarantine can freeze self-deploy +//! forever (the probe never clears, but the recovery asset must not be +//! deleted). +//! +//! This module owns the single convention that breaks that deadlock **without +//! destroying data**: a durable, per-artifact `.ack` sidecar. Acknowledging a +//! quarantine writes `/.ack`; the probe and the cleanup sweep +//! both treat an artifact with a live `.ack` sidecar as "seen" and stop failing +//! on it, while the quarantined store itself is retained on disk for recovery. +//! +//! The marker is **filename-keyed** (the quarantine name embeds a timestamp), +//! so acknowledging `cognitive.corrupt-20260101` never silences a *new* +//! `cognitive.corrupt-20260202` — fresh corruption still re-fails the probe. +//! +//! ## Contract +//! +//! * [`ack_marker_path`] — the sidecar path for a *valid* corrupt-quarantine +//! basename directly under `state_root`; `None` for any unsafe / non-quarantine +//! name (path separators, `..`, absolute paths, the live store). +//! * [`acknowledge`] — idempotently write the sidecar. Never deletes the +//! quarantined artifact. Refuses unsafe names and refuses to overwrite a +//! non-regular-file sidecar target (planted symlink defence). +//! * [`is_acknowledged`] — true iff a durable regular-file sidecar exists. +//! * [`is_ack_marker_name`] — true for the sidecar files themselves (`*.ack`), +//! so scanners never mistake a marker for a quarantine. +//! +//! See `docs/reference/self-deploy-quarantine-acknowledge.md` and +//! `docs/howto/clear-a-stuck-memory-quarantine.md`. + +use std::ffi::OsStr; +use std::io::Write; +use std::path::{Component, Path, PathBuf}; + +use crate::error::{SimardError, SimardResult}; + +/// Suffix appended to a quarantine artifact's name to form its durable +/// acknowledgement sidecar. +pub const ACK_SUFFIX: &str = ".ack"; + +/// The sidecar's payload. The marker is a presence flag, not a data store; +/// keeping it tiny bounds disk use and forgery blast radius. +const ACK_MARKER_BYTES: &[u8] = b"acknowledged\n"; + +/// True when `name` is an acknowledgement sidecar (`*.ack`) rather than a +/// quarantine artifact. Scanners MUST exclude these so a marker is never +/// itself treated as a corrupt store. +pub fn is_ack_marker_name(name: &str) -> bool { + name.ends_with(ACK_SUFFIX) +} + +/// True iff `name` is a safe, single-component corrupt-quarantine basename that +/// may be acknowledged: no separators, no `..`/absolute components, non-empty, +/// not itself an `.ack` marker, and a genuine `cognitive*.corrupt-*` artifact. +fn is_ackable_quarantine_basename(name: &str) -> bool { + if name.is_empty() || is_ack_marker_name(name) { + return false; + } + if name.contains('/') || name.contains('\\') { + return false; + } + // Exactly one Normal component, equal to the whole name (rejects `..`, `.`, + // absolute prefixes, and anything platform-specific like a drive/root). + let mut components = Path::new(name).components(); + match (components.next(), components.next()) { + (Some(Component::Normal(c)), None) if c == OsStr::new(name) => {} + _ => return false, + } + // Delegate to the canonical predicate so the cleanup sweep, the health + // probe, and this acknowledge path can never disagree about which artifacts + // are corrupt-quarantines. + crate::cmd_cleanup::is_corrupt_quarantine_name(name) +} + +/// Compute the durable ack-marker path for the corrupt-quarantine artifact +/// `quarantine_name` directly under `state_root`. +/// +/// Returns `None` when `quarantine_name` is not a safe, single-component +/// corrupt-quarantine basename: anything containing a path separator, a `..` +/// component, an absolute path, an empty string, an existing `.ack` marker +/// name, or a name that is not a corrupt-quarantine artifact is rejected. +/// +/// Kept `pub` (not `pub(crate)`): the `self_deploy_convergence` integration +/// test — a separate crate — asserts the marker path against this helper, so +/// narrowing visibility would break the build. +pub fn ack_marker_path(state_root: &Path, quarantine_name: &str) -> Option { + if !is_ackable_quarantine_basename(quarantine_name) { + return None; + } + Some(state_root.join(format!("{quarantine_name}{ACK_SUFFIX}"))) +} + +/// Build the `PersistentStoreIo` error used for every acknowledgement failure. +fn ack_error(path: PathBuf, reason: impl Into) -> SimardError { + SimardError::PersistentStoreIo { + store: "cognitive_memory_quarantine".to_string(), + action: "acknowledge".to_string(), + path, + reason: reason.into(), + } +} + +/// Durably acknowledge the quarantine artifact `quarantine_name` under +/// `state_root` by writing its `.ack` sidecar. +/// +/// * **Idempotent** — acknowledging an already-acknowledged artifact succeeds +/// and leaves a single sidecar. +/// * **Non-destructive** — the quarantined artifact itself is never touched. +/// * **Safe** — rejects unsafe names (see [`ack_marker_path`]) and refuses to +/// overwrite a sidecar path that already exists as a non-regular file (a +/// planted symlink or directory), returning `Err` rather than following it. +/// +/// Returns the written sidecar path on success. +pub fn acknowledge(state_root: &Path, quarantine_name: &str) -> SimardResult { + let marker = ack_marker_path(state_root, quarantine_name).ok_or_else(|| { + ack_error( + state_root.join(quarantine_name), + format!("refusing to acknowledge unsafe or non-quarantine name {quarantine_name:?}"), + ) + })?; + + // Inspect the sidecar path WITHOUT following symlinks. A pre-existing + // regular file means the artifact is already acknowledged (idempotent); + // anything else at that path (symlink, directory) is a hostile plant we + // refuse to touch rather than write through. + match std::fs::symlink_metadata(&marker) { + Ok(meta) if meta.file_type().is_file() => return Ok(marker), + Ok(_) => { + return Err(ack_error( + marker, + "sidecar path already exists as a non-regular file (symlink/dir); refusing to overwrite", + )); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(ack_error(marker, format!("stat sidecar: {e}"))), + } + + // `create_new` opens with O_EXCL: it never follows a symlink and fails if + // the path already exists, closing the TOCTOU window from the stat above. + let mut file = match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&marker) + { + Ok(f) => f, + // Lost a race but the winner left a regular file — still acknowledged. + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + return match std::fs::symlink_metadata(&marker) { + Ok(meta) if meta.file_type().is_file() => Ok(marker), + _ => Err(ack_error( + marker, + "sidecar path raced into a non-regular file; refusing to overwrite", + )), + }; + } + Err(e) => return Err(ack_error(marker, format!("create sidecar: {e}"))), + }; + file.write_all(ACK_MARKER_BYTES) + .and_then(|()| file.sync_all()) + .map_err(|e| ack_error(marker.clone(), format!("write sidecar: {e}")))?; + Ok(marker) +} + +/// True when a durable regular-file `.ack` sidecar exists for +/// `quarantine_name` under `state_root`. A non-regular-file at the sidecar +/// path (symlink, directory) is NOT a valid acknowledgement. +pub fn is_acknowledged(state_root: &Path, quarantine_name: &str) -> bool { + match ack_marker_path(state_root, quarantine_name) { + Some(marker) => std::fs::symlink_metadata(&marker) + .map(|meta| meta.file_type().is_file()) + .unwrap_or(false), + None => false, + } +} + +/// List the acknowledgeable corrupt-quarantine artifact basenames present +/// directly under `state_root` (issue #4469). +/// +/// Excludes `.ack` sidecars and anything that is not a safe, single-component +/// `cognitive*.corrupt-*` artifact (so the live store is never returned). The +/// operator `--acknowledge-quarantine` path iterates this list, keeping +/// `quarantine_ack` the single owner of "what is an acknowledgeable quarantine". +/// Absent/unreadable dir ⇒ empty. +pub fn present_quarantine_artifacts(state_root: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(state_root) else { + return Vec::new(); + }; + entries + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|name| is_ackable_quarantine_basename(name)) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + const QUARANTINE: &str = "cognitive.corrupt-20260101120000"; + + // ── is_ack_marker_name ── + + #[test] + fn ack_marker_name_matches_only_ack_suffix() { + assert!(is_ack_marker_name("cognitive.corrupt-20260101.ack")); + assert!(!is_ack_marker_name("cognitive.corrupt-20260101")); + assert!(!is_ack_marker_name("cognitive")); + } + + // ── ack_marker_path ── + + #[test] + fn marker_path_is_sibling_with_ack_suffix() { + let root = Path::new("/var/lib/simard"); + let marker = ack_marker_path(root, QUARANTINE).expect("valid quarantine name"); + assert_eq!(marker, root.join(format!("{QUARANTINE}{ACK_SUFFIX}"))); + // Marker is a single component directly under the state root. + assert_eq!(marker.parent(), Some(root)); + assert_eq!( + marker.file_name().and_then(|s| s.to_str()), + Some(format!("{QUARANTINE}{ACK_SUFFIX}").as_str()) + ); + } + + #[test] + fn marker_path_rejects_path_separators() { + let root = Path::new("/var/lib/simard"); + assert!(ack_marker_path(root, "cognitive.corrupt-1/evil").is_none()); + assert!(ack_marker_path(root, "sub/cognitive.corrupt-1").is_none()); + assert!(ack_marker_path(root, "cognitive.corrupt-1\\evil").is_none()); + } + + #[test] + fn marker_path_rejects_parent_and_absolute() { + let root = Path::new("/var/lib/simard"); + assert!(ack_marker_path(root, "..").is_none()); + assert!(ack_marker_path(root, "../cognitive.corrupt-1").is_none()); + assert!(ack_marker_path(root, "/etc/passwd").is_none()); + assert!(ack_marker_path(root, "").is_none()); + } + + #[test] + fn marker_path_rejects_non_quarantine_and_marker_names() { + let root = Path::new("/var/lib/simard"); + // The live store and unrelated files are not acknowledgeable. + assert!(ack_marker_path(root, "cognitive").is_none()); + assert!(ack_marker_path(root, "cognitive.wal").is_none()); + assert!(ack_marker_path(root, "unrelated.corrupt-1").is_none()); + // An existing marker must not be re-acknowledged into `*.ack.ack`. + assert!(ack_marker_path(root, "cognitive.corrupt-1.ack").is_none()); + } + + // ── acknowledge / is_acknowledged ── + + #[test] + fn acknowledge_creates_durable_marker_and_retains_artifact() { + let dir = tempfile::tempdir().unwrap(); + let artifact = dir.path().join(QUARANTINE); + std::fs::write(&artifact, b"quarantined-store-bytes").unwrap(); + + assert!(!is_acknowledged(dir.path(), QUARANTINE)); + + let marker = acknowledge(dir.path(), QUARANTINE).expect("acknowledge succeeds"); + assert!(marker.is_file(), "sidecar must be a regular file"); + assert!(is_acknowledged(dir.path(), QUARANTINE)); + // Non-destructive: the quarantined artifact is retained for recovery. + assert!(artifact.is_file(), "quarantine artifact must be retained"); + } + + #[test] + fn acknowledge_is_idempotent() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(QUARANTINE), b"x").unwrap(); + + let first = acknowledge(dir.path(), QUARANTINE).unwrap(); + let second = acknowledge(dir.path(), QUARANTINE).unwrap(); + assert_eq!(first, second, "same marker path on repeat ack"); + assert!(is_acknowledged(dir.path(), QUARANTINE)); + + // Exactly one sidecar exists for this artifact. + let markers = std::fs::read_dir(dir.path()) + .unwrap() + .flatten() + .filter(|e| is_ack_marker_name(&e.file_name().to_string_lossy())) + .count(); + assert_eq!(markers, 1); + } + + #[test] + fn acknowledge_rejects_unsafe_names() { + let dir = tempfile::tempdir().unwrap(); + assert!(acknowledge(dir.path(), "../escape").is_err()); + assert!(acknowledge(dir.path(), "sub/cognitive.corrupt-1").is_err()); + assert!(acknowledge(dir.path(), "/etc/passwd").is_err()); + assert!(acknowledge(dir.path(), "cognitive").is_err()); + } + + #[cfg(unix)] + #[test] + fn acknowledge_refuses_to_overwrite_planted_symlink_marker() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(QUARANTINE), b"x").unwrap(); + + // Plant a hostile sidecar that points at a sensitive file. + let victim = dir.path().join("victim.txt"); + std::fs::write(&victim, b"original").unwrap(); + let marker = dir.path().join(format!("{QUARANTINE}{ACK_SUFFIX}")); + std::os::unix::fs::symlink(&victim, &marker).unwrap(); + + // Acknowledgement must refuse rather than follow the symlink. + assert!( + acknowledge(dir.path(), QUARANTINE).is_err(), + "must not overwrite a non-regular-file sidecar" + ); + // The victim's contents must be untouched (no write-through). + assert_eq!(std::fs::read(&victim).unwrap(), b"original"); + // A symlink is not a valid acknowledgement. + assert!(!is_acknowledged(dir.path(), QUARANTINE)); + } + + #[test] + fn is_acknowledged_false_without_marker() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(QUARANTINE), b"x").unwrap(); + assert!(!is_acknowledged(dir.path(), QUARANTINE)); + } +} diff --git a/src/self_deploy/tests_health.rs b/src/self_deploy/tests_health.rs index 1aa43aaac..84c020618 100644 --- a/src/self_deploy/tests_health.rs +++ b/src/self_deploy/tests_health.rs @@ -262,16 +262,19 @@ fn no_quarantine_passes_with_only_historical_quarantines_in_state_dir() { ); } -/// Directory-targeting regression: a fresh quarantine at TOP-LEVEL -/// `/` (the pre-fix scan location) must NOT fail the probe, because -/// the live store and its quarantines live under `/state/`. This -/// pins that probe and cleanup agree on the same live-store directory. +/// Directory-parity contract (#4469): the `no_quarantine` probe scans the SAME +/// directory set the cleanup sweep reclaims — BOTH the live-store subdir +/// `/state/` AND the top-level `/` (the native pre-#2307 +/// quarantine location). A fresh quarantine at the top level must therefore fail +/// the probe too, so the probe and cleanup can never disagree on where +/// quarantines live. (The `state/` side is covered by +/// `no_quarantine_fails_on_fresh_quarantine_in_state_dir`.) #[test] #[serial_test::serial(simard_state_root_env, cognitive_memory)] -fn no_quarantine_scans_state_dir_not_top_level() { +fn no_quarantine_scans_both_state_root_and_state_subdir() { let root = tempfile::tempdir().unwrap(); let _g = StateRootGuard::set(root.path()); - // Empty live-store dir; the only quarantine is at the wrong (top) level. + // Empty live-store dir; the only quarantine is at the TOP level. std::fs::create_dir_all(root.path().join("state")).unwrap(); std::fs::write(root.path().join("cognitive.corrupt-toplevel"), b"corrupt").unwrap(); @@ -280,8 +283,10 @@ fn no_quarantine_scans_state_dir_not_top_level() { let report = run_self_health_probe(&mem, "deadbeef", None, 0, window).unwrap(); assert!( - !report.probes.no_quarantine.quarantined, - "the probe must scan /state/, not top-level " + report.probes.no_quarantine.quarantined, + "a fresh top-level quarantine must fail the probe — it scans the same \ + directory set (top-level AND state/) that cleanup sweeps" ); - assert!(report.probes.no_quarantine.healthy); + assert!(!report.probes.no_quarantine.healthy); + assert_eq!(report.probes.no_quarantine.fresh_quarantines, 1); } diff --git a/src/self_relaunch/gates.rs b/src/self_relaunch/gates.rs index 78a120ca1..f040d5668 100644 --- a/src/self_relaunch/gates.rs +++ b/src/self_relaunch/gates.rs @@ -76,12 +76,31 @@ fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { detail: "all tests passed".to_string(), }, Ok(output) => { + let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); - let truncated = truncate_output(&stderr, 200); + // Surface WHICH test produced the red canary (#4470) instead of an + // opaque "exit 101". `cargo test` prints the `... FAILED` lines to + // stdout; fall back to a sanitized stderr tail when none is found + // (e.g. a compile error rather than a test failure). + let failing = extract_first_failure(&stdout).or_else(|| extract_first_failure(&stderr)); + let detail = match failing { + Some(test) => format!( + "tests failed (exit {}): first failing test {}", + output.status, test + ), + None => format!( + "tests failed (exit {}): {}", + output.status, + crate::util::log_sanitize::sanitize_to_single_line( + &stderr, + GATE_DETAIL_MAX_BYTES + ) + ), + }; GateResult { gate: RelaunchGate::UnitTest, passed: false, - detail: format!("tests failed (exit {}): {}", output.status, truncated), + detail, } } Err(e) => GateResult { @@ -144,18 +163,41 @@ fn run_rpc_health_gate(binary: &Path, config: &RelaunchConfig) -> GateResult { } } -fn truncate_output(s: &str, max_len: usize) -> String { - if s.len() <= max_len { - s.trim().to_string() - } else { - // Use char-boundary-safe truncation to avoid panic on multi-byte UTF-8. - let boundary = s - .char_indices() - .take_while(|(i, _)| *i < max_len) - .last() - .map_or(0, |(i, c)| i + c.len_utf8()); - format!("{}...", s[..boundary].trim()) +/// Upper bound (bytes) on any gate `detail` derived from untrusted subprocess +/// output (#4470). Bounds log/JSON size and blast radius of forged content. +pub(crate) const GATE_DETAIL_MAX_BYTES: usize = 512; + +/// Extract the fully-qualified path of the FIRST failing test from `cargo test` +/// stdout/stderr (#4470 diagnosability). +/// +/// `cargo test` prints `test module::path::name ... FAILED` for each failure and +/// a `failures:` summary block listing ` module::path::name`. This returns the +/// first failing test's path (e.g. `self_deploy::tests_health::foo`), sanitized +/// via [`crate::util::log_sanitize::sanitize_to_single_line`] and bounded to +/// [`GATE_DETAIL_MAX_BYTES`], so the canary can surface WHICH test produced the +/// red canary instead of an opaque "exit 101". Returns `None` when no +/// failing-test line is present. +pub(crate) fn extract_first_failure(cargo_test_output: &str) -> Option { + for line in cargo_test_output.lines() { + // Per-test result lines look like `test ... FAILED`. The summary + // line `test result: FAILED. ...` starts with `test ` too but never ends + // in ` ... FAILED`, so the suffix match below excludes it. + let trimmed = line.trim(); + let Some(rest) = trimmed.strip_prefix("test ") else { + continue; + }; + let Some(path) = rest.strip_suffix(" ... FAILED") else { + continue; + }; + let path = path.trim(); + if !path.is_empty() { + return Some(crate::util::log_sanitize::sanitize_to_single_line( + path, + GATE_DETAIL_MAX_BYTES, + )); + } } + None } #[cfg(test)] @@ -168,61 +210,6 @@ mod tests { assert!(!result.passed); } - // --- truncate_output --- - - #[test] - fn truncate_output_short_string_unchanged() { - let result = truncate_output("hello world", 100); - assert_eq!(result, "hello world"); - } - - #[test] - fn truncate_output_exact_length() { - let input = "abcde"; - let result = truncate_output(input, 5); - assert_eq!(result, "abcde"); - } - - #[test] - fn truncate_output_over_limit_appends_ellipsis() { - let input = "abcdefghij"; - let result = truncate_output(input, 5); - assert!( - result.ends_with("..."), - "should end with ellipsis: {result}" - ); - assert!(result.len() <= 8, "should be truncated: {result}"); - } - - #[test] - fn truncate_output_trims_whitespace() { - let result = truncate_output(" hello ", 100); - assert_eq!(result, "hello"); - } - - #[test] - fn truncate_output_empty_string() { - let result = truncate_output("", 100); - assert_eq!(result, ""); - } - - #[test] - fn truncate_output_multibyte_utf8_safe() { - let input = "héllo wörld café"; - let result = truncate_output(input, 8); - assert!( - result.ends_with("..."), - "should end with ellipsis: {result}" - ); - // Must not panic on multi-byte boundary - } - - #[test] - fn truncate_output_zero_max_len() { - let result = truncate_output("hello", 0); - assert_eq!(result, "..."); - } - // --- all_gates_passed --- #[test] @@ -316,4 +303,54 @@ mod tests { let results = verify_canary(Path::new("/no-such-binary"), &[], &config).unwrap(); assert!(results.is_empty()); } + + // ── #4470: failing-test diagnosability (extract_first_failure / sanitize) ── + + #[test] + fn extract_first_failure_parses_failed_test_path() { + let output = "\ +running 3 tests +test self_deploy::tests_health::report_is_healthy_only_when_every_probe_is_healthy ... ok +test self_deploy::tests_health::any_single_unhealthy_probe_fails_the_report ... FAILED +test self_relaunch::gates::tests::smoke_gate_handles_missing_binary ... FAILED + +failures: + +failures: + self_deploy::tests_health::any_single_unhealthy_probe_fails_the_report + self_relaunch::gates::tests::smoke_gate_handles_missing_binary + +test result: FAILED. 1 passed; 2 failed; 0 ignored; +"; + assert_eq!( + extract_first_failure(output).as_deref(), + Some("self_deploy::tests_health::any_single_unhealthy_probe_fails_the_report"), + "must surface the FIRST failing test's fully-qualified path" + ); + } + + #[test] + fn extract_first_failure_none_when_all_pass() { + let output = "\ +running 2 tests +test a::b ... ok +test c::d ... ok + +test result: ok. 2 passed; 0 failed; +"; + assert_eq!(extract_first_failure(output), None); + } + + #[test] + fn extract_first_failure_is_bounded() { + // A pathological, very long "test path" must be bounded to the cap. + let long = "x".repeat(5000); + let line = format!("test {long} ... FAILED\n"); + let extracted = extract_first_failure(&line).expect("a failure was present"); + assert!( + extracted.len() <= GATE_DETAIL_MAX_BYTES, + "extracted failure must be bounded to {GATE_DETAIL_MAX_BYTES} bytes, got {}", + extracted.len() + ); + } } diff --git a/src/state_root.rs b/src/state_root.rs index 6d794fe33..31b86e5cb 100644 --- a/src/state_root.rs +++ b/src/state_root.rs @@ -54,6 +54,48 @@ pub fn resolve_subdir(name: &str) -> PathBuf { simard_state_root().join(name) } +/// The canonical set of directories that hold corrupt cognitive-memory +/// quarantine artifacts (issue #4469). +/// +/// Two directories can accumulate quarantines and must both be reconciled: +/// - the **top-level state root** (`~/.simard`) — the native pre-#2307 +/// quarantine location, and +/// - the **live-store subdir** `/state/` — where the de-forked +/// LadybugDB backend drops corrupt snapshots next to the live `cognitive` +/// store (62 corrupt artifacts accumulated here unbounded on the live host). +/// +/// The set is **deduped**: `state/` is normally distinct from the root, but if +/// they ever resolve to the same path a directory is returned only once so it is +/// never scanned twice. +/// +/// Single-sourced deliberately so the cleanup sweep +/// ([`crate::cmd_cleanup::disk::remove_old_corrupt_dbs`]), the self-health +/// `no_quarantine` probe / autonomous auto-ack ([`crate::self_deploy::health`]), +/// and the operator `--acknowledge-quarantine` remediation +/// ([`crate::operator_cli`]) all scan the **identical** directory set — they can +/// never disagree about where the quarantines live (the divergence that caused +/// the stuck-quarantine self-deploy deadlock). +pub fn quarantine_scan_dirs() -> Vec { + quarantine_scan_dirs_under(&simard_state_root()) +} + +/// The quarantine-scan directory set computed under an explicit `root`, rather +/// than the process-global [`simard_state_root`] (issue #4469). +/// +/// Same `[root, root/state]` (deduped) contract as [`quarantine_scan_dirs`], but +/// path-injected so callers with an already-resolved root — and tests passing a +/// tempdir — get the SAME two-directory coverage without reading (or racing on) +/// the process-global `SIMARD_STATE_ROOT`/`HOME` env. [`quarantine_scan_dirs`] +/// is the thin production wrapper over this. +pub fn quarantine_scan_dirs_under(root: &Path) -> Vec { + let live_store = root.join("state"); + if live_store == *root { + vec![root.to_path_buf()] + } else { + vec![root.to_path_buf(), live_store] + } +} + /// Canonical path for the file-backed goal store. /// /// Resolves to `/state/goal_store.json`. All consumers diff --git a/src/test_support/serial_guard.rs b/src/test_support/serial_guard.rs index bd8dbee30..faf15d063 100644 --- a/src/test_support/serial_guard.rs +++ b/src/test_support/serial_guard.rs @@ -189,6 +189,10 @@ pub(crate) enum Reason { /// meeting-persistence resolver (`write_auto_save` / `write_transcript` / /// `write_meeting_bundle`). CallsEnvReadingHandler { handler: String }, + /// Constructs `OodaConfig::default()`, which reads the OODA concurrency env + /// surface (`SIMARD_OODA_MAX_CONCURRENT` / `SIMARD_MAX_CONCURRENT_ACTIONS` / + /// `SIMARD_SCALING`) indirectly through the constructor (issue #4433). + ConstructsOodaConfigDefault, /// An allowlist entry was added without a justification. EmptyAllowlistJustification, } @@ -212,6 +216,11 @@ impl Reason { "calls env-reading handler `{handler}` (resolves the state-root / meetings surface)" ) } + Reason::ConstructsOodaConfigDefault => { + "constructs OodaConfig::default() (reads SIMARD_OODA_MAX_CONCURRENT / \ + SIMARD_MAX_CONCURRENT_ACTIONS / SIMARD_SCALING)" + .to_string() + } Reason::EmptyAllowlistJustification => { "allowlist entry has no justification".to_string() } @@ -227,6 +236,7 @@ impl Reason { Reason::ConstructsHermeticState => 2, Reason::CallsEnvReadingHandler { .. } => 3, Reason::ReadsStateRootDefault => 4, + Reason::ConstructsOodaConfigDefault => 5, } } @@ -543,6 +553,16 @@ impl<'a, 'ast> Visit<'ast> for BodyScan<'a> { self.reasons.push(Reason::ReadsStateRootDefault); } + // (E) OodaConfig::default() — an INDIRECT read of the OODA + // concurrency env (SIMARD_OODA_MAX_CONCURRENT / + // SIMARD_MAX_CONCURRENT_ACTIONS / SIMARD_SCALING) hidden inside the + // constructor (issue #4433). Matched by the fuller `OodaConfig::default` + // path — never a bare `default()` — so only the concurrency-config + // constructor is watched, not every `::default()` in the tree. + if penult == "OodaConfig" && last == "default" { + self.reasons.push(Reason::ConstructsOodaConfigDefault); + } + // (D) env-reading async goal route handlers and meeting-persistence // resolvers (write_auto_save / write_transcript / write_meeting_bundle). if segs.len() == 1 && ENV_READING_HANDLERS.contains(&last.as_str()) { @@ -848,3 +868,186 @@ fn skip_guard_helper_mutation_requires_the_key() { "a SkipGuard writer sharing the cognitive_memory key must be accepted: {flagged:?}" ); } + +// =========================================================================== +// TDD (Step 7): Race A — the `OodaConfig::default()` concurrency-env blind spot +// =========================================================================== +// +// These tests are written FIRST and are expected to FAIL against the current +// code. They pin the contract for the fix of the deterministic race that flaked +// `src/ooda_loop/tests_types.rs::ooda_config_default_values` (PR #4433 cluster). +// +// Root cause (grounded in the live source, NOT the retracted HOME/serial +// theory): `ooda_config_default_values` calls `OodaConfig::default()`, which +// reads `SIMARD_OODA_MAX_CONCURRENT` / `SIMARD_MAX_CONCURRENT_ACTIONS` / +// `SIMARD_SCALING` from the process-global environment (see +// `src/ooda_loop/types.rs` `impl Default for OodaConfig`). That read is +// concurrent with sibling tests in `types.rs` that `set_var` those same vars — +// but `ooda_config_default_values` carries NO `serial(cognitive_memory)` key, +// so it races them and intermittently observes an override instead of 24. +// +// The `serial_guard` meta-test is supposed to catch exactly this class, yet it +// has a real blind spot: the concurrency-env read is INDIRECT (hidden inside +// `OodaConfig::default()`), and those vars are not in `READ_WATCHED_VARS`, so +// the current scanner never flags the offending test. The fix has two coupled +// parts, both encoded below: +// (1) Guardrail: teach the scanner that a `#[test]` calling +// `OodaConfig::default()` is an env reader of the cognitive-memory serial +// group and must carry the key (mirrors the `HermeticState::new` / +// `resolve_state_root` recognizers). +// (2) Race A remediation: add `#[serial_test::serial(cognitive_memory)]` to +// the real `ooda_config_default_values` test (and clear the concurrency +// env before `OodaConfig::default()`, mirroring the already-correct twin +// `max_concurrent_defaults_to_24_when_unset` in `types.rs`). +// +// The single canonical serial key is `cognitive_memory`: a second key would not +// help, because glibc `setenv` can `realloc(environ)` and tear ANY concurrent +// `getenv`, so every env reader/writer in the lib binary must funnel through the +// one key. Exemptions use the existing `(name, justification)` allowlist. + +/// (1a) The scanner MUST flag a `#[test]` that reads concurrency env indirectly +/// via `OodaConfig::default()` and lacks the `cognitive_memory` key, and MUST +/// accept the same reader once it carries the key. +/// +/// FAILS today: `OodaConfig::default()` is an unrecognized indirect env read, +/// so `ooda_default_reader_without_key` is not flagged. +/// +/// Reads in-memory source fixtures only; mutates no env, so it carries no key. +#[test] +fn ooda_config_default_read_requires_the_key() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("fixture.rs"), + "#[test]\n\ + #[serial]\n\ + fn ooda_default_reader_without_key() {\n\ + let _c = OodaConfig::default();\n\ + }\n\ + #[test]\n\ + #[serial_test::serial(cognitive_memory)]\n\ + fn ooda_default_reader_with_key() {\n\ + let _c = OodaConfig::default();\n\ + }\n", + ) + .unwrap(); + + let opts = AuditOptions { + roots: vec![dir.path().to_path_buf()], + excluded_prefixes: Vec::new(), + watched: EnvWatch::AnyVar, + allowlist: Vec::new(), + }; + let flagged: BTreeSet = audit_env_mutating_tests(&opts) + .into_iter() + .map(|o| o.test_name) + .collect(); + + assert!( + flagged.contains("ooda_default_reader_without_key"), + "a test calling OodaConfig::default() (an indirect read of \ + SIMARD_OODA_MAX_CONCURRENT / SIMARD_MAX_CONCURRENT_ACTIONS / \ + SIMARD_SCALING) without the cognitive_memory key must be flagged: \ + {flagged:?}" + ); + assert!( + !flagged.contains("ooda_default_reader_with_key"), + "an OodaConfig::default() reader sharing the cognitive_memory key must \ + be accepted: {flagged:?}" + ); +} + +/// (1b) A flagged `OodaConfig::default()` reader MUST be exemptible through the +/// existing `(name, justification)` allowlist — and only that entry is spared; +/// an un-allowlisted sibling reader is still flagged. +/// +/// FAILS today: with no detection, the un-allowlisted reader is not flagged, so +/// the "still flagged" assertion fails (the detection precondition is missing). +/// +/// Reads in-memory source fixtures only; mutates no env, so it carries no key. +#[test] +fn ooda_config_default_reader_is_exemptible_via_allowlist() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("fixture.rs"), + "#[test]\n\ + #[serial]\n\ + fn exempt_ooda_reader() {\n\ + let _c = OodaConfig::default();\n\ + }\n\ + #[test]\n\ + #[serial]\n\ + fn non_exempt_ooda_reader() {\n\ + let _c = OodaConfig::default();\n\ + }\n", + ) + .unwrap(); + + let opts = AuditOptions { + roots: vec![dir.path().to_path_buf()], + excluded_prefixes: Vec::new(), + watched: EnvWatch::AnyVar, + allowlist: vec![( + "exempt_ooda_reader".to_string(), + "reads only concurrency env in isolation; serialized elsewhere".to_string(), + )], + }; + let flagged: BTreeSet = audit_env_mutating_tests(&opts) + .into_iter() + .map(|o| o.test_name) + .collect(); + + assert!( + !flagged.contains("exempt_ooda_reader"), + "an OodaConfig::default() reader with a justified allowlist entry must \ + be exempt: {flagged:?}" + ); + assert!( + flagged.contains("non_exempt_ooda_reader"), + "an un-allowlisted OodaConfig::default() reader must still be flagged: \ + {flagged:?}" + ); +} + +/// (2) Race A remediation contract, asserted against the REAL source: the live +/// `ooda_config_default_values` test in `src/ooda_loop/tests_types.rs` MUST +/// carry the `cognitive_memory` serial key, so its `OodaConfig::default()` read +/// can never run concurrently with the sibling concurrency-env writers. +/// +/// FAILS today: the real test currently has no `serial(cognitive_memory)` key. +/// This is decoupled from the scanner extension above — it directly parses the +/// source and checks the annotation, so it pins the fix even if the guardrail +/// recognizer is implemented differently. +/// +/// Reads source only; mutates no env, so it carries no key. +#[test] +fn real_ooda_config_default_values_test_carries_the_cognitive_memory_key() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/ooda_loop/tests_types.rs"); + let src = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + let ast = + syn::parse_file(&src).unwrap_or_else(|e| panic!("cannot parse {}: {e}", path.display())); + + let target = ast.items.iter().find_map(|item| match item { + syn::Item::Fn(f) if f.sig.ident == "ooda_config_default_values" => Some(f), + _ => None, + }); + let target = target.unwrap_or_else(|| { + panic!( + "expected a `#[test] fn ooda_config_default_values` in {}", + path.display() + ) + }); + + assert!( + is_test_fn(&target.attrs), + "ooda_config_default_values must remain a #[test]" + ); + assert!( + serial_keys(&target.attrs).contains(REQUIRED_KEY), + "Race A (PR #4433): ooda_config_default_values calls OodaConfig::default() \ + (an indirect read of SIMARD_OODA_MAX_CONCURRENT / \ + SIMARD_MAX_CONCURRENT_ACTIONS / SIMARD_SCALING) and MUST carry \ + #[serial_test::serial({REQUIRED_KEY})] so it never races the sibling \ + concurrency-env writers in src/ooda_loop/types.rs" + ); +} diff --git a/src/util/log_sanitize.rs b/src/util/log_sanitize.rs new file mode 100644 index 000000000..451dcfe19 --- /dev/null +++ b/src/util/log_sanitize.rs @@ -0,0 +1,91 @@ +//! Shared control-character log sanitizer. +//! +//! Neutralizes an untrusted string for embedding in single-line, +//! operator-facing output. Two independent callers rely on it: +//! +//! - the self-relaunch canary, which embeds subprocess stderr / a failing test +//! name into a `GateResult.detail` (#4470); and +//! - the cleanup sweep, which renders an untrusted on-disk quarantine basename +//! into stderr / a `CleanupReport` (#4469, LOW-1). +//! +//! Living here (a neutral cross-cutting util) rather than inside either caller +//! keeps `cmd_cleanup` from depending on `self_relaunch` for a generic string +//! sanitizer (#4469 philosophy review S6). + +/// Strip control characters, collapse to a single line, and bound the result to +/// `max_bytes` on a UTF-8 char boundary. +/// +/// Every run of control characters (CR/LF, tabs, ANSI escapes, NUL) collapses to +/// a single space, so the output is one readable line with no log-line-forgery +/// or terminal-control-injection vectors. The length bound never splits a +/// multi-byte character. +pub fn sanitize_to_single_line(raw: &str, max_bytes: usize) -> String { + let mut collapsed = String::with_capacity(raw.len()); + for c in raw.chars() { + if c.is_control() { + if !collapsed.ends_with(' ') { + collapsed.push(' '); + } + } else { + collapsed.push(c); + } + } + let trimmed = collapsed.trim(); + if trimmed.len() <= max_bytes { + return trimmed.to_string(); + } + // Bound on a UTF-8 char boundary so we never split a multi-byte char. + let mut end = max_bytes; + while end > 0 && !trimmed.is_char_boundary(end) { + end -= 1; + } + trimmed[..end].to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + + const MAX: usize = 512; + + #[test] + fn strips_control_chars_and_newlines() { + let raw = "line one\nline two\r\n\ttabbed\x1b[31mred\x00nul"; + let clean = sanitize_to_single_line(raw, MAX); + assert!(!clean.contains('\n'), "newlines stripped: {clean:?}"); + assert!( + !clean.contains('\r'), + "carriage returns stripped: {clean:?}" + ); + assert!( + !clean.contains('\x1b'), + "escape sequences stripped: {clean:?}" + ); + assert!(!clean.contains('\0'), "NUL stripped: {clean:?}"); + assert!( + !clean.contains('\t') || clean.contains(' '), + "no raw tabs: {clean:?}" + ); + } + + #[test] + fn bounds_length() { + let raw = "a".repeat(2000); + let clean = sanitize_to_single_line(&raw, MAX); + assert!( + clean.len() <= MAX, + "must bound to {MAX} bytes, got {}", + clean.len() + ); + } + + #[test] + fn utf8_boundary_safe() { + // Bounding must never split a multi-byte char (no panic, valid UTF-8). + let raw = "héllo wörld café ".repeat(100); + let clean = sanitize_to_single_line(&raw, 10); + assert!(clean.len() <= 10); + // Round-trips as valid UTF-8 (String is always valid; the point is no panic). + let _ = clean.chars().count(); + } +} diff --git a/src/util/mod.rs b/src/util/mod.rs index 82ea8e3ad..a106463f8 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -6,5 +6,8 @@ //! `String::truncate(N)` at every site where `N` is a byte budget rather //! than a code-point count. See //! `docs/reference/string-truncation-helpers.md`. +//! - [`log_sanitize`] — a shared control-character sanitizer for untrusted +//! strings embedded in single-line operator-facing output. +pub mod log_sanitize; pub mod string_truncate; diff --git a/tests/adaptive_scaling.rs b/tests/adaptive_scaling.rs index d3aa38961..36e8b1401 100644 --- a/tests/adaptive_scaling.rs +++ b/tests/adaptive_scaling.rs @@ -312,9 +312,17 @@ fn scaler_current_max_can_override_config() { }) .collect(); - // Use scaler's current_max as the config limit. + // Use scaler's current_max as the config limit. Set `scaler: None` + // explicitly instead of relying on `..OodaConfig::default()`: + // `OodaConfig::default()` reads process env (`SIMARD_SCALING`), so on a host + // with `SIMARD_SCALING=auto` the inherited default scaler would drive the + // limit via `scaler.adjust()` and override the explicit + // `max_concurrent_actions` under test, making the result depend on the + // environment rather than the config. Building it explicitly keeps the test + // hermetic (issue #2732). let config = OodaConfig { max_concurrent_actions: scaler.current_max(), + scaler: None, ..OodaConfig::default() }; diff --git a/tests/self_deploy_convergence.rs b/tests/self_deploy_convergence.rs new file mode 100644 index 000000000..5329e0a22 --- /dev/null +++ b/tests/self_deploy_convergence.rs @@ -0,0 +1,124 @@ +//! End-to-end convergence contract for the stuck-quarantine deadlock (#4469). +//! +//! A genuinely-stuck `cognitive*.corrupt-*` quarantine freezes self-deploy: the +//! `no_quarantine` health probe never clears, yet the #2550 retention rule +//! protects the recovery asset from deletion. The durable acknowledgement path +//! breaks the deadlock **without destroying data** — acknowledging a quarantine +//! writes a `.ack` sidecar that lets the probe (and the cleanup sweep) treat the +//! artifact as "seen" while the quarantined store itself is retained on disk. +//! +//! These tests exercise the public `simard::self_deploy` acknowledgement API end +//! to end against a hermetic state root. They are the outside-in specification +//! for the convergence path; they FAIL until #4469 is implemented and PASS once +//! the durable ack sidecar is in place. + +use simard::self_deploy::{ack_marker_path, acknowledge, is_ack_marker_name, is_acknowledged}; + +const QUARANTINE: &str = "cognitive.corrupt-20260101120000"; + +/// Seed a hermetic state root with a substantial, stuck quarantine artifact and +/// return the guard plus the artifact path. The `HermeticState` pins +/// `SIMARD_STATE_ROOT` to a tempdir for the duration of the test. +fn seed_stuck_quarantine() -> (simard::test_support::HermeticState, std::path::PathBuf) { + let hermetic = simard::test_support::HermeticState::new(); + let artifact = hermetic.state_root().join(QUARANTINE); + // A multi-MB recovery asset: exactly the kind #2550 protects and refuses to + // delete, so the ONLY way to converge is a non-destructive acknowledgement. + std::fs::write(&artifact, vec![0u8; 2 * 1024 * 1024]).unwrap(); + (hermetic, artifact) +} + +#[test] +#[serial_test::serial(cognitive_memory)] +fn acknowledge_is_durable_and_retains_the_recovery_asset() { + let (hermetic, artifact) = seed_stuck_quarantine(); + let root = hermetic.state_root(); + + // Before: the quarantine is unacknowledged (the probe would fail here). + assert!(!is_acknowledged(root, QUARANTINE)); + + // Acknowledge: writes a durable sidecar under the SAME state root the probe + // scans, and returns that path. + let marker = acknowledge(root, QUARANTINE).expect("acknowledge succeeds"); + let expected = ack_marker_path(root, QUARANTINE).expect("valid quarantine name"); + assert_eq!(marker, expected, "marker path must match ack_marker_path"); + assert_eq!( + marker.parent(), + Some(root), + "marker lives under the state root" + ); + assert!(is_ack_marker_name( + &marker.file_name().unwrap().to_string_lossy() + )); + + // After: acknowledged, and the recovery asset is RETAINED (not deleted). + assert!(is_acknowledged(root, QUARANTINE)); + assert!(marker.is_file(), "sidecar is a durable regular file"); + assert!( + artifact.is_file(), + "the quarantined recovery asset must be retained for recovery" + ); +} + +#[test] +#[serial_test::serial(cognitive_memory)] +fn acknowledge_is_idempotent_across_repeated_convergence_attempts() { + let (hermetic, _artifact) = seed_stuck_quarantine(); + let root = hermetic.state_root(); + + let first = acknowledge(root, QUARANTINE).unwrap(); + let second = acknowledge(root, QUARANTINE).unwrap(); + assert_eq!(first, second, "repeated ack is idempotent"); + assert!(is_acknowledged(root, QUARANTINE)); + + // Exactly one durable marker exists — no accumulation across OODA cycles. + let markers = std::fs::read_dir(root) + .unwrap() + .flatten() + .filter(|e| is_ack_marker_name(&e.file_name().to_string_lossy())) + .count(); + assert_eq!(markers, 1, "acknowledgement must not accumulate markers"); +} + +#[test] +#[serial_test::serial(cognitive_memory)] +fn fresh_corruption_after_ack_is_not_silenced() { + let (hermetic, _artifact) = seed_stuck_quarantine(); + let root = hermetic.state_root(); + + acknowledge(root, QUARANTINE).unwrap(); + assert!(is_acknowledged(root, QUARANTINE)); + + // A NEW corruption event lands under the same root. Filename-keyed markers + // must not mark the fresh artifact as acknowledged. + let fresh = "cognitive.corrupt-20260202235959"; + std::fs::write(root.join(fresh), vec![0u8; 1024]).unwrap(); + assert!( + !is_acknowledged(root, fresh), + "a prior ack must never silence a new corruption event" + ); +} + +#[test] +#[serial_test::serial(cognitive_memory)] +fn acknowledge_rejects_unsafe_names_end_to_end() { + let hermetic = simard::test_support::HermeticState::new(); + let root = hermetic.state_root(); + + // Path traversal / separators / absolute paths / non-quarantine names are + // all refused, so an operator (or a compromised caller) cannot use the ack + // path to write outside the state root or silence the live store. + for bad in [ + "../escape", + "sub/cognitive.corrupt-1", + "/etc/passwd", + "cognitive", // the live store, not a quarantine + "cognitive.wal", // live WAL, not a quarantine + ] { + assert!( + acknowledge(root, bad).is_err(), + "acknowledge must reject unsafe/non-quarantine name: {bad:?}" + ); + assert!(ack_marker_path(root, bad).is_none()); + } +}