indexer: break the permission-events throttle livelock

[bgm-malbeclabs]

Summary

• A rate-limited getTransaction aborted the entire permission-events refresh cycle. Committed chunks stayed durable, but the run was recorded as an error and the drain advanced only ~one chunk per ~27min cycle — once a backlog appeared it could never catch up. Prod sat 3h26m behind and widening for 4.5 hours.
• A transient decode failure with committed progress is now treated as a budget stop rather than a cycle failure — the same semantic drainAccount already used for its deadline check. Zero progress still returns an error, so a persistently throttled account escalates instead of no-op succeeding.
• getTransaction is now paced view-wide. The existing semaphore capped in-flight calls but not request rate, which is what a per-method provider limit actually measures — 10 concurrent calls against a fast endpoint is an unbounded rate, so the drain kept earning Too many requests for a specific RPC call on every cycle.

Why the cycle could never catch up

cycle 16:09 → frontier 16:06   (3m behind)
cycle 20:39 → frontier 17:13   (3h26m behind)

Each cycle committed its first chunk, hit a 429, and discarded the rest — advancing the frontier ~5 minutes per 27-minute cycle.

The graceful stop alone would have converted a loud livelock into a silent one (success-with-pending, frontier still falling behind), which is why the pacing change is in the same PR — they fix the same failure together.

Note for review

defaultFetchesPerSecond = 25 is a calibration knob, not a derived constant — no provider publishes a per-method number and it differs per endpoint and plan. It is sized to drain a 200-signature chunk in ~8s so the ~4.5min usable window fits ~30 chunks. It is behind --permission-events-fetches-per-second so it can be retuned without a rebuild. This is the value most worth a second opinion.

Testing Verification

• New test reproduces the production failure exactly (RPCError{Code: 429}, the same shape RPCPool returns) and fails on the pre-change code with the identical production error message; passes after.
• ThrottledChunkStopsWithProgressAndResumes — asserts the first chunk stays committed, the cursor sits at the chunk boundary, SourceMaxEventTS reports the honest committed frontier rather than now (this is what feeds the ingest-staleness alert), and the next cycle drains the remainder with no signature skipped or double-counted.
• ThrottledFirstChunkStillErrors — pins the guard: a throttle before any commit must still surface an error, so suppression can't hide a persistently throttled account.
• PacesTransactionFetches — asserts the drain cannot finish faster than the configured rate allows.
• go test -race green across dz/serviceability/..., dzingest, indexer.

Relationship to #747

This is the permission-events instance of the follow-up #747 deliberately deferred:

Durable partial progress — sub-batch inserts that advance the watermark for what was actually written, so a failed cycle stops discarding 5+ minutes of completed Flux work.

Same shape, different view. The part worth reading before the telemetry-usage version gets written is the guard, not the happy path: start > 0 && dberror.IsTransient(decodeErr) stops a cycle discarding committed chunks, but zero progress still returns an error, so a genuinely stuck account escalates instead of quietly succeeding forever. Getting only the first half would convert a loud livelock into a silent one.

Note that telemetry-usage is harder for the reason #747 gives: its baseline cache must only advance once rows are durably written, and its deltas depend on ordered whole-window processing. Permission events has no equivalent coupling — chunks are independent and the cursor is explicit — which is why it was tractable here first.

Deploy note

No outstanding gap. Permission events caught up on its own once the upstream RPC pressure eased, before this shipped:

20:12Z  ERR  (10th consecutive failure — the one that paged)
21:06Z  ERR
21:34Z  refresh completed in 7.9s
22:32Z  refresh completed in 0.3s   <- nothing left to drain

So this is not a recovery deploy. It ships so the next throttle costs minutes of freshness instead of hours, and so a failed cycle stops discarding work it already committed.

After deploy, the thing to verify is whether the 429s stop. If they continue, --permission-events-fetches-per-second is the dial — the approach doesn't change. (Since this PR was opened, the provider confirmed these are per-method and per-IP caps over a 10s rolling window on their edge, and every 429 carries a Retry-After; honoring that header is tracked separately in malbeclabs/doublezero#4161 and is the more durable fix than any pacing value.)

[bgm-malbeclabs]

Prod recovered on its own — no longer an urgent merge

The upstream throttle lifted between 21:06Z and 21:34Z. One successful cycle cleared the entire ~4h backlog in 8 seconds:

