fix(bridgeservice): correct l1-info-tree-index fallback and cross-syncer error codes

[arnaubennassar]

🔄 Changes Summary

• Fixes the INC-129 root cause. The fallback in getFirstL1InfoTreeIndexForL1Bridge took root.Index from bridgeL1.GetLastRoot — a position in the L1 bridge exit tree, i.e. a deposit count (~1,146,035) — and passed it to l1InfoTree.GetInfoByIndex, which queries SELECT * FROM l1info_leaf WHERE position = $1, an L1 info tree index. Two different trees, two different counters, both bare uint32. The row could not exist, so /bridge/v1/l1-info-tree-index returned HTTP 500 sql: no rows in result set whenever the L1 bridge syncer trailed the L1 info tree syncer.
• The fallback now calls GetLatestL1InfoLeafUntilBlock(ctx, root.BlockNum), clamping to the most recent info-tree leaf at or before the last block the bridge syncer indexed — the clamp the original code intended, in the correct index space. root.BlockNum == 0 is guarded. The tip-anchored binary search, the GetRootByLER join, and ErrNotOnL1Info are all retained; the search's upperLimit/bestResult inherit the clamp from the reassigned lastInfo without a second assignment.
• Cross-syncer error taxonomy. Endpoints that join syncers which can sit at different heights now distinguish "not indexed yet" from a genuine fault, via a single shared httpStatusForSyncerError helper: ErrBlockNotProcessed, l1infotreesync.ErrNotFound, ErrNoBlock0, db.ErrNotFound, ErrNotOnL1Info → 404; sync.ErrInconsistentState → 503; anything else → 500.
• ClaimProofHandler metrics fix. statusCode was initialised to 200 and never reassigned, so all 13 exit paths reported 200 to reportMetrics — every failure was invisible in metrics. All exits now set it correctly.
• GetInfoByIndex not-found was unwrapped. getInfoByIndexWithTx skipped db.ReturnErrNotFound, returning a raw sql: no rows in result set. That was the literal 500 body in INC-129, and it meant errors.Is(err, db.ErrNotFound) was dead code at three call sites. One-line wrap added.
• Log levels. Not-ready conditions now log at Debug (404) / Warn (503) instead of Error, so the INC-129 alert volume does not simply migrate from HTTP 500s into ERROR logs.
• Same bug fixed in autoclaim (closes fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795). autoclaim/proof/preparer.go carried a verbatim copy of the index-space confusion in its own GetRootByLER fallback. It now clamps on root.BlockNum too. After this, grep -rn "GetInfoByIndex(ctx, .*\.Index)" over non-test Go returns zero hits — that was the last one.

⚠️ Breaking Changes

• 🔌 API/CLI: HTTP 500 → 404 / 503 on three endpoints. /bridge/v1/l1-info-tree-index, /bridge/v1/claim-proof, and the L1 path of /bridge/v1/injected-l1-info-leaf now return 404 when a syncer has not indexed the requested data yet (callers should treat it as not ready yet, retry later) and 503 when a syncer is halted / in an inconsistent state. Previously all of these were 500.
  • /injected-l1-info-leaf's L2 path already returned 404; only its L1 path changes.
  • bridgeservice/client: GetL1InfoTreeIndex and GetClaimProof switch from doRequest to doRequestAllowNotFound, so they now return the ErrNotFound sentinel on 404 instead of a generic error. Signatures and return types are unchanged, but callers that treated any error as fatal may now want to retry on ErrNotFound. A 503 deliberately stays a plain non-ErrNotFound error so a halted syncer is not mistaken for "not indexed yet".
• No success-path response body or schema changed anywhere.

📋 Config Updates

• None.

✅ Testing

