test(db): hold the viable-standby predicate's two copies in sync

[Nicolas0315]

What

A parity test for the viable-standby predicate, which currently exists twice:

• isViableFailoverTarget in @haru/core, consulted in memory by detectDegradedEscalation
• the same conditions inlined as EXISTS / NOT EXISTS subqueries inside escalateDomainIfFleetIdle, so a concurrent heartbeat cannot strip the standby between the decision and the escalation

Both source comments end with a request to keep the two in sync by hand. This does that mechanically instead: each case drives the same database state through both implementations and asserts the verdicts agree, across the dimensions the predicate judges (fully viable, never heartbeated, stale heartbeat, not ready, failed inference slot).

Why it is worth a test rather than a comment

Divergence here is not a cosmetic drift. The escalation deliberately sacrifices the active's remaining healthy models on the bet that failover succeeds, so an SQL predicate that is broader than core's means the bet gets placed on a standby the in-memory decision had already rejected, and the promotion then fails at the probe step. That is a bad trade made silently.

Confirming the test can actually fail

A parity test that passes proves nothing on its own, so I checked its sensitivity: deleting notExists(standbyFailedInferenceSlot) from the SQL predicate makes the failed-slot case report SQL true against core false and the test fails with expected true to be false. Restored, all six pass.

One case is a tripwire, not a parity assertion

The two predicates are not literally identical, and the difference is deliberate. Core requires an inference slot with at least one bound model (a bindingless target fails the probe step); the SQL requires only that an inference slot exist, justified in the comment with "the schema guarantees an inference slot binds >= 1 model".

That is true today - models: z.array(modelBindingSchema).min(1) in @haru/protocol - but it makes a zod constraint two packages away load-bearing for a SQL predicate, with nothing connecting them. If that .min(1) is ever relaxed (a slot provisioned before binding would be a reasonable motivation), the SQL side silently becomes the broader of the two.

So rather than assert a parity that does not hold, the last case asserts the layout schema still rejects a bindingless inference slot, with a comment explaining which SQL simplification depends on it. Relaxing the constraint breaks a test here instead of quietly widening the escalation.

Happy to drop that case if you would rather keep the schema free to change and instead tighten the SQL to check the binding count.

Notes

Both source comments now point at the test rather than asking for hand sync. No behavior change, no schema change, so no db:generate. Runs on the existing PGlite harness - no GPUs, no cloud, no live database.

pnpm test (12/12 tasks), typecheck, lint, format:check all pass locally.

---

Summary by cubic

Add a parity test that runs both viable-standby checks—isViableFailoverTarget in @haru/core and the SQL in escalateDomainIfFleetIdle—against the same DB state to keep them in sync and prevent silent divergence. The suite now covers all branches (including missing supervisor URL and missing inference slot) and pins preconditions (routing pointer, no op in flight) so results reflect viability only; also adds a schema tripwire to enforce at least one model per inference slot and updates source comments to point to the test; no behavior changes.

^{Written for commit 1200867. Summary will update on new commits.}

[coderabbitai]

Warning

Review limit reached

@Nicolas0315, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 943beb43-0fed-48ba-be88-519feeb8b6bc

📥 Commits

Reviewing files that changed from the base of the PR and between 9bf0062 and 1200867.

📒 Files selected for processing (3)

• packages/core/src/failover.ts
• packages/db/src/failover-parity.test.ts
• packages/db/src/repo/domains.ts

---

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.}

[drift-check]

Code Review Bot

No comment/code divergences or documentation drift detected. Reviewed 3 file(s); skipped 0.

[greptile-apps]

Greptile Summary

The PR adds regression coverage that compares the in-memory and SQL viable-standby predicates against identical database states.

• Covers viable, missing-heartbeat, stale-heartbeat, non-ready, failed-slot, missing-supervisor, and missing-inference-slot states.
• Adds a schema-invariant tripwire for bindingless inference slots.
• Updates both predicate comments to reference the parity suite.

Confidence Score: 5/5

The PR appears safe to merge.

The previously reported coverage gap is resolved by explicit null-supervisor and absent-inference-slot parity cases, and no blocking failure remains.

Important Files Changed

Filename	Overview
packages/core/src/failover.ts	Updates the predicate-maintenance comment to direct future changes to the parity suite.
packages/db/src/failover-parity.test.ts	Adds shared-state parity coverage, including the null-supervisor and absent-inference-slot cases requested in the prior review.
packages/db/src/repo/domains.ts	Updates SQL predicate documentation to identify the parity suite as the synchronization mechanism.

_{Reviews (2): Last reviewed commit: "test(db): cover every branch of the dupl..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6b56159f9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cubic-dev-ai]

All reported issues were addressed across 3 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Nicolas0315]

All three correct. Fixed in 1200867.

@greptile-apps and @chatgpt-codex-connector (cover every duplicated branch): right, and I checked it rather than assuming. Deleting isNotNull(standby.supervisorUrl) from the SQL predicate left all six original cases passing, so the gap was real and not theoretical.

It matters more than an ordinary coverage gap here, because this PR also rewrites both source comments to point at the suite instead of asking for hand sync. A suite covering only some branches makes that promise partly false, which is worse than the comment it replaced.

Both branches are covered now, and I verified every condition in the SQL predicate is load-bearing by deleting them one at a time:

supervisorUrl   -> 1 failure
inference slot  -> 1 failure
failed slot     -> 1 failure
ready state     -> 1 failure
heartbeat       -> 2 failures (never-seen and stale)

Neither new branch has a repository helper to reach it - layout apply is additive, and nothing unbinds a supervisor - so those two cases write the columns directly, with a comment saying why rather than leaving it looking like carelessness.

@cubic-dev-ai (assert the preconditions): right. The SQL side also guards on the routing pointer and on no operation being in flight, so a fixture change could make it answer false for a reason unrelated to viability while the assertions still "agreed" and the suite stayed green over nothing. degradeActive now asserts the transition landed and that the pointer still targets the active domain.

Worth noting where this lands: the per-branch verification above is the same discipline as publishability-samples.txt in #27, which pins one sample per denylist branch precisely because a large alternation stays "covered" when a branch is deleted. I had not applied that idea to my own predicate suite, which is what this round caught.

build, typecheck, lint, test (12/12 each) and format:check green locally; the parity file is 8 tests. CI here is still action_required.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[cubic-dev-ai]

All three correct. Fixed in 1200867.

@greptile-apps and @chatgpt-codex-connector (cover every duplicated branch): right, and I checked it rather than assuming. Deleting isNotNull(standby.supervisorUrl) from the SQL predicate left all six original cases passing, so the gap was real and not theoretical.

It matters more than an ordinary coverage gap here, because this PR also rewrites both source comments to point at the suite instead of asking for hand sync. A suite covering only some branches makes that promise partly false, which is worse than the comment it replaced.
...

@Nicolas0315 Looks right. degradeActive now pins both preconditions the SQL helper relies on that have nothing to do with viability—the routing pointer still targeting the active domain and the degraded transition actually landing—instead of trusting the fixture. That closes the hole I flagged: before, a fixture drift could make the SQL side answer false for an unrelated reason while the parity assertions still "agreed" and the suite stayed green over nothing.

The direct column writes for supervisorUrl and the missing-slot case read fine too, and the per-condition delete-verification table is the right way to prove the suite is actually load-bearing rather than just claiming it is. The degradeActive assertion on live.activeDomainId is the key addition, since that's the one precondition that could silently flip the SQL verdict.

Nothing else from my side—the tripwire case and the comment rewrites were already solid.

_{Tip: get faster answers by chatting with cubic’s review copilot in the review UI}

[greptile-apps]

LGTM. The two new cases land cleanly, and the direct column writes are the right call — adding a removeSupervisor helper just to serve a test would be worse than the comment explaining why the write is direct.

The deletion verification matrix is exactly the right discipline, and writing it down in the commit message means it's recoverable if the file is ever touched again. The degradeActive precondition assertions also close the real gap — a false-green over an unrelated false from the SQL side was a genuine trap, not a hypothetical one.

The parallel to #27's publishability-samples.txt is apt. The same principle (one sample / one case per branch so deletion is immediately visible) applied here as it does there.

Eight cases, every condition load-bearing, preconditions pinned. Good to merge from this side.

[greptile-apps]

Agreed on all counts. The degradeActive assertion on live.activeDomainId is doing the right work—it's the precondition that actually gates the SQL subquery's domain scope, so verifying it lands before the parity assertions means a fixture drift there will fail loudly rather than silently agree from the wrong starting state.

The per-condition delete table is a clean way to make the suite's load-bearing property self-documenting. This is in good shape—nothing left from my side.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[Nicolas0315]

Heads-up on merge order with #27, because git will not warn you about this one.

git merge-tree reports this branch and #27 as conflict-free. They are not compatible. e23c43d on #27 changes escalateDomainIfFleetIdle's fifth parameter from heartbeatStaleMs: number to budgets: Pick<FleetPolicy, "heartbeatStaleMs" | "degradedGraceMs">, and this test calls it positionally. Dropping the test file onto the #27 tree and running tsc --noEmit in @haru/db:

src/failover-parity.test.ts(95,5): error TS2345: Argument of type 'number' is not
assignable to parameter of type 'Pick<..., "degradedGraceMs" | "heartbeatStaleMs">'

Whichever of the two lands second needs a follow-up commit. A green merge button is not evidence here.

The fix is not only the signature. On #27 the SQL side also re-checks the degraded grace against the live stateUpdatedAt; on main it does not check grace at all, which is why bothVerdicts currently injects degradedGraceMs into the in-memory side only. Filling the new object with just heartbeatStaleMs would leave the two sides on different grace terms and the suite would stay green, which is precisely the failure mode this test exists to catch. On rebase I will pass the same DEGRADED_GRACE_MS to both, and re-run the per-condition deletion check so the grace guard is proven load-bearing on the SQL side too.

I am happy to rebase this onto #27 now if you would rather land #27 first; just say which order you want.

[Nicolas0315]

Status check from the contributor side: this is approved and every required check is green (CodeRabbit, cubic, Seer, Socket Security x2, License compliance). mergeable is true; the only remaining state is BLOCKED, which resolves with a maintainer merge — I only have pull access here, so I can't take it further.

Nothing is pending on my end. Happy to rebase if the base has moved by the time you get to it.

[Nicolas0315]

Review gate update at head 1200867: existing review threads are resolved, but CI and CodeQL remain action_required. Please authorize the fork workflows and provide an independent maintainer review after exact-head checks complete; until then this stays HOLD.