toasterson/Anima

Watch

Fork 0

WIP (red test): report central lane health to the circuit breaker #753

Open

toasterson wants to merge 1 commit from fix/central-lane-circuit-breaker into main

toasterson commented

2026-09-22 13:26:51 +00:00

Owner

⚠️ Do not merge — the new PG regression is RED

The production change is complete and does not regress anything. The regression test asserting it is not green yet, and I have located but not fixed why. Details at the bottom. Flagging loudly rather than presenting this as finished.

The gap

Central sessions never reach services::runner, which was the only caller of LaneCircuitBreaker. A central executor could therefore emit unbounded consecutive runtime faults and never leave rotation.

Observed live on astra-impl-2: 21 consecutive unknown_outcome attempts between 2026-09-14 and 2026-09-22, ~50 min per cycle, breaker never tripped. The legacy path trips correctly after 3 — WI-298 was parked by exactly that, on imhotep. So the breaker works; it simply never saw central lanes.

Classification — named accurately

unknown_outcome is runtime-attributed and NOT a business refusal. The runtime never learned the execution outcome, so no verdict was produced and there is nothing to refuse. completion.rs already says this of the witness ("the runtime, rather than an implementation attempt, exhausted reconciliation"); Business::lane_attribution now says the same for the lane, sharing infrastructure_failure() with witness() so the two cannot drift.

attribution	lane effect
`RuntimeFault`	charge
`Healthy`	reset
`BusinessFailure`	neither — the lane did its job

Reported through the existing lane API — no new breaker semantics

record_lane_dispatch_failure already means "counts toward the breaker but does NOT charge the work item's phase revisit budget" and already has four callers in scheduler.rs. Using it keeps this path out of the WI budget by construction. record_refusal is deliberately not used — refusals are business verdicts.

No migration. Migration 151 is NOT reserved. Persistence is unchanged and matches the existing callers exactly: the trip (circuit_breaker_tripped, paused_until) persists and survives restart; a partial streak below threshold is in-memory, as load_from_db has always documented.

Exactly once per durable attempt

The hook sits after outbox::apply_close_in_tx, which short-circuits an already-applied close — so repeated reconciliation polls of the same attempt cannot inflate the count. The executor is read inside the transaction that finishes the session; the breaker update is applied only after tx.commit(), because the breaker writes through the pool and an uncommitted attempt must not move lane state.

Legacy behaviour preserved

Relay::circuit_breaker is Option, default None — an unwired Relay behaves exactly as before rather than inventing lane state. main.rs wires the real one.

Central routes were already gated. I verified scheduler.rs:2210 sits in the same candidate loop as the legacy NoCapacity check, so it covers central candidates too — only the feeding was missing. No global or fresh-review route is silently disabled; only genuinely sick lanes pause, for the existing cooldown.

Verification

Both pre-existing PG tests in this file pass unchanged — natural_completion_commits_canonical_outcome_policy_and_owner_once and pending_reconciliation_backs_off_instead_of_reclaiming_every_tick — so the normal completion path is not regressed.
cargo clippy -p anima-server --lib --tests: 0 errors, no lints on touched files.
Run against a real disposable Postgres, not mocks.

The red test, honestly

central_runtime_faults_charge_the_lane_once_and_never_the_work_item asserts all four required properties. Its first assertions pass — one durable attempt charges exactly once, four replays do not inflate it, and the WI phase-revisit budget is untouched. It then fails driving the fixture to a second close.

Root cause located: anima-akh/src/outbox/selected.rs:45 decodes closing_intent as a non-Option Value:

let (admission, intent): (Value, Value) = sqlx::query_as(
    "SELECT admission,closing_intent FROM akh_session_dispatches WHERE ...")

A NULL closing_intent therefore raises decoding column 1: unexpected null. The runtime-fault fixture reaches advance_selected_close with it still NULL. That is a pre-existing latent fragility in the akh outbox, surfaced by this test rather than caused by it — and arguably worth fixing on its own merits, since a nullable column decoded as non-nullable is a crash waiting for the right state.

I stopped iterating on the harness rather than burn more budget guessing. Next step is to make the runtime-fault fixture establish closing_intent the way the natural-completion fixture does, or to make that decode Option.

Ownership

Touches central_sessions.rs, central_sessions/completion.rs, lane_circuit_breaker.rs, main.rs, and this test file. No overlap with Sol1 (staging/reviews), Sol2 (scheduler/runner claim), or Opus1 (PR752 scheduler/lifecycle). main.rs is a one-line builder call — flagging it in case anyone else is in there.

🤖 Generated with Claude Code

## ⚠️ Do not merge — the new PG regression is RED The production change is complete and does not regress anything. The regression test asserting it is **not green yet**, and I have located but not fixed why. Details at the bottom. Flagging loudly rather than presenting this as finished. --- ## The gap Central sessions never reach `services::runner`, which was the **only** caller of `LaneCircuitBreaker`. A central executor could therefore emit unbounded consecutive runtime faults and never leave rotation. Observed live on `astra-impl-2`: **21 consecutive `unknown_outcome` attempts** between 2026-09-14 and 2026-09-22, ~50 min per cycle, breaker never tripped. The legacy path trips correctly after 3 — WI-298 was parked by exactly that, on `imhotep`. So the breaker works; it simply never saw central lanes. ## Classification — named accurately `unknown_outcome` is **runtime-attributed and NOT a business refusal**. The runtime never learned the execution outcome, so no verdict was produced and there is nothing to refuse. `completion.rs` already says this of the witness (*"the runtime, rather than an implementation attempt, exhausted reconciliation"*); `Business::lane_attribution` now says the same for the lane, sharing `infrastructure_failure()` with `witness()` so the two cannot drift. | attribution | lane effect | |---|---| | `RuntimeFault` | charge | | `Healthy` | reset | | `BusinessFailure` | **neither** — the lane did its job | ## Reported through the existing lane API — no new breaker semantics `record_lane_dispatch_failure` already means *"counts toward the breaker but does NOT charge the work item's phase revisit budget"* and already has four callers in `scheduler.rs`. Using it keeps this path out of the WI budget **by construction**. `record_refusal` is deliberately not used — refusals are business verdicts. **No migration. Migration 151 is NOT reserved.** Persistence is unchanged and matches the existing callers exactly: the trip (`circuit_breaker_tripped`, `paused_until`) persists and survives restart; a partial streak below threshold is in-memory, as `load_from_db` has always documented. ## Exactly once per durable attempt The hook sits **after `outbox::apply_close_in_tx`**, which short-circuits an already-applied close — so repeated reconciliation polls of the same attempt cannot inflate the count. The executor is read inside the transaction that finishes the session; the breaker update is applied only **after `tx.commit()`**, because the breaker writes through the pool and an uncommitted attempt must not move lane state. ## Legacy behaviour preserved `Relay::circuit_breaker` is `Option`, default `None` — an unwired Relay behaves exactly as before rather than inventing lane state. `main.rs` wires the real one. **Central routes were already gated.** I verified `scheduler.rs:2210` sits in the same candidate loop as the legacy `NoCapacity` check, so it covers central candidates too — only the *feeding* was missing. No global or fresh-review route is silently disabled; only genuinely sick lanes pause, for the existing cooldown. ## Verification - Both pre-existing PG tests in this file **pass unchanged** — `natural_completion_commits_canonical_outcome_policy_and_owner_once` and `pending_reconciliation_backs_off_instead_of_reclaiming_every_tick` — so the normal completion path is not regressed. - `cargo clippy -p anima-server --lib --tests`: **0 errors**, no lints on touched files. - Run against a real disposable Postgres, not mocks. ## The red test, honestly `central_runtime_faults_charge_the_lane_once_and_never_the_work_item` asserts all four required properties. Its **first assertions pass** — one durable attempt charges exactly once, four replays do not inflate it, and the WI phase-revisit budget is untouched. It then fails driving the fixture to a **second** close. **Root cause located:** `anima-akh/src/outbox/selected.rs:45` decodes `closing_intent` as a non-`Option` `Value`: ```rust let (admission, intent): (Value, Value) = sqlx::query_as( "SELECT admission,closing_intent FROM akh_session_dispatches WHERE ...") ``` A NULL `closing_intent` therefore raises `decoding column 1: unexpected null`. The runtime-fault fixture reaches `advance_selected_close` with it still NULL. That is a **pre-existing latent fragility in the akh outbox**, surfaced by this test rather than caused by it — and arguably worth fixing on its own merits, since a nullable column decoded as non-nullable is a crash waiting for the right state. I stopped iterating on the harness rather than burn more budget guessing. Next step is to make the runtime-fault fixture establish `closing_intent` the way the natural-completion fixture does, or to make that decode `Option`. ## Ownership Touches `central_sessions.rs`, `central_sessions/completion.rs`, `lane_circuit_breaker.rs`, `main.rs`, and this test file. No overlap with Sol1 (`staging`/`reviews`), Sol2 (scheduler/runner claim), or Opus1 (PR752 scheduler/lifecycle). `main.rs` is a one-line builder call — flagging it in case anyone else is in there. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

toasterson added 1 commit

2026-09-22 13:26:51 +00:00

fix(central): report central lane health to the circuit breaker

CI / conflict-check (push) Successful in 4m41s

Details

CI / test (push) Successful in 9m30s

Details

60a79155ea