• 🤖 Automatic:
  • make lint → 0 issues.
  • make test-unit → exit 0 (whole repo).
  • New regression test TestGetFirstL1InfoTreeIndexForL1Bridge_KinexysIndexSpaceSkew reproduces the exact skew: GetRootByLER cannot resolve the tip leaf's MER, and GetLastRoot returns Index = 1_146_035 (a deposit count) with BlockNum = 8_412. The fixtures keep the two ~136× apart on purpose — and assert that skew — because every pre-existing fixture set Index == BlockNum == depositCount, which is precisely the coincidence that let an index-space confusion hide for so long.
  • Red/green proof: reverting only the one fallback line back to GetInfoByIndex(ctx, root.Index) makes both subtests fail on the unexpected mock call for GetLatestL1InfoLeafUntilBlock; restoring it byte-for-byte returns them to green.
  • The four existing fallback subtests were rewritten, not deleted. One of them previously pinned the bug by asserting the error contained "failed to get last info for L1"; it now asserts ErrorIs(l1infotreesync.ErrNotFound), a 404 mapping, and NotContains that old message. A new subtest covers the root.BlockNum == 0 guard.
  • TestHTTPStatusForSyncerError (14 rows — every sentinel bare and wrapped) and TestRespondSyncerError cover the mapping directly, including db.ErrNotFound and l1infotreesync.ErrNotFound as separate 404 cases, since the new fallback returns the latter while its siblings return the former.
  • Per-endpoint status tables (10/12/24 rows). ClaimProofHandler's exits are asserted through the Prometheus counter, and every failure row also asserts the 200 counter did not move — pinning the metrics bug shut.
  • Client tests strengthened: TestGetL1InfoTreeIndex's weak require.Error became require.ErrorIs(err, ErrNotFound); TestGetClaimProof gained its first 404 subtest; both gained a 503 subtest.
  • autoclaim (fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795): TestFirstL1InfoTreeIndexForL1BridgeIndexSpaceSkew reproduces the same skew against the proof preparer (GetLastRoot().Index = 1_146_035 vs BlockNum = 8_412, skew asserted), plus tests for the BlockNum == 0 guard and the clamp-lookup error wrap. Its probed block (6_200, the midpoint of [4_000, 8_400]) doubles as proof the clamp actually tightened upperLimit — unclamped, the search would probe 6_250 and miss the fixture. Proven red by reverting the one line: the test fails with l1infotreesync has no L1 info tree leaf at or before L1 block 8412.
• 🖱️ Manual: not reproduced against a live environment. The kinexys condition is reproduced deterministically in the regression test above rather than by hand.

🐞 Issues

• Fixes INC-129 (kinexys /l1-info-tree-index HTTP 500, 89 alerts).
• Closes fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795 (the same index-space bug duplicated in autoclaim/proof/preparer.go).

🔗 Related PRs

• None.

📝 Notes

Scope widening — l1infotreesync/processor.go. The diff intentionally reaches one line outside bridgeservice/: getInfoByIndexWithTx now wraps its return in db.ReturnErrNotFound(...). Without it, the handlers' errors.Is(err, db.ErrNotFound) checks are dead code and the endpoint keeps returning a raw sql: no rows in result set. All 9 non-test callers were audited; the decisive evidence that this is the right layer is that three autoclaim call sites already work around the asymmetry by checking errors.Is(db.ErrNotFound) || errors.Is(sql.ErrNoRows). translateError was deliberately not added at the syncer wrapper, so the sentinel stays db.ErrNotFound, matching every sibling method.

Incidental swagger drift (pre-existing, not new). Regenerating the swagger artefacts also picks up /l1-info-tree-leaf-by-ger and /root-by-ler entries plus the types.RootByLERResponse schema. These were already annotated in bridge.go on develop but had never been regenerated into the committed docs. They are not new functionality introduced here — verified with git show origin/develop:bridgeservice/bridge.go.

