Repository navigation
Conversation
The worker checks each new connection to the sidecar with a Hello call that had no deadline. After a host sleep or a network change the new connection can be left half-open, and that Hello then waits forever. It runs on the worker's event loop, so the worker never reconnects while the process looks healthy. Give the Hello a 30 second deadline, so a stuck attempt fails and the existing retry loop tries again. Also close the channel of a failed attempt instead of dropping it, so repeated failures do not leak channels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
is_worker_ready() was set on the first connect and never cleared, so it kept returning True while the worker had lost its work-item stream and was trying to reconnect. Nothing outside the worker could tell a worker that receives work from one that has been cut off from the sidecar. Clear the ready flag whenever the stream is dropped; it is set again when the next stream opens. This matches what WorkflowRuntime documents for wait_for_worker_ready(): that the stream is ready to receive work items. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
Each activity and workflow work item carried the stub of the connection it arrived on, and sent its result through that stub. When the sidecar ends the work-item stream the worker opens a new connection and closes the old channel ten seconds later. Any work item that runs longer than that sends its result on a closed channel: grpc raises ValueError, the worker logs it at debug level, and the result is lost. The sidecar then runs the activity again from the start. Send results (and the history fetch for a cache miss) through the worker's current connection instead. If the result is ready while the worker is between connections, wait up to 30 seconds for the next one; if the worker is stopping, give up at once and let the sidecar re-dispatch the work item. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
…l on it grpcio does not refuse a call on a closed channel when the channel has client interceptors (the workflow runtime always adds one). The intercepted call registers its method on the underlying channel first, which locks a mutex inside the already freed native channel, and the whole process dies with SIGSEGV instead of getting a ValueError. The worker closed channels without regard to calls about to start on them: the old channel ten seconds after a reconnect, and the current one at once in stop(), racing result sends, the keepalive Hello, the connect Hello and GetWorkItems. Count the calls in flight on each channel. Every call the worker makes now takes the channel through _sidecar_stub, which only hands out the current connection, and a channel is closed only after it is retired and its count drops to zero. stop() waits up to five seconds for those calls and then closes the channel anyway; by then any such call has long started, so closing it only cancels it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
…very call on it Addresses review feedback on the reconnect fix: - Hello runs on the new connection before it is published, so a result waiting for a reconnect is never sent on a connection that then fails. - Result sends and the history fetch get a 30s deadline. A retired channel is closed only once no call is in flight on it, so these deadlines are what bound that close and stop(); the close itself never runs early, since that could land between a result and its oversized-result fallback. - stop() keeps the connection open for its drain, so a result that becomes ready while the worker stops is still delivered. - Releasing a call wakes waiters only when the channel's count reaches zero. - The run loop tracks a connected flag instead of an unused stub. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
The worker replaced its gRPC channel every time the work-item stream ended. That forced the retire/publish handshake, per-channel call counts and the deferred-close threads, because results and other calls could still be using the old channel when it was closed. A grpc.Channel reconnects on its own, so only the GetWorkItems call needs to be re-issued. start() now opens one channel and stop() closes it. The run loop only sends Hello and opens a new stream on it. Result sends and the history fetch use wait_for_ready=True within their 30s deadline, so a result that is ready during a reconnect waits for the channel instead of a separate reconnect wait. The connect Hello waits for the channel too, so a returning sidecar is seen as soon as gRPC reconnects rather than after the run loop's retry delay on top of gRPC's own backoff, which is capped at 15s (default 120s). stop() still drains calls in flight for up to 5s and closes the channel only once no call is in flight, since grpcio crashes on a call started on a closed channel with interceptors. Closing the channel is also what ends the work-item stream: through an interceptor it is a generator with no cancel(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1317 +/- ##
==========================================
+ Coverage 86.98% 87.22% +0.23%
==========================================
Files 163 163
Lines 12997 13029 +32
==========================================
+ Hits 11306 11364 +58
+ Misses 1691 1665 -26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Description
Stacked on #1312. This branch contains #1312's commits, so only the last commit,
refactor(workflow): keep one gRPC channel for the worker's lifetime, is new here. Review that commit on its own until #1312 merges.This follows @JoshVanL's review suggestion on #1312: keep one gRPC channel for the worker's lifetime instead of replacing it every time the work-item stream ends.
Why the worker replaced its channel. Recreating the channel on every reconnect came from microsoft/durabletask-python#50 (2025), which was vendored here in #963. Neither PR says why. Upstream has since moved toward this design: microsoft/durabletask-python#135 keeps the channel, re-opens only the stream, and recreates the channel only after 5 transport failures in a row. A
grpc.Channelreconnects by itself, re-resolving DNS when its connection drops, so onlyGetWorkItemshas to be re-sent.What changes
start()opens one channel andstop()closes it. On reconnect the run loop only sendsHelloand a newGetWorkItemson that channel._RESULT_RECONNECT_WAITand the stream-reconnect wait on result sends.wait_for_ready=Truewith their existing 30s deadline. A result that is ready during a reconnect waits for the channel, instead of the worker tracking connections.Helloalso useswait_for_ready=True. Without it, aHellosent while gRPC is between reconnect attempts fails at once. The run loop's retry delay then adds to gRPC's own backoff, and in testing the worker took 8.7s to reconnect after the sidecar came back, against 0.7s on fix(workflow): keep the workflow worker working across sidecar reconnects #1312.grpc.max_reconnect_backoff_ms), matching the run loop's retry cap. The default is 120s, which could leave a worker idle for 2 minutes after a long outage. A caller'schannel_optionsstill override it.What stays
_sidecar_stubwith one in-flight counter) sostop()never closes the channel while a call is starting on it. With interceptors, grpcio crashes with SIGSEGV on a call started on a closed channel (see fix(workflow): keep the workflow worker working across sidecar reconnects #1312), and one channel does not remove that risk at shutdown.stop()keeps fix(workflow): keep the workflow worker working across sidecar reconnects #1312's order: wait up to 5s for calls in flight, close the channel, then join the run loop. The channel has to close first because closing it is the only way to end the work-item stream. Through an interceptor,GetWorkItemsreturns a plain generator with nocancel().Found along the way, not fixed here: for the same reason,
_make_stream_teardown(used when a history fetch fails) does nothing when the channel has interceptors, which is always the case underWorkflowRuntime. Itscancel()raisesAttributeError, which is logged at debug level. This predates both PRs, and I can open an issue for it.Checked against a real gRPC server (in-process, with the SDK's metadata interceptor): start the worker, kill the sidecar while an activity runs, let the activity finish while the sidecar is down, then restart the sidecar on the same port after 6s.
stop()On #1312 the 0.7s depends on when the run loop's retry happens to land. On both branches the worst case is about 15s.
Issue reference
No issue filed. This is a follow-up to review feedback on #1312.
Checklist
dapr/ext/workflow/AGENTS.md)🤖 Generated with Claude Code