Plan: Rules Evaluation Timeout (#784)

On this page

Status

Step Description Status

0

Commit this plan + nav link as the branch’s first commit; apply the scoped plan label to #784

Done (2026-07-17) — eb6b418f; Plan::Rules-Eval-Timeout applied

1

Budget config: pure typed parse helper + post-bootstrap env read in main.rs (no ServiceSettings change)

Done (2026-07-17) — d626ec0c (config.rs: typed error, example tests + 3 proptests)

2

Engine: EvalTimeout, dispatch_eval, best-effort skip via the seamed eval_loop_inner

Done (2026-07-17) — bc85afff (red, kill @120.003s recorded) + d626ec0c (green)

3

API: 503 mapping via extracted engine_error_to_api + utoipa truth-up (200/401/403/404/500/503) + --service craig-rules page regen

Done (2026-07-17) — mapping + annotation in d626ec0c; page regenerated from the branch’s own ApiDoc::openapi() (503 row verified) in the Step 5 doc commit

4

Tests: paused-time suite + real-loop + cross-thread + mapping + config proptests (all inline — bin crate)

Done (2026-07-17) — d626ec0c; 108/108 crate tests green (test 6 adapted: runtime-failing expressionNode graph — the echo fixture coerces null input to {}, so the #785 path was not reachable; strengthening recorded)

5

Contracts & docs: ADR-006 (both sites) + implementation-guide, CHANGELOG entry, config/deploy/testing docs

Done (2026-07-17) — all six surfaces in the Step 5 doc commit; .claude/CLAUDE.md explicitly N/A (weight-3 fix, no status-table surface)

6

File 7 follow-up issues (full quality bar) + reconcile #876/#783/#778/#971 + bump #1046

Done (2026-07-17) — filed #1048–#1054 (+ #1055, an 8th from the Step-5 J-review: the api-docs generator’s In/Required defect), all `/relate`d; #876 amended to shipped reality; #783 AC 1 checked + related to #1046; #1046 bumped to P2-medium with the severity note

Issues: #784
Branch: feature/784-rules-eval-timeout
MR: !1004 (2026-07-17; archived in-MR once the IID was known)

Provenance: 5 contextless review rounds (3 internal-lens rounds + 1 external + 1 confirmation; findings 40 → ~35 → 19 → 11 → polish-only, all folded in). User-decided forks: ADR-006 amended + failure-audit follow-up (not designed here); the setting is rules-local.

Context

run_eval (services/craig-rules/src/engine.rs:208) sends an EvalRequest to the single-threaded zen-engine eval thread and awaits the reply unbounded. Two callers share it: the HTTP evaluate endpoint (authz-gated via rules_authz_or_admin_fallback; caller-supplied JSON, non-admin-reachable) and the RabbitMQ domain-event subscriber (main.rs:318-332). One slow evaluation hangs its caller; a wedged one eventually blocks all callers (256-slot queue). #911 made the lying comments honest; engine.rs:26 and :205-207 defer to #784 — this plan implements the bound they promise.

Severity above the original "medium": arbitrary caller JSON can wedge the sole eval thread, and a running zen evaluation cannot be cancelled — real recovery is #1046 (priority bumped in Step 6).

Scope

In: rules-local budget config · one budget over send+reply · EvalTimeout → 503 · best-effort dead-work skip · truthful MQ/audit/docs contracts · full test suite · utoipa + api-page truth-up.

Out (every item tracked in Step 6 or already open):

  • Wedge recovery → #1046 (priority bump — done, P2-medium)

  • craig-authz mirrored loop (must fail closed) → #1047

  • Failure/attempt audit rows (user-deferred; the gap predates this plan) → #1048

  • application/problem+json media-type debt → #1049

  • configuration-reference page-wide audit → #1050

  • cases-side 503 pass-through → #1051

  • CHANGELOG NUL-byte fix (separate chore — found-mid-implementation rule; the byte is load-bearing inside a backticked literal at CHANGELOG.adoc:1796 and needs a printable escape, not deletion) → #1052

  • MQ subscriber/inbox retry discrepancy (pre-existing: subscriber DLXes on 2nd delivery; inbox’s 5-retry path unreachable; failed_at never stamped) → #1053

  • case.intake_created payload-key mismatch + the eligibility.submitted dead-from-birth subscription (pre-existing; placement.requested is already #971’s) → #1054

  • api-docs generator In/Required rendering defect (found by the Step-5 J-review) → #1055

Design

D1 — One budget encloses send + reply

Capacity bounds queue depth, not wait time; a reply-only bound leaves senders hanging on a wedged thread + full queue. tokio mpsc send is cancel-safe: timing out mid-send drops the request unqueued (pinned by test 2’s drain assertion).

D2 — 503 via the existing ApiError::service_unavailable

craig-rules is the origin server; zen is an internal worker — 504 was semantically wrong. Reuse costs nothing new (variant, constant, 503 test all exist) and the wire arm echoes the detail we pass: the fixed PII-free rules evaluation timed out after {budget_ms} ms. Recorded residuals: (a) clients discriminate this 503 from others only by detail string — if a client ever needs to branch, service_unavailable_typed is a small precedented addition (the #311 sub-typing pattern); (b) until the cases pass-through follow-up lands, the two s2s consumers fold this into their own redacted 500 — the 503 contract holds for direct callers (CLI, curl) today. No Retry-After (no truthful estimate; no consumer auto-retries today).

D3 — MQ path: propagate and disclose the REAL semantics

The subscriber’s actual discipline (verified): first handler Err → nack requeue=true; the redelivered attempt → nack requeue=false → broker DLX on the 2nd failure, the two failures ~budget+3-5s apart (inbox Path C sleeps backoff_for_attempt(1) = 4s ±25% before the re-run). The inbox’s 5-retry/backoff path is unreachable on this route and failed_at is never stamped (pre-existing discrepancy → Step 6 issue, related to open #778 whose premise it corrects). Nothing replays the DLX (craig-security’s DLQ consumer is forensic capture + alerting only).

Accepted because today’s blast radius is ~zero, with evidence: of the three subscribed events, placement.requested has no producer (tracked in open 971) and eligibility.submitted has never had one (dead from birth — no emission commit in full history), while case.intake_created’s evaluation is advisory today: `rule_evaluations has no readers outside craig-rules; rules.evaluated feeds a craig-financial no-op logger (open #245 would make it functional — a timestamped fact, not a permanent one) and craig-security’s wildcard () audit subscriber, so a timed-out MQ eval also leaves no security-audit row (same class as the D4 gap; stated in the ADR amendment). The intake_created handler also reads a payload key the producer never sends (case_id vs investigation_id/referral_id → context_id always None; pre-existing, filed Step 6). No separate MQ budget (YAGNI on dead/advisory paths). CHANGELOG discloses: domain-event evaluations that exceed the budget dead-letter after ~2 attempts.

D4 — Audit contract amended, not silently violated

ADR-006’s "every evaluation is recorded" is already false for zen runtime errors (? before the audit write). Amend BOTH ADR sites (adr-006-rules-engine.adoc:30 and :36) plus implementation-guide.adoc:1054 to completed-evaluation wording with the failure-class gap explicit (including timed-out-then-late-completing evals being discarded unaudited). Failure-audit rows for all classes → Step 6 follow-up (user-decided).

D5 — Dead-work skip: best-effort, with an honest test seam

A guard at the loop top skips requests whose caller gave up — racy by nature (post-check cancellation happens; running evals untouchable). Seam: eval_loop becomes a thin wrapper over eval_loop_inner(rx, evals_run: &AtomicUsize) — the counter param IS the test oracle (no cfg(test) production code beyond one delegation line; each test owns a private counter; safe under any runner). The guard’s else-path reply send keeps the existing // SILENT-OK: marker semantics.

D6 — Tie semantics: reply wins at the deadline

tokio Timeout polls the inner future first; a reply ready in the same tick as the deadline succeeds. Documented on dispatch_eval, pinned by test 4. Corollary (verified): no await sits between send returning and the dispatched = true assignment, so the flag is reliable; its meaning is "queued, but no reply within budget" — NOT a wedge diagnosis (deep queue ≠ wedged thread; the doc says so).

D7 — Config: pure typed parse helper + post-bootstrap env read (no wrapper struct)

Review round 3 killed a wrapper-struct design: craig-rules boots via craig_api::bootstrap("CRAIG_RULES", …) which runs dotenvy and loads ServiceSettings internally — a wrapper double-loads or dead-codes, and a pre-bootstrap load misses dotenvy. Rules-local instead means: services/craig-rules/src/config.rs exposes parse_eval_timeout_ms(raw: Option<&str>) → Result<u64, EvalTimeoutConfigError> (pure, typed error, unit- and property-tested, no env) and eval_timeout_ms_from_env() (2-line wrapper reading CRAIG_RULES__EVAL_TIMEOUT_MS), called in main.rs after bootstrap(…​) returns. Nothing shared changes.

D8 — Range 1..=25_000 ms, default 5_000, honestly worded

Default justified by the seeded corpus (largest ruleset: 9 nodes/19 edges — single-digit-ms evals; ~3 orders of magnitude headroom) while keeping a timeout storm’s per-request cost short. Cap rationale, correctly scoped: bounds wedge exposure everywhere, and for HTTP callers stays under the shared 30s client budget with headroom for authz + audit + network (crates/craig-common/src/http.rs:20); the MQ path has no HTTP budget, so the cap there is purely wedge-exposure bounding. Success latency ≈ budget + audit write (the audit tx runs after the dispatch, bounded separately by db_statement_timeout_ms) — stated in the config docs.

Steps

Step 0 — Commit the plan first

This file + the nav Active entry are the branch’s first commit (docs(plan): commit the #784 rules-eval-timeout plan). Apply the scoped Plan::Rules-Eval-Timeout label to #784 (auto-drops Plan::NEEDED).

Step 1 — Budget config (services/craig-rules/src/config.rs, new)

/// Why parsing failed. Typed per the errors-are-typed rule; Display
/// carries the operator-facing D8 message.
#[derive(Debug, thiserror::Error)]
pub(crate) enum EvalTimeoutConfigError {
    #[error("CRAIG_RULES__EVAL_TIMEOUT_MS must be in 1..=25000 (got {got}): values above 25s leave no headroom under HTTP callers' shared 30s budget and extend wedge exposure")]
    OutOfRange { got: u64 },
    #[error("CRAIG_RULES__EVAL_TIMEOUT_MS must be a positive integer (milliseconds), got {raw:?}")]
    NotANumber { raw: String },
}

/// Parse the eval-dispatch budget from its env string. Pure so the
/// range contract is unit- and property-testable without env mutation.
pub(crate) fn parse_eval_timeout_ms(raw: Option<&str>) -> Result<u64, EvalTimeoutConfigError>

  • None → 5_000. Some → parse u64, then range 1..=25_000.

  • pub(crate) fn eval_timeout_ms_from_env() → Result<u64, EvalTimeoutConfigError> reads CRAIG_RULES__EVAL_TIMEOUT_MS and delegates.

  • main.rs calls it after bootstrap(…​), propagates the Err through main’s existing anyhow context, passes the value into RulesEngine::new, and `info!`s the budget (the live-verification hook).

  • Tests (inline mod, pure fn — no env, no unsafe): None-default; valid value; 0, 25_001, "abc" rejected via matches! + Display-message assertions. Plus the mandatory proptests (parser, universally-quantified invariants): ∀ v in 1..=25_000 → Ok(v); ∀ u64 outside → OutOfRange; ∀ non-numeric string → NotANumber, never a panic.

Step 2 — Engine (services/craig-rules/src/engine.rs)

  1. Variant:

    /// The eval dispatch missed its budget. `dispatched: false` means
/// send never completed (queue full); `true` means queued but no
/// reply within budget — deep queue OR slow/wedged evaluation, the
/// flag does not distinguish. The evaluation is NOT cancelled
/// (#1046); the loop skips dead requests best-effort.
#[error("rules evaluation of '{rule_set}' exceeded the {budget_ms} ms budget (dispatched: {dispatched})")]
EvalTimeout { rule_set: String, budget_ms: u64, dispatched: bool },

  2. RulesEngine.eval_budget_ms: u64, threaded through the single constructor (engine.rs:97) from main.rs:95.

  3. Free fn (the engine is not unit-constructible — channel/thread/DB in new()):

    async fn dispatch_eval(
    eval_tx: &mpsc::Sender<EvalRequest>,
    rule_set: &str,
    decision: Arc<ZenDecision>,
    // STRUCTURAL-VALUE: JDM rule content — zen-engine polymorphic
    input: serde_json::Value,
    budget_ms: u64,
    // STRUCTURAL-VALUE: JDM rule content — zen-engine polymorphic
) -> Result<serde_json::Value, EngineError>

    One tokio::time::timeout(Duration::from_millis(budget_ms), fut) — ms stay u64; the sole conversion is at the timer boundary. fut: send (Err → WorkerUnavailable), set local dispatched = true, reply await (Err → WorkerUnavailable), inner Err → Evaluation. Elapsed → EvalTimeout { rule_set, budget_ms, dispatched }. The STRUCTURAL-VALUE markers are load-bearing (B3a budget gate). Doc records the D6 tie semantics.

  4. run_eval gains rule_set: &str (its one caller has it) and delegates.

  5. eval_loop → thin wrapper over eval_loop_inner(rx, evals_run: &AtomicUsize); the inner loop top gains:

    // Best-effort: skip requests whose caller already gave up. Racy by
// design — post-check cancellation still evaluates for a dead
// oneshot (SILENT-OK send below), and a RUNNING eval is
// uncancellable (#1046).
if req.reply.is_closed() { continue; }

  6. Truth up engine.rs:26 and :205-207.

Step 3 — API mapping + page truth-up

  1. services/craig-rules/src/api.rs (~:774) — extract the whole EngineError→ApiError closure into a named fn fn engine_error_to_api(e: EngineError) → ApiError (the handler’s .map_err calls it) so the mapping is testable. New arm, destructured (no unreachable!, no wildcard — both workspace-denied):

    crate::engine::EngineError::EvalTimeout { rule_set, budget_ms, dispatched } => {
    tracing::warn!(%rule_set, budget_ms, dispatched, "rules evaluation timed out");
    ApiError::service_unavailable(format!(
        "rules evaluation timed out after {budget_ms} ms"
    ))
}

    New test in api.rs’s existing inline #[cfg(test)] module: eval_timeout_maps_to_service_unavailable_with_budget_detail — asserts ApiError::ServiceUnavailable with the exact detail string (a mis-mapping into the internal arm would otherwise pass the whole suite).

  2. The evaluate #[utoipa::path] currently declares only 200/401/404 (api.rs:741-745) — the regen reflects annotations, not runtime, so amend the annotation to the real set: 200/401/403/404/500/503 (all ProblemDetails-bodied like adjacent entries).

  3. Regen ONLY this page: cargo xtask api-docs --service craig-rules against the branch-built devstack (output is deterministic: sorted paths/codes, no timestamps/ports); discard any diff to other services' pages (that sweep is #1022’s single-docs-MR mandate — the MR carries Relates to #1022); post-regen, assert the page contains the 503 row (guards against a stale running image).

Step 4 — Tests

All inline #[cfg(test)] in engine.rs/config.rs/api.rs — craig-rules is a bin-only crate; tests/ integration files cannot reach pub(crate) internals. Dev-deps: tokio = { workspace = true, features = ["test-util"] } + proptest = { workspace = true } (neither present today; craig-test-lib re-exports neither).

Harness facts: spawn the dispatch (tokio::spawn + async move with a cloned sender); yield_now().await before tokio::time::advance; matches! assertions (EngineError: !PartialEq); import Duration; requests built from the minimal JDM fixture (crates/craig-test-lib/src/builders.rs:40) through the pub compile_rule_set(&Value) — an empty graph is invalid JDM, so use the fixture. Seam-drive mechanics (the one 'static trap): a test-local AtomicUsize cannot go through spawn_local — test 7 drives loop and client concurrently in ONE task (tokio::join! inside LocalSet::run_until, dropping the sender to end the loop); test 8 moves an Arc<AtomicUsize> clone into the std::thread and passes &*arc inside. Production eval_loop wraps eval_loop_inner(rx, &local_counter) in its own future — no 'static needed; the rt.block_on topology at engine.rs:117 is unchanged.

Paused-time (#[tokio::test(start_paused = true)]):

  1. eval_timeout_fires_at_budget_when_reply_never_arrives — drain request, hold oneshot; advance(budget); EvalTimeout { dispatched: true, .. }, exact budget + rule set.

  2. eval_timeout_on_full_queue_reports_undispatched — capacity-1 pre-filled, receiver silent; EvalTimeout { dispatched: false, .. }; then drain and assert only the pre-fill was queued (cancel-safety).

  3. eval_reply_within_budget_is_unaffected.

  4. reply_ready_at_exact_deadline_wins (pins D6).

  5. worker_gone_maps_to_worker_unavailable — send-err (receiver dropped) and reply-err (oneshot sender dropped post-drain) halves.

Real-loop (current-thread + LocalSet; zen futures are !Send):

  1. zen_error_maps_to_evaluation — invalid input through the real loop.

  2. eval_loop_skips_requests_whose_caller_gave_up — two requests (dead first, live second) through eval_loop_inner with a private counter; assert counter == 1 AND live answered. Real oracle — fails without the guard.

  3. Cross-thread variant of 7: real std::thread + its own single-thread runtime (production topology), plain #[tokio::test] (no paused clock across threads).

API mapping (api.rs inline module):

  1. eval_timeout_maps_to_service_unavailable_with_budget_detail — pins the extracted engine_error_to_api per Step 3.1.

Config (config.rs inline module): the example + property tests per Step 1.

Dropped, deliberately:

  • Reply-send-failure-after-guard test — the only reachable window is the check-then-cancel race, not deterministically constructible; the absorption is the pre-existing SILENT-OK contract, unchanged here.

  • MQ-propagation unit test — handle_domain_event takes a concrete RulesEngine (not unit-constructible) and the coupling is a single ? at main.rs:332; the subscriber/inbox machinery has its own suites; the MQ path’s blast radius today is ~zero (D3). The dead-subscription follow-up owns any revisit.

Red-before-green: the first implementation commit extracts dispatch_eval WITHOUT the timeout; test 1 → nextest slow-timeout kill (.config/nextest.toml:9) proves the defect; the next commit adds the timeout; both runs recorded in the MR.

Step 5 — Contracts & docs

  1. ADR-006 amendment at both claim sites (:30, :36) + implementation-guide.adoc:1054, per D4.

  2. CHANGELOG.adoc: issue-scoped entry (match the existing heading shape) disclosing the HTTP 503 AND the MQ effect in D3’s honest form. (NUL byte: separate chore issue — not this MR.)

  3. configuration-reference.adoc: the env-var row (name, default 5000, range, D8 wording). Page-wide audit → Step 6.

  4. Deployment guide + .env.example + devstack compose passthrough (CRAIG_RULESEVAL_TIMEOUT_MS: ${CRAIG_RULESEVAL_TIMEOUT_MS:-5000}, per the existing optional-var pattern).

  5. testing-reference.adoc: paused-time/test-util technique note + the counter-seam oracle pattern.

  6. .claude/CLAUDE.md: explicitly N/A (no status-table surface for a weight-3 fix) — recorded so the checklist item is addressed, not skipped.

Step 6 — Follow-ups + open-tracker reconciliation

New issues (each to the full bar: what+why, testable AC, type+priority labels, weight, /relate #784):

  1. feat(rules): failure/attempt audit rows for errored evaluations (all classes) — from D4; note the craig-security wildcard-audit gap too.

  2. fix(common): ApiError emits application/json; architecture documents application/problem+json.

  3. docs: configuration-reference page audit (stale counts/defaults, naming).

  4. fix(cases): pass upstream rules 503/timeout through instead of folding into 500.

  5. chore: replace the raw NUL byte in CHANGELOG.adoc:1796 with a printable escape (load-bearing literal; docs-only).

  6. fix(mq): subscriber dead-letters on 2nd delivery — inbox 5-retry/backoff path unreachable, failed_at never stamped — /relate #778.

  7. fix(rules): case.intake_created handler reads case_id but the producer emits investigation_id/referral_id (context_id always None) — include the stale reporting/main.rs:409 comment + the eligibility.submitted dead-from-birth subscription; /relate #971 (which owns placement.requested — do NOT re-file that half).

Open-tracker reconciliation:

  1. #876 — comment + amend its AC to shipped reality (503 via ServiceUnavailable; inline tests); its coerced-Null audit half stays open.

  2. #783 — comment that AC 1 is satisfied by this MR, check the box, /relate #1046 for the recovery track.

  3. Bump #1046 to P2 with the severity note (caseworker-reachable arbitrary JSON, uncancellable eval, DLX has no replay).

Files Touched

File Change

services/craig-rules/src/config.rs (new)

typed parse fn + env wrapper + tests + proptests

services/craig-rules/src/main.rs

post-bootstrap read, propagate Err, thread budget, boot info!

services/craig-rules/src/engine.rs

variant, budget field, dispatch_eval, delegation, eval_loop_inner seam + guard, comments, engine tests

services/craig-rules/src/api.rs

extracted engine_error_to_api + destructured arm (warn! + 503) + utoipa truth-up + mapping test

services/craig-rules/Cargo.toml

dev-deps: tokio test-util + proptest

docs/modules/ROOT/pages/api/craig-rules.adoc

regenerated (--service craig-rules only)

docs/modules/ROOT/pages/adrs/adr-006-rules-engine.adoc (2 sites) + implementation-guide.adoc

audit-contract amendment

docs/modules/ROOT/pages/plans/rules-eval-timeout.adoc + nav.adoc

this Step 0; at completion: file → plans/archive/, row in plans/archive.adoc, Active nav line removed, Status Done (YYYY-MM-DD) — MR !N

configuration-reference.adoc, deployment guide, .env.example, devstack compose, testing-reference.adoc

Step 5

CHANGELOG.adoc

issue-scoped entry only (NUL untouched here)

Not touched: crates/craig-common/**, crates/craig-api/bootstrap.rs, shared-crates.adoc, architecture.adoc, services.adoc (thin index — no settings surface), .claude/CLAUDE.md (N/A per Step 5.6).

Verification

  1. cargo nextest run -p craig-rules — suite green; red-before-green for test 1 recorded.

  2. cargo clippy --workspace --all-targets --locked — -D warnings.

  3. Deterministic live check: devstack up on the branch build; craig-rules boot log shows the compose-provided budget; the regenerated api page contains the 503 row. (A 1 ms 503 probe is optional/probabilistic — the deterministic evidence is the tests + wiring checks.)

  4. Full pre-push battery (B3a/B4 locks stay LOCKED).

  5. Grep sweeps: no #784 deferral comments remain; both ADR-006 sites + implementation-guide amended; the api page’s response set matches the annotation.

Delivery

Commit order: Step 0 plan → red-before-green pair → remaining steps. Every commit runs the full pre-commit token gate AND the J1–J8 subagent pass — no carve-out applies. The fix commit is fix(rules): implement the evaluate() dispatch timeout (#784) (d626ec0c, Relates to #784 — trailing doc/true-up commits follow it); the Closes #784 rides the branch’s final commit and the MR description. Battery push → MR (template body; Relates to #1022; MQ disclosure) → merge per standing policy → close-out comment (impl+merge SHAs bare, files, checked criteria, the Step-6 filings) → &62 update → branch delete + prune → archive mechanics (prepared inside the MR once the IID is known) → Plan Completion Audit.

Edit this page · latest