fix(autoclaim): don't let a permanently disabled source pin the l2-to-lx block cursor

[arnaubennassar]

🔄 Changes Summary

• Fix: a source network permanently excluded from bridge service resolution no longer pins the L2ToLx detector's shared L1 block-window cursor, which previously stopped all L2-origin claim discovery.

autoclaim/bridgedetector/l2_to_lx.go keeps one block-window cursor (l2-to-lx) shared by every source network. processSource returned sourceRetryLater for any fetcher.GetURL error, and PollOnce then held that shared cursor just before the earliest skipped source's verify row — or, when that row sat at the very start of the window, return result, nil without saving the cursor at all, re-scanning the identical window on the next poll.

bridgeservicefinder.GetURL returns ErrNetworkDisabled for every network listed in Config.IgnoreNetworkIDs. That error is a deliberate, permanent exclusion — it can never clear. So a single ignored network with a VerifyBatches row anywhere in the scanned range wedged the shared cursor forever, and no L2-origin claim request could ever be created for any source.

Observed in a deployment whose ignore list covers most registered rollups: over 8 hours the only skip source line emitted named an ignored network, and the request store contained L1-origin requests exclusively — not one L2-origin request.

The fix: a permanently excluded source is sourceUpToDate ("nothing to do") instead of sourceRetryLater. It no longer contributes to retryBlock and no longer holds the window back. Its own (source, destination) LER cursors are still left untouched, so when the network is later un-ignored, its next verify row is fetched from the old pair cursor and covers everything bridged in between. That recovery does depend on the source publishing another verify row: the shared window has already moved past the row seen while the source was excluded and only rewinds by overlapBlocks (default 1), so it never returns to it. To un-ignore a source that has stopped producing verified batches, reset the l2-to-lx block-window cursor so its old verify row falls inside the scanned range again. The trade is deliberate — the old behaviour bought that re-observability at the cost of deadlocking every source. A genuinely retryable GetURL failure (service not yet announced, or unhealthy) keeps its existing hold-the-window behaviour, unchanged.

Two smaller points:

• The permanence distinction is expressed in the fetcher contract rather than read out of bridgeservicefinder by the detector. ClaimCandidatesFetcher gains a detector-local ErrSourceDisabled, the permanent counterpart of ErrURLNotFound; ServiceFetcher.GetURL — the designated translation point — maps the finder's disabled error to it, checked before the not-found branch that error wraps, and still wrapping ErrURLNotFound since an excluded source is also a source with no URL. processSource branches on the local sentinel and no longer imports bridgeservicefinder, and both sentinels are documented on the GetURL contract: the whole bug was a permanence distinction the contract never expressed. A test pins the mapping, so a future refactor cannot silently reintroduce the deadlock, and any non-finder-backed fetcher can now express the same thing.
• An excluded source is now reported once per source rather than on every poll. The exclusion never clears and the detector polls for the lifetime of the process, so the repetition carried no information.

The doc comments on sourceOutcome, PollOnce and processSource said "transient" where the code accepted any error; they are updated to match the new semantics.

⚠️ Breaking Changes

• None. No config, API, CLI or storage change.

📋 Config Updates

• None.

🔌 API Updates

• None.

✅ Testing

• 🤖 Automatic: 6 new tests in autoclaim/bridgedetector:
  • TestL2ToLxDisabledSourceDoesNotHoldBlockCursor — an excluded source does not hold the cursor back; the window advances past its verify row.
  • TestL2ToLxTransientURLErrorStillHoldsBlockCursor — a retryable URL miss still holds the cursor (guards against regressing the existing behaviour).
  • TestL2ToLxDisabledSourceDoesNotStarveHealthySource — mixed window: one excluded source plus one healthy source at a later block; the healthy source processes and the cursor advances.
  • TestL2ToLxDisabledSourceAtWindowStartDoesNotPinCursor — the production regression: an excluded source's verify row at the very start of the window must not take the "return without saving the cursor" path.
  • TestL2ToLxDisabledSourceIsReportedOncePerSource — the report is emitted once, not per poll.
  • TestServiceFetcherGetURL/network_disabled_maps_to_the_permanent_sentinel — pins that the finder's disabled error arrives at the detector as ErrSourceDisabled, never as a plain retryable ErrURLNotFound, including the branch ordering that makes it so.
