fix: only announce the connection if it is still live after the stream open
Copilot review round 4: every await before the `connect` dispatch is a window for a `disconnect()` or a transport close to overtake the connect, and the listen round-trip widened it. Announcing anyway restarts every managed list refresh against a session being torn down or already dead. The dispatch is now gated on `status === "connected" && !disconnecting` — the second because an explicit teardown claims ownership before it awaits `client.close()`, leaving the status untouched until that block finishes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
This commit is contained in:
parent
b2dfd4099a
commit
2a4e93c621
2 changed files with 57 additions and 1 deletions
|
|
@ -397,6 +397,52 @@ describe("resource subscriptions era fork (#1630)", () => {
|
||||||
expect(streamAtConnectEvent).toBe(true);
|
expect(streamAtConnectEvent).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not announce the connection when a disconnect overtakes the stream open", async () => {
|
||||||
|
// The listen round-trip widened the window between the handshake and the
|
||||||
|
// `connect` announcement, so a `disconnect()` can land inside it.
|
||||||
|
// Announcing anyway would restart every managed list refresh against a
|
||||||
|
// session being torn down.
|
||||||
|
const started = await startToolsOnlyServer({ tools: true });
|
||||||
|
const connected = new InspectorClient(
|
||||||
|
{ type: "streamable-http", url: started.url },
|
||||||
|
{
|
||||||
|
environment: { transport: createTransportNode },
|
||||||
|
versionNegotiation: eraToVersionNegotiation("modern"),
|
||||||
|
},
|
||||||
|
);
|
||||||
|
client = connected;
|
||||||
|
let connectAnnounced = false;
|
||||||
|
connected.addEventListener("connect", () => {
|
||||||
|
connectAnnounced = true;
|
||||||
|
});
|
||||||
|
|
||||||
|
// Hold the stream open so the disconnect lands while it is in flight.
|
||||||
|
// `openStarted` is what makes the race deterministic: the stub is only
|
||||||
|
// reached after the handshake, so disconnecting before it runs would leave
|
||||||
|
// `releaseOpen` unset and hang the connect.
|
||||||
|
const int = internals(connected);
|
||||||
|
let releaseOpen: () => void = () => {};
|
||||||
|
let markOpenStarted: () => void = () => {};
|
||||||
|
const openStarted = new Promise<void>((resolve) => {
|
||||||
|
markOpenStarted = resolve;
|
||||||
|
});
|
||||||
|
int.refreshModernSubscription = () => {
|
||||||
|
markOpenStarted();
|
||||||
|
return new Promise<void>((resolve) => {
|
||||||
|
releaseOpen = resolve;
|
||||||
|
});
|
||||||
|
};
|
||||||
|
|
||||||
|
const connecting = connected.connect();
|
||||||
|
await openStarted;
|
||||||
|
const disconnecting = connected.disconnect();
|
||||||
|
releaseOpen();
|
||||||
|
await connecting;
|
||||||
|
await disconnecting;
|
||||||
|
|
||||||
|
expect(connectAnnounced).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
it("connects anyway when the connect-time listen fails, and retries", async () => {
|
it("connects anyway when the connect-time listen fails, and retries", async () => {
|
||||||
// The handshake succeeded; every request-scoped feature works without the
|
// The handshake succeeded; every request-scoped feature works without the
|
||||||
// stream, so the failure is handed to the reconnect machinery instead of
|
// stream, so the failure is handed to the reconnect machinery instead of
|
||||||
|
|
|
||||||
|
|
@ -2073,7 +2073,17 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
// list the server changes in that window would notify nobody, leaving the
|
// list the server changes in that window would notify nobody, leaving the
|
||||||
// UI stale with no way to notice (#1920). The cost is one listen
|
// UI stale with no way to notice (#1920). The cost is one listen
|
||||||
// round-trip added to connect on the modern era.
|
// round-trip added to connect on the modern era.
|
||||||
this.dispatchTypedEvent("connect");
|
//
|
||||||
|
// …which is also why the announcement is conditional. Every await above
|
||||||
|
// is a window for a `disconnect()` or a transport `onclose`/`onerror` to
|
||||||
|
// overtake this connect, and the listen round-trip widened it. Announcing
|
||||||
|
// then would restart every managed list refresh against a session being
|
||||||
|
// torn down or already dead. `disconnecting` covers the teardown that has
|
||||||
|
// claimed ownership but is still awaiting `client.close()` — the status is
|
||||||
|
// whatever it was until that block finishes.
|
||||||
|
if (this.status === "connected" && !this.disconnecting) {
|
||||||
|
this.dispatchTypedEvent("connect");
|
||||||
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (!isConnectAuthRecoveryError(error)) {
|
if (!isConnectAuthRecoveryError(error)) {
|
||||||
this.status = "error";
|
this.status = "error";
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue