Skip to content

fix(bots): discard stale async connection handshakes - #5950

Open
jackeyfaker77 wants to merge 2 commits into
apache:mainfrom
jackeyfaker77:fix/stale-bot-async-operations
Open

jackeyfaker77 wants to merge 2 commits into
apache:mainfrom
jackeyfaker77:fix/stale-bot-async-operations

Conversation

@jackeyfaker77

Copy link
Copy Markdown
Contributor

Summary

Fixes #5949. Refs #5947 and #5948.

  • Bind gateway/Stream HTTP handshakes to a connection generation, invalidated by stop, close, intentional reconnect, or a newer attempt.
  • Bind awaited Identify/Resume payloads to their originating socket and discard stale results before sending authentication or reconnecting.
  • Discard retired provider status/token-cache writes. In-flight HTTP responses still finish and are consumed; current authentication and retry policy are preserved.

Independent follow-up from main; includes a separate regression file.

Verification

  • All bot suites: 147 tests passed, including 38 new cases (35 regressions and 3 current-request retry controls).
  • The same 38 cases against unchanged main ab5996b: 35 fail, 3 controls pass. Deferred production HTTP requests use Undici MockAgent and fake WebSockets.
  • Root build and all workspace typechecks passed.
  • Repository source lint/format passed (4038/2295 files; explicit source paths avoid pre-existing scratch checkout configs).
  • Desktop and UI Knip passed; staged Biome, ASF license and protocol epoch checks passed.
  • No live provider session or full Desktop end-to-end run.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex — investigation, implementation, regression tests, and contribution text. The commit includes Generated-by: OpenAI Codex.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

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
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Oct 2, 2026
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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, buildIdentifyPayload and buildResumePayload now receive an isCurrent: () => 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.
  • connectionGeneration advances 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.ts and 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.

@jackeyfaker77

Copy link
Copy Markdown
Contributor Author

Combined merge and live Discord verification

Tested #5948 and #5950 together at these exact heads:

Merge check: ws-bridge-base.ts is the only shared file. Git's three-way merge succeeds in both orders with zero textual conflicts and identical resulting contents. I compiled and tested the actual combined runtime. The socket retirement guards and the asynchronous attempt guards work together; these heads do not require a particular merge order.

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:

Runner Result Verified behavior
Gateway lifecycle 7/7 stages passed Bot authentication, Identify/READY, reconnect with RESUMED, stop/start, retired socket events, and late gateway results after stop and after replacement
Channel messaging 9/9 stages passed Channel access, gateway readiness, Bot send and REST readback, real user message reception before stop and after restart, reply reference/readback, own-message filtering, and retired MESSAGE_CREATE handling

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(bots): stale async handshakes outlive stopped or replaced connections

2 participants