The autoclaim copy of the bug is now fixed here too (#1795). autoclaim/proof/preparer.go had a verbatim copy — a bridge-exit-tree root.Index from GetLastRoot fed into GetInfoByIndex, with the same "failed to get last info for L1" wrapper. It was first flagged with a TODO, then fixed in this PR. Two details worth a reviewer's eye:

• The root.BlockNum == 0 guard reuses the existing bridgeservice.ErrNotOnL1Info rather than introducing a sentinel. Its only caller, selectL1InfoTreeIndex, already treats that sentinel as retry next cycle (Result{Ready: false}), which is the correct reading of "bridgesync L1 has indexed nothing yet" — a transient startup state, not a fault. Letting l1infotreesync.ErrNoBlock0 escape instead would leak an l1infotreesync-internal sentinel into autoclaim's error contract.
• The GetLastRoot fallback in autoclaim/proof had zero test coverage — no pre-existing test ever set lastRoot or rootErrs on the fake. The copied bug came with a copied coverage gap, which is why it survived. preparer.go:111 retains a legitimate GetInfoByIndex call whose argument is already in info-tree space; that one is correct and should not be "fixed".

Deliberate non-changes. The deposit-anchored rewrite (GetBridgeByDepositCount + a (block_num, block_pos) > query) is the better long-term design and is not attempted here — this fix is deliberately minimal and in place. getFirstL1InfoTreeIndexForL2Bridge is untouched beyond error mapping, and the syncers are not merged. L1InfoTreeLeafByGERHandler already matched the taxonomy and was left alone. One pre-existing inconsistency remains: L1InfoTreeIndexForBridgeHandler's nil-syncer path still answers 500 while the same condition answers 503 in ClaimProofHandler; fixing it cleanly needs a new sentinel, so it is left as-is.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WvzbGpjp6VeiW6ccThguGC

---

🔍 Review follow-up

Applied from the automated review:

• Pessimistic statusCode default (suggestion 2). L1InfoTreeIndexForBridgeHandler, InjectedL1InfoLeafHandler, and ClaimProofHandler now initialise statusCode to 500 and set 200 explicitly on the success path. Worth noting the suggestion as literally written would have introduced a bug: each of these handlers' success responses used c.JSON(statusCode, ...) relying on the http.StatusOK initialiser, so flipping the default alone would have returned HTTP 500 on success. Setting 200 explicitly is what makes the pessimistic default safe.

• Autoclaim duplicate bug (suggestion 3). Filed as fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795, and then fixed in this PR rather than deferred — see the Notes section. The TODO comment is gone; the audit the issue asked for is clean.

• DB-level coverage of the clamp (integration-coverage suggestion). Added TestGetLatestL1InfoLeafUntilBlockClamp in l1infotreesync/processor_test.go. The pre-existing table test for that method only asserted the three error sentinels (ErrNoBlock0, ErrBlockNotProcessed, ErrNotFound), so the actual "newest leaf at or before blockNum" semantics the fallback now depends on were never exercised against real rows. This was the one genuinely untested link, and it is the link a mocked syncer cannot cover: a mock returns whatever it is told, so it can never catch an index-space error — it only proves the handler passes root.BlockNum to the clamp, not that the query answers correctly.

  The new test inserts leaves at L1 blocks 1_000 / 5_000 / 9_000 (indexes 0 / 1 / 2 — kept orders of magnitude apart on purpose, same discipline as the handler fixtures) and asserts index, block, and leaf.BlockNumber <= requested across eight cases: exact-boundary, above-all, between-leaves, ±1 around a leaf, and below-all → ErrNotFound. Verified non-vacuous by mutation: flipping ORDER BY … DESC to ASC and block_num <= $1 to >= $1 each make it fail (source restored, md5 verified).

Declined, with reasons:

• Nil-syncer taxonomy unification (suggestion 1) — the review itself notes this may be intentional, and it is: a nil syncer is a configuration fault, not a syncer state, and routing it through respondSyncerError would require a new sentinel. Left as documented under Deliberate non-changes.

• GetLatestL1InfoLeafUntilBlock pointer parameter (suggestion 4) — the premise is incorrect. The *uint64 is the pre-existing processor signature, and its nil path is exercised: l1infotreesync/l1infotreesync.go:301 passes nil for the unbounded "latest leaf" variant, so making it non-pointer would break that caller. The public wrapper this PR consumes already takes a plain uint64, which is exactly what the L1InfoTreeSyncer interface addition mirrors.

• Test constant renaming (suggestion 5) — marked optional by the review, and the proposed names (e.g. fallbackL1Block_DepositCountBelowBlock) use underscores in mixedCaps identifiers, which Go style and this repo's linters discourage.

• A full HTTP → handler → syncer → database wired test (the rest of the integration-coverage suggestion). Two of its three bullets were already satisfied before the suggestion was made: the handler status tests go through a real gin router via performRequest and assert w.Code and w.Body, and assertHandlerStatus reads the actual Prometheus counter before and after each request, asserting both that the expected status was reported to reportMetrics and that the 200 counter did not move. So "verify the full HTTP response (status + body + metrics)" and "test the reportMetrics calls with the correct status codes" are covered. The mocks are also already at the syncer-interface seam — there is no higher seam short of a real database.

  What remained was standing up real l1infotreesync + bridgesync instances with populated sqlite behind a live BridgeService. That is a substantial bespoke harness, and it duplicates what the repo's kurtosis/docker e2e suites already do end-to-end (they are excluded from make test-unit by -short). The targeted DB-level test above closes the actual coverage hole at a fraction of the cost and maintenance surface. Happy to add the wired harness if reviewers want it, but it did not look like the right trade here.

[arnaubennassar]

@claude

[arnaubennassar]

@claude

[claude]

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

---

Code Review Complete ✅

Reviewing PR #1794: fix(bridgeservice): correct l1-info-tree-index fallback and cross-syncer error codes

Todo List

• Read and understand the core index-space bug fix
• Review bridgeservice changes (bridge.go, error handling)
• Review autoclaim duplicate bug fix
• Review test coverage and regression tests
• Review client changes and breaking changes
• Check for security/performance issues
• Provide comprehensive feedback with priority levels
• Fill out summary template

---

🚀 What's New

This PR fixes the INC-129 root cause — a critical index-space confusion bug where the L1 bridge exit tree deposit count (~1.1M) was incorrectly passed to L1 info tree index queries, causing HTTP 500 errors. The fix introduces proper index space clamping via GetLatestL1InfoLeafUntilBlock and establishes a consistent cross-syncer error taxonomy (404 for "not indexed yet", 503 for inconsistent state, 500 for genuine faults).

🐛 Bug Fixes

• Index-space confusion (INC-129): Fixed getFirstL1InfoTreeIndexForL1Bridge fallback that incorrectly used root.Index (deposit count in L1 bridge exit tree) as an L1 info tree index. Now clamps by root.BlockNum using GetLatestL1InfoLeafUntilBlock.
• Autoclaim duplicate bug (fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795): Fixed identical index-space confusion in autoclaim/proof/preparer.go.
• GetInfoByIndex unwrapping: Added missing db.ReturnErrNotFound wrapper in l1infotreesync/processor.go:getInfoByIndexWithTx so errors.Is(err, db.ErrNotFound) checks work correctly.
• ClaimProofHandler metrics: Fixed statusCode initialized to 200 and never reassigned, making all 13 failure exit paths invisible in metrics.

📋 Config Updates

None.

⚠️ Breaking Changes

API/CLI Changes

HTTP status code changes on three endpoints (500 → 404/503):

• /bridge/v1/l1-info-tree-index now returns 404 when syncers haven't indexed requested data (vs. 500 previously)
• /bridge/v1/claim-proof now returns 404/503 for syncer-related errors (vs. 500 previously)
• /bridge/v1/injected-l1-info-leaf (L1 path only) now returns 404/503 (L2 path already returned 404)

Client Library Changes

bridgeservice/client:

• GetL1InfoTreeIndex and GetClaimProof now use doRequestAllowNotFound, returning ErrNotFound sentinel on 404
• Callers treating any error as fatal should update to retry on ErrNotFound
• 503 deliberately stays a plain error (not ErrNotFound) to distinguish halted syncer from "not indexed yet"

---

🔍 Detailed Review

🟢 Strengths

1. Excellent root cause analysis: The PR description clearly explains the index-space confusion and why it's a bug (two different trees, two different counters, both uint32).

2. Comprehensive test coverage:

   • TestGetFirstL1InfoTreeIndexForL1Bridge_KinexysIndexSpaceSkew reproduces the exact 136× skew condition
   • Red/green proof included (reverting the fix breaks tests)
   • All 4 existing fallback subtests rewritten (not deleted) to test the correct behavior
   • Autoclaim tests added for the duplicate bug

3. Systematic error handling: The new httpStatusForSyncerError helper provides a single source of truth for error-to-status mapping, reducing future inconsistencies.

4. Minimal scope: The fix is surgical — it changes only what's necessary to fix the bug, avoiding over-engineering.

5. Proper sentinel handling: Both db.ErrNotFound and l1infotreesync.ErrNotFound are handled correctly in the error taxonomy.

🟡 Suggestions

1. 🟡 Consider adding integration test coverage

File: bridgeservice/bridge_test.go
Concern: While unit tests are comprehensive, there's no integration test that exercises the full handler → business logic → syncer → database path for the 404/503 status codes.

Suggestion: Consider adding integration tests that:

• Mock the syncers at a higher level
• Verify the full HTTP response (status + body + metrics) for each error scenario
• Test the reportMetrics calls with the correct status codes

This would provide stronger guarantees that the metrics fix actually works end-to-end.

2. 🟡 Log level consistency documentation

File: bridgeservice/bridge.go (lines 1799-1806)
Observation: The PR changes log levels (404 → Debug, 503 → Warn, 500 → Error), which is good for reducing alert noise, but this logging strategy isn't documented.

Suggestion: Consider adding a comment above respondSyncerError explaining the logging philosophy:

// respondSyncerError classifies err with httpStatusForSyncerError, logs it at a level matching the
// resulting status, and writes the error response. Log levels:
//   - Debug (404): transient "not ready yet" condition, expected during normal operation
//   - Warn (503): syncer halted/inconsistent, requires operator attention but may self-heal
//   - Error (500): genuine fault requiring investigation

3. 🟡 BlockNum == 0 guard placement

Files:

• bridgeservice/bridge.go:1463-1465
• autoclaim/proof/preparer.go:232-234

Observation: The root.BlockNum == 0 guard appears in both places with slightly different error messages. The bridgeservice version wraps ErrNotOnL1Info, while the autoclaim version reuses bridgeservice.ErrNotOnL1Info.

Suggestion: This is already correct (reusing the sentinel is good), but consider extracting the guard into a helper function if this pattern appears in a third location:

func validateLastRoot(root *treetypes.Root) error {
    if root.BlockNum == 0 {
        return fmt.Errorf("bridgesync L1 has not indexed any block yet: %w", ErrNotOnL1Info)
    }
    return nil
}

This is a minor optimization and not required for this PR.

4. 🟡 Test constant naming (optional, per review follow-up)

File: bridgeservice/bridge_test.go
Observation: Test constants use names like fallbackL1Block, fallbackBlockNumHighIndex, etc. The automated review suggested renaming to fallbackL1Block_DepositCountBelowBlock, but you correctly declined this because underscores in mixedCaps identifiers violate Go style.

Alternative suggestion (optional): If you want to improve clarity without violating Go style, consider:

const (
    fallbackL1BlockDepositCountBelowBlock = 8_000  // camelCase, no underscores
    fallbackBlockNumForHighIndex = 8_200
    fallbackBlockNumForLowIndex = 8_000
)

This is purely cosmetic and not required.

🔴 Critical Issues

None identified. The code is well-written, thoroughly tested, and the fix correctly addresses the root cause.

🔒 Security Review

No security issues found. The changes are primarily error handling and index lookups — no new attack surface introduced.

⚡ Performance Review

No performance concerns. The new GetLatestL1InfoLeafUntilBlock call in the fallback path is a single database query with an index on block_num, so it's efficient. The fallback only triggers when GetRootByLER fails, which is already an error path.

---

📊 Test Coverage Analysis

Excellent Coverage

1. Index-space bug regression tests:

   • TestGetFirstL1InfoTreeIndexForL1Bridge_KinexysIndexSpaceSkew (bridgeservice)
   • TestFirstL1InfoTreeIndexForL1BridgeIndexSpaceSkew (autoclaim)
   • Both tests reproduce the 136× skew and assert correct clamping

2. Error taxonomy tests:

   • TestHTTPStatusForSyncerError (14 rows, all sentinels bare + wrapped)
   • TestRespondSyncerError (covers status + body + logging)
   • Per-endpoint status tables (10/12/24 rows)

3. Metrics tests:

   • ClaimProofHandler tests assert Prometheus counters
   • Verify failure paths didn't increment 200 counter

4. Client tests:

   • TestGetL1InfoTreeIndex strengthened (require.ErrorIs(err, ErrNotFound))
   • TestGetClaimProof gained 404 + 503 subtests

Minor Gap

The GetLatestL1InfoLeafUntilBlock error wrap case (lines 1470-1473 in bridge.go, lines 238-242 in preparer.go) has indirect coverage through the "GetLatestL1InfoLeafUntilBlock fails" subtest, but could benefit from an explicit test that:

1. GetRootByLER fails
2. GetLastRoot succeeds with non-zero BlockNum
3. GetLatestL1InfoLeafUntilBlock returns an error
4. The error is properly wrapped and returns 404

This is a minor gap and not blocking, but would strengthen test coverage.

---

📝 Summary

This is a high-quality PR that fixes a critical production bug (INC-129) with:

• ✅ Clear root cause analysis
• ✅ Minimal, surgical fix
• ✅ Comprehensive test coverage (including red/green proof)
• ✅ Consistent error handling taxonomy
• ✅ Proper breaking change documentation
• ✅ No security or performance concerns

Recommendation: Approve ✅

The suggested improvements above are minor enhancements and not blockers. The PR is ready to merge as-is.

---

🔗 Related Links

• Fixes INC-129 (kinexys /l1-info-tree-index HTTP 500)
• Closes fix(autoclaim): proof preparer reuses the INC-129 index-space bug (deposit count used as L1 info tree index) #1795 (autoclaim duplicate bug)

---