21:06  error    frontier 17:23   (260m behind)
21:34  success  frontier 21:34   (9m behind — normal cycle cadence)

Lake Indexer: Ingest Data Stale has cleared. This can go through normal review.

What the recovery confirms, and one correction to the PR description above

The 8-second catch-up is a sharper confirmation of the mechanism than the failure logs were — but it corrects the framing. I described the frontier advancing ~5min per 27min cycle as a throughput problem. It wasn't. The actual work was trivial the whole time: every cycle did a few seconds of it, hit the 429, and discarded the rest. The backlog was only ever large in wall-clock, because nothing could finish.

That doesn't change either half of the fix, but it sharpens why each is here:

• The graceful stop is the real fix. It's what makes a cycle bank its committed chunks instead of throwing them away, so a throttle costs a few minutes of freshness rather than hours.
• The rate limiter is not about draining faster — it's about not tripping the per-method limit at all. The 8-second catch-up shows this workload was never close to needing 10-way concurrency, which makes lowering the effective request rate nearly free.

On the defaultFetchesPerSecond = 25 question I flagged for review: these numbers argue it is generous rather than risky. Steady state is 0–1 rows per cycle in 0–1 seconds, and the worst backlog we have observed drained in 8. If reviewers want to be conservative here, conservative means lower, and there is plenty of headroom to go lower without risking the drain keeping up.

Worth watching separately

A provider rate limit that stays engaged for ~4.5 hours straight is a long time for a rate limit. If this recurs on a similar cadence, the interesting question stops being "how do we survive it" and becomes "what changed upstream at 16:09Z" — plausibly a plan or quota change on the ledger endpoint rather than anything on our side. This PR makes the symptom survivable either way; it does not explain the trigger.

[bgm-malbeclabs]

Correction: the retry claim in this PR's description is wrong

The description says the SDK does retry the 429 (isRetryableHTTPStatus(429) → true) and merely exhausts a too-short budget. That is not true of the SDK version lake is running. I read the retry classifier from a newer version sitting in my module cache, not the one pinned in go.mod.

On the pinned SDK (v0.8.1-0.20260724204114-f7981ab6f7a3), the 429 is not retryable by either path:

• Classification uses errors.As against interface{ Code() int } / interface{ StatusCode() int }. jsonrpc.RPCError exposes Code as a struct field, not a method, so neither assertion matches. The newer SDK names this as a known bug that "made every Go ledger reader give up after one attempt on RPCPool's 503s."
• The message fallback lists "rate limited", not "too many requests".

So each cycle got exactly one attempt and aborted — which is why those runs died in 8–14 seconds, nowhere near any budget. #757 bumps the SDK and fixes that.

What this does and does not change here

Does not change: both halves of this PR are still correct and still wanted.

• The graceful stop is unaffected. Whatever the retry behavior, a cycle must bank its committed chunks rather than discard them when a fetch ultimately fails.
• The pacing is unaffected, and arguably matters more now: with retries actually working after deps: bump doublezero SDK to get working RPC retry classification #757, not tripping the limit in the first place is what keeps the retry budget available for real blips.

Does change: the mechanism of why the throttle was fatal. Not "3s of linear backoff was too short for a sustained rate limit" but "no retry occurred at all." The defaultFetchesPerSecond sizing rationale is unaffected — it was derived from chunk size and the drain window, not from retry behavior.

Merge order: #757 should land first, or alongside. It is the fix closest to the cause; this PR is the resilience layer that keeps a future throttle from costing hours of freshness even when retries do run out.

Sorry for the churn on the description — I should have read the pinned version rather than whatever was in the module cache.

[nikw9944]

Two notes, both minor.

Prod already recovered on its own. The deploy note says permission events won't recover until this ships, but the drain caught up once the RPC pressure eased:

20:12Z  ERR  (10th consecutive failure — this is the one that paged)
21:06Z  ERR
21:34Z  refresh completed in 7.9s
22:32Z  refresh completed in 0.3s   <- nothing left to drain

Still worth shipping, since this will happen again — but there's no outstanding gap waiting on the deploy.

