fix(archiver): pass context through rearchiveRange for cancellation#61
Open
faisalnugroho wants to merge 1 commit into
Open
fix(archiver): pass context through rearchiveRange for cancellation#61faisalnugroho wants to merge 1 commit into
faisalnugroho wants to merge 1 commit into
Conversation
rearchiveRange was using context.Background() for both the retry loop and the persistBlobsForBlockToS3 call. This meant: - Shutdown is not respected: in-progress rearchive continues running after the archiver is stopped, holding references to the beacon client and storage. - HTTP timeout is not respected: the /rearchive handler has a 60s middleware timeout, but the underlying goroutine keeps running after the response is aborted. Now accepts a context parameter and passes it through to both the retry loop and the persist call. The HTTP handler passes r.Context() so cancellation propagates correctly from both client disconnects and server shutdown. Fixes base#58 (Bug 1)
Collaborator
🟡 Heimdall Review Status
|
Author
|
Hi! Following up on this bug fix PR. This PR passes the caller's context through rearchiveRange so that cancellation signals propagate correctly. Without this, calling Stop() during a rearchive operation leaves goroutines running with context.Background(). Happy to make adjustments. All CI checks pass (StepSecurity ✅). Would love to get your review when you have a moment. Thanks! |
Author
|
Hi! I'd love to get this PR reviewed when you have a moment. This PR addresses pass context through rearchiveRange for cancellation. Happy to make any adjustments based on your feedback. Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Passes a
context.ContextthroughrearchiveRangeso that cancellation propagates from both the HTTP handler and service shutdown.The problem
rearchiveRangehardcodescontext.Background()for both theretry.Docall andpersistBlobsForBlockToS3:This causes two issues:
Shutdown leaks: When the archiver is stopped, any in-progress rearchive continues running indefinitely. It holds references to the beacon client and storage, preventing clean shutdown.
HTTP timeout leaks: The
/rearchivehandler has a 60-second middleware timeout. When the timeout fires, the HTTP response is aborted, but the underlying rearchive goroutine keeps running in the background — a silent resource leak for large slot ranges.The fix
ctx context.Contextparameter torearchiveRangecontext.Background()calls withctxr.Context(), which is cancelled on client disconnect or server shutdowncontext.Background()(no cancellation needed in tests)Fixes #58 (Bug 1)