fix(lifi): harden recovery and fill lifecycle

[alrxy]

No description provided.

[oxsteins]

Reviewed the recovery/lifecycle changes end to end. Build is green, the -race suite passes, and I traced the generation/barrier logic, the retry FIFO, the reservation snapshot-excluding, and the pagination. The worker/retry FIFO, the shutdown drain, the gas leg reorder, and the quote-range revalidation all check out. Nothing here is a blocking correctness bug. A few things worth addressing, most consequential first.

1. Verify the recovery scope against the LI.FI API (potential silent no-op)

orderclient.go:186 scopes the recovery sweep with ExclusiveFor(executor). The vendored OpenAPI documents exclusiveFor as "Exclusive for" address referencing a specific solver, which is the exclusivity-window field, set only on exclusive orders (context 0xe0/0xe1). The primary supported type is the non-exclusive limit order (context 0x00).

If the server does not return non-exclusive matched orders for exclusiveFor=<executor>, every sweep returns zero rows, recovery converges immediately (discovered == 0 -> tryEndRecovery true), and quotes publish with the publish-before-recovery race left open, with no error logged. That is the exact race this PR is meant to close, silently defeated.

Please confirm with LI.FI that active matched orders (including non-exclusive limit orders) are returned by exclusiveFor=<executor> during their fillable window. If the API key already scopes results to this solver, exclusiveFor may instead be over-filtering. This is the one item I would gate the merge on.

2. A recovered, capacity-blocked order can be dropped on a transient error after recovery ends

markRecoveryRetry (execution.go:208) is a no-op once recoverySeen == nil, i.e. after recovery has ended. A capacity-blocked recovered order gets parked in the worker retry FIFO and is retried when its blocking fill completes. If that retry hits a transient error (retryable with empty blockedOn), the process closure neither re-enqueues it (blockedOn is empty) nor catches it for recovery (recoverySeen is nil), so it is dropped. It only comes back on the next reconnect or server re-delivery.

It is an asymmetry (the same order is caught if it fails during recovery, lost if it fails after) that a hardening change should probably close. Not permanent and not fund-threatening, but worth fixing.

3. More than 1050 active orders in one status wedges quoting permanently

The offset cap (orderclient.go:231) correctly fails closed past offset 1000 (max 1050 rows per status). But the error loops forever with backoff in recoverOrdersUntilSuccess and never sends connectionCtx, so quotes never resume. That is fail-closed by design, but it is an unbounded operational cliff with only silent infinite backoff and no alert. It also becomes reachable if item 1 is misconfigured and the sweep pulls every solver's rows. Worth an explicit alert or metric rather than silent backoff.

4. (low) Live-order overflow during recovery is best-effort

execution.go:102: a live feed order dropped because the 4096 inbox is full during recovery sets recoveryOverflow, which forces exactly one more sweep. If REST does not list that order yet, recovery ends and the order is lost until reconnect. Requires a 4096-deep backlog and self-heals on reconnect, so low severity, but the overflow flag only papers over it on a best-effort basis.

Nits

• quotes.go:72 handles connectionCtx == nil, but nothing ever sends nil on feedConnections (disconnect is signaled by cancelling the delivered context). Dead branch, and it is misleading about the shutdown protocol.
• takeRecoveryRetries (execution.go:226) resets the seen-set ring pointer to 0, which corrupts age-order for eviction after the ring has wrapped. It only triggers with more than 4096 distinct keys in one recovery, which is unreachable given the ~2100-row REST cap, so effectively dead, but the pointer reset is still wrong.
• Worth a one-line comment on the load-bearing invariant that keeps the worker from hanging on graceful drain: retries.len() > 0 implies pending.len() > 0 (the pending.len() == 0 short-circuit in process).

[oxsteins]

Re-reviewed at 369cdd8. The recovery-barrier redesign correctly closes the dropped-retry gap from my last review (the worker now holds the barrier until the retry FIFO drains, and the new regression test reproduces exactly that scenario), the seen-ring compaction is fixed, and moving beginRecovery into a synchronous beforeRead hook closes an ordering race I had not even flagged. Race suite is green. Three things remain that I would settle before merge.

1. New in this update: a webhook strategy error can wedge recovery permanently

The new permanent-error classification only protects the default strategy:

• internal/solvers/lifi/planning.go:97 return orderProcessingResult{retryable: !types.IsPermanentFillDecisionError(err)}
• only strategies/default/fill.go marks errors permanent; strategies/webhook/strategy.go never does

So during recovery, a webhook that deterministically errors on one order (say a persistent 4xx on a malformed input, instead of the contractual null) is always retryable: the order re-enters every sweep via markRecoveryRetry, recovery never converges, and after maximumOrderRecoverySweeps = 8 (execution.go:389) it backs off and starts over (execution.go:399), forever. One poisoned order plus one webhook bug means quotes never resume. A per-order retry cap, or marking 4xx-class webhook responses permanent, would bound it.

2. submitQuotes ignores the server's quotesAdded

internal/solvers/lifi/orderclient.go:256 discards the response body:

_, httpResp, err := c.api.SolverAPIAPI.
    QuotesControllerSubmitQuotes(c.withAuth(ctx)).

The spec's SubmitQuotesResponseDto.quotesAdded ("Number of quotes successfully added") implies a 200 can accept fewer quotes than submitted. reconcile treats any 200 as full-batch and deletes the expire pairs from local state, so a dropped expire-overwrite leaves the previous quote live server-side with nothing left to ever expire it. The server can then match orders against that stale price until its original TTL. Cheap hardening: compare quotesAdded to the submitted count and treat a mismatch as a submit error, the existing uncertain-submit path already handles that case correctly.

3. exclusiveFor scoping: rationale added, live check still open

The new comment at orderclient.go:182 states the intended semantics, and since every published quote pins ExclusiveFor to this solver, matched orders should carry it. That converts my earlier concern from accident to documented intent, but it is still an assumption about the order server. One restart against order-dev with an active match confirming the row actually comes back from GET /orders?exclusiveFor=<executor> would close it.

Merge-order conflict with #115

Same note as on #115: the two PRs conflict on cmd/vault-solver/run.go and internal/solver/solver.go, and the LI.FI completion paths auto-merge cleanly while contradicting each other. This PR defers reservation release until after the retry batch (execution.go:557) to guarantee capacity is never transiently freed; #115 releases immediately inside completeFill (its submission.go:59). Merged blindly, the immediate release runs first and this PR's invariant silently breaks. Worth agreeing which lifecycle model wins before either lands.

[oxsteins]

Reviewed through 4e28200. Every finding from my earlier comments is fixed and verified: the recovery barrier now holds until the capacity-retry FIFO drains (with the regression test reproducing the exact drop scenario), strategy failures are bounded to three attempts per order per recovery session while chain/RPC failures stay fail-closed, webhook 400/422 responses are classified permanent through the typed status error, quotesAdded is verified range-for-range against the re-vendored spec so a partial acknowledgement keeps the pair tracked and retried, and the seen-ring compaction plus the smaller cleanups are in. Full race suite passes.

Two non-blocking notes for the record: worth confirming on the first order-dev deploy that a matched non-exclusive order actually comes back from GET /orders?exclusiveFor=<executor> (the design now documents this assumption, the live check would close it), and merge ordering with #115 needs coordination since both PRs touch run.go, solver.go, and the LI.FI completion path.

Approving.