• Confirmed to be genuine regression tests. With the test files in place and l2_to_lx.go reverted to its pre-fix state, 4 of the 5 behavioural tests fail:

  --- FAIL: TestL2ToLxDisabledSourceDoesNotHoldBlockCursor        (cursor 0x13 = 19, want 0x31 = 49)
  --- FAIL: TestL2ToLxDisabledSourceDoesNotStarveHealthySource    (cursor 0x13 = 19, want 0x31 = 49)
  --- FAIL: TestL2ToLxDisabledSourceAtWindowStartDoesNotPinCursor (CursorAdvanced: should be true)
  --- FAIL: TestL2ToLxDisabledSourceIsReportedOncePerSource       (mock: Unexpected Method Call)
  --- PASS: TestL2ToLxTransientURLErrorStillHoldsBlockCursor      (behaviour intentionally unchanged)
  --- PASS: TestServiceFetcherGetURL                              (pins existing behaviour by design)
  

  All pass with the fix applied, alongside the pre-existing retry tests (TestL2ToLxNotSyncedSkipsSourceWithoutAdvancingCursor, TestL2ToLxRetriesSkippedSourceOnLaterPollWithoutNewLER, TestL2ToLxRetrySkipAtWindowStartKeepsStoredCursor).
• go test ./autoclaim/... ./bridgeservicefinder/... — all packages ok.
• golangci-lint run --timeout 5m — 0 issues.
• 🖱️ Manual: none required.

🐞 Issues

• No tracking issue.

🔗 Related PRs

• fix(autoclaim): EthTxManager defaults and start-block defaulting #1875 also touches autoclaim/bridgedetector/l2_to_lx.go (and its test file) in the same processSource GetURL error branch, so the two will conflict textually on merge. The changes are logically independent: fix(autoclaim): EthTxManager defaults and start-block defaulting #1875 only lowers the log level of the skip line, this PR changes the control flow so the shared cursor is not held. This PR is based directly on develop and contains nothing from fix(autoclaim): EthTxManager defaults and start-block defaulting #1875.

📝 Notes

• Design decision: the excluded source's per-pair LER cursors are deliberately not advanced. The block window is only the trigger for re-examining a source; the per-(source, destination) LER cursor is what bounds the actual candidate fetch. Leaving it untouched means that if the operator later un-ignores the network, its next verify row is fetched from the old pair cursor and still covers every bridge exit made while it was excluded — even though the block window has long since moved past. The converse is the operational caveat above: a re-enabled source that never publishes another verify row is never re-examined, because the window does not rewind to the row it already passed. Resetting the block-window cursor is the recovery for that case.
• The once-per-source reporting is a plain map[uint32]struct{} on the detector, touched only from the single poll goroutine — deliberately not a general log-dedup mechanism.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DJuYNKE9jRLRrCWTrrEJ2T

[arnaubennassar]

@claude

[claude]

Claude finished @arnaubennassar's task in 2m 45s —— View job

---

Review: PR #1882 — disabled source no longer pins the l2-to-lx block cursor

• Gather context (diff, related files)
• Analyze the fix in l2_to_lx.go
• Review tests
• Post review + summary template

---

Verdict

The diagnosis is correct and the fix is minimal and targeted. bridgeservicefinder.ErrNetworkDisabled (bridgeservicefinder/interfaces.go:44) is built by wrapping ErrURLNotFound, so ServiceFetcher.GetURL (autoclaim/bridgedetector/l2_to_lx_fetcher.go:58) funnels it into the same detector-local ErrURLNotFound, and processSource previously returned sourceRetryLater for it. With a single shared retryBlock in PollOnce (l2_to_lx.go:403-443) and retryBlock <= fromBlock short-circuiting to return result, nil, a permanently ignored network with a verify row in the window really does wedge every source forever. Verified by reading the code, not just the PR body.

The decision to leave the per-(source, destination) LER cursors untouched is the right one, and the regression tests are genuine (the reported pre-fix failures match what the code paths would do).

