e2e/qa: settlement test self-heals stuck seats and retries withdraws with endpoint rotation#4066
Conversation
|
Pushed a follow-up (ab7fd67) for the post-build finding on run 29588245744: the seat re-stuck immediately after a manual unstick, with no pool failover logged, confirming the CLI withdraw preflight and the harness read shared one primary endpoint that serves stale
|
ben-dz
changed the title
nikw9944
left a comment
nikw9944
left a comment
There was a problem hiding this comment.
The design is sound: onchain-state confirmation instead of CLI error-text matching, sentinel-based not-found detection, bounded retries, and a self-heal pass that converges across runs. Build, go vet, golangci-lint, and the new unit tests all pass. One High finding: seatIsWithdrawn treats TenureEpochs == 0 as proof of withdrawal but ignores HasPendingInstantRequest — with the ack-wait step removed, the withdraw retry can declare false success on a paid-but-not-yet-acked seat and recreate the exact poisoned state this PR eliminates. The fix is one line. A Medium on trusting a single not-found read from a possibly-stale endpoint, plus four Lows.
- The two items the PR description already defers (no funding-authority filter in filterActiveSeats; discriminator-only FetchAllClientSeats scan) look acceptable at QA scale; no need to re-litigate.
- Non-test additions are ~250 lines, within the repo's 500-line guidance. Build, go vet, golangci-lint, and the new unit tests all pass locally.
…not-found) Address review on #4066: seatIsWithdrawn also requires no pending instant request (withdrawal is blocked onchain while the flag is set, so a pending seat is not withdrawn regardless of tenure) and trusts a not-found read only when a second read from a rotated endpoint agrees (a first-pay seat can be minutes old and invisible to a stale endpoint). Log the confirm-read error on retry warns, inline single-caller ActiveClientSeats into SelfHealStuckSeats, note that genuine in-flight rejections also rotate the shared pool, and fix the changelog to say what the confirm actually checks.
ensure_multicast_disconnected now scans onchain client seats for the client's public IP and withdraws any left active (TenureEpochs > 0) by a previous run whose withdraw bailed. Both the cleanup and self-heal withdraws retry over a bounded window instead of a single shot that the spurious "request in flight" preflight rejection could fail every run. Refs #2002
Address review: the substring classifier matched bare "not found", so a transient "Blockhash not found" would read as "already withdrawn" and silently leave a seat stuck. Replace it with authoritative onchain confirmation (isSeatNotFound sentinel match + TenureEpochs==0). Also scrub RPC errors on the new read paths, make the heal poll tolerant of a transient read blip after a successful withdraw, and bound the cleanup context and raise its failure log to Warn. Refs #2002
Post-build QA finding: a fixed endpoint serves stale getMultipleAccounts (what the withdraw CLI preflight reads) for seconds to hours while getAccountInfo stays fresh, so a same-endpoint retry can't converge — the in-flight preflight bail recurs every attempt. On that specific bail the retry now calls solanaRPCPool.Failover() so the next FeedSeatWithdraw preflights against a different endpoint, and logs the redacted endpoint per attempt. Rotation fires only for the pre-submission in-flight bail, never for submission timeouts (which may have landed onchain). Refs #2002
…persedes #4065) Co-authored-by: Thijs van Emmerik <thijs@doublezero.xyz>
…not-found) Address review on #4066: seatIsWithdrawn also requires no pending instant request (withdrawal is blocked onchain while the flag is set, so a pending seat is not withdrawn regardless of tenure) and trusts a not-found read only when a second read from a rotated endpoint agrees (a first-pay seat can be minutes old and invisible to a stale endpoint). Log the confirm-read error on retry warns, inline single-caller ActiveClientSeats into SelfHealStuckSeats, note that genuine in-flight rejections also rotate the shared pool, and fix the changelog to say what the confirm actually checks.
nikw9944
left a comment
nikw9944
left a comment
There was a problem hiding this comment.
All six findings from the previous round are addressed and verified in the current diff at head 5b9cc9b: H1 (pending-flag guard adopted in seatIsWithdrawn), M1 (not-found trusted only after a second read on a rotated endpoint), L1 (confirm_read_error logged), L2 (ActiveClientSeats inlined, duplicate shredsQuery removed), L3 (CHANGELOG reworded), L4 (genuine-in-flight rotation comment added). Build, unit tests, gofmt, and golangci-lint pass on the new head. One new Low: the self-heal confirmation poll still trusts a single not-found read, and its 'closed/absent seat account' comment contradicts the fact the hardening commit itself documents (withdraw does not close the seat account).
- All previous findings (H1, M1, L1-L4) verified as fixed in the current code at head 5b9cc9b — dropped based on the diff, not on the author's replies. External-program claims in the replies (tenure set at pay; withdraw keeps the account) remain unverifiable from this repo, but the adopted fixes are correct independent of them.
Address L5 on #4066: the self-heal confirmation poll trusted a single not-found read and its comment contradicted the documented fact that withdrawal does not close the seat account. Reuse seatIsWithdrawn instead, which brings the rotate-and-reread not-found handling and the pending-flag check to the poll and removes the bespoke third confirmation branch.
2 participants
Resolves: malbeclabs/infra#2002
Supersedes #4065 — carries over its ack-wait removal and retry-instead-of-flag-polling approach (credit @thijsvanemmerik for the diagnosis and design there; co-authored on the absorbing commit).
Summary of Changes
TestQA_MulticastSettlementnow self-heals from a seat left active onchain by a previous run whose withdraw failed — the poisoned state that turnedqa.mainnet-betared for 11+ consecutive hourly runs.ensure_multicast_disconnectedscans onchain client seats for the client's public IP (shreds SDKFetchAllClientSeats) and withdraws any withTenureEpochs > 0, in addition to the existing session check. A stuck-active seat with no session (the actual failure state) is now detected instead of passing untouched.withdraw_seatstep, the self-heal, and the cleanup — retries over a bounded window (WithdrawSeatWithRetry) instead of a single shot; the same spurious "request in flight" preflight bail failed on every run, so the escrow grew ~$30/run and never healed.solanaRPCPool.Failover()) before retrying: a fixed endpoint serves the stalegetMultipleAccountsread behind the bail for seconds to hours, so a same-endpoint retry cannot converge. Rotation fires only for that pre-submission bail, never for submission timeouts (which may have landed onchain). Completion is confirmed against freshgetAccountInfoonchain state, not the CLI's error text.wait_for_seat_allocation_ackedstep and itsWaitForSeatAllocationAckedhelper are removed (from e2e/qa: retry seat withdraw while in-flight rejection persists instead of polling pending flag #4065): polling the seat's pending flag cannot distinguish a fast ack from a re-fund of an already-active seat that never creates a request, and the retrying withdraw checks the request state onchain directly. Tradeoff: a genuine oracle outage now surfaces asvalidate_tunnel_uptiming out rather than a specific "not acked" error.Context:
shreds payon an already-active seat only tops up the escrow and never creates a newInstantSeatAllocationRequest, so the pending flag is never set, the oracle never acks, and the multicast tunnel can't come up — every subsequent run was poisoned. Reference failing runs: 29554004596 (first, in-flight bail), 29580032054 (steady state,saw_pending=false), 29588245744 (re-stuck immediately after a manual unstick — no pool failover was logged, confirming the CLI preflight and the harness read shared one stale primary endpoint).Scope note: no changes to the pay/withdraw CLI or the QA workflow.
Testing Verification
withdraw_seatabsorption: dispatch 29590898632 hit the in-flight bail on the primary endpoint (logged, redacted), rotated, and the cleanup withdraw succeeded on attempt 2 via the fallback — leaving the seat clean onchain instead of poisoning the next run.TestFilterActiveSeats(active-seat-by-IP selection: our-IP active kept, tenure-0 skipped, other-IP skipped, multi-device all-kept),TestIsSeatNotFound(sentinel match — asserts a transient "Blockhash not found" is not treated as a missing seat), andTestIsInFlightPreflightBail(matches the in-flight bail forms; asserts a submission timeout does not trigger endpoint rotation).TenureEpochs == 0/ account gone viagetAccountInfo, which stays fresh even when the endpoint'sgetMultipleAccountsis stale) rather than pattern-matching the external CLI's error text, so a transient RPC failure can't be misread as "already withdrawn" and silently leave a seat stuck.qa.mainnet-beta#6566) — fully green, the first green run since the incident began. The self-heal detected and withdrew the seat left stuck by the immediately preceding scheduled failure (Self-healed stuck-active seat(s) count=1: attempt 1 bailed in-flight on the stale primary, rotated, succeeded on attempt 3). The absorbedwithdraw_seatstep then converged after 5 attempts (~63s) across both endpoints on a different device's fresh seat — alternating genuine in-flight windows and stale reads that a single-shot withdraw (or fixed-endpoint retry) would have failed.Review follow-ups (non-blocking)
The blocking finding (over-broad error-text matching) is fixed via onchain verification. Deferred Low/nit items:
FetchAllClientSeatsis a discriminator-onlygetProgramAccountsscan (fine at QA scale; add aClientIPBitsmemcmp filter in the SDK if it grows); seats are selected by IP only per the approved plan (filtering additionally by funding authority would harden against a same-IP seat funded by a different wallet);SetPublicIPvalidates non-nil but not IPv4-only (pre-existing).