perf(routing): build the recorded decision compact, and stop health reads from probing breakers

[LMPrado-DZ23]

Two MEDIUM findings from the architecture audit of the final v3.8.54 tree, plus an eighth occurrence of the second finding folded in on review. One concern per commit, all red-first.

---

Finding 1 — MEDIUM, new in 3.8.54: routing-decision recording cost 12.5 ms and ~594 KiB on every live auto request

buildRoutingDecision materialised every candidate with its full factor breakdown, and only then did decisionStore.recordRoutingDecision compact the result down to 40 candidates / 10 factor breakdowns. MAX_STORED_CANDIDATES bounded what was retained, not what was allocated, and recording is ungated — it runs on every live auto request.

What changed. The live recording path (open-sse/services/combo/autoRoutingDecision.ts) now passes the decision store's own bounds to buildRoutingDecision as a new optional retention input, so the decision is built in the shape it is retained in. Factors are computed after ordering and selection, only for the candidates that keep them plus the selected one; nothing before that point reads factors, so the retained candidates and their order are identical to before. The build carries the same omittedCandidates count the store produces. The store still compacts, so a decision that arrives already compact passes through unchanged — the store remains the safety net.

Explainability is untouched. The decision id, the policy version and the selected candidate's full factor breakdown are recorded for every routed request exactly as before. The decision-id / policy-version headers are unaffected.

Preview is untouched. POST /api/omniroute/route/preview / previewRoutingDecision passes no retention and keeps returning the full, uncompacted candidate list with every candidate's factors. Covered by a test.

Before / after (real numbers)

Benchmark shape: build + record a decision over a pool of N candidates, with the engine selection done once up front and reused. ms/decision is the mean of 2000 iterations after a 300-iteration warm-up; bytes/decision is the heap retained by one built decision graph (mean of 200, --expose-gc, measured on both sides of a forced GC) — the live path discards all of it except what the store keeps, so that is the transient allocation the request pays.

candidates	before (ms)	after (ms)	before (bytes)	after (bytes)
300	17.037	4.602	605 451 (591.3 KiB)	27 120 (26.5 KiB)
50	3.087	1.229	101 634 (99.3 KiB)	27 258 (26.6 KiB)

Byte figures are deterministic across runs; the 300-candidate before-figure (591.3 KiB) matches the auditor's measured ~594 KiB. The remaining ~4.6 ms at 300 candidates is the unavoidable part: every candidate still has to be scored into a RoutingCandidate to be ordered and excluded correctly, plus the policy-version hash and the store's size estimate. The factor objects, which were the bulk of the allocation, drop from 4800 to 160.

Proof of equivalence

tests/unit/routing-decision-compact-build.test.ts builds a decision over the same pool both ways, records each, reads it back through the same accessor GET /api/omniroute/route/decisions/{id} uses, and asserts the serialised results are identical — for pools of 300, 50, 40 and 7 candidates. It also pins the retained ordering, the omittedCandidates count, the selected candidate keeping its factors, and preview staying uncompacted. On the previous code it fails (actual: 300, expected: 40).

---

Finding 2 — MEDIUM, pre-existing (identical in v3.8.53): read paths mutated circuit-breaker state

getAllCircuitBreakerStatuses() calls getStatus(), which transitions an OPEN breaker whose cooldown has elapsed to HALF_OPEN, persists that, and grants it a half-open probe. So merely reading health flipped a breaker and let the next request through to a provider that is still broken — and a dashboard that polls did it on every poll. PR #38 fixed /api/metrics and the SLO timer with the read-only getAllCircuitBreakerSnapshots() / peekStatus() API.

All remaining paths were true reads. All were converted — none legitimately needed the probing transition. The audit named seven; an eighth occurrence of the same bug was found by grep and folded in on review (row 8, its own commit dea0030c4).

