Fix async shutdown #14
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes async shutdown issues by adding proper synchronization and shutdown handling to prevent race conditions and ensure clean termination of database operations.
- Adds mutex protection to Stats struct to prevent concurrent access to metrics
- Implements shutdown flag in database loader to prevent new flushes during shutdown
- Adds graceful handling of shutdown-related errors during final flush operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sinker/stats.go | Adds mutex synchronization to protect concurrent access to Stats fields |
| sinker/sinker.go | Adds error handling for shutdown-related flush failures |
| db/db.go | Implements shutdown flag and improved async flush synchronization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| _, err := s.loader.Flush(ctx, s.OutputModuleHash(), cursor, cursor.Block().Num()) | ||
| if err != nil { | ||
| // Check if this is a shutdown-related error and handle accordingly | ||
| if strings.Contains(err.Error(), "shutting down") { |
There was a problem hiding this comment.
Using string matching on error messages is fragile and error-prone. Consider defining a specific error type or using error wrapping with errors.Is() for more robust error handling.
| l.logger.Warn("async flush failed after retries", zap.Error(err)) | ||
|
|
||
| l.cond.L.Lock() | ||
| l.isShuttingDown = true |
There was a problem hiding this comment.
Setting isShuttingDown flag on flush error without checking if already shutting down could cause race conditions. Consider checking the flag state before setting it or using atomic operations.
| l.logger.Debug("async flush: starting flush", zap.Int("active_flushes", l.activeFlushes), zap.Uint64("last_final_block", lastFinalBlock)) | ||
| l.cond.L.Lock() |
There was a problem hiding this comment.
Reading l.activeFlushes without holding the lock creates a race condition. The lock should be acquired before accessing any shared state including for logging.
No description provided.