Skip to content

fix: avoid spurious error log when block range is too large in l2gersync#1607

Merged
joanestebanr merged 5 commits intorelease/0.10from
fix/1604-spurious-ger-error-log
Apr 29, 2026
Merged

fix: avoid spurious error log when block range is too large in l2gersync#1607
joanestebanr merged 5 commits intorelease/0.10from
fix/1604-spurious-ger-error-log

Conversation

@joanestebanr
Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • Move log.Errorf from internal fetch functions (fetchInjectedGERs, fetchRemovedGERs) to their callers (GetInjectedGERsForRange, GetRemovedGERsForRange), so the error is only logged when it is a real failure — not when it is a recoverable max-block-range condition that is transparently handled by splitting into chunks.

⚠️ Breaking Changes

  • None

✅ Testing

  • 🤖 Automatic: existing unit tests pass

🐞 Issues

📝 Notes

  • Root cause: fetchRemovedGERs/fetchInjectedGERs logged Errorf unconditionally on any filter error, but their callers detect isMaxRangeErr and retry transparently with ChunkedRangeQuery. The log fired before the caller had a chance to handle the error gracefully, producing a confusing ERROR in the logs that was not actually an error.

Move log.Errorf to callers (GetInjectedGERsForRange / GetRemovedGERsForRange)
so it only fires for real errors, not for max-range errors that are
transparently handled by splitting into chunks.

Closes #1604

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

Claude finished @joanestebanr's task in 1m 27s —— View job


PR Review Complete ✅


🚀 What's New

  • Improved error logging behavior in l2gersync to avoid spurious error logs when block range queries exceed the RPC node's maximum range limit. Error logging now occurs only at the caller level after determining if the error is truly unrecoverable.

🐛 Bug Fixes

  • Fixed spurious error logs in l2gersync: Moved log.Errorf statements from fetchInjectedGERs and fetchRemovedGERs to their respective callers (GetInjectedGERsForRange, GetRemovedGERsForRange). This prevents logging errors for recoverable max-block-range conditions that are transparently handled by automatic chunking.

📋 Config Updates

None.

⚠️ Breaking Changes

None.


Code Review

🟢 Summary

This is a clean, well-targeted fix that addresses a specific UX issue where legitimate error recovery was producing confusing ERROR-level log messages. The change is minimal, well-tested, and follows good error handling practices.

✅ What Works Well

  1. Correct Problem Identification - The root cause analysis is spot-on. The internal fetch* functions were logging errors before callers had a chance to handle recoverable conditions.

  2. Minimal, Surgical Change - Only 4 lines changed (2 moved up, 2 moved down). This follows the principle of making the smallest change necessary to fix the issue.

  3. Preserved Error Handling Logic - The fix doesn't change any error handling behavior, only where logging occurs. The transparent chunking retry mechanism remains intact.

  4. Excellent Test Coverage - The existing tests at l2gersync/l2_evm_ger_reader_test.go:122-147 and l2gersync/l2_evm_ger_reader_test.go:191-211 specifically test the chunking behavior when block ranges are too large, ensuring this fix is well-covered:

    • Tests verify that "block range too large" errors trigger transparent chunking
    • Tests verify that non-parseable errors are properly propagated
    • Tests verify that real errors still produce error logs (through the caller's error path)
  5. Consistent Pattern - Both GetInjectedGERsForRange and GetRemovedGERsForRange are updated symmetrically, maintaining consistency across the codebase.

Code Analysis

Before the fix (l2gersync/l2_evm_ger_reader.go:100-101, 195-196):

// In fetchInjectedGERs and fetchRemovedGERs
if err != nil {
    log.Errorf("failed to create ... event iterator: %v", err)  // Logged EVERY error
    return nil, err
}

After the fix (l2gersync/l2_evm_ger_reader.go:83-84, 179-180):

// In GetInjectedGERsForRange and GetRemovedGERsForRange (callers)
if err != nil {
    maxRange, isMaxRangeErr := aggkitcommon.ParseMaxRangeFromError(err.Error())
    if isMaxRangeErr {
        // Handle transparently with chunking - no error log needed
        return aggkitcommon.ChunkedRangeQuery(...)
    }
    log.Errorf("failed to create ... event iterator: %v", err)  // Only log REAL errors
    return nil, err
}

This is the correct approach because:

  • Max-range errors are expected conditions that the code handles automatically
  • Only true failures (RPC errors, network issues, etc.) should produce ERROR logs
  • Users/operators won't see confusing logs during normal operation

🟢 No Issues Found

I reviewed the code for:

  • Security: No security implications - this is purely a logging change
  • Performance: No performance impact - same execution paths
  • Bugs: No bugs introduced - error handling logic is preserved
  • Best Practices: Follows Go error handling best practices
  • Code Style: Matches repository conventions (see CLAUDE.md)

Test Coverage Verification

The existing test suite thoroughly covers this scenario:

  • TestL2EVMGERReader_GetInjectedGERsForRange/block range too large triggers chunking (lines 122-147)
  • TestL2EVMGERReader_GetRemovedGERsForRange/block range too large triggers chunking (lines 191-211, 385-410)
  • TestL2EVMGERReader_GetInjectedGERsForRange/non-parseable error returns original error (lines 149-168)
  • TestL2EVMGERReader_GetRemovedGERsForRange/non-parseable error returns original error (lines 213-227, 412-434)

These tests ensure that:

  • ✅ Chunking is triggered correctly for max-range errors
  • ✅ Real errors are still propagated and logged
  • ✅ The transparent retry mechanism works as expected

Recommendation

✅ APPROVE - This PR is ready to merge. The fix is:

  • Correct and well-reasoned
  • Minimal in scope
  • Well-tested with existing coverage
  • Has no breaking changes or side effects
  • Improves user experience by reducing log noise

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c8b8ca508

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread l2gersync/l2_evm_ger_reader.go Outdated
Comment thread l2gersync/l2_evm_ger_reader.go Outdated
joanestebanr and others added 2 commits April 29, 2026 11:49
Capture the result of ChunkedRangeQuery and emit log.Errorf when a
chunk fails, restoring error visibility for real failures during the
chunked retry path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts L2 GER syncing log behavior so “max block range” filter errors (which are handled by chunking) don’t emit confusing ERROR logs before the fallback retry path runs.

Changes:

  • Moved log.Errorf out of fetchInjectedGERs/fetchRemovedGERs into GetInjectedGERsForRange/GetRemovedGERsForRange.
  • Kept the max-range detection + ChunkedRangeQuery fallback behavior unchanged.

Comment thread l2gersync/l2_evm_ger_reader.go Outdated
Comment thread l2gersync/l2_evm_ger_reader.go Outdated
joanestebanr and others added 2 commits April 29, 2026 11:56
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@joanestebanr joanestebanr enabled auto-merge (squash) April 29, 2026 09:57
@sonarqubecloud
Copy link
Copy Markdown

@joanestebanr joanestebanr merged commit 25e5ad7 into release/0.10 Apr 29, 2026
24 of 25 checks passed
@joanestebanr joanestebanr deleted the fix/1604-spurious-ger-error-log branch April 29, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants