Skip to content

Fix send on closed channel panic in epoch handler#3529

Merged
cthulhu-rider merged 1 commit intomasterfrom
3508-sn-panic-in-epoch-handler-on-exit
Aug 21, 2025
Merged

Fix send on closed channel panic in epoch handler#3529
cthulhu-rider merged 1 commit intomasterfrom
3508-sn-panic-in-epoch-handler-on-exit

Conversation

@End-rey
Copy link
Contributor

@End-rey End-rey commented Aug 14, 2025

Closes #3508.

Is this enough or is it necessary to fix the problem in the node? It's just not very clear if there is a problem in the system.

@codecov
Copy link

codecov bot commented Aug 14, 2025

Codecov Report

❌ Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.87%. Comparing base (0a90006) to head (ff7977f).
⚠️ Report is 54 commits behind head on master.

Files with missing lines Patch % Lines
pkg/local_object_storage/shard/gc.go 63.63% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
+ Coverage   23.93%   24.87%   +0.93%     
==========================================
  Files         669      675       +6     
  Lines       50251    50127     -124     
==========================================
+ Hits        12028    12467     +439     
+ Misses      37257    36667     -590     
- Partials      966      993      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

As a patch i think that is ok, but i feel like this will not be a problem at all after #3522 and at the same time fixing wrong channel usage with recovery not the best we can do, imo.

@End-rey End-rey force-pushed the 3508-sn-panic-in-epoch-handler-on-exit branch from 87396af to 2faaa0c Compare August 14, 2025 09:32
@cthulhu-rider
Copy link
Contributor

indeed, #3522 gonna replace these channels. But it is still better to have a fix. Besides, it is not difficult to make

proposed solution just hides the cause to me. The current implementation is unsafe: the channel is written and closed concurrently. The easiest solution i see is dropping channel closure as not required. Alternatively, manage channels by engine and pass them to Shard read-only

@End-rey End-rey force-pushed the 3508-sn-panic-in-epoch-handler-on-exit branch from 2faaa0c to bbee211 Compare August 20, 2025 19:56
@End-rey End-rey force-pushed the 3508-sn-panic-in-epoch-handler-on-exit branch from bbee211 to ff7977f Compare August 21, 2025 11:33
@End-rey End-rey requested a review from carpawell August 21, 2025 11:33
Stop via stopChannel and keep eventChan open.

Closes #3508.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
@cthulhu-rider cthulhu-rider merged commit 0a7447b into master Aug 21, 2025
22 checks passed
@cthulhu-rider cthulhu-rider deleted the 3508-sn-panic-in-epoch-handler-on-exit branch August 21, 2025 13:23
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.

SN panic in epoch handler on exit

3 participants