diff --git a/Cargo.lock b/Cargo.lock index 2835dd75c..220d914bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3883,7 +3883,7 @@ dependencies = [ [[package]] name = "simard" -version = "0.36.0" +version = "0.37.0" dependencies = [ "amplihack-agent-eval", "amplihack-memory", diff --git a/Cargo.toml b/Cargo.toml index 689564263..84602654b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "simard" -version = "0.36.0" +version = "0.37.0" edition = "2024" default-run = "simard" diff --git a/docs/howto/diagnose-typed-ooda-database-locked.md b/docs/howto/diagnose-typed-ooda-database-locked.md new file mode 100644 index 000000000..3de7daa90 --- /dev/null +++ b/docs/howto/diagnose-typed-ooda-database-locked.md @@ -0,0 +1,108 @@ +--- +title: 'How-to: diagnose a typed-OODA "database is locked" crash-loop' +description: > + Confirm, diagnose, and clear the `typed outcome persistence failed: database + is locked` crash-loop in the typed-OODA ledger. Covers reading the + fail-visible tracing lines, verifying the WAL journal mode and 30s + busy_timeout are applied at open, and checking for the `-wal`/`-shm` sidecars, + so OODA cycles persist outcomes reliably. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: howto +status: implemented +related: + - ../reference/typed-ooda-ledger-concurrency.md + - ../reference/claim-reaper-api.md + - ../operations/cognitive-memory-durability.md + - ./diagnose-leaked-engineer-claims.md + - ./diagnose-and-recover-ooda-step-failures.md +--- + +# Diagnose a typed-OODA "database is locked" crash-loop + +> **Status: implemented (issue #4483).** +> The concurrency hardening described here ships in +> [`src/typed_ooda/ledger.rs`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs) +> and [`src/typed_ooda/schema.rs`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/schema.rs). +> Contract: +> [Typed-OODA ledger concurrency hardening](../reference/typed-ooda-ledger-concurrency.md). + +## Symptom + +The daemon log repeats a persistence failure and OODA cycles stop making +progress across many goals: + +```text +typed outcome persistence failed: database is locked +``` + +On a hardened daemon this message should not recur. A single transient +occurrence immediately followed by a successful retry is expected and benign; +a **crash-loop** (the same message every cycle, no forward progress) means one +of the concurrency settings below is not in effect — for example a ledger that +was created before the fix and re-opened in rollback journal mode. + +## 1. Confirm it is the typed-OODA ledger + +The message originates in the ledger persistence path +([`persistence`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs) +error mapper). Confirm the failing writes are terminal-outcome / progress / +effect-job persistence, and note whether the error clears on retry (benign) or +loops (needs action). All lock diagnostics are structured `tracing` / OTel +lines — there is no `print!`/`println!` output to grep for. + +## 2. Verify WAL and busy_timeout are applied + +WAL journal mode and the 30s `busy_timeout` are applied **at every connection +open**, unconditionally — not only during a schema migration. Inspect the live +ledger database: + +```bash +sqlite3 'PRAGMA journal_mode;' # expect: wal +``` + +If this reports `delete` (rollback mode), the ledger is running the +pre-fix configuration. Restarting the daemon on the hardened binary re-opens the +database and switches it to WAL; verify with the command above. + +## 3. Check the WAL sidecar files + +A WAL-mode ledger has two sidecar files next to the database: + +```bash +ls -l /-wal /-shm +``` + +Both should be present and owned/permissioned like the ledger directory (not +world-writable, not in a temp path). When taking a **cold** backup of the +ledger, copy the `-wal` and `-shm` files alongside the main database, or +checkpoint first — the same discipline used for the cognitive store +([Cognitive Memory Durability](../operations/cognitive-memory-durability.md)). + +## 4. Confirm write transactions serialize, not fail + +Writers use `TransactionBehavior::Immediate`, so concurrent writers acquire the +write lock at `BEGIN` and wait out the `busy_timeout` instead of racing and +failing late. If you still see sustained lock errors after confirming WAL + +busy_timeout, look for a writer holding a transaction open across slow work +(network / agent I/O) — transaction bodies are meant to contain only +bound-parameter SQL. A write that cannot acquire the lock within the +`busy_timeout` surfaces the error to the log and metrics rather than looping +forever; that surfaced error is the signal to investigate the slow holder. + +## 5. Rule out false reaps / leaked claims + +Persistent lock contention can coincide with engineer-claim lifecycle issues. +The reaper lease-ownership guard that prevents false stale-engineer reaps is +tracked separately (#4467/#4464/#4462/#4500) and is **not** part of this +ledger-open hardening. To inspect leaked or reaped claims, follow +[Diagnose and clear leaked engineer claims](./diagnose-leaked-engineer-claims.md). + +## Resolution checklist + +- [ ] Ledger reports `journal_mode = wal`. +- [ ] `-wal` / `-shm` sidecars present with correct permissions. +- [ ] `database is locked` no longer recurs every cycle (transient + retry OK). +- [ ] OODA cycles persist terminal outcomes and progress again. +- [ ] No false stale-engineer reaps or leaked `engineer_claims` rows. diff --git a/docs/operations/index.md b/docs/operations/index.md index 59d0c82c9..eb304d2b2 100644 --- a/docs/operations/index.md +++ b/docs/operations/index.md @@ -12,11 +12,20 @@ Simard deployment. | [Meeting REPL & Handoff Ingestion](meeting-handoffs.md) | Routing operator intent into the OODA loop | | [Progress-Evidence Kill Switch](progress-evidence-kill-switch.md) | `SIMARD_PROGRESS_EVIDENCE=off` and when to use it | +Related reference pages: + +| Page | Topic | +|---|---| +| [Typed-OODA ledger concurrency hardening](../reference/typed-ooda-ledger-concurrency.md) | WAL + 30s busy_timeout applied at every ledger open, Immediate write txns, fail-visible lock propagation (#4483) | +| [Deploy-gate canary unit-test stage](../reference/deploy-gate-unit-test-canary.md) | The self-deploy canary unit-test gate and the exit-101 red-canary root-cause fix (#4470/#4471/#4481/#4475) | +| [Gym self-eval status wiring](../reference/gym-self-eval-status.md) | Real scenario count + non-idle self-eval in `simard status` | + Related how-to guides: | Guide | Topic | |---|---| | [Diagnose handoff accumulation](../howto/diagnose-handoff-accumulation.md) | Detect, resolve, prevent handoff file buildup (#2268) | +| [Diagnose a typed-OODA "database is locked" crash-loop](../howto/diagnose-typed-ooda-database-locked.md) | Confirm WAL + busy_timeout, clear the persistence crash-loop (#4483) | For contributor workflow (branching, merge policy, PR evidence requirements), see [`CONTRIBUTING.md`](https://github.com/rysweet/Simard/blob/main/CONTRIBUTING.md) at the diff --git a/docs/reference/deploy-gate-unit-test-canary.md b/docs/reference/deploy-gate-unit-test-canary.md new file mode 100644 index 000000000..2c6032f76 --- /dev/null +++ b/docs/reference/deploy-gate-unit-test-canary.md @@ -0,0 +1,136 @@ +--- +title: "Reference: Deploy-gate canary unit-test stage" +description: > + The contract for the self-deploy canary unit-test gate + (run_unit_test_gate in src/self_relaunch/gates.rs): how the gate invokes the + canary test suite, how a red canary (exit 101) blocks self-deploy, and the + root-cause fix that cleared the recurring exit-101 red canary so the running + daemon can self-deploy to merged main. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./self-deploy-api.md + - ./overseer-deploy-canary-diagnostics.md + - ./typed-ooda-ledger-concurrency.md + - ../howto/enable-autonomous-self-merge-canary.md + - ../howto/verify-and-roll-back-a-self-deploy.md + - ../safe-self-update.md + - ../../src/self_relaunch/gates.rs + - ../../src/self_relaunch/types.rs +--- + +# Reference: Deploy-gate canary unit-test stage + +> **Status: implemented (issues #4470, #4471, #4481, #4475).** Present-tense +> description of shipped behaviour. Primary source: +> [`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs) +> (`run_unit_test_gate`, `verify_canary`). +> +> This change root-causes and clears the recurring **red canary** — every +> self-deploy was failing the `deploy_gate` unit-test stage with +> `exit status: 101`, which blocked the running daemon from advancing to merged +> `main` (`simard status`: *"running binary is 1 commit(s) behind merged main — +> self-deploy required"*). The failing test lived in the typed-OODA concurrency +> surface, so the root fix is delivered by the +> [ledger concurrency hardening](./typed-ooda-ledger-concurrency.md); this page +> documents the gate contract and the canary-green resolution. + +--- + +## The canary gate sequence + +Before a freshly built candidate binary replaces the running daemon, the +self-deploy path runs it through a sequence of gates via +[`verify_canary`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs). +The sequence **does not short-circuit** — every gate runs so diagnostics report +all failures, not just the first: + +| Gate (`RelaunchGate`) | What it proves | +|---|---| +| `Smoke` | The candidate binary starts and answers `--version`. | +| `UnitTest` | The canary test suite passes (`cargo test`). | +| `GymBaseline` | `gym list` succeeds against the candidate. | +| `RpcHealth` | The candidate answers an RPC health probe within `health_timeout`. | + +A single failed gate produces a **red canary** and the self-deploy is aborted; +the running binary stays in place. This is the intended fail-closed posture: +**a red canary must never be papered over by disabling the gate.** + +--- + +## The unit-test gate contract + +`run_unit_test_gate(config: &RelaunchConfig)` shells out to `cargo test` with +**fixed arguments** (no `sh -c`, no dynamic interpolation of caller input): + +```text +cargo test \ + --manifest-path /Cargo.toml \ + --target-dir +``` + +with `CARGO_BUILD_JOBS` set from +[`cargo_jobs`](https://github.com/rysweet/Simard/blob/main/src/cargo_jobs.rs). +The relevant [`RelaunchConfig`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/types.rs) +fields are: + +| Field | Meaning | +|---|---| +| `manifest_dir` | Directory holding the candidate's `Cargo.toml` (default `.`). | +| `canary_target_dir` | Isolated, PID-scoped `--target-dir` under the temp dir, so the canary build never clobbers the live target. | +| `health_timeout` | Deadline for the RPC-health gate. | + +Result mapping: + +| `cargo test` outcome | `GateResult` | +|---|---| +| exit `0` | `passed: true`, detail `"all tests passed"`. | +| non-zero (e.g. `exit 101` = test failures) | `passed: false`, detail `"tests failed (exit ): "`. | +| failed to spawn | `passed: false`, detail `"cargo test failed to run: "`. | + +Captured `stderr` is truncated (200 chars) before it is logged, and only the +gate verdict and truncated detail are emitted through structured `tracing` / +OTel — never full test output, tokens, or approval payloads. + +--- + +## Root cause of the recurring exit-101 canary + +Exit status `101` is `cargo test`'s exit code for **test failures** (not a gate +or harness bug). Reproducing the canary suite locally with the same fixed +arguments surfaced the failing test in the typed-OODA persistence surface: the +same `database is locked` contention described in +[typed-OODA ledger concurrency hardening](./typed-ooda-ledger-concurrency.md) +made the affected tests fail non-deterministically under the canary's parallel +test execution. + +The resolution root-causes the defect rather than quarantining the symptom: + +- The underlying ledger concurrency defect is fixed at connection open (WAL + + 30s busy_timeout), so the previously-failing tests now pass deterministically. +- The gate itself is unchanged in posture — it is **not** disabled, weakened, or + made non-blocking. +- Per issue #4471, deliberate quarantine remains available **only** for a test + proven obsolete/wrong, applied narrowly with a justification comment citing + #4471. It was not needed here. + +The fix lands on a fresh, non-conflicting branch, superseding the stale +conflicting PRs #4480 / #4454 / #4436 / #4429 and coordinating with the +root-cause/quarantine/hardening issues #4470 / #4471 / #4481 / #4475. + +--- + +## Verifying a green canary + +After deploy, `simard status` no longer reports the running binary as behind +merged `main`, and the deploy log records `deploy_gate: green canary` instead of +the previous `red canary (gate unit-test: tests failed exit status: 101)`. To +reproduce the gate manually, see +[Enable the autonomous self-merge canary](../howto/enable-autonomous-self-merge-canary.md) +and +[Verify and roll back a self-deploy](../howto/verify-and-roll-back-a-self-deploy.md). +For deep canary telemetry see +[Overseer deploy-canary diagnostics](./overseer-deploy-canary-diagnostics.md). diff --git a/docs/reference/gym-self-eval-status.md b/docs/reference/gym-self-eval-status.md new file mode 100644 index 000000000..2fd7aeb01 --- /dev/null +++ b/docs/reference/gym-self-eval-status.md @@ -0,0 +1,128 @@ +--- +title: "Reference: Gym self-eval status wiring" +description: > + The status-snapshot contract for the GYM section: assemble_gym now reports + the real configured scenario count (benchmark_scenarios) and a non-idle + self-eval state when the gym is enabled (SIMARD_SKIP_GYM unset), so + `simard status` reflects a live self-evaluation quality signal instead of the + previous hardcoded "0 configured / idle" stub. Purely additive status wiring — + no change to gym execution behaviour. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./status-snapshot-api.md + - ./coin-benchmark.md + - ../howto/run-the-coin-gym-harness.md + - ../howto/simard-status.md + - ../../src/status/provider.rs + - ../../src/status/mod.rs + - ../../src/status/render.rs + - ../../src/gym/scenarios/mod.rs + - ../../src/gym_runner_client.rs +--- + +# Reference: Gym self-eval status wiring + +> **Status: implemented (issue: gym self-eval inert, goal_hygiene).** +> Present-tense description of shipped behaviour. Primary source: +> [`assemble_gym`](https://github.com/rysweet/Simard/blob/main/src/status/provider.rs) +> in `src/status/provider.rs`, with the section type in +> [`src/status/mod.rs`](https://github.com/rysweet/Simard/blob/main/src/status/mod.rs) +> and the terminal renderer in +> [`src/status/render.rs`](https://github.com/rysweet/Simard/blob/main/src/status/render.rs). +> +> Before this change the GYM status section was a stub: even with the gym +> enabled (`SIMARD_SKIP_GYM` unset) `simard status` reported +> *"scenarios absent configured"* and *"self-eval idle"*, so the self-evaluation +> loop looked inert and produced no visible quality signal. This change wires the +> section to the real scenario set. It is **purely additive** — it reports state +> only and does not alter gym execution. + +--- + +## The GYM status section + +`assemble_gym(skip_gym: bool)` builds the +[`Gym`](https://github.com/rysweet/Simard/blob/main/src/status/mod.rs) section +of the status snapshot: + +```rust +pub struct Gym { + pub skip_gym: bool, + pub configured_scenarios: Option, + pub self_eval_state: String, +} +``` + +Behaviour by mode: + +| Condition | `configured_scenarios` | `self_eval_state` | +|---|---|---| +| Gym enabled (`skip_gym == false`) | `Some(N)` where `N` = number of built-in benchmark scenarios | non-idle (`"enabled"`) | +| Gym skipped (`skip_gym == true`, `SIMARD_SKIP_GYM=1`) | `None` | `"idle"` | + +The configured count is the length of the canonical built-in scenario set +returned by +[`benchmark_scenarios()`](https://github.com/rysweet/Simard/blob/main/src/gym/scenarios/mod.rs) +(the `'static SCENARIOS` array in +[`src/gym/scenarios/data.rs`](https://github.com/rysweet/Simard/blob/main/src/gym/scenarios/data.rs), +currently 12 curated V1 scenarios) — the status layer reports that same count +rather than a hardcoded `0`/`None`. When the gym is enabled the self-eval state is reported as +non-idle so the section honestly reflects that the self-evaluation path is live. + +> **Fast-path parity.** The `SIMARD_SKIP_GYM=1` fast path (see +> [`gym_runner_client`](https://github.com/rysweet/Simard/blob/main/src/gym_runner_client.rs)) +> continues to report `None` / `"idle"`, so a deliberately-skipped gym still +> reads as skipped and idle — the wiring never fabricates a signal when the gym +> is off. + +--- + +## Rendered output + +The terminal renderer +([`render_gym`](https://github.com/rysweet/Simard/blob/main/src/status/render.rs)) +prints: + +```text +GYM + SIMARD_SKIP_GYM unset (gym enabled) + scenarios 12 configured + self-eval enabled +``` + +versus the skipped case: + +```text +GYM + SIMARD_SKIP_GYM set (gym skipped) + scenarios absent configured + self-eval idle +``` + +The JSON status API surfaces the same `Gym` fields +(`skip_gym`, `configured_scenarios`, `self_eval_state`) — see +[Status snapshot API](./status-snapshot-api.md). + +--- + +## Scope and non-goals + +- **Status-only.** This change reports the scenario count and self-eval state; + it does **not** schedule, execute, or change gym scenarios. Runtime behaviour + when scenarios were previously absent is unchanged except that the enabled-gym + path is now honestly reported as configured/enabled. +- No new environment variables or configuration flags are introduced. The gym + is enabled by default; set `SIMARD_SKIP_GYM=1` to skip it (unchanged). + +--- + +## Tests + +| Test surface | Guarantee | +|---|---| +| `provider.rs` — `assemble_gym` unit test | Enabled gym → `Some(N)` / non-idle; skipped gym → `None` / `"idle"`. | +| `tests/status_render_contract.rs` | The rendered GYM section shows the configured count and non-idle self-eval when enabled, and `absent` / `idle` when skipped. | diff --git a/docs/reference/typed-ooda-ledger-concurrency.md b/docs/reference/typed-ooda-ledger-concurrency.md new file mode 100644 index 000000000..983551437 --- /dev/null +++ b/docs/reference/typed-ooda-ledger-concurrency.md @@ -0,0 +1,180 @@ +--- +title: "Reference: Typed-OODA ledger concurrency hardening" +description: > + The concurrency contract for the typed-OODA SQLite ledger: unconditional + WAL journal mode and a 30s busy_timeout applied at every connection open, + Immediate write transactions with minimized hold time, and fail-visible + propagation of `database is locked` faults through CapabilityResult + (never swallowed). +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./claim-reaper-api.md + - ./typed-ooda-goal-session-rails.md + - ./ooda-capability-api.md + - ./engineer-claim-release-api.md + - ../operations/cognitive-memory-durability.md + - ../howto/diagnose-typed-ooda-database-locked.md + - ../../src/typed_ooda/ledger.rs + - ../../src/typed_ooda/schema.rs +--- + +# Reference: Typed-OODA ledger concurrency hardening + +> **Status: implemented (issue #4483).** Present-tense description of shipped +> behaviour. Primary sources: +> [`src/typed_ooda/ledger.rs`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs) +> and +> [`src/typed_ooda/schema.rs`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/schema.rs). +> +> This change eliminates the `typed outcome persistence failed: database is +> locked` crash-loop (#4483) that was failing OODA cycles across many goals, by +> correcting the ledger's SQLite concurrency configuration at connection open. +> The adjacent decide→act effect-dispatch (#4468) and claim-reaper lifecycle +> (#4467/#4464/#4462/#4500) concurrency defects are tracked and delivered +> **separately** — see [Stale-Engineer-Claim Reaper API](./claim-reaper-api.md). + +--- + +## Why this exists + +The typed-OODA ledger is a single SQLite database +([`CapabilityHandler::open`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs)) +that multiple in-process writers touch concurrently: the OODA Act loop +persisting terminal outcomes and progress records, the effect dispatcher +leasing and completing `effect_jobs`, and the overseer claim reaper releasing +`engineer_claims`. Under load these writers contended on a single rollback-mode +connection and SQLite returned `SQLITE_BUSY` (`database is locked`), which the +persistence path surfaced as a terminal error and re-entered on the next cycle — +a crash-loop that produced no forward progress. + +The fix has three parts, all **additive / non-breaking** and requiring **no +`SCHEMA_VERSION` bump** (the changes are runtime connection settings, not schema +migrations): + +1. Correct SQLite concurrency configuration at every open. +2. Immediate, short-lived write transactions. +3. Fail-visible lock-error propagation. + +--- + +## 1. Connection configuration (WAL + busy_timeout) + +`CapabilityHandler::open` applies concurrency-critical pragmas +**unconditionally on every connection open**, independent of whether the +database is being migrated: + +| Setting | Value | Rationale | +|---|---|---| +| `PRAGMA journal_mode` | `WAL` | Write-ahead logging lets one writer proceed concurrently with readers, removing the dominant `SQLITE_BUSY` source. | +| `busy_timeout` | `30s` | A generous timeout lets a blocked writer wait out a peer's short transaction instead of failing immediately. | +| `PRAGMA foreign_keys` | `ON` | Preserved from prior behaviour. | + +> **Design note — WAL moved out of the migration branch.** Previously +> `journal_mode = WAL` was set only inside the schema-migration branch of +> [`schema::initialize`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/schema.rs), +> so a database already at `SCHEMA_VERSION` re-opened in **rollback** journal +> mode and never got WAL. WAL is now applied at open time for **every** ledger, +> including pre-existing v1 databases, so concurrency settings are correct +> regardless of migration state. + +### Sidecar files and permissions + +Opening in WAL creates `-wal` and `-shm` sidecar files next to the ledger +database. These inherit the ledger directory's permissions. The ledger path is +derived internally (never from an environment variable or CLI argument), so the +sidecars are never created in a world-writable or temp location. Operators +performing a cold backup of the ledger must copy the `-wal` and `-shm` files +alongside the main database, or checkpoint first — see +[Cognitive Memory Durability](../operations/cognitive-memory-durability.md) for +the equivalent WAL/checkpoint discipline on the cognitive store. + +--- + +## 2. Write transactions are Immediate and short + +Every method that writes uses an **Immediate** transaction: + +```rust +let transaction = connection + .transaction_with_behavior(TransactionBehavior::Immediate)?; +// ... bound-parameter writes only ... +transaction.commit()?; +``` + +`TransactionBehavior::Immediate` acquires the write lock at `BEGIN` rather than +at first write, so two writers serialize deterministically at the start of their +transactions (waiting out the `busy_timeout`) instead of racing to upgrade a +deferred transaction and one of them failing late with `SQLITE_BUSY`. + +Transaction bodies are kept minimal: no network calls, no agent I/O, and no +long computation happen while the write lock is held. All SQL is +**parameter-bound** (`params![…]` / positional `?n`); no write statement is +assembled with `format!`, preserving the injection-safe contract of the +capability layer. + +Writer methods covered include (non-exhaustive): +[`record_action`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs), +`record_progress`, `record_completed`, `record_blocked`, `record_no_action`, +`execute_process`, `claim_next_effect`, `claim_effect_for_outcome`, +`recover_expired_effects`, `register_actor_session`, `issue_privileged_approval`, +and `release_engineer_claim`. + +--- + +## 3. Lock errors are propagated, never swallowed + +`database is locked` is treated as a **first-class** condition — never a +silently-discarded one: + +- The 30s `busy_timeout` (part 1) absorbs contention at the SQLite layer: a + writer that cannot immediately acquire the write lock waits and retries + internally for up to 30s before SQLite surfaces `SQLITE_BUSY`. +- If a write still cannot acquire the lock within that window, the fault + surfaces through `CapabilityResult` (the + [`persistence`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs) + error mapper) and is returned to the caller — visible in the daemon log and + metrics — rather than being swallowed or converted to a silent no-op. No + application-level retry loop wraps the persistence call; the busy-timeout wait + is the sole retry mechanism. + +> **Security requirement.** A swallowed lock error on a claim-release or +> outcome-persist would leak a privileged `engineer_claims` row or drop a +> terminal outcome. Lock errors are therefore **always** propagated: the +> [`persistence`](https://github.com/rysweet/Simard/blob/main/src/typed_ooda/ledger.rs) +> mapper returns an `Err` on the `CapabilityResult` path and never converts a +> lock failure into a silent no-op. + +--- + +## 4. Related: reaper lifecycle and decide→act persistence + +The claim-reaper lease-ownership guard (#4467/#4464/#4462/#4500) and the +decide→act outcome-persistence fix (#4468) address adjacent concurrency defects +in the same subsystem, but are tracked and delivered **separately** from this +ledger-open hardening — they are not part of the change documented here. See the +[Stale-Engineer-Claim Reaper API](./claim-reaper-api.md) for the reaper +contract. + +--- + +## Regression tests + +| Test surface | Guarantee | +|---|---| +| `ledger.rs` — `open_applies_wal_journal_mode_on_preexisting_v1_db` | Re-opening a pre-existing v1 ledger whose journal was reverted to rollback mode leaves it in WAL mode. | +| `ledger.rs` — `open_sets_busy_timeout_at_least_30s` | A `busy_timeout` of at least 30s is configured at every open. | +| `ledger.rs` — `concurrent_cross_connection_writers_never_hit_database_locked` | Multiple writers on separate connections to the same file all succeed; no `database is locked` error surfaces. | + +--- + +## Operator guidance + +If you observe `typed outcome persistence failed: database is locked` in the +daemon log, follow +[Diagnose a typed-OODA "database is locked" crash-loop](../howto/diagnose-typed-ooda-database-locked.md). +On a correctly-hardened daemon this message should not recur; a single +transient occurrence followed by a successful retry is expected and benign. diff --git a/mkdocs.yml b/mkdocs.yml index b8d2a8a85..1d9697655 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -184,6 +184,7 @@ nav: - Attach to a Running Engineer: howto/attach-to-a-running-engineer.md - Inspect and Clean Engineer Worktrees: howto/inspect-and-clean-engineer-worktrees.md - Diagnose and Clear Leaked Engineer Claims: howto/diagnose-leaked-engineer-claims.md + - Diagnose a Typed-OODA "database is locked" Crash-Loop: howto/diagnose-typed-ooda-database-locked.md - Investigate a Stale Engineer Before Reap: howto/investigate-a-stale-engineer-before-reap.md - Diagnose a Reaped Engineer After Goal Removal: howto/diagnose-a-reaped-engineer-after-goal-removal.md - Grant Engineer Write Permissions: howto/grant-engineer-write-permissions.md @@ -291,6 +292,7 @@ nav: - Investigate-Before-Reap API: reference/investigate-stale-engineer-api.md - Tombstoned-Goal Engineer Reaper API: reference/tombstoned-goal-engineer-reaper-api.md - Typed OODA Goal-Session Deterministic Rails: reference/typed-ooda-goal-session-rails.md + - Typed-OODA Ledger Concurrency Hardening: reference/typed-ooda-ledger-concurrency.md - Stable Goal-Session Identity API: reference/stable-goal-session-identity-api.md - Standing-Research Novelty-Directive API: reference/standing-research-goal-novelty-directive-api.md - Research-Goal Never-Idle Rail API: reference/research-goal-never-idle-rail-api.md @@ -305,6 +307,7 @@ nav: - Knowledge-Pack Article Relevance Ranking: reference/knowledge-pack-article-relevance-ranking.md - Enrichment Observability API: reference/enrichment-observability-api.md - StatusSnapshot API: reference/status-snapshot-api.md + - Gym Self-Eval Status Wiring: reference/gym-self-eval-status.md - Daily-Budget Display Guard: reference/daily-budget-display-guard.md - Overseer Activity Feed: reference/overseer-activity-feed.md - Overseer Tick Details: reference/overseer-tick-details.md @@ -330,6 +333,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 + - Deploy-Gate Canary Unit-Test Stage: reference/deploy-gate-unit-test-canary.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 diff --git a/src/self_relaunch/types.rs b/src/self_relaunch/types.rs index 461d647a6..f3ceddff6 100644 --- a/src/self_relaunch/types.rs +++ b/src/self_relaunch/types.rs @@ -120,6 +120,22 @@ mod tests { assert_eq!(RelaunchGate::RpcHealth.to_string(), "rpc-health"); } + /// P1 invariant (issues #4470/#4471): the blocking `UnitTest` canary gate + /// must remain part of the default self-deploy gate sequence. The sanctioned + /// resolution of the red-canary (exit 101) is to ROOT-CAUSE the failing unit + /// test — never to paper over it by dropping or disabling the gate. This + /// guard fails loudly if a future change quietly removes the unit-test canary + /// stage to make self-deploy "pass". + #[test] + fn canary_default_gates_include_blocking_unit_test_gate() { + let gates = default_gates(); + assert!( + gates.contains(&RelaunchGate::UnitTest), + "the blocking unit-test canary gate must not be disabled or removed \ + from the default self-deploy sequence: {gates:?}" + ); + } + #[test] fn gate_result_display_pass() { let result = GateResult { diff --git a/src/status/provider.rs b/src/status/provider.rs index 865fee5aa..9c0175ebf 100644 --- a/src/status/provider.rs +++ b/src/status/provider.rs @@ -611,11 +611,23 @@ fn clamp_u64(v: i64) -> u64 { // ── gym ───────────────────────────────────────────────────────────────────── fn assemble_gym(skip_gym: bool) -> SectionEnvelope { + // Issue #4483 (P3): when the gym is enabled, surface the REAL curated + // benchmark-scenario count and a non-idle self-eval state so the + // self-evaluation loop is no longer inert. When skipped, stay + // absent/idle (no behaviour change). + let (configured_scenarios, self_eval_state) = if skip_gym { + (None, "idle") + } else { + ( + Some(crate::gym::benchmark_scenarios().len() as u32), + "enabled", + ) + }; SectionEnvelope::live( Gym { skip_gym, - configured_scenarios: None, - self_eval_state: "idle".to_string(), + configured_scenarios, + self_eval_state: self_eval_state.to_string(), }, None, ) @@ -1019,6 +1031,39 @@ mod pure_helper_tests { assert!(!off.data.unwrap().skip_gym); } + /// Issue #4483 (P3): when the gym is ENABLED (`SIMARD_SKIP_GYM` unset) the + /// status section must report the REAL configured scenario count and a + /// non-idle self-eval state, so the self-evaluation loop is no longer inert. + /// When the gym is skipped it stays `absent`/`idle` (no behaviour change). + /// RED (TDD Step 7): fails until `assemble_gym` is wired to + /// `benchmark_scenarios()` and an `enabled` state. + #[test] + fn assemble_gym_reports_real_scenarios_and_enabled_when_enabled() { + let enabled = assemble_gym(false); + let g = enabled.data.as_ref().unwrap(); + assert!(!g.skip_gym); + let expected = crate::gym::benchmark_scenarios().len() as u32; + assert!(expected >= 1, "the curated benchmark set must be non-empty"); + assert_eq!( + g.configured_scenarios, + Some(expected), + "enabled gym must report the real benchmark-scenario count" + ); + assert_eq!( + g.self_eval_state, "enabled", + "enabled gym must report a non-idle self-eval state" + ); + + let skipped = assemble_gym(true); + let s = skipped.data.as_ref().unwrap(); + assert!(s.skip_gym); + assert_eq!( + s.configured_scenarios, None, + "skipped gym reports no configured scenarios" + ); + assert_eq!(s.self_eval_state, "idle", "skipped gym stays idle"); + } + #[test] fn goal_short_id_prefers_trailing_hex_suffix() { assert_eq!( diff --git a/src/status/render.rs b/src/status/render.rs index cfd7c5540..76a95fcbe 100644 --- a/src/status/render.rs +++ b/src/status/render.rs @@ -694,7 +694,7 @@ mod tests { Gym { skip_gym: true, configured_scenarios: Some(9), - self_eval_state: "active".to_string(), + self_eval_state: "idle".to_string(), }, Some(AS_OF.to_string()), ); diff --git a/src/typed_ooda/ledger.rs b/src/typed_ooda/ledger.rs index cf677f2f8..bcc19a92a 100644 --- a/src/typed_ooda/ledger.rs +++ b/src/typed_ooda/ledger.rs @@ -254,9 +254,42 @@ impl std::fmt::Debug for CapabilityHandler { impl CapabilityHandler { pub fn open(path: impl AsRef, policy: CapabilityPolicy) -> CapabilityResult { let mut connection = Connection::open(path.as_ref()).map_err(persistence)?; + // Issue #4483: absorb cross-process contention (daemon + engineer + // worktree + reaper share one ledger file) by waiting-and-retrying for + // up to 30s instead of erroring out with `database is locked`. connection - .busy_timeout(Duration::from_secs(5)) + .busy_timeout(Duration::from_secs(30)) .map_err(persistence)?; + // Issue #4483: apply WAL journal mode UNCONDITIONALLY at every open, not + // only inside the one-time schema-migration branch. A ledger created + // before WAL (or whose journal was reverted to rollback mode) is at + // `user_version == 1`, so `schema::initialize` short-circuits before it + // would re-assert WAL — leaving readers and writers serializing on a + // whole-file lock and surfacing the persistence crash-loop. + connection + .pragma_update(None, "journal_mode", "WAL") + .map_err(persistence)?; + // `pragma_update` ignores the journal-mode SQLite echoes back, so on an + // exotic filesystem that silently refuses WAL (e.g. some network mounts) + // the ledger would fall back to rollback journaling with no signal. + // Read the mode back and warn — non-fatal, since rollback journaling + // fails toward stricter (whole-file) locking, not data loss. + match connection.query_row("PRAGMA journal_mode", [], |row| row.get::<_, String>(0)) { + Ok(mode) if !mode.eq_ignore_ascii_case("wal") => { + tracing::warn!( + journal_mode = %mode, + "ledger open() requested WAL but SQLite reports a different journal mode; \ + concurrent readers/writers may serialize on a whole-file lock" + ); + } + Ok(_) => {} + Err(err) => { + tracing::warn!( + error = %err, + "ledger open() could not read back journal_mode to confirm WAL" + ); + } + } super::schema::initialize(&mut connection, now_millis()).map_err(persistence)?; Ok(Self { connection: Mutex::new(connection), @@ -3519,3 +3552,148 @@ mod actor_session_scope_tests { } } } + +/// Issue #4483 — typed-OODA outcome-persistence "database is locked" crash-loop +/// (RED phase, TDD Step 7). These tests specify the connection-tuning contract in +/// `docs/reference/typed-ooda-ledger-concurrency.md`: +/// +/// 1. WAL journal mode is applied UNCONDITIONALLY at every `open()`, so a +/// pre-existing v1 database (created before WAL, or whose journal was +/// reverted) still gets WAL — not only during the one-time schema +/// migration branch (`schema.rs`). Without this, two OS processes sharing +/// the ledger serialize on a whole-file lock and a writer that cannot +/// acquire it within the busy timeout fails with `database is locked`. +/// 2. A generous busy timeout (>= 30s) is set at open so a briefly-contended +/// writer waits and retries instead of erroring out. +/// 3. Concurrent writers on SEPARATE connections to the same file never +/// surface a `database is locked` persistence error. +/// +/// They MUST fail before the fix lands (WAL-in-migration-only, 5s timeout) and +/// MUST pass once the fix lands without further test edits. +#[cfg(test)] +mod ledger_concurrency_tests { + use super::*; + use std::sync::{Arc, Barrier}; + use std::thread; + + const POLICY_REVISION: &str = "policy-v1"; + + fn open_handler(path: &std::path::Path) -> CapabilityHandler { + CapabilityHandler::open(path, CapabilityPolicy::new(POLICY_REVISION)) + .expect("open capability handler") + } + + /// Read the live `PRAGMA journal_mode` of the handler's own connection. + fn journal_mode(handler: &CapabilityHandler) -> String { + let connection = handler.lock().expect("lock ledger"); + connection + .query_row("PRAGMA journal_mode", [], |row| row.get::<_, String>(0)) + .expect("query journal_mode") + } + + /// Read the live `PRAGMA busy_timeout` (milliseconds) of the handler's own + /// connection. + fn busy_timeout_millis(handler: &CapabilityHandler) -> i64 { + let connection = handler.lock().expect("lock ledger"); + connection + .query_row("PRAGMA busy_timeout", [], |row| row.get::<_, i64>(0)) + .expect("query busy_timeout") + } + + /// A1 (issue #4483): opening a PRE-EXISTING v1 ledger whose journal mode is + /// NOT WAL must still leave the database in WAL mode. This is the exact + /// crash-loop trigger: the WAL pragma lives only inside the schema-migration + /// branch, which is skipped once `user_version == 1`, so a database created + /// before WAL (or reverted to rollback journaling) opens WITHOUT the + /// concurrency mode that lets a reader and a writer coexist. + #[test] + fn open_applies_wal_journal_mode_on_preexisting_v1_db() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("outcomes.sqlite3"); + + // First open creates the schema at v1. Drop it so nothing else holds the + // file, then forcibly revert the journal to rollback mode to emulate a + // ledger created before WAL was introduced. + drop(open_handler(&path)); + { + let raw = rusqlite::Connection::open(&path).expect("raw open"); + let mode: String = raw + .query_row("PRAGMA journal_mode=DELETE", [], |row| row.get(0)) + .expect("revert journal mode"); + assert_eq!( + mode.to_ascii_lowercase(), + "delete", + "precondition: journal must be reverted to rollback mode" + ); + } + + // Re-open the now-pre-existing v1 database. The open path MUST re-assert + // WAL unconditionally. + let handler = open_handler(&path); + assert_eq!( + journal_mode(&handler).to_ascii_lowercase(), + "wal", + "open() must apply WAL journal mode even on a pre-existing v1 database" + ); + } + + /// A generous busy timeout (>= 30s) must be configured at open so a briefly + /// contended writer waits-and-retries instead of failing with + /// `database is locked`. + #[test] + fn open_sets_busy_timeout_at_least_30s() { + let dir = tempfile::tempdir().expect("tempdir"); + let handler = open_handler(&dir.path().join("outcomes.sqlite3")); + let timeout = busy_timeout_millis(&handler); + assert!( + timeout >= 30_000, + "busy_timeout must be >= 30000ms to absorb cross-process contention, got {timeout}ms" + ); + } + + /// Regression: concurrent writers on SEPARATE connections to the same ledger + /// file must never surface a `database is locked` persistence error. Each + /// thread opens its OWN `CapabilityHandler` (a distinct SQLite connection, + /// modelling the daemon + engineer-worktree + reaper processes) and hammers a + /// real write path (`release_engineer_claim`, an idempotent immediate-txn + /// DELETE) against a shared, pre-existing database. + #[test] + fn concurrent_cross_connection_writers_never_hit_database_locked() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("outcomes.sqlite3"); + // Materialise the schema once so every worker opens a pre-existing DB. + drop(open_handler(&path)); + + const WRITERS: usize = 6; + const ITERATIONS: usize = 40; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut handles = Vec::with_capacity(WRITERS); + for writer in 0..WRITERS { + let path = path.clone(); + let barrier = Arc::clone(&barrier); + handles.push(thread::spawn(move || { + let handler = open_handler(&path); + barrier.wait(); + for i in 0..ITERATIONS { + let claim_key = format!("rysweet/Simard:goal-{writer}-{i}"); + if let Err(err) = handler.release_engineer_claim(&claim_key) { + return Err(err.to_string()); + } + } + Ok(()) + })); + } + + for handle in handles { + let result = handle.join().expect("writer thread must not panic"); + if let Err(message) = result { + assert!( + !message.to_ascii_lowercase().contains("database is locked"), + "concurrent cross-connection writers must not hit a lock error: {message}" + ); + panic!("concurrent writer failed: {message}"); + } + } + } +} diff --git a/src/typed_ooda/schema.rs b/src/typed_ooda/schema.rs index 188a2023d..21bd621dc 100644 --- a/src/typed_ooda/schema.rs +++ b/src/typed_ooda/schema.rs @@ -12,7 +12,10 @@ pub(super) fn initialize(connection: &mut Connection, now_millis: i64) -> rusqli return Err(rusqlite::Error::InvalidQuery); } - connection.execute_batch("PRAGMA journal_mode = WAL;")?; + // WAL journal mode is applied unconditionally at connection open + // (`CapabilityHandler::open`), so it is guaranteed to be in effect here for + // both fresh and pre-existing databases — no need to re-assert it inside the + // one-time migration branch. let transaction = connection.transaction_with_behavior(TransactionBehavior::Immediate)?; let version = schema_version(&transaction)?; if version == SCHEMA_VERSION { diff --git a/tests/adaptive_scaling.rs b/tests/adaptive_scaling.rs index d3aa38961..ce54bf3a6 100644 --- a/tests/adaptive_scaling.rs +++ b/tests/adaptive_scaling.rs @@ -312,9 +312,16 @@ 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 `SIMARD_SCALING` from the process env, so on + // a host with `SIMARD_SCALING=auto` the default scaler would override the + // explicit `max_concurrent_actions` under test and the result would depend + // on the environment rather than the config. Forcing `scaler: None` keeps + // the test hermetic (issue #2732). let config = OodaConfig { max_concurrent_actions: scaler.current_max(), + scaler: None, ..OodaConfig::default() }; diff --git a/tests/status_snapshot.rs b/tests/status_snapshot.rs index 4d2103a76..665c433d6 100644 --- a/tests/status_snapshot.rs +++ b/tests/status_snapshot.rs @@ -168,6 +168,13 @@ fn absent_section_serializes_as_unavailable_absent_not_zero() { fn assemble_on_empty_state_root_never_panics_and_degrades() { let _skip = EnvGuard::unset("SIMARD_SKIP_GYM"); let dir = tempfile::tempdir().expect("tempdir"); + // The daemon section falls back to the durable `daemon_health.json` + // heartbeat under `dirs::data_local_dir()/simard/`, which is NOT under the + // state root. Pin `XDG_DATA_HOME` to the empty tempdir so the heartbeat + // resolves to a nonexistent path — otherwise a live daemon writing its + // heartbeat on the host would leak in and this test would depend on the + // environment rather than the (empty) state root. + let _data_home = EnvGuard::set("XDG_DATA_HOME", &dir.path().to_string_lossy()); let snap = status::assemble(&hermetic_opts(dir.path())); // Structurally complete: generated + schema version set.