P0: Replace or remove flaky SQLite WAL performanceAssertionTest — microbench premise unsound on CI

[totalslacker]

Summary

SQLiteStorageConcurrencyTests.performanceAssertionTest() was disabled on CI runners via @Test(.disabled(if: CI)) because its assertion (tBoth < tRead + tWrite × 0.7) regularly fails at 4–7× the threshold on GitHub Actions, even though WAL concurrency is working correctly. The CI skip is a policy violation. This issue resolves it: either the test is rewritten so it passes reliably everywhere, or it is deleted with an ADR. The skip annotation must be gone either way.

Requirements

• The .disabled(if: CI) annotation in SQLiteStorageConcurrencyTests.swift must be removed. "Skip on CI" is not an acceptable resolution.
• If the test is rewritten: it must pass ≥20/20 consecutive runs on CI in both debug and release configuration, with no flakiness.
• If the test is deleted: an ADR must document why the original assertion was unmeasurable on macOS GitHub Actions runners, and must explain how livenessTest() and safariUnfuckerRegressionTest() together cover the actor-serialization regression the deleted test was meant to catch.
• No new tests that fail locally or intermittently may be introduced anywhere in the suite.
• No new skip annotations of any kind (@Test(.disabled(...)), XCTSkipIf(...), XCTSkipUnless(...), #if !CI, etc.) may be added to any test that previously ran.
• The assertion may not be "fixed" by deleting it, weakening it to a tautology, or making the test body a no-op.
• Any threshold or budget value in a rewritten test must be supported by measurement evidence (median of ≥11 runs on a quiet machine) recorded in a code comment beside the new value.
• CI must be green on both swift test (debug) and swift test -c release jobs on the fix branch.
• The Validate stage must verify: (a) the target test passes ≥10 consecutive runs; (b) no other test newly fails relative to main; (c) CI is green on both jobs.
• If a candidate fix makes the target test pass only by introducing fragility elsewhere, implementation must stop and escalate to a human comment rather than merging.

Scope

In scope:

• performanceAssertionTest() in SQLiteStorageConcurrencyTests.swift — rewrite or deletion
• ADR documentation (required only if the test is deleted)

Out of scope:

• livenessTest() (sibling test, separate issue)
• Changes to the SQLite storage actor split itself
• Changes to any other concurrency test not named above

Prior Art / Context

• The test asserts tBoth < tRead + tWrite × 0.7 using async let + actor-hopping + SQLite I/O, median of 3 iterations (100 writes + one FTS read each).
• At that scale, runner contention and scheduler jitter dominate — the concurrency speedup signal is below the noise floor on shared GitHub Actions runners.
• Skip commit 20c55c25 (2026-05-06) acknowledged this directly: "the small-N async-let + actor + SQLite-IO pattern measures runner contention more than concurrency speedup."
• Candidate approaches for Research to evaluate: larger N (10k+ writes, longer FTS scan), coarser threshold (e.g. tBoth < tRead + tWrite without the 0.7 factor), larger sample count (e.g. median-of-11), or a proper benchmark harness with warm-up and outlier rejection.
• livenessTest() and safariUnfuckerRegressionTest() are the two coverage fallbacks if deletion is chosen.

Risks / Dependencies

• If no rewrite produces a stable signal on macOS GitHub Actions runners, deletion is mandatory — there is no "keep but skip" fallback.
• Threshold inflation without evidence is explicitly prohibited; Research must produce real timing data to justify any new assertion value.
• Any fix that passes the target test by degrading another test is treated as equivalent to no fix.

[totalslacker]

Hard constraint for the agent picking up this issue

Do not introduce any new failing tests, anywhere, for any reason.

This includes:

• ❌ No new tests that fail locally on a clean checkout (debug or release).
• ❌ No new tests that fail intermittently — "flaky" tests count as failing tests under the project policy in .claude/CLAUDE.md.
• ❌ No new .disabled(if: ...), XCTSkipIf(...), XCTSkipUnless(...), #if !CI, @Test(.disabled(...)), or any other skip annotation on tests that previously ran.
• ❌ No "fixing" the existing failure by deleting the assertion, weakening it to a tautology, or making the test a no-op.
• ❌ No raising of a budget/threshold without measurement evidence (e.g. median of 11+ runs on a quiet machine) recorded in a comment next to the new value.

The Validate stage on this issue must verify with a fresh swift test (debug) and swift test -c release that:

1. The test this issue targets passes ≥10 consecutive runs.
2. No other test newly fails — i.e. the full suite is at least as healthy as main was before this PR, with one fewer failing/skipped test (the one you just fixed).
3. CI on the PR is green on both jobs (swift test (macOS) and swift test -c release (macOS)).

If you find a way to make this issue's target test pass only by introducing fragility somewhere else, that's not a fix — it's moving the bug. Stop and escalate to a human comment instead of merging.

(See also: issue #93 thread comment about why .disabled(if: CI) and "skip flaky on CI" are not acceptable resolutions on this project.)

[totalslacker]

🏭 Fabrik — stage: Specify
branch: fabrik/issue-96 | commit: d814646 | main: d814646 | 2026-05-07 21:14 UTC

The stage-Specify.md file doesn't exist yet — this is the first comment on this issue. I'll process the comment by incorporating the hard constraints into the spec and restructuring the issue body to the standard Specify format.

The comment adds concrete implementation constraints and Validate-stage verification requirements. There are no open questions in the spec — the requirements are clear, and the Research stage will investigate which path (rewrite vs. delete) is viable.

[totalslacker]

🏭 Fabrik — stage: Research
branch: fabrik/issue-96 | commit: e83d644 | main: e83d644 | 2026-05-07 21:27 UTC

I now have all the data needed. Let me write the research findings.

---

Research Findings

Relevant Code

• Tests/SwitchcraftTests/SQLiteStorageConcurrencyTests.swift: The file containing all three tests. No .serialized trait on the suite → all three tests run concurrently by default. performanceAssertionTest is at lines 143–182 (the target test). safariUnfuckerRegressionTest is at lines 192–240 (supposed CI coverage fallback). livenessTest is at lines 80–116 (also CI-disabled, out of scope).

• Sources/SwitchcraftSQLite/SQLiteStorage.swift: The SQLiteStorage actor. File-backed mode delegates reads to SQLiteReaderActor and writes to SQLiteWriterActor. In-memory mode uses a single connection. The actor-hop overhead measured in ADR 019 is ~14–15µs per hop (two hops per write call, ~29µs total).

• Sources/SwitchcraftSQLite/SQLiteWriterActor.swift + SQLiteReaderActor.swift: Internal actor pair. SQLiteReaderActor owns a SQLITE_OPEN_READONLY connection; SQLiteWriterActor owns a READWRITE WAL connection. These are the actors whose concurrency the target test was meant to validate.

• adrs/019-sqlite-writer-reader-split.md: Documents the writer/reader split architecture. Notes the per-call overhead (~18% sequential regression) and the user-visible stall the split resolves.

• adrs/012-performance-regression-thresholds.md: Documents the pattern for dealing with CI runner flakiness. Uses .enabled(if: isReleaseBuild) gating and explicitly avoids debug-mode timing on shared runners. Notes: "Release-mode gating: .enabled(if: isReleaseBuild, …) — eliminates ~3 minutes of noisy debug-mode runtime per CI run and removes the false-positive failure mode."

• .github/workflows/ci.yml: CI has two jobs — swift test (debug, no env var gating) and swift test -c release. Both run on macos-15 runners (~7 GB RAM, 3 cores). The CI environment variable is set by GitHub Actions automatically.

Architecture Notes

SQLiteStorage is an actor that dispatches to two child actors for file-backed databases: SQLiteWriterActor (READWRITE + WAL) and SQLiteReaderActor (READONLY). Every external call enters the SQLiteStorage actor, then hops to the appropriate child actor, then hops back — two actor hops per call, ~29µs overhead per write call.

performanceAssertionTest measures:

1. Isolated read time tRead (median of 3 iterations of searchFullText over 5,000 docs)
2. Isolated write time tWrite (median of 3 iterations of 100 sequential upsertDocument calls)
3. Concurrent time tBoth (median of 3 iterations where async let launches the FTS read, then 100 sequential writes are issued, then both results are awaited)
4. Asserts tBoth < tRead + tWrite × 0.7

Relevant ADRs

• ADR 019 (019-sqlite-writer-reader-split.md): The actor split this test was meant to validate. Documents the ~18% sequential write regression from actor-hopping overhead (~29µs/write) and states the tradeoff is acceptable given the benefit of eliminating search-induced stalls. Directly relevant to understanding why tBoth includes actor-hop overhead that adds non-trivial overhead in the concurrent measurement path.

• ADR 012 (012-performance-regression-thresholds.md): Establishes the project pattern for CI-stable performance tests. Key precedent: the performance suite is gated to release mode (.enabled(if: isReleaseBuild)) specifically because debug-mode timing on shared runners is "too noisy to defend a meaningful floor." Also establishes the policy: "increase iterations before loosening" and requires 20-run CI history before tightening. Directly relevant because performanceAssertionTest uses 3 iterations (too low) and has no warm-up.

Constraints

Root Cause 1 — Suite serialization absent: The @Suite("SQLiteStorage WAL Concurrency") declaration has no .serialized trait. Swift Testing runs all three tests in the suite concurrently by default. All three are I/O-heavy (each builds a 5,000-document SQLite database). This causes filesystem-level contention (APFS I/O, WAL journal flushes) that inflates tBoth. Local measurements confirm: when performanceAssertionTest runs in isolation, it passes reliably (8/8 observed passes). When run with its two siblings, it fails at ~50% rate (4/8 observed failures) with tBoth varying from 0.064–0.091s (nominal threshold ~0.068s at typical timing).

Root Cause 2 — Swift cooperative scheduler on 3-core CI runners: The async let + sequential writes pattern doesn't guarantee true parallelism. On 3-core macOS runners under full test-suite load, the cooperative thread pool may serialize the read task and write tasks rather than running them on separate threads. When serialized: tBoth ≈ tRead + tWrite + scheduling_overhead, which exceeds tRead + tWrite × 0.7 trivially. This is why CI sees tBoth at 4–7× the threshold even when livenessTest (which only needs Date() resolution) passes on the same runner.

Root Cause 3 — 3-iteration median is noise-sensitive: With median of 3, the middle value is the 2nd of 3 sorted samples. A single outlier (one iteration where the scheduler de-prioritized the test task) shifts the median significantly. ADR 012 documents that 50 iterations are used for p50 stability in the performance suite; 3 iterations is far below the stability threshold.

safariUnfuckerRegressionTest also has observed local flakiness: In 1 of 8 full-suite runs locally, this test also failed (tConcurrentWrites → 0.044s < ceiling → 0.0079s). The failure mode: measureMedian(iterations: 1) produces a single isolated baseline measurement (tIsolated ≈ 5ms for 50 writes, when the true median is ~11ms). This single-sample baseline can be anomalously low under concurrent I/O from sibling tests, making the 1.5× ceiling impossibly tight. This is worth noting because the deletion ADR would cite this test as the primary CI coverage — a test with its own single-sample reliability risk.

No .serialized precedent in this suite vs. comparison suite: PerformanceTests.swift uses @Suite(.serialized) explicitly, per ADR 012. The SQLite concurrency suite does not, which was an oversight given that all three tests create separate temp databases and run simultaneously.

Clock used: Date() (wall clock), not ContinuousClock. Date() is susceptible to NTP adjustments and has lower resolution than ContinuousClock. ADR 012 uses ContinuousClock explicitly for the performance suite, noting "Not affected by wall-clock adjustments." The concurrency tests use Date() — any NTP tick during a measurement inflates or deflates the result.

Technical Questions

• Which path does the owner prefer: rewrite or delete? The spec allows either; research shows both are technically feasible but with different risk profiles. Deletion + ADR is lower-risk for CI stability; rewrite requires addressing both root causes (suite serialization + CI parallelism guarantee) and may still fail on CI if the cooperative scheduler serializes the concurrent tasks.

• Does safariUnfuckerRegressionTest require a reliability fix before deletion is viable? The test uses measureMedian(iterations: 1) for its isolated baseline, which produced one local failure in 8 runs. If the ADR relies on this test as the sole CI gate, the iterations: 1 baseline is a latent reliability risk. Fixing it to iterations: 3 (a small, surgical change) would make the coverage argument in the deletion ADR more sound — but that's technically a modification to a test outside the stated scope.

• What is the ADR number for the deletion record? ADRs currently exist up to 022-embedder-overflow-guard.md; the next would be 023. Should it be 023-wal-microbench-deletion.md or similar? The README also needs updating when the ADR is added.

Risks

Rewrite risk — Cooperative scheduler doesn't guarantee parallelism on CI: Even with .serialized on the suite and 11 iterations, if the Swift cooperative thread pool doesn't assign the read and write tasks to separate threads (likely on a 3-core CI runner under full test load), tBoth will equal tRead + tWrite + overhead, not max(tRead, tWrite). No timing threshold will make the speedup assertion pass reliably in this case. This is the fundamental reason the CI skip was added.

Deletion risk — safariUnfuckerRegressionTest single-sample baseline: The deletion ADR's coverage argument relies on this test catching actor-serialization regressions. But its isolated baseline uses iterations: 1, meaning one anomalously fast measurement makes the ceiling impossibly tight. The Plan stage should evaluate whether to simultaneously fix this test (change to iterations: 3) as a prerequisite — or accept the risk and note it in the ADR.

Deletion risk — livenessTest also CI-disabled: The spec says the deletion ADR must explain how "livenessTest() and safariUnfuckerRegressionTest() together cover the regression." But livenessTest is also CI-disabled (separate issue #N). If we delete performanceAssertionTest, the CI coverage comes from safariUnfuckerRegressionTest alone, not the pair. The ADR should accurately describe the actual CI coverage state.

Rewrite risk — Measurement evidence requirement: The spec explicitly requires "median of ≥11 runs on a quiet machine" for any new threshold value. Satisfying this requires actually running 11 iterations on a dedicated, quiet machine and recording the timing. This cannot be done in the CI environment; it requires local evidence. The Implement stage would need to produce this evidence before committing any rewritten threshold.

Documentation Impact

No user-visible documentation impact. This change affects only:

• SQLiteStorageConcurrencyTests.swift (test file, internal)
• A new ADR document in adrs/ (required only if the test is deleted)
• adrs/README.md (index entry update, required if ADR is added)

No CLI commands, config options, or public APIs are affected.

---

Summary for the Plan stage: The test is locally flaky at ~50% failure rate when run alongside its two sibling tests (root cause: no .serialized on suite), and reliably fails on CI (root cause: 3-core cooperative scheduler serializes async let tasks, making the speedup premise unmeasurable). Deletion + ADR 023 is the lower-risk path and should be the primary recommendation. If rewrite is chosen, the plan must address: (1) add .serialized to suite, (2) switch clock to ContinuousClock, (3) raise to 11+ iterations, (4) change assertion from speedup to write-throughput-overhead (avoiding duplication with safariUnfuckerRegressionTest), and (5) produce 11 timing runs on a quiet machine before committing any threshold. Either path should also evaluate whether safariUnfuckerRegressionTest's iterations: 1 baseline is a latent P0 risk worth fixing in the same PR.

---

Used 27/50 turns, 0k input / 19k output tokens.