Conversation
Keep the complete regression suite in this shared test-only commit so both proposed fixes are evaluated against exactly the same scenarios. Use upstream cluster layers and the real in-memory message-storage driver; no Rhino service code or PostgreSQL instance is required. On rc.118, replacement handler acquisition replays unfinished requests before publication. A synchronous replay defect reaches recovery while its restart guard is set and is discarded. The repeated-defect test requires four attempts after three defects; vanilla rc.118 stops at two. A concurrent-defect control requires only one rebuild for two defects from the same handler generation. Add arrival-during-acquisition cases for persisted and volatile RPCs. After another replay defect, a request still waiting for first dispatch must not be submitted both by recovery and by its original waiting writer. Assert ordering, handler generations, results and one execution of the arriving request. For persistence, query MemoryDriver.encoded.repliesFor at each handler entry and record the requestId plus whether its successful WithExit reply already exists. The queued-defect proposal executes the same requestId twice: the first entry sees no success reply, the second sees the stored success from the first execution. This is a correctness risk for non-idempotent side effects, beyond redundant work or duplicate replies. The passing behavior executes once and leaves a successful reply stored. This verifies storage-write-before-reexecution ordering, not PostgreSQL crash durability or transaction isolation. Set Persisted on each RPC directly, since group annotations do not override existing RPC annotations. Assert stored success presence for persisted RPCs and absence for volatile RPCs to verify the fixture. Also cover shutdown starting during replacement acquisition. Once acquisition finishes, shutdown must complete before its termination timeout and must not replay application requests in the retiring entity. Validation on unmodified rc.118: four regressions fail and the concurrent control passes. pnpm check, focused oxlint and explicit-config dprint checks pass. The lifecycle proposal passes all five shared cases; the queued proposal's remaining failures are documented in its own commit.
The shared regression commit demonstrates lost recovery when a replayed request defects during replacement acquisition. Queuing that defect restores recovery but leaves duplicate execution after a successful reply has been persisted, and application replay after shutdown has begun. This alternative addresses all five shared scenarios. ResourceRef completes acquisition and publication before EntityManager replays unfinished requests. No ResourceRef API changes are required. Replace the global restart guard with one defect guard per handler generation: a generation reports its first defect only, while a defect from a published replacement can immediately start another rebuild. Bind replay to the acquired resource's identity and stop its loop when another rebuild takes ownership. Hold incoming writes behind a replay latch until the backlog has been submitted so new arrivals cannot overtake it. Track whether an active request reached handlers; a request waiting for its original dispatch must not also be included in a replay snapshot. This prevents the queued proposal's same-requestId re-execution after a WithExit/Success reply is already stored, as asserted by the shared tests. Preserve retry backoff, request payloads, stream progress and shutdown interruption suppression. When shutdown removes an activation, open the replay latch so waiting writers can observe shutdown. Reject queued application writes unless their exact activation is still registered, while allowing control messages to drain. Skip application replay after shutdown even if replacement acquisition completes successfully. The first gated prototype left EOF waiting for the termination timeout when shutdown began during acquisition. Independent review reproduced that issue; opening the gate and checking activation identity fixed it. A real-clock probe then completed in 23ms instead of the 1000ms timeout. Compared with 52b46b7 on cluster-defect-replay-queued, this removes the pending-defect queue and moves replay out of acquisition, at the cost of an admission latch and per-request delivery bookkeeping. All regression tests, including the encoded-store persistence assertion, live in the shared parent commit rather than in either implementation commit. Validation: all five shared regressions and all 184 cluster tests pass; pnpm check, oxlint and formatting checks pass. Both runtime implementations are unchanged by moving tests into their common parent. Two adversarial review passes resolved the shutdown finding with no outstanding findings.
Co-authored-by: Adrian Gierakowski <agierakowski@gmail.com>
🦋 Changeset detectedLatest commit: 7d50b0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 31 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 |
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
This branch has not been deployed
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.
Problem
When an entity handler defects,
EntityManagerrebuilds the handlers and replays unfinished requests inside theResourceRefacquisition. That has three problems:onDefectwhileisRestartingDueToDefectis still set, and it is dropped. The request stays inactiveRequestswith no handler and never completes.Fix
replayReadylatch holds new arrivals until the backlog has been resubmitted. Shutdown opens it so waiting writers and EOF can proceed.Requestwrites to an activation that is no longer registered are interrupted instead of being written to a retiring server. Callers already handle this outcome, sinceResourceMap.getreturns the same interrupt once the map closes:RpcClient, which resumes the caller with the interrupt.RunnerServerturns an interruptedsharding.sendinto an interrupt exit for the remote caller.entityTerminationTimeout.The implementation and regression tests are by Adrian Gierakowski (@adrian-gierakowski), cherry-picked from the
cluster-defect-replay-lifecycleandcluster-defect-replay-reprobranches of rhinofi/effect with authorship kept.Tests
packages/effect/test/cluster/DefectRecovery.test.tscovers repeated synchronous replay defects, coalescing of concurrent defects, arrivals during acquisition for persisted and volatile RPCs (including a check of the encoded reply store at handler entry), and shutdown during replacement acquisition. On unmodifiedmain, 4 of the 5 fail and the concurrent-defect control passes.DefectRecovery.test.ts: 5/5 pass on each of 3 repeated runs.packages/effect/test/clusterandpackages/effect/test/workflow: 207 tests pass.tsc -bforpackages/effectpass.Closes EFF-1632