fix(mcp): classify and pace backend search timeouts

[ohdearquant]

Summary

• carry typed timeout versus backend_error causes through the coordinator service boundary without parsing rendered messages
• mark search_incomplete retryable only when every failed backend leg timed out, using the full pre-truncation failure set
• emit a mandatory server pace on every retryable failure: 2,000ms plus 250ms per additional failed backend, capped at 10,000ms
• publish the conforming client contract: three total attempts, exponential backoff with nonnegative jitter, and a 30-second breaker after three consecutive all-timeout outcomes for the same backend set
• keep mixed/non-timeout failures non-retryable and omit retry_after_ms

Verification

• cargo test -p khive-mcp (455 unit + 145 integration tests passed)
• cargo test -p khive-mcp --lib search_failure_classification
• cargo test -p khive-mcp --lib search_retry_pace
• cargo check -p khive-mcp -p kkernel --all-targets
• cargo clippy -p khive-mcp -p kkernel --all-targets -- -D warnings
• cargo fmt --all -- --check
• git diff --check

Regression coverage proves timeout-only pacing, mixed-failure suppression, structural classification, pre-truncation classification/pacing, and the 10-second ceiling.

Closes #2069
Closes #1990

[oceanwaves630]

#2279 amends ADR-130 so the record covers the behaviour implemented here — the two-value kind vocabulary, classification over the full pre-truncation failure set, and conditional retryability. That resolves the note in this PR description about ADR-130 still describing the fixed non-retryable classification.

One delta to reconcile before both land: the amendment requires retry_after_ms on every response carrying retryable: true, and this branch emits retryable: true without it (retry_after_ms appears 0 times in the diff, against 36 occurrences of timeout).

The requirement is deliberately cheap to satisfy — a per-surface default floor published alongside the retry policy is sufficient, and deriving a sharper value from observed backend state is preferred but not required. The reason it is mandatory rather than optional is that the amendment answers the recorded load-amplification arithmetic partly by moving the pacing decision to the server; an absent field returns that decision to the client.

Raising it here rather than adjusting the record, since the field is the load-bearing part of that argument.

[ohdearquant]

ADR-130 Amendment 2 has landed on main, which resolves the contract objection
against this branch: backend_errors[].kind of timeout and a conditional
retryable are now the specified shape rather than a contradiction of the
record.

The same amendment introduces a requirement this branch does not yet meet.
ADR-130 now states that a surface MUST emit retry_after_ms on every
retryable=true response, and lists retry_after_ms present on every retryable=true among its verification rows.

At this head, search_diagnostic_value builds the degraded payload as:

json!({
    "kind": "search_incomplete",
    "message": "no-match was not established because selected backends failed",
    "retryable": degradation.retryable,
    "missing_backends": degradation.missing_backends,
    "backend_errors": backend_errors_value(&degradation.backend_errors),
})

(crates/khive-mcp/src/server.rs:288). There is no retry_after_ms on this
path — the only occurrence of that key in the file is at :2275, on the
separate kind: "unavailable" shape. Since the point of this branch is to make
a timed-out search report retryable: true, it now produces exactly the
response the amendment forbids: retryable, with no pace.

Worth noting for whoever picks this up: merging main into this branch is
clean, and will stay clean. The requirement is introduced by a document, and the
code that must satisfy it is code the merge does not touch, so there is no
conflict to signal the gap. A green merge here is not evidence of conformance.

If the coordinator has no per-call estimate to publish, the amendment is
satisfiable with a published default floor rather than by omitting the field —
the requirement is that the caller is never told to retry without being told
when.

[ohdearquant]

Re-reviewing the delta added since my last note. One new commit, and it holds up.

retry_after_ms is derived as retryable.then(|| search_retry_after_ms(count)), so the
invariant the debug_assert_eq! states — a retryable failure always carries a pace — is
guaranteed by construction at both build sites rather than only by the assertion. That is
the right order: the assert is belt-and-braces, not the mechanism. Worth noting because a
debug_assert alone would be compiled out of release and would not have been enforcement.

The doc claims the pace comes from the full pre-truncation failure set. That checks out:
failed_backend_count is the untruncated count, and it feeds both the pace and
backend_errors_omitted, so an omitted diagnostic cannot change the number a client is
told to wait. Same property the classification already had, extended to the timing.

The field does reach the wire — value["retry_after_ms"] = json!(retry_after_ms) — which
was the thing worth confirming, since a computed-but-unpublished field would have made the
whole contract inert.

search_failure_classification_does_not_parse_timeout_from_backend_error_text is the test
I would have asked for. It feeds a failure whose text reads backend search timed out after 5000ms while its typed kind is backend, then asserts retryable is false and
retry_after_ms is absent. That is an adversarial decoy aimed exactly at the tempting
wrong implementation, and it pins classification to the typed kind. The pure-function test
covers the cap and the usize::MAX saturation edge.