This is the same fix that #740 deliberately left for later. #747 listed "advance the watermark for whatever was actually written, so a failed cycle stops throwing away completed work" as an explicit follow-up. This PR does that for permission events, including the part that's easy to get wrong: zero progress still returns an error, so a genuinely stuck account still escalates instead of quietly succeeding. Worth reading before the telemetry-usage version gets written.

After deploy, the thing to check is whether the 429s actually stop. If they don't, the fetches-per-second value is what to adjust, not the approach.

[bgm-malbeclabs]

Both fair — thanks. Description updated.

Deploy note corrected. You're right and I'd left the description stale: I posted the recovery in a comment but never fixed the note itself, so the PR still claimed prod was waiting on this deploy. It now says there's no outstanding gap, with your timeline, and states plainly that this ships so the next throttle costs minutes rather than hours — not as a recovery.

#747 cross-reference added, and it checks out precisely against that PR's deferred follow-up #1:

Durable partial progress — sub-batch inserts that advance the watermark for what was actually written, so a failed cycle stops discarding 5+ minutes of completed Flux work.

I've called out the guard explicitly for whoever writes the telemetry-usage version, since it's the half that's easy to miss: start > 0 && dberror.IsTransient(decodeErr) banks committed chunks, but zero progress still errors. Taking only the first half converts a loud livelock into a silent one — the cycle reports success-with-pending while the frontier falls behind, and the only thing left to notice is the staleness alert.

Worth flagging that telemetry-usage is genuinely harder for the reason #747 already identified: its baseline cache must only advance once rows are durably written, and its deltas depend on ordered whole-window processing. Permission events has no equivalent coupling — independent chunks, explicit cursor — which is why it was tractable here first. I'd not assume this pattern ports over cheaply.

On your post-deploy check — agreed, and there's now a better answer than tuning the dial. The provider confirmed after this PR was opened that these are per-method and overall per-IP caps, both over a 10s rolling window on their edge, and that every rate-limit 429 carries a Retry-After (currently 10, but some of their limits use 30s windows). So:

• The pinned SDK couldn't retry these at all — dead classification, fixed in deps: bump doublezero SDK to get working RPC retry classification #757.
• Even with deps: bump doublezero SDK to get working RPC retry classification #757, its backoff is ~3s across 4 attempts, which sits entirely inside a 10s penalty window. Honoring Retry-After is the durable fix; tracked in solana/rpc: record endpoint rate-limit headroom per method doublezero#4161.
• defaultFetchesPerSecond = 25 remains a calibration guess. Measured unpaced peak is ~43 rps (10 concurrent × ~230ms round-trip), which does trip the cap, so the ceiling is below that. They're reviewing raising our getTransaction headroom, which may make the value moot.

Also relevant to the per-IP half: the whole EKS cluster egresses through a single NAT (single_nat_gateway = true), so that limit is shared with every other service in the cluster, not just this job.

[nikw9944]

Re-checked against the current head — cb06934, unchanged since the review was posted. No new commits, no force-push, and no replies on any of the three line threads. All three findings are verbatim-intact at the same line numbers; CI is green and the merge block is this review.

The author's one new comment updates the PR description and answers my earlier issue-level notes, but does not respond to any code finding. Two things in it bear on the review, and both were already folded into my severities:

• Budget margin → Medium (from High). The reported ~230ms round-trip and ~43 rps unpaced peak confirm the limiter rather than concurrency is binding at 25/s, so the mechanism holds. But the full 4h backlog drained in 7.9s — corroborated by the reviewer's own log excerpt, not only the author's — which is roughly 340 signatures total. Triggering this needs four accounts each holding a full 200-signature chunk, which the observed workload never approaches. Latent defect, not an active one.
• Transient swallow → Low (from High), reframed. My original claim that dropping the ERROR line leaves nothing to page was too strong: Lake Indexer: Ingest Data Stale demonstrably fired and cleared during the incident, the partial path reports the frontier honestly (test-pinned), and CLAUDE.md explicitly places served-stale/degraded fallbacks at WARN. The author's "zero progress still errors" defense is correct as far as it goes and I'm not disputing it. What survives is only the nil-block-time hole, where a partial cycle emits no freshness value at all and the one remaining alert path has nothing to read.
• Secondary-network wiring → Medium, unchanged. The new per-IP-cap detail — shared cluster egress through a single NAT — makes per-environment tuning more important, not less. This is a two-line omission that defeats the stated purpose of the flag this PR adds, and it is what the verdict rests on.