I could not run go test / golangci-lint here — the sandbox denied go build and go test. Findings below are from static reading only.

---

🟡 Suggested

1. SkippedSourceCount's doc comment wasn't updated with the others — autoclaim/bridgedetector/l2_to_lx.go:199-202

The PR carefully updated the doc comments on sourceOutcome, PollOnce and processSource, but this one still reads:

SkippedSourceCount is the number of sources skipped this poll: nothing new for any pair, or at least one pair's fetch group must be retried (finder miss or not synced yet). A source whose groups partially succeeded counts as skipped, since its block-window position is held back.

A permanently excluded source now lands here (via sourceUpToDate at l2_to_lx.go:421) and is neither "nothing new for any pair" — NewLERSourceCount was already incremented at line 500 — nor held back. It's the one remaining place that still describes the old semantics.

Fix this →

2. The "re-enabling loses nothing" guarantee holds only if the source publishes another verify row

The PR body says removing the network from the ignore list later "loses nothing". That's true for the LER cursor, but a source only enters latestBySource if it has a VerifyBatches row inside the current block window (l2_to_lx.go:390-396), and nextFromBlock's overlap (l2_to_lx.go:853-861) is a small fixed number of blocks, not a rewind. So after the fix:

• the window advances past the excluded source's verify row and never comes back to it;
• if the network is later un-ignored (a config change, so a restart) but has stopped producing verified batches, it is never re-examined and its pending exits are never discovered.

That's exactly the shape of network most likely to be on an ignore list — a halted or broken rollup. The old buggy behaviour accidentally preserved re-observability at the cost of the deadlock; this trade is clearly the right one, but it is a real trade and the guarantee as worded is stronger than the code delivers. Worth either narrowing the claim in the comment at l2_to_lx.go:511-513 or adding the operational note (to un-ignore a dormant network, reset the l2-to-lx block cursor so its old verify row is re-scanned).

3. The detector core now reaches into bridgeservicefinder for a sentinel the fetcher abstraction was built to hide — l2_to_lx.go:9, l2_to_lx.go:504

ClaimCandidatesFetcher exists so "unit tests can mock the remote bridge service" and so the detector talks in detector-local errors; ServiceFetcher.GetURL is the designated mapping point. After this change the detector branches on a finder-package sentinel that only leaks through by accident of ErrNetworkDisabled wrapping ErrURLNotFound — which is why the new fetcher test is needed to pin it. A detector-local ErrSourceDisabled (or ErrURLPermanentlyUnavailable), mapped in ServiceFetcher.GetURL alongside the existing ErrURLNotFound mapping, would keep the permanence signal inside the interface contract and let any non-finder-backed fetcher express it.

Relatedly, the interface doc at l2_to_lx.go:97-99 still documents only ErrURLNotFound → "skip this source this round", with no mention that some URL failures are now permanent and treated differently. Since the whole bug was a permanence distinction the contract didn't express, that contract is the natural place to write it down.

Fix this →

---

🟢 Optional

4. The once-per-source log matcher is loose — l2_to_lx_test.go:~490

logger.EXPECT().Infof(mock.Anything, mock.Anything).Once() matches any Infof with exactly one variadic arg, so the test would also be satisfied by an unrelated future one-arg log line. mock.MatchedBy(func(f string) bool { return strings.Contains(f, "is disabled") }) for the format, plus asserting the source ID, would pin what it means to pin.

5. logDisabledSourceOnce's nil-map guard is unreachable — l2_to_lx.go:879-881

NewL2ToLx always initialises the map and options only mutate an already-constructed detector, so the lazy init never fires. Harmless, but it slightly obscures that the map has exactly one owner. Worth noting too that the "only ever touched from the single poll goroutine" invariant (l2_to_lx.go:233-234) is doc-only — PollOnce is exported, and Start happens to be its only production caller. Fine as-is given the current call graph; just don't let the map grow into something a second caller would touch.

6. NewLERSourceCount keeps incrementing for an excluded source on every poll — l2_to_lx.go:500

Its pair cursor never advances, so it re-counts as "has new LERs" forever. Nothing consumes this field outside tests today, so it's cosmetic — but if it ever becomes a metric, an ignored network will be a permanent non-zero floor.