#	path	file
1	GET /api/resilience/connections	src/app/api/resilience/connections/route.ts
2	provider health matrix	src/lib/monitoring/providerHealthMatrix.ts
3	provider health autopilot	src/lib/monitoring/providerHealthAutopilot.ts
4	omniroute status	src/lib/omnirouteStatus.ts
5	GET /api/monitoring/health	src/app/api/monitoring/health/route.ts
6	web-session pool health	open-sse/services/webSessionPoolHealth.ts
7	getProvidersInCooldown	open-sse/services/accountFallback.ts
8	A2A provider discovery (not in the original seven)	src/lib/a2a/skills/providerDiscovery.ts

Paths 6 and 7 also used the probing accessors on individual breakers (canExecute(), getRetryAfterMs(), getStatus()), so switching only the registry call would have left them mutating. New read-only API in src/shared/utils/circuitBreaker.ts, adapted at each call site rather than cast:

• CircuitBreaker.peekCanExecute() — canExecute() without the transition. An elapsed OPEN breaker reads as executable, because the next live call would grant it a probe, but nothing transitions, persists or consumes one.
• peekCircuitBreaker(name) — the registered breaker, or an unregistered copy of its persisted state. Never registers, evicts or transitions.
• getBlockedCircuitBreakerSnapshots() — snapshots of the breakers that would refuse a request right now, the read-only counterpart of filtering by !canExecute().

Path 8 builds a name-to-breaker map for the health field of each listed candidate. Its map is now typed with the real CircuitBreakerStatus instead of a callback annotated with the file's loose CircuitBreakerLike alias, which also drops the breaker.name || "" fallback — name is required on the snapshot type. CircuitBreakerLike stays as the parameter type of healthFromBreaker, which legitimately takes a possibly absent, partially known breaker. No cast was added anywhere.

Reported output is unchanged. The effective state of an elapsed breaker is still reported as HALF_OPEN — that is what PR #38 established and what dashboards must show. Reporting that state is not the same as causing it. (healthFromBreaker maps HALF_OPEN to "recovering" either way.) Two further side effects are gone: a read no longer registers persisted breakers into the in-process registry, and therefore no longer triggers cold-breaker eviction.

Left alone, deliberately

• DELETE /api/monitoring/health and POST /api/resilience/reset — resets, not reads. Enumerating breakers through the mutating call is how the reset reaches persisted breakers this process has not loaded.
• Live routing's isProviderInCooldown / checkFallbackError path — there the OPEN to HALF_OPEN transition is the point: it is how a recovered provider gets its next request.
• Nothing else. grep for getAllCircuitBreakerStatuses now returns only its own definition, the two reset endpoints above, and the doc comment on getAllCircuitBreakerSnapshots.

Regression test

tests/unit/health-reads-do-not-probe-breakers.test.ts opens a breaker, lets its cooldown elapse, then calls each of the eight converted read paths twice and asserts the breaker is still OPEN, recorded no transition, was granted no half-open probe, and has a byte-identical persisted row — while live routing afterwards still gets its probe. It also pins that the reads still report the breaker (effective state HALF_OPEN) and that a breaker still inside its cooldown is still reported in cooldown. Verified red on the previous code for each path individually (the discovery case fails with actual: 'HALF_OPEN', expected: 'OPEN'), and every case asserts the read really did something — the route answered 200, the report carried a breaker, discovery listed candidates — so no assertion can pass vacuously.

---

Verification (real output, run on this branch)

Typechecks — all four clean, no output (run with tsc directly; the scripts/check/check-*-typecheck.mjs wrappers fail on Windows with spawnSync npx.cmd EINVAL):

node node_modules/typescript/bin/tsc --pretty false --noEmit -p tsconfig.typecheck-core.json
node node_modules/typescript/bin/tsc --pretty false --noEmit -p tsconfig.typecheck-api.json
node node_modules/typescript/bin/tsc --pretty false --noEmit -p tsconfig.typecheck-dashboard.json
node node_modules/typescript/bin/tsc --pretty false --noEmit -p open-sse/tsconfig.json

ESLint — clean, no output:

npx eslint --max-warnings=0 --suppressions-location config/quality/eslint-suppressions.json \
  --pass-on-unpruned-suppressions --no-warn-ignored <changed files>