[bgm-malbeclabs]

All three fixed in 32d92e7. Thanks — the budget-margin one in particular I would not have found.

Budget margin — fixed as suggested, derived rather than fixed. The chain you traced is right, including the part that makes it nasty: rate.Limiter.Wait refuses a reservation that would outlast the deadline rather than blocking, its error says "would exceed context deadline", dberror classifies that transient on the substring, and so it lands on the stop-with-progress path and reports success having committed nothing. I'd introduced a silent failure mode by pacing work whose deadline check didn't know about pacing.

chunkBudget(n) now returns n × maxConcurrentFetches / fetchesPerSecond + drainCommitReserve, computed per chunk so a short final chunk isn't charged for a full one. I split the old constant: drainCommitReserve (30s, insert + checkpoint) is the fixed part, and drainBudgetMargin stays a fixed 30s for the pagination phase — that's getSignaturesForAddress, which the limiter doesn't pace, so a constant is still right there.

One consequence worth naming: at a very low configured rate the requirement can exceed the whole activity window, and then the zero-progress branch reports budget exhausted before first chunk every cycle. I left that loud on purpose — a rate that can't move one chunk per window is a misconfiguration, and silently draining nothing is how this class of bug hides.

Frontier hole — fixed, and it's broader than the nil-block-time case. Chunk tip now comes from the newest dated signature in the chunk rather than only the last element, so the fallback covers everything except a chunk where nothing is dated.

But while verifying I found the wider version of your point, and it's worse than either of us framed it: no alert reads source_max_event_ts at all. lake-indexer-ingest-stale keys on max(finished_at) WHERE status='success'. So the honest-frontier reporting I defended is recorded and consumed by nothing, and any partial-success cycle resets the staleness clock regardless of how far behind the frontier is.

Scoping that fairly: partial-success-with-pending is pre-existing — the budget-stop path already did return res, nil with pending before this PR. I added a second route to that semantic, not the semantic. So I've fixed the hole you identified and I'm not expanding this PR to cover the rest, but the real fix is either an ingestion-log status that distinguishes partial from success, or pointing the alert at the frontier. Happy to file it; say which shape you'd prefer. (Related: malbeclabs/infra#2204 — that alert had never fired in its entire existence for an unrelated reason, so this gap has been fully masked until now.)

Secondary-network wiring — fixed. Straightforwardly correct; the flag reached exactly one assignment. Secondaries now inherit the primary value via cmp.Or, with a per-env PERMISSION_EVENTS_FETCHES_PER_SECOND_<ENV> override following the existing DZ_LEDGER_RPC_URL_<ENV> convention. Your point about per-env mattering more given shared egress is right and is why I added the override rather than only inheriting.

Tests added for the first two: ChunkBudgetScalesWithFetchRate pins that a slower rate reserves more (the assertion that fails on a fixed constant) and that a short chunk costs less; PartialCycleAlwaysReportsAFrontier builds a chunk whose newest signature is undated, throttles into the next chunk, and asserts a frontier is still reported. go test -race green across dz/serviceability/... and indexer; lint clean.

[bgm-malbeclabs]

Folded the partial status in as ab9ca79, closing the wider version of the frontier finding rather than leaving it as a follow-up.

What it does. RefreshResult gains Partial; a run that completes without error but knowingly left work undone is recorded as status="partial". An error still outranks it, so a failure never loses its cause. Permission events sets it from totalPending > 0, which covers both stop paths — the deadline budget stop and the transient stop this PR added.

Why status and not the frontier. The alert asks max(finished_at) WHERE status='success'. Reporting an honest source_max_event_ts doesn't help because nothing reads that column — so the only way a partial cycle stops resetting the staleness clock is for it to stop claiming success.

Blast radius. Four places filter status='success':

consumer	affected
api/handlers/multicast_delivery.go:498	No — SyncIPMroute/SyncMSDP only, snapshot-replace, no partial mode
lake-indexer-rollup-stale	No — rollup activities have no partial mode
lake-indexer-ingest-stale	Yes — the intent
lake-ops.json (4 refs)	Possibly cosmetic; a status breakdown gains a series

So the one thing that changes is the thing we wanted to change. Worth a glance at those dashboard panels when this lands, but nothing there pages.

