Add worker reconnection logging to durabletask-go - #151
Open
Jean Carlos Magalhães (jeanmartins) wants to merge 2 commits into
Open
Add worker reconnection logging to durabletask-go#151Jean Carlos Magalhães (jeanmartins) wants to merge 2 commits into
Jean Carlos Magalhães (jeanmartins) wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds tracking of repeated fetch errors to improve logging when the worker recovers and can process items again.
Changes:
- Introduces an atomic counter on
workerto track consecutive fetch failures. - Adds “reconnected and ready…” log lines when the worker recovers after prior failures.
- Increments the counter on non-context fetch errors and resets it on successful/no-item fetches.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Remove dependency on w.waiting for reconnection detection - Update comment from 'connection-related errors' to 'fetch failures' for accuracy - Keep reconnection logging in both branches (with and without work items) - Maintain error log duplication as preferred This addresses feedback about w.waiting representing work item state rather than connection state, and makes error tracking more accurate.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (5)
backend/worker.go:120
- This "reconnected" log is currently gated by
w.waiting, which appears to represent "no work items currently" rather than "connection was down". As a result, the message can be misleading (e.g., it will fire when the queue becomes non-empty after normal idle waiting, even if there was no connection failure). Consider keying this log off the error-tracking state (e.g.,consecutiveErrors > 0) and resetting that state when logging, or changing the wording to reflect resuming from idle rather than reconnection.
if w.waiting {
w.logger.Infof("%v: reconnected and ready to process work items", w.Name())
}
backend/worker.go:186
- The recovery logging/reset logic is duplicated across multiple branches. This increases the chance of inconsistencies (and makes it harder to adjust behavior later). Consider centralizing this into a small helper (e.g.,
maybeLogFetchRecovery()) or restructuring so the log/reset happens in exactly one place when a recovery condition is met.
// Check if we recovered from errors when no work items
if w.consecutiveErrors.Load() > 0 {
w.logger.Infof("%v: reconnected and ready to process work items", w.Name())
w.consecutiveErrors.Store(0)
}
backend/worker.go:204
- The recovery logging/reset logic is duplicated across multiple branches. This increases the chance of inconsistencies (and makes it harder to adjust behavior later). Consider centralizing this into a small helper (e.g.,
maybeLogFetchRecovery()) or restructuring so the log/reset happens in exactly one place when a recovery condition is met.
// Check if we recovered from errors when successfully fetching work item
if w.consecutiveErrors.Load() > 0 {
w.logger.Infof("%v: reconnected and ready to process work items", w.Name())
w.consecutiveErrors.Store(0)
}
backend/worker.go:50
- The field name/comment indicate "connection failures", but the counter is incremented on any
FetchWorkItemerror (not necessarily connection-related). To avoid confusing future maintainers, either rename the field to match current usage (e.g.,consecutiveFetchErrors) and update the comment accordingly, or narrow the increment logic to only count errors that actually represent connection failures.
// consecutiveErrors tracks connection failures for reconnection logging
consecutiveErrors atomic.Int32
backend/worker.go:186
- The
Load() > 0followed byStore(0)is not an atomic "check-and-reset". Since the counter is anatomic.Int32, it’s safer to use a single atomic operation (e.g.,Swap(0)and check the returned value) to avoid losing increments that happen between the load and store.
if w.consecutiveErrors.Load() > 0 {
w.logger.Infof("%v: reconnected and ready to process work items", w.Name())
w.consecutiveErrors.Store(0)
}
Contributor
Author
|
Hi Chris Gillum (@cgillum) , please review it |
Member
|
Tomer Rosenthal (@torosent) is there’s another engineer that can review this PR until I get back from vacation? |
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
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.
Changes:
consecutiveErrorscounter to track connection failuresBenefits:
Testing:
Tested by stopping PostgreSQL container and observing reconnection logs when connection
Evidence: