Skip to content

eth: cancel milestone subscriber on shutdown#2199

Merged
kamuikatsurgi merged 1 commit intodevelopfrom
fix/milestone-subscriber-shutdown
Apr 27, 2026
Merged

eth: cancel milestone subscriber on shutdown#2199
kamuikatsurgi merged 1 commit intodevelopfrom
fix/milestone-subscriber-shutdown

Conversation

@kamuikatsurgi
Copy link
Copy Markdown
Member

Description

The Heimdall milestone WS subscriber in startMilestoneWhitelistService was launched with context.Background(), so it kept running after close(s.closeCh) and outlived s.chainDb.Close().

Logs:

INFO  Persisting dirty state head=86,085,215 ...                      ← chainDb about to close
INFO  Whitelisting milestone deferred err="chain out of sync"
ERROR error handling milestone ws event err="chain out of sync"
ERROR Failed to store the lock field struct err="pebble: closed"
ERROR Error in writing lock data of milestone to db err="...: pebble: closed for lock field struct"
ERROR Failed to store the future milestone field struct err="pebble: closed"
ERROR connection lost; will attempt to reconnect on heimdall ws subscription error="websocket: close 1006"
INFO  successfully connected on heimdall ws subscription url=ws://localhost:26657/websocket

After the fix, the subscriber returns as soon as closeCh closes, and none of the above appear post-shutdown.

Copilot AI review requested due to automatic review settings April 27, 2026 12:09
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@sonarqubecloud
Copy link
Copy Markdown

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

Fixes the Heimdall milestone WebSocket subscriber lifecycle so it shuts down cleanly when the Ethereum service stops, preventing post-shutdown DB writes and noisy reconnect/error logs.

Changes:

  • Create a cancellable context for milestone WS subscriptions and cancel it when s.closeCh closes.
  • Pass the cancellable context into subscribeAndHandleMilestone to ensure it exits on shutdown.
  • Update doc/comments around the milestone whitelist service and retry behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread eth/backend.go
@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

❌ Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.28%. Comparing base (1a5510f) to head (0459203).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
eth/backend.go 12.50% 7 Missing ⚠️

❌ Your patch status has failed because the patch coverage (12.50%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2199      +/-   ##
===========================================
+ Coverage    52.24%   52.28%   +0.04%     
===========================================
  Files          884      884              
  Lines       155551   155557       +6     
===========================================
+ Hits         81262    81329      +67     
+ Misses       69055    68995      -60     
+ Partials      5234     5233       -1     
Files with missing lines Coverage Δ
eth/backend.go 52.10% <12.50%> (-0.43%) ⬇️

... and 22 files with indirect coverage changes

Files with missing lines Coverage Δ
eth/backend.go 52.10% <12.50%> (-0.43%) ⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@manav2401 manav2401 mentioned this pull request Apr 27, 2026
@kamuikatsurgi kamuikatsurgi merged commit d9a6231 into develop Apr 27, 2026
26 of 27 checks passed
@kamuikatsurgi kamuikatsurgi deleted the fix/milestone-subscriber-shutdown branch April 27, 2026 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants