Update documentation with 6 changed files (#4468)

[rysweet]

Summary

Concise workflow-generated PR for documentation.

Issue

Closes #4468

Changed files

• docs/index.md
• docs/reference/typed-ooda-benign-goal-race-and-outbox-serialization.md
• src/ooda_actions/advance_goal/typed_goal_session.rs
• src/typed_ooda/executor.rs
• src/typed_ooda/ledger.rs
• src/typed_ooda/types.rs

Diff stat

 docs/index.md                                      |   1 +
 ...da-benign-goal-race-and-outbox-serialization.md | 603 +++++++++++++
 .../advance_goal/typed_goal_session.rs             |  22 +-
 src/typed_ooda/executor.rs                         | 179 ++++
 src/typed_ooda/ledger.rs                           | 931 ++++++++++++++++-----
 src/typed_ooda/types.rs                            |   7 +
 6 files changed, 1547 insertions(+), 196 deletions(-)

Behavior

Implemented behavior through these branch commits:
b4bdb69 fix(typed-ooda): make ledger open() lock-resilient + de-flake outbox concurrency test (#4468)
50e5407 test(typed-ooda): direct unit coverage for busy/locked retry logic
c34d360 perf(typed-ooda): avoid per-retry String alloc in busy classifier
28d479f refactor(typed-ooda): extract benign no-op finish into named helper
f593341 fix(typed-ooda): benign goal-race no-op + outbox busy-retry (#4468)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by getting the branch name and inspecting the current changes to detect toolchains.

● Get branch name and changed files (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4468-fix-a-systemic-concurrencyli
│ fecycle-defect-in-sima && git branch --show-current && echo "---CHANGED---" && git status
│ --porcelain && echo "---DIFFSTAT---" && git --no-pager diff --stat HEAD
└ 7 lines…

Let me review the diff and recent commits to understand what's been changed.

● Review commits and working diff (shell)

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 5 commit(s) ahead of main
• Code review completed
• Philosophy check passed

---

This PR was created as a draft for review before merging.

---

Step 16b: Outside-In Testing Results

Tested from the PR branch (feat/issue-4468-fix-a-systemic-concurrencylifecycle-defect-in-sima) as a consumer would, exercising the compiled binary and the typed-OODA effect-dispatch / outbox-persistence boundary.

Detected toolchains: Rust / Cargo (Cargo.toml, edition 2024, Cargo.lock; cargo 1.95.0). Auxiliary Node harness (package.json) present but the changed code (src/typed_ooda/, src/ooda_actions/advance_goal/) is pure Rust.

Chosen strategy: Per the qa-team skill's Rust-CLI repo-type detection, native cargo is the outside-in boundary. Validate the operator binary builds and runs, then drive the concurrency/lifecycle regressions through cargo test.

#	Scenario	Command	Result	Key output
1 (simple)	Operator binary builds and responds at its CLI boundary	cargo build --bin simard + ./target/debug/simard --help	✅ PASS	Finished dev profile ... in 3m 16s; help text printed, EXIT=0
2 (edge/integration)	Concurrency + lifecycle regressions for dispatch-after-goal-removed and outbox-under-lock	cargo test --lib typed_ooda	✅ PASS	test result: ok. 61 passed; 0 failed

Regression coverage verified (new tests, all passing):

• executor::tests::dispatch_after_goal_removed_is_benign_no_op — goal removed between prepare and dispatch is a benign, counted no-op (no DownstreamFailed).
• executor::tests::permanent_effect_failure_is_not_treated_as_benign_no_op — genuine permanent failures are NOT masked (no silent fallback).
• ledger::outbox_serialization_tests::concurrent_outbox_writes_never_surface_database_is_locked — concurrent writers no longer surface database is locked.
• ledger::outbox_serialization_tests::concurrent_open_of_shared_ledger_survives_contention — startup-recovery/open contention is lock-resilient.
• ledger::outbox_serialization_tests::retry_on_busy_{returns_immediately_on_success, retries_transient_contention_then_succeeds, bounds_attempts_and_surfaces_the_contention_error, does_not_retry_non_contention_errors} — bounded busy/locked retry classifier behaves correctly and still surfaces non-contention errors.

Fix count during outside-in testing: 0. Both the simple and edge/integration scenarios passed on the first run against the current branch head; no additional fixes or commits were required.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	194777	163599	84.0%

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

[rysweet]

Comprehensive Code Review — #4468 (benign goal-race no-op + outbox busy-retry)

Verdict: Approve. Reviewed the full change set (origin/main...HEAD, 6 files, +1547/-196) across ledger.rs, executor.rs, types.rs, typed_goal_session.rs, and docs. Verified compilation and ran the targeted suite: 61/61 typed_ooda tests pass, including all new regression coverage. No blocking issues found.

Review checklist

• Code quality & standards — additive-only; existing constructors, schema, and EffectResult untouched. eprintln! correctly replaced with structured tracing::warn!. No print!/println!, no Bridge naming.
• Test coverage adequate — new coverage is strong: benign-no-op constructor flags, dispatch-after-goal-removed (asserts Ok, not DownstreamFailed, closed outbox row, idempotent redispatch), negative guard that a real permanent failure still fails, busy classifier (typed-code + marker), retry loop (success/transient/exhaustion/non-busy), plus two real-thread concurrency tests (8×60 writers + recovery sweeper; 12-way concurrent open).
• No TODOs, stubs, or swallowed exceptions — every swallowed race emits a structured tracing event + record_metric counter; retry exhaustion surfaces the original PersistenceFailed unmasked.
• No unimplemented functions — none.
• Logic correctness — verified below.
• Edge case handling — verified below.

Correctness verification

1. retry_on_busy (ledger.rs) — Correct. Bounded at MAX_ATTEMPTS=6 (guard attempt + 1 < MAX_ATTEMPTS → exactly 6 op() calls, test-confirmed). 1u32 << attempt cannot overflow (attempt caps at 4 before the guard stops it) and backoff is .min(400ms). The connection Mutex guard is acquired inside op and dropped before sleep, so backoff never holds the lock — verified drop ordering and the INVARIANT doc-comment matches the call sites.

2. Busy classifier — Robust, not brittle. is_busy_locked matches typed rusqlite::ErrorCode::{DatabaseBusy,DatabaseLocked}, never message text. capability_error_is_busy requires both PersistenceFailed and the [sqlite-busy] marker, and that marker is stamped only by persistence() from the typed code — so log/payload text cannot steer retries (test capability_error_is_busy_keys_on_typed_marker_not_free_text proves free text and wrong-code spoofing are rejected). Constraint/logic errors correctly never retry.

3. Transaction boundaries under retry — Safe. Each retry re-runs a fresh BEGIN IMMEDIATE; any error drops the Transaction → rollback, no partial writes. Idempotent: every wrapped closure begins with replay_request (keyed on request_id), and terminal outcomes are built once before the loop so outcome_id/timestamp stay stable across retries (the outcome.clone() inside the closure is the right fix). Good catch hoisting outcome construction — a naive wrap would have minted a fresh id per attempt.

4. Benign goal-race no-op (typed_goal_session.rs + executor.rs) — Correct and observable. Both reclassified sites (L338 "before spawn", L467 "before effect dispatch") are read-only goal lookups before any side effect, so nothing is half-executed. Routes to finish_effect_as_benign_no_op → closes the outbox row Succeeded{evidence:vec![]} (never redispatched) + tracing::warn + counter. benign_no_op stays permanent as a defense-in-depth safety net, and permanent/retryable do not set no_op (test-confirmed) so a real failure can never be masked as success. The no_op check is correctly ordered before the !permanent retry branch in execute_claimed.

Non-blocking observations (not defects)

1. Intentional scope boundary. Of the write paths using BEGIN IMMEDIATE, retry_on_busy wraps the startup-recovery/live-cycle collision paths (open/init, record_action/commit_terminal, claim_effect_for_outcome, recover_expired_effects, release_effect_for_retry, finish_effect). Others — claim_next_effect, renew_effect, record_progress, register_actor_session, issue_privileged_approval, engineer-claim mgmt, reserve/update_process_execution — still rely on the pre-existing WAL + busy_timeout(5s). This is a deliberate, sound boundary for the startup-recovery vs. live-cycle target of #4468, and the concurrency test confirms no database is locked surfaces. Flagging only so the boundary is a conscious decision: if future load shows contention on claim_next_effect/renew_effect, the same wrapper extends cleanly.
2. Docs are thorough and accurately describe the additive behaviour, journal signatures eliminated, and no-op semantics.

Gates: cargo build clean · 61/61 targeted tests green · additive-only, no happy-path change. Merge-ready.

[rysweet]

🔒 Security Review — #4468 (benign goal-race no-op + outbox busy-retry)

Verdict: PASS — no exploitable security vulnerabilities found. Reviewed the full change set (origin/main...HEAD, 6 files, +1547/-196) across ledger.rs, executor.rs, types.rs, typed_goal_session.rs, and docs. Independent dual review (self + security specialist).

Checklist

• Injection (SQL/command/path/log) — no findings
• New vulnerabilities — none introduced
• Sensitive-data handling — no secret/PII/payload leakage
• Authn/Authz — no bypass on the new no-op path
• DoS / resource exhaustion — bounded retry, no amplification

Findings by area

1. SQL injection — CLEAN. Every SQLite statement in the diff (UPDATE effect_jobs claim/recover/retry/finish, all SELECT/query_row paths) uses bound parameters (?1/?2/params![…]), all static string literals. The only format! strings touching SQL-adjacent code build the expected_claim equality value (bound, never concatenated into SQL) and human-readable error/log text. No string-interpolated SQL exists.

2. Retry / DoS — BOUNDED, no overflow. retry_on_busy caps at MAX_ATTEMPTS = 6; retry guard attempt + 1 < MAX_ATTEMPTS ⇒ attempt ∈ {0..4}, so backoff shift 1u32 << attempt never exceeds 1 << 4 = 16 — no shift/integer overflow. Backoff .min(400ms). Connection mutex is released across each sleep (acquired inside the closure). Exhaustion surfaces the real error rather than looping — no unbounded loop, no unbounded memory, no attacker-forced amplification.

3. Busy classifier — NOT steerable by untrusted input. is_busy_locked matches only on typed rusqlite::ErrorCode::{DatabaseBusy,DatabaseLocked}. The [sqlite-busy] marker is stamped exclusively inside persistence() and only when is_busy_locked is true; capability_error_is_busy additionally requires code() == PersistenceFailed. Tests confirm free text ("database is locked") and a marker on a wrong error code are both rejected. Worst residual (a DB error Display coincidentally containing the marker substring) would cause only ≤6 bounded retries of an idempotent op — no security impact.

4. Transaction / idempotency — PRESERVED. Each retried closure re-runs replay_request(...) first (returns the existing row on hit), so retries never double-write. Terminal outcome/outcome_id/timestamp are minted once before the retry loop and cloned per attempt, so a re-committed attempt re-commits the same identity. Lease-guarded updates keep state='running' AND lease_owner=? AND lease_generation=? predicates with the changed != 1 → stale_lease guard. No double-write or state-corruption path exploitable by a concurrent actor.

5. Benign no-op — NO authz/lease bypass. benign_no_op is reached only when a read-only goal lookup fails (goal completed/removed before any side effect ran). It closes the outbox row via the normal finish_effect path (retaining its lease/state predicates) as a succeeded empty-evidence no-op. The effect was already admitted at record time; with the goal gone there is nothing to authorize or execute. No authorization or lease check is skipped.

6. Sensitive data / log injection — NONE. New tracing::warn! and record_metric calls emit internal IDs (effect_id, outcome_id, goal_id) and static reason strings, not secrets/tokens/PII/payloads. Metric context is serialized via serde_json::to_string before write to metrics.jsonl, so special chars are JSON-escaped — no structured-log injection. The eprintln! → tracing::warn! change reduces exposure.

Non-blocking hardening note (defense-in-depth, not a vulnerability)

capability_error_is_busy recognizes the busy class via a substring .contains("[sqlite-busy] ") on the error message rather than a dedicated typed field. This is safe today (marker stamped only on typed ErrorCode; wrong-code/free-text rejected by tests) and any residual false-positive is bounded to ≤6 idempotent retries. A future refactor could carry the busy flag as a typed field on CapabilityError to remove the string channel entirely. Not required for merge.

Conclusion: No findings meet the reporting threshold. Approved from a security standpoint.

[rysweet]

🛡️ Philosophy-Guardian Review — #4468 (benign goal-race no-op + outbox busy-retry)

Scope reviewed: ledger.rs, executor.rs, typed_goal_session.rs, types.rs, and the two docs files (+1547/−196). Read the actual diffs and cross-checked against existing repo conventions.

Compliance Checklist

Principle	Verdict	Notes
Ruthless simplicity / Occam's razor	✅	retry_on_busy is one ~25-line bounded loop; classifiers are two tiny typed-code predicates. No unnecessary layers.
Bricks & studs / clean module boundaries	✅	All changes stay inside typed_ooda; new studs (benign_no_op, message()) are minimal and scoped (pub(crate)).
Zero-BS (no stubs / faked APIs / swallowed exceptions)	✅	No TODOs, stubs, or dead code. Every race is surfaced (tracing + metric); retry exhaustion returns the original busy error unmasked. Fakes are test-only.
No over-engineering / no speculative future-proofing	✅	Scope boundary (only recovery/retry/finish/terminal/claim/open writes wrapped) is deliberate and documented; other paths rely on WAL + busy_timeout.
Error transparency	✅	Retry keys on the typed rusqlite::ErrorCode, never on message text; tests prove free-text can't steer retries and constraint errors never loop.

Detailed Findings

Strengths ✓

• Retry classification is injection-proof and code-driven. is_busy_locked matches only DatabaseBusy/DatabaseLocked; capability_error_is_busy requires both PersistenceFailed and the typed-only marker. A test explicitly proves message text alone cannot trigger a retry.
• Retry exhaustion never masks failure. retry_on_busy hard-caps at 6 attempts and returns the original busy error; a test asserts the surfaced error is still the contention failure. Non-busy errors pass through on the first call.
• Benign no-op cannot mask a real failure. benign_no_op sets no_op: true and keeps permanent: true as a defense-in-depth net. The dispatch arm checks error.no_op before the retryable branch, and a guard test proves a genuine permanent failure still yields DownstreamFailed and a failed row.
• Race stays observable, not swallowed. finish_effect_as_benign_no_op emits a structured tracing::warn + a counter and closes the row idempotently; call-site comments correctly justify the reclassification as pre-side-effect (read-only lookups before any side effect).
• Correct retry-loop invariant. The Mutex<Connection> guard is acquired inside each closure so backoff sleeps hold no lock. Outcome identity is built once outside the loop so retries re-commit the same outcome_id.
• Concurrency tests are real regressions, not smoke tests: 8 writers × 60 iters + a concurrent recovery sweeper on the same file, plus a 12-way concurrent-open race, both asserting zero database is locked and durable persistence.

Non-blocking notes ⚠

• Best-effort let _ = record_metric(...) discards the metric-write Result. Matches an existing repo pattern, and the primary tracing::warn already makes the event visible, so the race is not hidden. Consider if let Err(e) for parity so a metrics-pipeline regression is itself visible. Not a violation.
• Retry classification piggybacks on a [sqlite-busy] string marker in the human-readable message rather than a typed discriminant. A mild leaky abstraction, but well-mitigated (marker stamped only from the typed error code, check also requires PersistenceFailed, explicitly tested against spoofing). A cleaner long-term design would add a typed busy variant to CapabilityError — a larger refactor, correctly out of scope here.

Violations ✗ — None.

Docs Assessment (603 lines)

Proportionate — not bloat. A genuine design reference for a systemic concurrency/lifecycle defect: covers both fixes, the exact journal signatures eliminated, which sites are reclassified vs. stay permanent, the API contract, PRAGMAs, observability, a copy-pasteable verification section, a security model (why text can't steer retries), and a negative-scope section. Correctly linked from docs/index.md.

Overall Verdict: ✅ PASS-WITH-NOTES

Philosophy-aligned: minimal, well-bounded, zero-BS, and exemplary on error transparency. The two notes are minor, defensively tested, consistent with existing repo patterns, and safe to merge as-is or address in follow-up.

Posted as Step 17d Philosophy Guardian Review evidence.