fix(go): preserve result subscriptions across reconnect - #4571
Merged
Conversation
Serialize replay and stream publication with new registrations so successful sends cannot be stranded on a retiring client.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Replace the glassbox handoff regression (which observed sendMu contention via a production-code hook) with two tests driven entirely through the public listener API and the constructor seam: - a targeted regression that holds a reconnect open mid-replay and asserts a registration made inside the window reaches the replacement - a seeded chaos test: concurrent registrations, per-send latency, and breaks at fixed registration progress, asserting the current stream at quiescence saw every successful registration (replayable via HATCHET_CHAOS_SEED) Both fail on the parent commit (5/5 and 11/20 iterations) and pass with the fix, so the TryLock/onSendMuContention scaffolding is removed from reconnecting_stream.go; the production diff is now the pure critical section change.
Drop references to the pre-fix behavior and trim wording; the comments now describe only what the tests do and why, for readers of the merged code.
igor-kupczynski
marked this pull request as ready for review
July 31, 2026 12:35
juliusgeo
reviewed
Jul 31, 2026
connectOnce installs under its own sendMu hold via installClientLocked; the self-locking wrapper had no production callers left. The one test that installs out-of-band now takes sendMu at the call site.
juliusgeo
approved these changes
Jul 31, 2026
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
2 participants
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
Context: On the client, during reconnect, we have basically two stream: the old (now broken), the new (the one being established). Let's call this process a "handoff" from the old stream to the new stream. Concretely (in
connectOnce) the following steps happen during the handoff:clientfield to the new stream and bump generation, so every futuresnapshot()returns the new stream. [until this completes, thesnapshot()returns the old stream].CloseSendthe old stream.The bug is that 1 and 2 are not on the same critical section: if a new registration happened after 1, but before 2, it would be registered on the old broken stream (so therefore wouldn't happen).
This is a minimal fix that brings all 3 steps under a single
sendMumutex. (The side effect is that the registrations are stalled during the handoff, but it's the right tradeoff here).Type of change
What's Changed
Testing
I've iterated a bit on this, and settled on
pkg/client/listener_reconnect_invariant_test.go. It tests the invariant not the mechanism so even if it +300 it's worth adding. The invariant under test: everyAddWorkflowRunthat returns no error has its run ID sent on the valid stream.Two tests:
TestAddWorkflowRunDuringReconnectLandsOnReplacementtargets the bug.TestWorkflowRunsListenerReconnectChaosPreservesRegistrationschaos testing.Both tests were run against the parent commit (red) and fix (green).
Checklist
Changes have been:
🤖 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: Used Cursor agents to investigate the concurrency interleaving, implement the focused fix and regression test, and review the resulting diff. All listed tests and linters were run locally.