Accepted consequence. A legitimate multi-cycle drain now trips the 15-minute staleness alert. I think that's correct rather than noisy: during such a drain the data genuinely is behind, which is what the alert exists to say, and it self-clears on convergence. It should also be rare here — the entire 4-hour backlog from the incident drained in 7.9s once unblocked, so sustained partial states aren't a normal mode for this workload. If it does turn out noisy, the lever is the alert's 15-minute threshold rather than this classification.

Still worth doing later, deliberately, and not here: point staleness at the frontier instead of run status —

dateDiff('minute', max(coalesce(source_max_event_ts, finished_at)), now())

That asks "is the data recent" rather than "did a run succeed recently", which is the question that stops needing new status values every time a view grows a partial mode. It needs care because source_max_event_ts means different things across views (snapshot views set it to fetchedAt, some don't set it at all), and it belongs with the alert rewrite in malbeclabs/infra#2204 rather than bolted on here.

Tests: writer_test.go is new and pins the three-way status mapping including error-outranks-partial; the throttle test now asserts res.Partial is true on the stopped cycle and false once it converges. go test -race green across ingestionlog, dz/serviceability/..., dzingest, indexer; lint clean.

[bgm-malbeclabs]

Filed the frontier-based alerting follow-up as malbeclabs/infra#2217, so the partial status here is explicitly the interim fix rather than the end state. It records the lake-side prerequisite too — source_max_event_ts means different things per view (snapshot views set it to fetchedAt, permission events now sets a real frontier, some views don't set it at all), so that needs auditing before an alert can key on it meaningfully.

[nikw9944]

Commit ab9ca79 adds a distinct partial ingestion-log status wired through RefreshResult.Partial, which closes the staleness-clock finding at its root rather than deferring it — verified and dropped, along with the three earlier findings, which remain fixed at this head. What is left is the chunk-budget reserve, untouched across both fix commits and unmentioned in either writeup: chunkBudget still applies the ×10 contention factor unconditionally, and pagination still reserves a flat 30s for a phase whose job is to leave room for a 110s chunk. Agreed that pointing staleness at the frontier belongs in infra#2217 rather than here, and that the per-view meaning of source_max_event_ts needs auditing before an alert can key on it. The multi-cycle-drain alert trade reads as correct to me as well; I could not verify the three out-of-repo status='success' consumers, so that enumeration is the load-bearing part of the blast-radius argument.

Findings not anchored to the current diff:

• indexer/pkg/dz/serviceability/permissionevents/view.go:657 — medium: Signature pagination can legitimately run the clock down to a point where no chunk can ever start, so the drain pages through everything and then hard-errors having indexed nothing. This check reserves drainBudgetMargin (30s), but the chunk loop at view.go:548 then demands chunkBudget(200) — 110s at the default rate — so an account whose pagination consumes the window down to that reserve takes the start == 0 error branch every cycle. The same unconditional × maxConcurrentFetches factor also means a configured rate below roughly 7.4/s makes the requirement exceed the whole 300s activity window, with nothing validating that at startup. Reserve chunkBudget(scanChunkSize) during pagination, and scale the factor to the number of accounts actually draining.
• indexer/pkg/dz/serviceability/permissionevents/view.go:334 — low: The comment explaining how a permanently stalled drain stays visible now describes behaviour this commit removed. It still says each cycle "reports success", but result.Partial at view.go:319 means a stopped-short cycle is recorded as partial in the ingestion log. Point it at the new status rather than the metric.

[nikw9944]

Commit 548cc2b changes only the flag help string, so all three open findings are unaddressed at this head — verified against the current code rather than inferred from the diff size. Adding one item on that new text: the ~43/s ceiling it documents understates the constraint, since maxConcurrentFetches is 10 against the client's 9-connection per-host cap, so a fetch queues on the pool at every configured rate rather than only above 43/s. Holding at comment — nothing open is blocking, every failure mode here is loud rather than silent, and the reserve mismatch at view.go:548 remains the one I would fix before merge. Worth noting the flag now carries a documented ceiling and a documented floor with no validation at either end, which is the same gap pointing in both directions.

[bgm-malbeclabs]

Pushed 6398e60 (rebased on main, so #757's pool sizing is in the diff's context now). All four open findings addressed, one of them differently from the suggestion — details below.

The two budget findings had one cause. chunkBudget multiplied by maxConcurrentFetches unconditionally. A 200-signature chunk at 25/s claimed 110s where it needs 8s alone. That term produced both symptoms you named: the 30s-to-110s band where a cycle pages everything and then refuses to start, and the ~7.4/s floor that puts the whole activity window out of reach with nothing validating it.

So the factor is gone. chunkBudget(n) = n/rate + drainCommitReserve, uncontended.

What the worst case bought was avoiding one chunk of wasted fetches on overrun. That is no longer worth its price. An overrun is not silent any more: the limiter refuses the reservation, the chunk's rows are dropped uncommitted, and the cycle records partial (with progress) or errors (with none). Contention also varies during a chunk, so no static factor is right — the uncontended cost is at least an honest lower bound with a visible failure, and it is one number instead of a guess.

Pagination now reserves what the chunk loop demands. The reserve is chunkBudget(min(len(allSigs), scanChunkSize)), and the check moved to after the first page. Both phases compute the same function over the same count, so the band closes. Putting it after the first page also means an account with nothing new costs one request and succeeds however little deadline is left, which a pre-check at the full chunk requirement would have broken. drainBudgetMargin is deleted.

On rejecting a bad rate at startup — I did not do that, and I think it is the wrong place. Two reasons:

1. Rejecting on the window needs the activity timeout, which the view does not know. Restating 300s inside the view puts the same number in two layers, and the copy would be wrong the first time anyone tunes the timeout.
2. Removing the ×10 factor moves the unusable floor from 7.4/s to ~0.74/s. The plausible typo is 2.5 for 25, which now drains a full chunk per cycle. The value that trips 0.74/s is 200 transactions in five minutes.

What I did instead: both budget-exhaustion messages name the configured rate, so the branch where a bad number lands says so. Your point stands that a starved backlog and a bad rate read identically — that was true, and the fix is the message, not a pre-flight check:

refresh budget exhausted before first chunk (200 signatures pending, 32s needed at 100 fetches/second)
refresh budget exhausted during signature pagination (200 signatures paged, 32s needed to drain a chunk of them at 100 fetches/second)

Validate also now rejects a negative rate, which was accepted and is not a slower drain but a limiter that refuses every reservation.

Flag help. You are right that the number pointed at the wrong constraint, and it is now wrong in the other direction too: #757 sizes that client to permissionevents.MaxConcurrentRPCRequests (2 × maxConcurrentFetches = 20), which is this view's own peak — concurrent pagination plus in-flight getTransaction. The pool is not the constraint at any rate, so the sentence is gone rather than corrected. The text now gives the floor instead.

Stale comment points at status="partial" rather than "each cycle reports success".

Test. TestLake_PermissionEvents_View_PaginationReservesWhatTheChunkLoopDemands runs at 100/s (chunk needs 32s) with a deadline 31s out — inside the old band — and pins that pagination is the phase that refuses. Against the reconstructed flat 30s reserve it fails with exactly the symptom you described:

Error "refresh permission accounts: drain account 9onLAj...: refresh budget exhausted
before first chunk (200 signatures pending, 32s needed at 100 fetches/second)"
does not contain "pagination"

Your note about the other two budget tests is why the band was invisible: at testFetchesPerSecond = 1e6 the two thresholds collapse to the same value. The band's width is also what the second half of the test spends waiting on the real limiter, which is why it is 2s wide and not 80s.

golangci-lint 0 issues. go test -race ./indexer/pkg/dz/serviceability/... ./indexer/pkg/dzingest/... ./indexer/pkg/ingestionlog/... green.

[nikw9944]

Rebased onto current main and pushed 6398e60; all four open findings are addressed in code, verified independently rather than taken from the writeup. The pagination and chunk phases now call the same chunkBudget over the same count with drainBudgetMargin deleted, and the new test lands a deadline inside the old band and pins that pagination is the phase that refuses. Your argument against startup validation is right and I am not pressing it — restating the 300s activity timeout inside the view is exactly the cross-layer constant duplication that #747 was written to stop, and naming the configured rate in both exhaustion messages solves the diagnosis problem that finding was actually about. One nit left on the other direction of the same change: dropping the contention factor makes the reserve an uncontended estimate, which is right for the N≈1 workload but under-reserves once several accounts drain together.