---

Security / performance

No security implications: no new external input, no new network call, no change to what is claimed or by whom — the only behavioural delta is which block cursor value gets persisted. Performance strictly improves: the pathological case this fixes was re-scanning an identical block window on every poll indefinitely.

Test coverage

Good. The five behavioural tests cover the cursor advance, the preserved retryable-hold behaviour (the important companion — easy to over-fix this and lose it), the mixed healthy/excluded window, the window-start regression that was actually observed in production, and the log-once behaviour. TestServiceFetcherGetURL/network_disabled_sentinel_survives_the_not-found_wrapping is a well-placed guard given how fragile the double-%w path is; note that finding 3, if applied, would make that guard structural rather than incidental.

---

🚀 What's New

A source network permanently excluded from bridge service resolution (bridgeservicefinder.Config.IgnoreNetworkIDs) is now treated as "nothing to do" rather than "retry later" by the L2ToLx bridge detector, so it no longer holds back the block-window cursor that all source networks share. Excluded sources are reported once per source instead of once per poll, and doc comments on sourceOutcome, PollOnce and processSource are updated to describe retryable-vs-permanent skips.

🐛 Bug Fixes

• A single ignored network with a VerifyBatches row in the scanned L1 range permanently pinned the shared l2-to-lx block-window cursor, stopping all L2-origin claim discovery for every source network. Observed in a deployment with a large ignore list: over 8 hours, zero L2-origin claim requests were created. Genuinely retryable URL failures (service not yet announced, or unhealthy) keep their existing hold-the-window behaviour.

📋 Config Updates

None.

⚠️ Breaking Changes

None. No config, API, CLI or storage change.

---

• fix/l2-to-lx-disabled-source-cursor-deadlock

[arnaubennassar]

Automated review addressed in 66b8848. The fix's behaviour is unchanged — a permanently excluded source still lets the shared block-window cursor advance, and a genuinely retryable GetURL failure still holds it just before its verify row (TestL2ToLxTransientURLErrorStillHoldsBlockCursor still passes).

Applied:

1. SkippedSourceCount doc — it claimed every skipped source has its block-window position held back; a permanently excluded source is counted there and does not. Reworded to name all three skip reasons and say which one holds the window.
2. "re-enabling loses nothing" — narrowed in the PR body and in the code comment. overlapBlocks defaults to 1, so the window rewinds a single block and never returns to a row it has passed: a source that is un-ignored but has stopped producing verified batches is never re-examined. The trade is still the right one, so the behaviour stands; the recovery (reset the l2-to-lx block cursor) is now written down where an operator will find it.
3. Detector-local sentinel — agreed, and the most useful item. Added ErrSourceDisabled, mapped in ServiceFetcher.GetURL before the not-found branch it wraps and still wrapping ErrURLNotFound; processSource branches on the local sentinel and l2_to_lx.go no longer imports bridgeservicefinder. Both sentinels, and what the detector does with each, are now on the ClaimCandidatesFetcher.GetURL contract — the permanence distinction the contract failed to express is what the bug was. The fetcher test is kept and rewritten to pin the mapping and its branch ordering, so it guards structure rather than an accident.
4. Loose log matcher — tightened to the format text plus the source id. Verified it bites: flipping the matched substring makes the test fail.
5. Unreachable nil-map guard — removed.

Declined:

6. NewLERSourceCount for an excluded source — the increment does happen before the disabled branch, so the observation is correct, but the field is defined as "at least one pair whose cursor differs from the source's newest LER". That is true for an excluded source, and equally true for the retryable-skip path the review does not object to — a transiently skipped source re-counts on every poll too. Moving the increment would make the two skip paths disagree about the same field, for a counter nothing consumes outside tests. Left as is, and recorded in the commit message.

One correction: the review's framing of the fetcher test as "incidental" was right as a description of the old code but the double-%w survival was never the mechanism the detector should have relied on — finding 3 is the actual fix for that, not a polish on top of it.

go build ./... clean, go test ./autoclaim/... ./bridgeservicefinder/... all ok, golangci-lint run --timeout 5m → 0 issues.

[krlosMata]

😸