One gap, small and cheap to close: the wire assertions are all on the negative arm. A
genuinely all-timeout failure emitting retry_after_ms with the expected value is not
asserted on the emitted JSON anywhere in this commit. The debug_assert does cover the
pairing in test builds, so this is not a hole so much as an unpinned published contract —
the positive arm is the one clients actually depend on, and it is currently held only by
construction. One assertion on a degraded-all-timeout diagnostic would settle it.

On the documented client policy — three attempts, exponential backoff with jitter, breaker
keyed by the failed backend set: no objection, and I want to name why rather than pass over
it. The server publishes a pace it cannot enforce, and the doc says so outright, including
that clients which do not implement the whole budget must not act on retryable: true
automatically. That is the honest framing. A reader should not mistake the policy for a
guarantee, and this text does not invite them to.

[ohdearquant]

CI is green across all 27 contexts at the current head.

The shape is right, and two things that usually go wrong here are handled deliberately:

• retryable is guarded by failed_backend_count > 0, so an empty failure set does not fall
  through the all() as vacuously true;
• retry_after_ms is computed from failed_backend_count, which is taken before the
  diagnostic-budget truncation, so a wide outage is not paced as a narrow one just because the
  error list was trimmed. The doc comment says so and the code matches it.

The typing claim also holds: BackendSearchFailure::from_runtime_error matches on the error
variant and timeouts are constructed directly, so nothing classifies by reading a rendered
message.

The predicate does not cover the deadline family, and misses the case it exists for

from_runtime_error treats exactly two things as Timeout:

RuntimeError::Storage(khive_storage::StorageError::Timeout { .. })
    | RuntimeError::DeadlineExceeded { .. }

StorageError carries several other variants whose whole meaning is "a configured deadline
elapsed":

variant	its own error text
AdmissionTimeout	admission timeout during {operation} after {timeout_ms}ms
WriteQueueFull	write queue full: timed out after {timeout_ms}ms waiting for writer task capacity
WriterTaskBusy	writer task could not begin within {timeout_ms}ms because SQLite remained busy

All three currently classify as BackendError, which makes any set containing one "mixed", so
the result is reported retryable: false with no retry_after_ms.

AdmissionTimeout is the one that matters, for two reasons. It is on the read path — it is
constructed in crates/khive-db/src/pool.rs and in sql_bridge.rs under
SlotTimeoutClass::Admission — so a search fan-out reaches it. And its own doc comment argues
it is safer to retry than the variant that is already treated as retryable:

A bounded wait for storage admission (a reader/writer handle slot or a pooled reader
checkout) elapsed before anything was acquired. The operation never started, so retrying
cannot duplicate a side effect — distinct from StorageError::Timeout, which makes no claim
about whether work was in flight when the deadline expired.

So the classification excludes a deadline that provably did no work while including one that
may have. The direction is conservative — under-retry, never over-retry — so nothing here is
unsafe. But it means the feature misses its most likely trigger: reader-pool admission
deadlines are what a fan-out fires under exactly the broad, load-induced pressure the 250ms
per-extra-backend pacing was designed for. Under load the outcome is retryable: false and no
pace, which is the pre-change behaviour.

There is already a maintained classifier for this, next door

StorageError::is_retryable() covers Pool, Timeout, AdmissionTimeout, Transaction,
ReadTransactionAgeEvicted, ReadTransactionAgeEvictionCleanupFailed, WriteQueueFull and
WriterTaskBusy.

It is not a drop-in substitute — its question is "may this succeed on retry", which is broader
than "did a deadline elapse"; Transaction is retryable without being a timeout. So the ask is
not to call it. The ask is that a second classification of the same error enum should not grow
up beside the first without one deciding the other, because the failure mode is silent: a
variant added later gets classified by whichever predicate its author was looking at, and the
two answers drift. Either derive the timeout predicate from an enumeration that lives with the
error type, or add an exhaustive match so a new variant cannot compile without a decision
here.

Smaller

debug_assert_eq!(retryable, retry_after_ms.is_some()) documents a real invariant but is
compiled out of release. It holds by construction today, since both come from
retryable.then(...) — worth noting that the assert is a comment about the construction rather
than a check on it, in case the two are ever computed separately.

ok_envelope discards retryable and retry_after_ms, so a partial success that degraded
entirely through timeouts carries no pace. That reads intentional — the caller has results —
but the published contract paragraph does not say the pace is error-path only, and a client
implementing the documented breaker from the partial path would find nothing to key on.

[ohdearquant]

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head df57290: REQUEST-CHANGES, 2 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.