Repository navigation
fix(bots): discard stale async connection handshakes - #5950
jackeyfaker77 wants to merge 2 commits into
Conversation
Bind gateway requests to their connection attempt and Identify/Resume work to its originating socket. Discard retired HTTP results before connecting, retrying, updating status, or caching handshake tokens. Cover deferred gateway/token responses and authentication across stop, restart, and reconnect, plus current provider failure/retry behavior. Fixes apache#5949 Refs apache#5947, apache#5948 Generated-by: OpenAI Codex
Use test-scoped timeout and interval mocks for the gateway and QQ authentication lifecycle cases so random initial heartbeats cannot race the assertions. Preserve strict frame checks and existing retry coverage. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed f4b325fc7dd79b41b4843e4e4bddc61f7d96e5a3 (6 files, +506/−24; the new lifecycle suite is +441, so production is about +65/−24 across five bridges).
No P0–P3 findings, and the fix is the right shape for the hazard.
What the change does. The connection flow is asynchronous at several points — fetching the gateway URL, building the identify payload, building the resume payload — so a continuation belonging to a superseded attempt could previously resume and send a handshake on a socket that was no longer current. The fix introduces a per-attempt currency predicate and checks it after every await:
fetchGatewayUrl,buildIdentifyPayloadandbuildResumePayloadnow receive anisCurrent: () => boolean, so each adapter can also bail out inside its own awaits rather than only at the boundaries.- The identify and resume paths bind the predicate to the specific socket —
const ws = this.ws; const isCurrent = () => this.ws === ws && !this.explicitlyStopped;— which is the same identity-plus-explicit-stop pattern the socket layer uses, so the two now agree on what "still current" means. connectionGenerationadvances per attempt, so a replacement attempt supersedes its predecessor rather than racing it.- Failure recording is currency-guarded too, and its comment says so: "Record failures only while isCurrent() holds, then return null — the caller schedules the reconnect for the current attempt." A stale attempt therefore cannot write a failure that would mislead the reconnect policy.
The inverse risk — dropping a legitimate handshake — does not arise. isCurrent() is true while the socket is still the bridge's current one and no explicit stop has happened, so a live attempt proceeds normally; only continuations of superseded attempts are discarded.
Tests include the case that names the hazard: "does not create a socket when a gateway request completes after stop". The suite is large (+441), mostly scaffolding around it.
Merge-order answer, since the dispatch asked whether these three bot PRs overlap. They are not independent:
- #5946 touches
bot-registry.tsand its test — no overlap with this PR or #5948, so it can land freely. - #5948 and #5950 both edit
packages/runtime/src/bots/ws-bridge-base.ts(from the same base), and they are not merely adjacent: #5948 adds retirement guards to the socket handlers while this PR adds nine lines around the same connection lifecycle. Expect a real merge there, and review that resolution rather than trusting it, since the two changes can interact in ways a textual merge will not settle.
Gate on this head: test completed successfully; mergeable is true; no Grok involvement.
What I could not judge
- I did not run the new suite; the verdict rests on the diff, the currency checks' placement, and the named case.
- No live bridge session, so the stale-continuation race is reasoned from the code rather than reproduced.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Combined merge and live Discord verificationTested #5948 and #5950 together at these exact heads:
Merge check: Local execution: 150/150 combined bot tests, 38/38 lifecycle tests with zero jitter, six additional socket/authentication interleavings, and eight local HTTP response-body completion cases passed. The latter exercise responses whose headers have arrived while their bodies are still pending, across stop/restart and stale token/failure writes. Real Discord execution: a dedicated test Bot connected to Discord's actual REST API and Gateway, with Message Content Intent enabled. Both live runners exited successfully:
For messaging, the user sent two real messages in the dedicated channel. The bridge received both, including the one sent after restart. The Bot sent the test text and then replied to the second message; readback confirmed the reply references that message. Both Bot messages also arrived through the real Gateway and were correctly excluded from forwarded user input. How the races were exercised: the harness replayed captured real packets on their original retired socket, and held the return of successful real gateway URL requests until after stop or replacement. Replaying the captured human MESSAGE_CREATE after stop and after restart did not forward a message or alter status, sequence/session state, connection generation, or retry state. These are controlled timing/event checks around real network sessions, rather than claims that Discord naturally emitted a late packet during the run. The delayed gateway checks cover the caller's await boundary; body-pending races were covered separately by the local HTTP tests. One gateway lookup hit HTTP 429 in the lifecycle run; the existing retry recovered and the stage passed. Both runners stopped their bridges on exit. No P0–P3 findings in the combined review or these runs. Live coverage here is the Discord bridge, including actual channel send/receive; QQ/DingTalk and full Desktop end-to-end execution remain outside this live verification. Prepared and executed with OpenAI Codex. |
Summary
Fixes #5949. Refs #5947 and #5948.
Independent follow-up from main; includes a separate regression file.
Verification
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex — investigation, implementation, regression tests, and contribution text. The commit includes Generated-by: OpenAI Codex.
Checklist
Does this PR entail a change in behavior?