Plan: Battery Isolation — Detection Standing-Alert Race + Contention Strata

On this page

Revision history. v1 was internally reviewed (10 findings folded) and externally REJECTED 2026-07-27 (~25 findings: 5 blockers, ~12 high-severity). This v2 folds every finding; the reviewer confirmed the valid v1 core, retained here: the partial unique index, explicit-arbiter ON CONFLICT, fetch_optional, and event/webhook work on the winning Some branch only. Rejected v1 designs are recorded in Rejected options.

Status

Unit Description Status

U0

GitLab choreography (#1172 filed/claimed/epic-attached/related; #1167 label → Plan::BATTERY-ISO; multi-MR exception documented) + this plan committed as the first commit on U1’s branch.

Done (2026-07-27) — #1172 id 195977730; plan+nav first commit on feature/1172-detection-standing-alert

U1

Detection race fix: lock-first migration, required-Uuid params, explicit-arbiter ON CONFLICT DO NOTHING, five test workstreams, mandatory docs. MR Closes #1172.

Done (2026-07-27) — migration 20260727100000 + fire_alert_for_rule arbiter branch; constraint test red-proven pre-migration, store-conflict test red-proven by ON-CONFLICT-removal mutation; the endpoint volley is probabilistic regression coverage (did not trip on one pre-migration run — determinism lives in the store-level pin, as designed)

U2

Devstack scheduler disable (CRAIG_SECURITY__DETECTION_INTERVAL_SECS: "0")
narrowed security-detection-scan nextest group. MR Relates to #1167.

Done (2026-07-27) — 8-member group (3 detection + volley + 4 alerts listing) verified via show-config test-groups --run-ignored=all on all three overridden profiles; D11 retire-when now preserves [test-groups]; testing-reference § nextest stale facts repaired; the volley oracle tightened ≤ 1 → exactly 1 (scheduler off + group serialization leave no competing actor)

U3

Template-clone 55006 serialization: measure truly-overlapping clones; contingent fix issue + protocol change only on confirmation. MR Relates to #1167.

Done (2026-07-27) — EXONERATED, no fix filed (D6’s parallelize branch): 3 runs × 8 barrier-overlapped clones of the 33-migration cases template = 0×55006, per-clone 108–135 ms, total ≤ 252 ms (grace-step laddering would read ~5 s steps). Structural confirmation: CountOtherDBBackends counts template CONNECTIONS — a concurrent CREATE DATABASE is not one, so clone-vs-clone 55006 cannot fire; the only window is ensure-vs-clone, already pinned by #1162’s hold test + CLONE_ATTEMPTS. Full tables on #1167; throwaway harness deleted (nothing shipped, so the status rides U4’s branch per the U0 pattern)

U4

Financial approve_adjustment owned-payment fixtures (test-only). MR Relates to #1167.

Done (2026-07-27) — owned_pending_payment raw-SQL fixture (no payment-create API exists) with drop-teardown; both concurrent tests + all six adjustments.rs fixture users own their rows; the two-instant-oracle caveat comment and the residual-collision salts deleted (moot on fresh owned rows)

U5

E2E tail ordering (auth-cookies dependencies incl. conditional screenshots)
durable config-guard regression test. MR Relates to #1167.

Done (2026-07-27) — authCookiesDependencies(screenshots) exported from the config (7 craig-web-visiting projects + conditional screenshots) and wired; config-coverage pins the resolved wiring, name validity, and BOTH env shapes; intake-*/config-guard deliberately outside the set (never touch craig-web); ordering observed in the full e2e

U6

Paired measurement protocol + close-out: results recorded here, Status → Done, archive move per ADR-030. The FINAL MR, Closes #1167.

Done (2026-07-27) — 6 samples (B,A ×3 alternating), 0 failures both cohorts, 0 strict-dominance regressions over 4694 common tests, Σ median deltas −46.6 s, warm battery wall ≈18% faster; acceptance PASS with the two single-sample outlier windows annotated (see Measurement results (U6))

Epic: &73
Issues: #1167 (program), #1172 (detection race; Closes target of U1), #1171 (related, not absorbed), a contingent template-clone fix issue filed only if U3’s measurement confirms
Branch: feature/1172-detection-standing-alert (U1); one branch per unit thereafter

MR-per-issue exception (per gitlab-issue-mr-standards): #1167 is a measurement program whose units are independently shippable and independently green; U2–U5 each ship Relates to #1167 and U6 closes it after the acceptance measurement — recorded on #1167 itself.

Context

The epic &73 perf audit (#1165 evidence trail) surfaced a standing flake class in the battery: services/craig-security detection tests intermittently observed duplicate unacknowledged alerts for one rule — a direct violation of the #1156 sustained-breach invariant ("one standing alert per rule; acknowledging re-arms"). Root cause: run_detection_scan is check-then-insert (store/alerts.rs EXISTS probe, then a plain INSERT), every scan is global over all enabled rules, and the devstack service runs a live 60 s scheduler (detection.rs DEFAULT_INTERVAL_SECS) — so any two overlapping scans (test-invoked or scheduled) can both pass the EXISTS check and both insert. #1172 tracks the fix; #1167 tracks the program: after the correctness fix, measure the battery’s remaining contention strata (template-clone DDL serialization, event fan-out volume, financial fixture sharing, e2e tail ordering) and remediate only what the data confirms.

The external stop-ship review of v1 reshaped the rollout (migration locking and timeout posture, deploy posture, measurement obligations) — its dispositions are inline in Decisions.

Decisions

# Fork Choice Why / review disposition

D1

Migration locking

LOCK TABLE security_alerts IN SHARE MODE acquired BEFORE the dedupe UPDATE (accepting a brief write outage), plus SET LOCAL statement_timeout = 0 and SET LOCAL lock_timeout = '10s' at the top.

Blocker 1: UPDATE-then-index upgrades locks and deadlocks against live acknowledgements (the ack path holds RowExclusive). Lock-first is deterministic. Blocker 2: migrations inherit the pool’s 30 s statement timeout on upgrades, but a prior migration’s session-scoped SET statement_timeout = 0 (20260726090000_archive_engine.sql) leaks on clean installs — SET LOCAL makes both paths identical; lock_timeout bounds the outage wait (boot fails loud, operator retries). Tested on BOTH paths (clean install + upgrade over seeded data).

D2

Deploy posture

Single-replica stop-the-world is the SUPPORTED path pre-1.0. The rolling-deploy hazard is RECORDED as unsupported: an old replica’s bare INSERT gets 23505, which aborts its whole scan (500 + later rules skipped). A future rolling deploy requires a compat phase (bare ON CONFLICT DO NOTHING, which needs no arbiter index, shipped fleet-wide BEFORE the index migration) — documented in the migration header, NOT built now.

Blocker 3. The v1 "worst case one crashed boot" claim was wrong and is retracted; devstack does not auto-restart on migration failure.

D3

Scheduler in devstack

DISABLE: CRAIG_SECURITY__DETECTION_INTERVAL_SECS: "0" in docker-compose.yml (0 is the documented disable sentinel in detection.rs).

v1’s retention rationale rested on a false premise (the sustained-breach test manually invokes every transition; its comment merely tolerates scheduler wins). Serialization can’t isolate the scheduler; the index fixes correctness but not the contention/latency of overlapping global scans. A production scan lease is NOT built here (recorded on #1172 as residual for a future multi-replica deployment).

D4

Detection nextest group

KEEP, but narrowed to the state-mutating tests only (post-cleanup-fix), charter-commented as tertiary behind the index (U1) and the scheduler disable (U2).

External finding: the v1 filter serialized 17 tests including pure input-validation/authz/EXPLAIN tests that mutate nothing.

D5

Exchange group

DROPPED.

External finding: the pair is designed cross-test-safe (each row invalid on one axis; bundle_orchestrator.rs documents it); "internally sequenced" referred to each test’s own halves. No failure evidence.

D6

Template-clone protocol (#1162)

MEASURE before trusting: truly-overlapping CREATE DATABASE … TEMPLATE clones may serialize in ~5 s CountOtherDBBackends steps (the 55006 grace) — exactly the sub-second→5–10 s texture the audit reported. If confirmed → file the fix issue
protocol fix (candidate: per-template advisory-lock queueing so waiters park on a lock instead of burning 5 s grace cycles; design finalized on the measurement).

Blocker 4: v1 dismissed the DDL class without measuring the replacement protocol.

D7

Event-heavy isolation

NOT rejected — measured in U6 as its own stratum (the #-wildcard security subscriber processes every sibling event; dead_letter budgets 30 s for a reason). Grouping decision follows the data.

External finding: #1169 fixed correlation, not volume.

D8

Financial adjustment race

Owned-payment fixtures for approve_adjustment_concurrent + the adjustments.rs helper (each test creates its own pending payment; no shared-row selection).

External finding: shared first-pending-payment row; the test’s own comment admits a mid-flight external approval breaks its oracle. Test-only; rides as U4.

D9

Migration auto-ack vs event contract

Maintenance repairs are EXEMPT from security.alert.acknowledged events — decided and documented (migration header + CHANGELOG): the attributed actor string is the audit record; staging events from a migration would fabricate operator actions.

External finding demanded an explicit decision.

D10

CreateAlertParams.rule_id

Becomes required Uuid (detection always passes Some today; no other caller). The invariant claim narrows to rule-scoped alerts; legacy NULL-rule rows stay unconstrained (partial-index NULLs are distinct).

External finding: Option invited future dedup bypass.

D11

sync-override

Refresh BOTH the reason AND retire-when for cfg-nextest: retirement must preserve the project’s [test-groups]/overrides.

External finding: the old retire-when could silently erase the groups.

D12

U2 membership check

nextest show-config test-groups run for ALL THREE profiles WITH the battery’s real selection posture (--run-ignored=all); assert exact expected counts post-narrowing (ignored tests included — the v1 command omitted them).

External finding.

D13

ci profile rationale

Corrected: ci runs --lib --bins (validate.rs), not "unit-only"; the detection members simply never execute there because they’re #[ignore]`d without `--run-ignored.

External finding.

Design

U1 — detection race fix (MR 1, Closes #1172)

Migration services/craig-security/migrations/<ts>_standing_alert_unique.sql

-- #1172: one standing (unacked) alert per rule — #1156's contract,
-- DB-enforced. LOCK FIRST (deadlock-free vs live acknowledgements: the
-- ack path holds RowExclusive; upgrading after row updates deadlocks),
-- accepting a bounded write outage. SET LOCAL makes the timeout posture
-- identical on clean installs (where an earlier migration's
-- session-scoped SET statement_timeout = 0 leaks) and upgrades (pool
-- default 30 s). Deploy posture: single-replica stop-the-world
-- (pre-1.0). Rolling deploys are UNSUPPORTED until a compat phase ships
-- (bare ON CONFLICT DO NOTHING needs no arbiter index and would go
-- fleet-wide first); an old replica racing this migration gets 23505 =
-- its scan 500s. Maintenance auto-acks are EXEMPT from
-- alert.acknowledged events (D9): the actor string is the audit record.
SET LOCAL statement_timeout = 0;
SET LOCAL lock_timeout = '10s';
LOCK TABLE security_alerts IN SHARE MODE;

UPDATE security_alerts a
   SET acknowledged = true,
       acknowledged_by = 'migration:standing-alert-dedupe (#1172)',
       acknowledged_at = now()
 WHERE NOT a.acknowledged AND a.rule_id IS NOT NULL
   AND EXISTS (SELECT 1 FROM security_alerts b
                WHERE b.rule_id = a.rule_id AND NOT b.acknowledged
                  AND (b.created_at, b.id) < (a.created_at, a.id));

CREATE UNIQUE INDEX uq_security_alerts_standing_rule
    ON security_alerts (rule_id) WHERE NOT acknowledged;

Grown-table posture (recorded): the dedupe self-join has no rule_id index behind it and the plain unique build scans the heap under the table lock — acceptable at current cardinality (devstack: dozens of rows; no deployed instance is large). A preflight SELECT count(*) figure goes in the MR description. -- no-transaction + staged CONCURRENTLY construction is the acknowledged alternative if a grown table ever appears.

Code

  • store/alerts.rs::create_alert — params take rule_id: Uuid (D10); ON CONFLICT (rule_id) WHERE NOT acknowledged DO NOTHING (explicit arbiter)
    fetch_optional → Option<SecurityAlert>; the executor generic stays.

  • detection.rs::run_detection_scan — the per-rule breach branch moves to a pub(crate) helper (fire_alert_for_rule) that owns the transaction: create_alert returning None → tx.rollback() + Ok(None) (nothing staged); Some → stage security.alert.created + commit. The scan continue`s on `None with a debug log; webhook dispatch and the response push happen on Some only. The "racy by design" comment is rewritten (false post-index).

  • Endpoint name in all docs/tests: POST /v1/security/detection/run.

Tests (five workstreams; devstack-bound tests #[ignore = "requires devstack"])

  1. Constraint tests/constraints/standing_alert_partial_unique.rs: raw-INSERT a fresh detection_rules row (FK; unique name, enabled = false, threshold 999999, valid CHECK values), two same-rule unacked INSERTs → 23505 naming the index; ack the first → re-insert succeeds (re-arm). Cleanup runs before the asserts (alerts then rule) so a failing assert doesn’t leak rows. Deterministically red pre-migration. @axis: sad.

  2. Backfill tests/constraints/standing_alert_dedupe_backfill.rs: clone the security template (craig_test_lib::template_db), DROP the new index to recreate the pre-head shape, seed one rule + three unacked alerts with distinct (created_at, id) including a timestamp tie, plus one NULL-rule unacked alert; run the migration file’s statements verbatim in one transaction → oldest survives unacked, later dupes acked with the attribution actor + timestamps set, tie broken by id, NULL-rule row untouched. @axis: sad.

  3. Store-level deterministic conflict — an in-crate devstack-bound unit test (the helper is pub(crate); the battery runs lib targets with --run-ignored=all): N=4 concurrent fire_alert_for_rule calls against one rule on a template clone via concurrent_fire_with_barrier — exactly one Some, all losers None, and exactly ONE staged security.alert.created outbox row for the rule (losing branches must not stage events). Deterministically exercises fetch_optional/None/rollback — the endpoint volley can’t. @axis: conc.

  4. Endpoint volley tests/api/detection_scan_concurrent.rs: 8 × POST /v1/security/detection/run via concurrent_fire_synchronized, fresh threshold-0 ENABLED rule; all 200; per response, the alerts array filtered by this rule_name (NOT the global alerts_created counter — it counts unrelated rules) sums to ≤ 1 across the volley; exactly one standing unacked alert in the DB by rule_name. Division of labor: with the U2 scheduler disable this volley becomes deterministic about actors; workstream 3 remains the deterministic branch pin. Rule cleanup at the end. @axis: conc.

  5. Hygiene, existing tests: (a) acknowledge_alert adopts the query-by-own-rule pattern (the scan response may not contain the alert if another actor created it); (b) panic-safe teardown for the enabled-rule fixtures in tests/api/detection.rs + tests/api/alerts.rs helpers via TestHarness::register_cleanup (the #1164 drop-teardown — delete the test’s alerts then its rule), ending the ~9-enabled-rules-per-battery leak that makes every subsequent scan slower.

Docs (mandatory)

data-model-security.adoc — add the two detection tables + the FK + the new index (the page currently claims seven tables and no FKs). state-machines.adoc — the one-standing-alert/re-arm invariant. CHANGELOG Fixed (incl. the D9 exemption
attributed auto-ack). #1172 closed with its comment right after THIS merge.

U2 — scheduler off + narrowed group (MR 2, Relates to #1167)

  • docker-compose.yml craig-security env: CRAIG_SECURITY__DETECTION_INTERVAL_SECS: "0" with a comment (D3; production default unchanged).

  • .config/nextest.toml: security-detection-scan = { max-threads = 1 } with the tertiary-defense charter comment; overrides on default/integration/ci-integration filtering the STATE-MUTATING tests only — exact list finalized post-U1 (scan-invoking + ack-invoking + the two new concurrency tests), package(craig-security) and kind(test) and (…). NO exchange group (D5).

  • Membership check per D12; sync-override reason + retire-when refresh (D11); testing-reference.adoc § nextest updated AND its stale junit/timeout/thread facts repaired. CHANGELOG.

U3 — template-clone measurement (MR 3, Relates to #1167)

Instrumented measurement (throwaway harness, not shipped): N=8 truly-overlapping clones of ONE pre-built template (barrier-synchronized across OS threads, template sized ≈ the cases one) — record per-clone wall + whether 55006 retries fired. If overlapping clones serialize in ~5 s grace steps → file the fix issue + implement the protocol fix (leading candidate: a per-template pg advisory lock around the clone), red-proof = the measurement rerun. If clones genuinely parallelize, record the exonerating measurement on #1167 and close the question. Either way the result is RECORDED (Blocker 4).

U4 — financial owned-payment fixtures (MR 4, Relates to #1167)

approve_adjustment_concurrent.rs + adjustments.rs helper: each test creates its OWN pending payment (existing builders/endpoints) instead of selecting the shared first-pending row; delete the two-instant-oracle caveat comment once the fixture is owned. @axis tags unchanged. No product code.

U5 — e2e tail ordering (MR 5, Relates to #1167)

  • auth-cookies.dependencies: the seven prior projects PLUS conditionally screenshots when SCREENSHOTS=1 (config already builds that project conditionally) — mirror the conditional in the dependency list. Both trades in the comment (skip semantics; --project auth-cookies now pulls dependencies).

  • Durable regression test: extend the config-introspection guard (tests/e2e/specs/config-coverage.spec.ts) to assert auth-cookies' resolved dependency set under BOTH default and SCREENSHOTS=1.

  • Verification: auth-cookies executes last in a full cargo xtask e2e. CHANGELOG.

U6 — measurement + close-out (MR 6, Closes #1167)

This unit IS the disposition of review Blocker 5 (#1167 must not close before its acceptance measurement): the close-out has a real delivery vehicle, and closure follows the measured acceptance criterion rather than preceding it.

  • State control: full devstack reseed before EVERY sample; template pre-warmed once before each cohort’s first sample; token cache deleted before each cohort.

  • Design: paired alternating order (B,A,B,A,B,A — before = pre-U2-merge worktree build, after = HEAD) on an idle host, ≥3 pairs; every junit retained under a unique name.

  • Statistics declared up front: per-test p95 + max deltas and a slow-test table (every test whose max shifted > 2 s), plus suite wall — NOT wall alone. Acceptance: "no test’s tail regressed > 2 s and no new failures", not a wall-time delta smaller than run-to-run spread.

  • Strata: (a) idle baseline; (b) the event-heavy question (D7) via per-test p95 of the dead_letter/security event suites; (c) any U3 cross-check.

  • Any mid-protocol red: root-caused per testing-discipline; contaminated samples annotated, never silently dropped.

  • Close-out IN THIS MR: this plan’s Status → Done, archive move + index/xref repair per ADR-030, measurement tables recorded here; #1167 closing comment; epic &73 tick. CLAUDE.md status only when the epic itself closes.

Measurement results (U6)

Executed 2026-07-27 on an idle host per the protocol. Cohorts: B = the pre-U2-merge worktree @ 863a3a0d (U1 merged; scheduler-on devstack, shared fixtures); A = HEAD (U1+U2+U4 merged; the U5 branch’s e2e-only diff was present in the tree and is nextest-inert by construction — recorded as the protocol’s one deviation). Per sample: dev reload from the cohort tree → dev reseed (full volume wipe) → token-cache purge → the battery’s exact nextest invocation → junit retained (test-results/u6-measure/{B,A}{1..3}.xml). Samples alternated B,A,B,A,B,A.

Headline

Metric Cohort B Cohort A

Failures across 3 samples

0

0

Warm battery wall (samples 2+3; sample 1 pairs include compile)

58.5 s / 63.7 s

50.3 s / 50.0 s (paired −14.0% / −21.5%; combined ≈18% faster)

Strict-dominance regressions (min(A) > max(B) by > 0.5 s), 4694 common tests

—

0

Strict-dominance improvements (max(A) < min(B) by > 0.5 s)

—

0

Σ per-test MEDIAN deltas (A − B)

—

−46.6 s

Tests with median improved / regressed > 0.2 s

—

49 / 17

New/removed tests between cohorts: 0/0 (U1’s tests are in both; U4 changed fixtures, not test names).

The face-value > 2 s tail table, annotated

Nine tests exceeded the declared "max shifted > 2 s" bar (worst: authz_audit_emission +5.9 s; the six cases boot_verify tests +3.4–3.5 s; retention_archive::ledger_row_and_event_are_atomic +2.7 s; the U1 in-crate conflict test +2.5 s). Per the annotate-don’t-drop rule, all nine are NON-regressions:

  • Every one has OVERLAPPING per-sample ranges across cohorts (hence zero strict-dominance regressions — a robustness instrument computed POST HOC and disclosed as such; the declared statistics were p95/max + this table) — the exceedances come from exactly two windows: sample A1 (first HEAD sample; boot_verify family at 9.9 s in A1 vs 0.9–4.2 s in A2/A3, against B samples of 2.7–6.5 s) and one A3 co-tenancy spike (the conflict
    rules-archive pair at ~4 s in A3 only, ~1.2 s elsewhere in BOTH cohorts).

  • No mechanism: U2 (compose env + nextest groups) and U4 (financial test fixtures) touch neither craig-bootstrap nor the cases boot-verify paths.

Twenty tests improved > 2 s at max, CONSISTENTLY: the whole craig-mq family (−4.4 to −10.7 s — outbox/inbox/retention), the craig-cli suite (−2.7 s each), the intake evil corpus (33.5 → 26.5 s), persons_search, financial retention. Mechanism: the U2 scheduler disable removed a 60 s global scan taxing the shared Postgres under every battery.

D7 — the event-heavy stratum decision

No volume-driven signal: dlq_threshold_exceeded_alert_fires p95 16.25 → 15.73 s (its tail is the test’s own 30 s-budget design), financial retention improved 2.5 s, the archive-engine bookkeeping pair drifted ~+1 s inside overlapping ranges. Decision: NO event-heavy serialization group — the data does not support one; the question closes with this measurement.

U3 cross-check

template_db::clone_waits_out_a_held_template_connection held ~7.8 s in BOTH cohorts (its designed 7 s hold); no clone-serialization texture anywhere in either cohort — consistent with U3’s exoneration.

Rejected options

  • v1 D1 — trust the #1162 template-clone protocol without measurement: rejected by the external review (Blocker 4); the 55006 grace-step serialization mechanism matches the audit’s latency texture and must be measured (U3).

  • v1 D4 — keep the devstack scheduler because the sustained-breach test needs it: the premise was false (the test drives every transition manually); replaced by the U2 disable.

  • v1 exchange nextest group: serialized tests explicitly designed to overlap safely; dropped (D5).

  • UPDATE-then-CREATE-INDEX migration without an explicit lock: deadlocks against live acknowledgements; replaced by lock-first (D1).

  • alerts_created global counter as the volley oracle: counts unrelated rules' alerts; replaced by per-response filtering on the test’s own rule_name.

  • Rolling-deploy compat phase built now: pre-1.0 single-replica posture makes it dead code today; recorded as the recipe instead (D2).

  • Auto-ack security.alert.acknowledged events from the migration: would fabricate operator actions; the attributed actor string is the audit record (D9).

Risks

Risk Mitigation

Migration write-outage window (SHARE lock)

lock_timeout 10 s bounds the wait; boot fails loud; single-replica posture (D2); preflight cardinality in the MR description.

Old-replica 23505 on an unsupported rolling deploy

D2 documented posture + compat-phase recipe in the migration header.

U3 measurement inconclusive

Both outcomes are recorded results; the fix issue only files on confirmation.

Narrowed group filter drifts

Charter comment; the D12 membership check is re-runnable.

Measurement still noisy

Paired design + per-test tails + declared thresholds; annotate-don’t-drop.

Scheduler-off devstack hides scheduler bugs

The sustained-breach test drives scans manually (its design); production default untouched; recorded on #1172 as residual for a future scan lease if multi-replica contention materializes.

Verification

  1. Per unit: cargo fmt --all + cargo clippy --workspace --all-targets --locked — -D warnings + targeted cargo nextest run (with devstack up for `#[ignore]`d tests) + the red-case proof named in each unit.

  2. The pre-push battery (full cargo xtask validate + e2e) per unit.

  3. U6’s paired measurement protocol is the program-level acceptance gate.

Documentation Updates

  • data-model-security.adoc — detection tables + FK + uq_security_alerts_standing_rule (U1)

  • state-machines.adoc — one-standing-alert/re-arm invariant (U1)

  • testing-reference.adoc § nextest — groups + stale-fact repair (U2)

  • .claude/sync-overrides.toml — cfg-nextest reason + retire-when (U2)

  • CHANGELOG.adoc — per-unit entries under == Unreleased

  • Archive move + measurement tables in this plan (U6)

Edit this page · latest