Fix cluster single-runner shutdown deadlock - #7032
Conversation
🦋 Changeset detectedLatest commit: efd541e The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Run failed. View the logs →
|
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one suggestion to extend the regression test inline.
Reviewed changes
.changeset/fix-cluster-shutdown-deadlock.md: patch changeset foreffect.packages/effect/src/unstable/cluster/Sharding.ts: insendOutgoing, abandonEntityNotAssignedToRunnerretries when the runner is shutting down or the target entity manager is no longer alive;RunnerUnavailablestill retries.packages/effect/test/cluster/Sharding.test.ts: regression test that arms an entity finalizer to send a volatile message during teardown and asserts theShardingscope closes for bothpreemptiveShutdownvalues.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This re-review covered the second commit on the branch (efd541ebaa), which refined the initial single-runner shutdown deadlock fix.
- Hoisted
isPersistedout of the retry suspend inSharding.sendOutgoingso the persisted annotation is resolved once per send instead of on every retry. - Persisted abandoned persisted messages during shutdown rather than dropping them: volatile messages are logged and abandoned, while persisted ones are saved via
storage.saveRequest/storage.saveEnvelopeand a warning is emitted. - Expanded the regression test to cover both
persisted=trueandpersisted=false, and to assert the expected in-memory storage journal length after shutdown completes. - Test robustness improvements: replaced the bounded
forwait loop with ashardAcquiredlatch, switched toEffect.forkDetach/interruptUnsafefor deterministic teardown, and added explicit timeouts.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since 74218fd
packages/effect/src/unstable/cluster/Sharding.ts:isPersistedis now read once beforeEffect.catchFilterso the abandon handler can branch on it; persisted outgoing messages are explicitly saved toMessageStorageduring shutdown instead of being dropped.packages/effect/test/cluster/Sharding.test.ts: the regression test now covers bothpersistedandpreemptiveShutdown, assertsdriver.journal.length, and uses ashardAcquiredlatch with cleanup and a shorter shutdown timeout.
The prior suggestion to cover the persisted path is resolved. Tests, lint, and type checks pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
2 participants
Type
Description
Hit this running a single-node cluster locally: shutdown would just hang forever, spamming
WARN No healthy runners available.If an entity fires an outgoing message while it's finalizing (e.g. a reactor pushing an update to another entity) and the target shard is no longer routable,
sendOutgoingretriesEntityNotAssignedToRunnerforever. On a single runner the shard never gets reassigned, so the finalizer never completes and the Sharding scope never closes. Multi-runner masks it since the shard just moves to a peer.Fix: bail out of the retry when it provably can't succeed — the runner is shutting down, or the target's local entity manager is already closing/closed.
RunnerUnavailablestill retries so rolling deploys are unaffected, and persisted messages stay durable in storage.Added a regression test that arms an entity finalizer to send during teardown and asserts the scope actually closes (both preemptive and non-preemptive shutdown).