Central sessions never reach `services::runner`, which was the only caller of
`LaneCircuitBreaker`. A central executor could therefore emit unbounded
consecutive runtime faults and never leave rotation. Observed live on
astra-impl-2: 21 consecutive `unknown_outcome` attempts between 2026-09-14 and
2026-09-22, ~50 min per cycle, breaker never tripped — while the legacy path
trips correctly after 3 (WI-298 was parked by exactly that).

Classification, named accurately.

  `unknown_outcome` is runtime-attributed and is NOT a business refusal. The
  runtime never learned the execution outcome, so no verdict was produced and
  there is nothing to refuse. `completion.rs` already says this of the witness
  ("the runtime, rather than an implementation attempt, exhausted
  reconciliation"); `Business::lane_attribution` now says the same for the
  lane, sharing `infrastructure_failure()` with `witness()` so the two cannot
  drift.

    RuntimeFault    -> charge the lane
    Healthy         -> reset the lane
    BusinessFailure -> move the lane in neither direction

  A work-item-attributable failure must not punish the lane; the lane did its
  job.

Reported through the existing lane-attributed API, not a new one.

  `record_lane_dispatch_failure` already means "counts toward the breaker but
  does NOT charge the work item's phase revisit budget", and already has four
  callers in `scheduler.rs`. Using it keeps this path out of the WI budget by
  construction and adds no new breaker semantics. `record_refusal` is
  deliberately NOT used: refusals are business verdicts.

  No migration. Persistence semantics are unchanged and match the existing
  callers exactly: the trip (`circuit_breaker_tripped`, `paused_until`) is
  persisted and survives restart; a partial streak below threshold is
  in-memory, as `load_from_db` has always documented. Migration 151 is NOT
  reserved.

Exactly once per durable central attempt.

  The hook sits after `outbox::apply_close_in_tx`, which short-circuits a close
  that was already applied, so repeated reconciliation polls of the same
  attempt cannot inflate the count. The executor is read inside the same
  transaction that finishes the session, but the breaker update is applied
  only AFTER `tx.commit()` — the breaker writes through the pool, and an
  uncommitted attempt must not move lane state.

Legacy behaviour preserved.

  `Relay::circuit_breaker` is `Option`, defaulting to `None`. An unwired Relay
  behaves exactly as before rather than inventing lane state. `main.rs` wires
  the real one. Fresh/global review routes are untouched: the existing
  scheduler gate at `scheduler.rs:2210` already covers central candidates in
  the same loop as legacy ones, so no route is silently disabled — only
  genuinely sick lanes pause, and only for the existing cooldown.

KNOWN RED — the PG regression in this commit does not pass yet.

  `central_runtime_faults_charge_the_lane_once_and_never_the_work_item`
  asserts the four required properties (one charge per durable attempt across
  replays; distinct attempts reach the threshold; success resets; no WI budget
  penalty). Its first two assertions pass; it then fails driving the fixture to
  a second close.

  Root cause located, not yet fixed: `anima-akh/src/outbox/selected.rs:45`
  decodes `closing_intent` as a non-`Option` `Value`, so a NULL there raises
  "decoding column 1: unexpected null". The runtime-fault fixture reaches
  `advance_selected_close` with `closing_intent` still NULL. That is a
  pre-existing latent fragility in the akh outbox, surfaced by this test rather
  than caused by it.

  DO NOT MERGE until it is green.

Both pre-existing tests in this file pass unchanged
(`natural_completion_commits_canonical_outcome_policy_and_owner_once`,
`pending_reconciliation_backs_off_instead_of_reclaiming_every_tick`), so the
production change does not regress the normal completion path.
`cargo clippy -p anima-server --lib --tests`: 0 errors, no lints on the
touched files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

CI / conflict-check (push) Successful in 4m41s

Details

CI / test (push) Successful in 9m30s

Details

This pull request has changes conflicting with the target branch.

crates/anima-server/src/central_sessions.rs

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'WIP (red test): report central lane health to the circuit breaker' (#753) from fix/central-lane-circuit-breaker into main

Merge commit body

Reviewed-on: https://code.aopc.cloud/toasterson/Anima/pulls/753

Merge pull request 'WIP (red test): report central lane health to the circuit breaker' (#753) from fix/central-lane-circuit-breaker into main

Reviewed-on: https://code.aopc.cloud/toasterson/Anima/pulls/753

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/central-lane-circuit-breaker:fix/central-lane-circuit-breaker

git switch fix/central-lane-circuit-breaker

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff fix/central-lane-circuit-breaker

git switch fix/central-lane-circuit-breaker

git rebase main

git switch main

git merge --ff-only fix/central-lane-circuit-breaker

git switch fix/central-lane-circuit-breaker

git rebase main

git switch main

git merge --no-ff fix/central-lane-circuit-breaker

git switch main

git merge --squash fix/central-lane-circuit-breaker