This repository was archived by the owner on Nov 25, 2025. It is now read-only.
feat(statesync): introduce Finalizer interface for syncer cleanup #1422
Conversation
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
… shutdown During graceful shutdown, syncers cancelled via context cancellation were being logged as ERROR level. This is misleading since cancellation during shutdown is expected behavior, not an error condition. - Use `errors.Is()` to detect `context.Canceled` and `context.DeadlineExceeded` (handles wrapped errors) and log as INFO instead of ERROR - Separate `RunSyncerTasks()` logic into a synchronous wrapper and `StartAsync()` method for async execution to gain more flexibility and handle more use cases. - Add early return optimization when context is already cancelled. Test improvements: - Add tests for cancellation scenarios (`Canceled`, `DeadlineExceeded`, wrapped errors, early return). - Fix flakiness by adding WaitGroup synchronization and replacing channel-based coordination. - Refactor tests to use `t.Context()` and extract common helpers. resolves #1410
During graceful shutdown, the State Syncer was hanging because multiple blocking operations did not check context cancellation. When shutdown occurred, these operations would block indefinitely, preventing syncers from detecting cancellation and exiting gracefully. - Add context.Context parameter to LeafSyncTask.OnLeafs() interface to enable context propagation through the leaf processing call chain. - Update CodeQueue.AddCode() to accept context and check ctx.Done() before blocking on channel sends, preventing indefinite blocking when Code Syncer stops consuming during shutdown. - Update all OnLeafs implementations (mainTrieTask, storageTrieTask, trieSegment, atomic syncer) to accept and pass context through the call chain. - Add context parameter to startSyncing() and createSegments() methods, checking cancellation before blocking channel sends to the segments work queue. - Add context cancellation check in BlockSyncer before checking blocks on disk, ensuring it responds during the initial scan phase. - Update sync/client/leaf_syncer.go to pass context to OnLeafs() callbacks. This ensures all syncers detect cancellation immediately and exit gracefully instead of hanging until timeout.
… tracking - Remove canceledWG from NewCancelAwareSyncer() and related test assertions. - Unify and simplify function comments. - Make startedWG nil-safe in all helpers. - Remove TestSyncerRegistry_WrappedContextCanceledError.
…eafs - Add context cancellation check in `storageTrieTask.OnLeafs` before processing each account to allow early exit during shutdown. - Add comment explaining `context.Background()` usage in `CodeQueue.init()` since it runs during construction before sync starts.
Add a `Finalizer` interface to provide explicit cleanup operations for syncers. This ensures cleanup (like flushing batches to disk) is performed reliably even on cancellation or early returns. - Add `Finalizer` interface to `sync/types.go` for explicit cleanup. - Attach `Finalize()` in `CodeQueue` that finalizes code fetching to this new interface. - Gather finalization logic in a `Finalize()` for StateSyncer to flush in-progress trie batches. - Implement `Finalize()` for AtomicSyncer to commit pending database changes. - Add `FinalizeAll()` to SyncerRegistry with defer to ensure cleanup runs. - Remove `OnFailure` callback mechanism (replaced by `Finalizer`). resolves #1089 Signed-off-by: Tsvetan Dimitrov ([email protected])
powerslider
force-pushed
the
powerslider/1089-finalize-syncers
branch
from
a977649 to
5131d0c
Compare
…owerslider/1089-finalize-syncers
…owerslider/1089-finalize-syncers
Base automatically changed from
powerslider/1410-context-cancel-misleading-logs
to
master
November 24, 2025 14:16
alarso16
reviewed
Nov 24, 2025
Comment on lines
+161
to
+165
| if err := eg.Wait(); err != nil { | ||
| return err | ||
| } | ||
| return err | ||
|
|
||
| return nil |
Contributor
There was a problem hiding this comment.
Suggested change
| if err := eg.Wait(); err != nil { | |
| return err | |
| } | |
| return err | |
| return nil | |
| return eg.Wait() |
| func (t *stateSync) onSyncFailure() { | ||
| // Finalize checks if there are any in-progress tries and flushes their batches to disk | ||
| // to preserve progress. This is called by the syncer registry on sync failure or cancellation. | ||
| func (t *stateSync) Finalize() error { |
Contributor
There was a problem hiding this comment.
Based off the name alone, it seems like this shouldn't occur on success
Contributor
There was a problem hiding this comment.
I think this is ok, but you should update the comments
|
|
||
| // Finalizer provides a mechanism to perform cleanup operations after a sync operation. | ||
| // This is useful for handling inflight requests, flushing to disk, or other cleanup tasks. | ||
| type Finalizer interface { |
Contributor
There was a problem hiding this comment.
It's interesting you made this a separate interface. I totally agree that we shouldn't force all syncers to adhere to this, but should we instead make it something like:
```suggestion
type DynamicSyncer interface {
Syncer
UpdateSyncTarget(???) error
I'm honestly not sure, what do you think? My only concern with the current approach is that it seems unintuitive to check the types all the time, and rather we would expect the caller to either dynamic sync everything or static sync everything, right?
Contributor
There was a problem hiding this comment.
I actually now agree we this. This seems necessary for the state syncer, for example. Should it take in an error? (e.g. handle the cleanup different in the error vs no error case?)
| // This ensures that even if the sync is cancelled or fails, we preserve | ||
| // the progress up to the last fully synced height. | ||
| func (s *Syncer) Finalize() error { | ||
| if s.db == nil { |
alarso16
reviewed
Nov 24, 2025
|
|
||
| // Finalizer provides a mechanism to perform cleanup operations after a sync operation. | ||
| // This is useful for handling inflight requests, flushing to disk, or other cleanup tasks. | ||
| type Finalizer interface { |
Contributor
There was a problem hiding this comment.
I also think you should add this sort of comment on the syncer registry, since it doesn't say it will call finalize
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Why this should be merged
Check ava-labs/avalanchego#4603
How this works
Add a
Finalizerinterface to provide explicit cleanup operations for syncers. This ensures cleanup (like flushing batches to disk) is performed reliably even on cancellation or early returns.Finalizerinterface tosync/types.gofor explicit cleanup.Finalize()inCodeQueuethat finalizes code fetching to this new interface.Finalize()for StateSyncer to flush in-progress trie batches.Finalize()for AtomicSyncer to commit pending database changes.FinalizeAll()to SyncerRegistry with defer to ensure cleanup runs.OnFailurecallback mechanism (replaced byFinalizer).How this was tested
existing UT
Need to be documented?
no
Need to update RELEASES.md?
no
resolves ava-labs/avalanchego#4603
Signed-off-by: Tsvetan Dimitrov ([email protected])