Tests — routing / auto-combo / decision-store suites:

tests 372
pass  372
fail  0

a2a / resilience / health / circuit-breaker / decision suites, after the eighth conversion:

tests 229
pass  229
fail  0

wider resilience / health / circuit-breaker sweep:

tests 259
pass  258
fail  1
FAIL tests/unit/provider-health-matrix.test.ts
  Error: EPERM, Permission denied: ...\Temp\omniroute-health-matrix-XXXXXX
      at Object.rmSync ... at tests/unit/provider-health-matrix.test.ts:58:6

All five of that file's tests pass; the failure is its after hook deleting its temp directory on Windows. Reproduced identically on origin/release/v3.8.54 with this branch's changes stashed — pre-existing environment flake, not from this PR.

Mutation test-coverage gate. tests/unit/health-reads-do-not-probe-breakers.test.ts imports two mutated modules (open-sse/services/accountFallback.ts, src/shared/utils/circuitBreaker.ts), so it is registered in stryker.conf.json tap.testFiles in sorted position (392 entries, still sorted), written with LF so the committed blob matches the base blob's line endings and stays Prettier-clean. tests/unit/routing-decision-compact-build.test.ts is deliberately not registered: the modules it imports (routingDecision.ts, autoRoutingDecision.ts, decisionStore.ts) are not in mutate, the gate does not ask for it, and listing it would make Stryker run an extra file against every mutant for no coverage.

The check-mutation-test-coverage.mjs CLI exits 0 without checking anything on Windows — its import.meta.url === file://${process.argv[1]} main-module guard never matches a drive-letter path — so this was verified by calling its exported findCoverageDrift directly:

before: 2 covering unit test(s) across 2 module(s) missing from tap.testFiles
          open-sse/services/accountFallback.ts
            + tests/unit/health-reads-do-not-probe-breakers.test.ts
          src/shared/utils/circuitBreaker.ts
            + tests/unit/health-reads-do-not-probe-breakers.test.ts
after:  No drift - every covering unit test is listed in tap.testFiles.
        (5039 unit test files scanned against 31 mutated modules)

File size:

[file-size] 1 violacao(oes):
  x src/lib/db/core.ts: 1770 > congelado 1745 (nao pode crescer)
[test-file-size] OK — 40 test files congelados, testCap 1200 para novos (5663 test files verificados)

src/lib/db/core.ts is not touched by this PR (git diff origin/release/v3.8.54 -- src/lib/db/core.ts is empty); it is red at the base commit. No baseline was adjusted. open-sse/services/accountFallback.ts sits at its frozen cap, so its change here is line-negative (2466 to 2465): the read-only helper lives in circuitBreaker.ts rather than being added to that file.

Complexity ratchets:

complexity=2747
cognitiveComplexity=1235
[complexity] OK — 2747 violacoes (baseline 3218)
[cognitive-complexity] OK — 1235 violacoes (baseline 1437)

Prettier. prettier --check reports every file in a Windows worktree as unformatted, because core.autocrlf checks out CRLF and the config wants LF; comparing Prettier's output against the line-ending-normalised file instead, 11 of the 13 changed files are clean. The two that are not were already not Prettier-clean at origin/release/v3.8.54 — this is pre-existing drift, not a regression from this PR:

• open-sse/services/accountFallback.ts — running Prettier against the base blob produces a 40-line diff there, and against this branch's version the same 40-line diff, none of it touching a line this PR adds (the drift is in ISO_RETRY_RE, the antigravityQuotaFamily import, persistAntigravityFamilyCooldownIfQuota, and two trailing commas, all untouched here).
• open-sse/services/webSessionPoolHealth.ts — 46 diff lines against the base blob versus 39 here; this PR's import change happens to remove one drifted block and adds none.

Reformatting either would have rewritten ~25 unrelated lines and pushed accountFallback.ts past its frozen size cap, so their existing formatting is left untouched.

open-sse/services/combo.ts was not edited.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cb63f5a2-38ca-4e96-b80e-fb0ee64b62cc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}