Skip to content

fix: serialize sends on shared listener gRPC streams - #4491

Merged
igor-kupczynski merged 4 commits into
mainfrom
serialize-listener-stream-sends
Jul 23, 2026
Merged

fix: serialize sends on shared listener gRPC streams#4491
igor-kupczynski merged 4 commits into
mainfrom
serialize-listener-stream-sends

Conversation

@igor-kupczynski

@igor-kupczynski igor-kupczynski commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Description

Problem

Concurrent Workflow.Run waits in the Go SDK share a single SubscribeToWorkflowRuns bidirectional gRPC stream, and each waiter sent its subscription message from its own goroutine with no serialization. grpc-go permits only one concurrent sender per stream (and forbids CloseSend concurrent with SendMsg), so concurrent waits raced on the shared stream. The same pattern affected the legacy DurableEventsListener. Reported by a customer investigating a lost child-workflow result.

Fix

reconnectingStream (the shared layer both listeners route sends through) gains a sendMu that serializes every SendMsg and CloseSend on published clients. It is held only around an individual snapshot+send attempt (or a CloseSend), never across reconnect, backoff, or error classification, so a slow send cannot wedge reconnection or shutdown. Reconnect replay needs no lock: it runs on a not-yet-published client inside the connect singleflight. Never-published clients (connect error paths) are likewise exempt.

This is similar to DurableTaskListener and the Python SDK's pooled listener; although there we have queues, here a mutex suffices because callers expect synchronous send results.

Discussion: queue vs mutex

Two ways to serialize senders on a shared stream: a mutex around Send, or a single writer goroutine fed by a queue (what DurableTaskListener and the Python SDK do). This PR uses the mutex.

-retrySend is synchronous and a queue would make sends async.

  • Latency: identical serialization either way, i.e., one send on the wire at a time. The mutex surfaces the wait at the caller (head-of-line blocking): N concurrent Workflow.Run calls take turns, but the subscribe messages are tiny and should clear in <1ms.
  • Worst case: a Send stuck on flow control holds the lock and everyone stalls until it errors or the stream dies. But then I'm not sure if this is a problem as the queue has the same worst case (a stuck single writer stalls the whole queue), but it just hides the wait from the caller.
  • CloseSend goes under the same lock (grpc forbids CloseSend concurrent with SendMsg).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

What's Changed

  • Added a sendMu to reconnectingStream that serializes SendMsg and CloseSend on published clients (pkg/client/reconnecting_stream.go)
  • Routed SubscribeToWorkflowRuns sends through the serialized path (pkg/client/stream_listen.go)
  • Strengthened TestRetrySend_ConcurrentSafety to detect overlapping sends (pkg/client/reconnecting_stream_test.go)

Checklist

Changes have been:

  • Tested (unit, integration, or manually with steps specified)
  • Linted and formatted
  • Documented (where applicable)
  • Added to CHANGELOG (where applicable) -- see Keep a Changelog

Testing

TestRetrySend_ConcurrentSafety was strengthened to detect overlapping sends via an atomic in-flight flag with a start barrier. Run under -race; verified to fail against the pre-fix code.


🤖 AI Disclosure
  • I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchet's AI_POLICY.md.

  • Details: Implementation and tests drafted with an LLM coding agent (Cursor); reviewed and hand-edited by the author.

Concurrent Workflow.Run waiters share one SubscribeToWorkflowRuns stream and
called Send from multiple goroutines, violating grpc-go's single-sender
contract. A sendMu in reconnectingStream now serializes SendMsg and CloseSend
on published clients. This covers both WorkflowRunsListener and
DurableEventsListener since both route through reconnectingStream.retrySend.
@vercel

vercel Bot commented Jul 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hatchet-docs Ready Ready Preview, Comment Jul 23, 2026 8:12pm

Request Review

@github-actions github-actions Bot added the engine Related to the core Hatchet engine label Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Optional test failure: The load-online-migrate job failed on this PR. This check is non-mandatory and does not block merging, but may be worth investigating. View logs

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Optional test failure: The load-online-migrate job failed on this PR. This check is non-mandatory and does not block merging, but may be worth investigating. View logs

@igor-kupczynski
igor-kupczynski marked this pull request as ready for review July 23, 2026 16:07
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Optional test failure: The load-online-migrate job failed on this PR. This check is non-mandatory and does not block merging, but may be worth investigating. View logs

@juliusgeo juliusgeo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@igor-kupczynski
igor-kupczynski merged commit 17a1082 into main Jul 23, 2026
60 checks passed
@igor-kupczynski
igor-kupczynski deleted the serialize-listener-stream-sends branch July 23, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine Related to the core Hatchet engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants