Scope cluster reply serialization failures and peer-delivered defects to their own request - #6579
Scope cluster reply serialization failures and peer-delivered defects to their own request#6579marbemac wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: fc3397a The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 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 |
marbemac
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCluster runner reply serialization and defect handling now isolate failures to their originating request or stream. Server adapters disable fatal defects, malformed replies become terminal defect replies, and RPC clients distinguish volatile, persisted, and transport failures. ChangesCluster reply defect isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunnerClient
participant RunnerServer
participant ReplySerializer
participant SiblingRequest
RunnerClient->>RunnerServer: multiplexed request
RunnerServer->>ReplySerializer: serialize reply
ReplySerializer-->>RunnerServer: MalformedMessage
RunnerServer->>RunnerClient: request-scoped defect reply
RunnerServer->>RunnerServer: terminate affected stream
SiblingRequest->>RunnerServer: concurrent request
RunnerServer-->>SiblingRequest: unaffected response
Suggested labels: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
| spanPrefix: "RunnerServer", | ||
| disableTracing: true | ||
| disableTracing: true, | ||
| disableFatalDefects: true |
There was a problem hiding this comment.
Least sure about this one — the other two changes fix the issue without it, this just stops other handler defect classes from failing every in-flight request on the shared connection. Happy to drop it if connection-fatal defects are intentional on this channel (same with the other 2 flips of disableFatalDefects to true).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
||
| const constVoid = constant(Effect.void) | ||
|
|
||
| // The original reply never left this runner, so its id can be reused for the |
There was a problem hiding this comment.
Remove these comments as they aren't really helping
There was a problem hiding this comment.
sure thing, done across the board
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
… to their own request
Signed-off-by: Marc MacLeod <marbemac+gh@gmail.com>
3d0b2d7 to
fc3397a
Compare
2 participants
Type
Description
If a volatile entity reply can't be serialized, the failure currently takes down the whole runner-to-runner connection.
RunnerServerdies with theReply.serializeerror,RpcServerbroadcasts it as a connection-levelDefectframe (killing every in-flight request on that connection), andRunners.makeRpctreats the delivered defect asRunnerUnavailable.Sharding.sendOutgoingthen re-sends the same request id everysendRetryInterval, and since the entity already processed it, the caller getsAlreadyProcessingMessagefor a request that actually succeeded — only the reply couldn't be encoded.Each step is reasonable on its own; it's the combination that turns one unencodable reply into a connection-wide failure and a retry loop. This PR changes all three steps, so no two of them can recombine this way:
RunnerServernow responds to the affected request with a defect reply (ReplyWithContext.fromDefect) when reply serialization fails, the same way request serialization failures are already handled inRunners. TheStreamhandler also ends its queue, since the fallback reply is terminal.Runners.makeRpconly converts transport failures (RpcClientError) toRunnerUnavailable. Defects delivered by the peer resolve the request with the real error instead of being re-sent. Persisted requests still map toRunnerUnavailable, because their reply can be recovered from storage.RunnerServer.layer,HttpRunner.toHttpEffect{,Websocket}) setdisableFatalDefects: true, so other handler defects also stay scoped to a single request. From what I can find nothing depends on connection-fatal defects on this channel (aDefectframe doesn't close the socket, and reconnection is driven by socket errors), and the entity level already works this way — but if connection-fatal defects are intentional here, this change can be dropped without affecting the other two.Tests:
test/cluster/Runners.test.tscovers both fixes at the unit level (also the first tests for theRunnerUnavailable/AlreadyProcessingMessagepaths),SocketRunner.test.tsadds a two-runner repro over real sockets checking that a sibling in-flight request survives, andRpcServer.test.tsgets adisableFatalDefectsisolation test.Related
Same direction as #2457, which did this for unknown request tags.
Summary by CodeRabbit