Skip to content

Commit 56ffc7b

Browse files
fix: SSE onerror should not poison EventSource lifetime with isAuthRetry
The isAuthRetry parameter approach works for recursive method calls (send, _startOrAuthSse) but not for the EventSource onerror callback. Passing _startOrAuth(true) on retry permanently captures isAuthRetry=true in the new EventSource's closure — if that EventSource auto-reconnects later (network blip on a long-lived stream) and gets 401, onUnauthorized is skipped and the transport cannot recover. Verified against eventsource lib: non-200 → failConnection (CLOSED, no reconnect); stream end after OPEN → scheduleReconnect → reconnect attempt can get 401 → failConnection → onerror fires. The 'hours later' scenario is real. Fix: retry always calls _startOrAuth() fresh (no parameter). Matches pre-PR _authThenStart() behavior. Trade-off: no circuit breaker on the SSE connect path — if onUnauthorized succeeds but server keeps 401ing, it loops (same as pre-PR). Also fixes double-onerror: two-arg .then(onSuccess, onFail) separates retry failures (inner _startOrAuth already fired onerror) from onUnauthorized failures (not yet reported). Added close + clear _last401Response before retry for hygiene. Two regression tests added, both verified to FAIL against the buggy code: - 401→401→200: onUnauthorized called TWICE, start() resolves - 401→onUnauthorized succeeds→401→onUnauthorized throws: onerror fires ONCE with the thrown error
1 parent 71a9260 commit 56ffc7b

File tree

2 files changed

+83
-12
lines changed

2 files changed

+83
-12
lines changed

packages/client/src/client/sse.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ export class SSEClientTransport implements Transport {
118118
});
119119
}
120120

121-
private _startOrAuth(isAuthRetry = false): Promise<void> {
121+
private _startOrAuth(): Promise<void> {
122122
const fetchImpl = (this?._eventSourceInit?.fetch ?? this._fetch ?? fetch) as typeof fetch;
123123
return new Promise((resolve, reject) => {
124124
this._eventSource = new EventSource(this._url.href, {
@@ -147,22 +147,22 @@ export class SSEClientTransport implements Transport {
147147

148148
this._eventSource.onerror = event => {
149149
if (event.code === 401 && this._authProvider) {
150-
if (this._authProvider.onUnauthorized && this._last401Response && !isAuthRetry) {
150+
if (this._authProvider.onUnauthorized && this._last401Response) {
151151
const response = this._last401Response;
152-
this._authProvider
153-
.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit })
154-
.then(() => this._startOrAuth(true))
155-
.then(resolve, error => {
152+
this._last401Response = undefined;
153+
this._eventSource?.close();
154+
this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then(
155+
// onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject.
156+
() => this._startOrAuth().then(resolve, reject),
157+
// onUnauthorized failed → not yet reported.
158+
error => {
156159
this.onerror?.(error);
157160
reject(error);
158-
});
161+
}
162+
);
159163
return;
160164
}
161-
const error = isAuthRetry
162-
? new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
163-
status: 401
164-
})
165-
: new UnauthorizedError();
165+
const error = new UnauthorizedError();
166166
reject(error);
167167
this.onerror?.(error);
168168
return;

packages/client/test/client/sse.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1624,5 +1624,76 @@ describe('SSEClientTransport', () => {
16241624

16251625
await expect(transport.finishAuth('auth-code')).rejects.toThrow('finishAuth requires an OAuthClientProvider');
16261626
});
1627+
1628+
it('SSE connect 401 retry does not poison future 401s — onUnauthorized called on each attempt', async () => {
1629+
// Regression: _startOrAuth(true) baked isAuthRetry=true into the retry EventSource's
1630+
// onerror closure, so a subsequent 401 (token expiry on reconnect) would throw
1631+
// instead of refreshing. Fix: retry always calls _startOrAuth() fresh.
1632+
await resourceServer.close();
1633+
1634+
let getAttempt = 0;
1635+
resourceServer = createServer((req, res) => {
1636+
if (req.method !== 'GET') {
1637+
res.writeHead(404).end();
1638+
return;
1639+
}
1640+
getAttempt++;
1641+
if (getAttempt < 3) {
1642+
res.writeHead(401).end();
1643+
return;
1644+
}
1645+
res.writeHead(200, {
1646+
'Content-Type': 'text/event-stream',
1647+
'Cache-Control': 'no-cache, no-transform',
1648+
Connection: 'keep-alive'
1649+
});
1650+
res.write('event: endpoint\n');
1651+
res.write(`data: ${resourceBaseUrl.href}post\n\n`);
1652+
});
1653+
resourceBaseUrl = await listenOnRandomPort(resourceServer);
1654+
1655+
const authProvider: AuthProvider = {
1656+
token: vi.fn(async () => 'token'),
1657+
onUnauthorized: vi.fn(async () => {})
1658+
};
1659+
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
1660+
1661+
await transport.start(); // should resolve on attempt 3
1662+
1663+
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(2);
1664+
expect(getAttempt).toBe(3);
1665+
});
1666+
1667+
it('retry failure during SSE connect fires onerror exactly once', async () => {
1668+
// Regression: when the retry EventSource rejected, its onerror fired inside, then
1669+
// the outer .then() rejection handler fired onerror AGAIN for the same error.
1670+
// Fix: inner retry chains to .then(resolve, reject) — no outer onerror call.
1671+
// onUnauthorized's own failure is handled separately and fires onerror once.
1672+
await resourceServer.close();
1673+
1674+
resourceServer = createServer((req, res) => {
1675+
if (req.method === 'GET') {
1676+
res.writeHead(401).end(); // always 401
1677+
}
1678+
});
1679+
resourceBaseUrl = await listenOnRandomPort(resourceServer);
1680+
1681+
const onUnauthorized: AuthProvider['onUnauthorized'] = vi
1682+
.fn()
1683+
.mockResolvedValueOnce(undefined) // first call succeeds → triggers retry
1684+
.mockRejectedValueOnce(new Error('refresh failed')); // second call (in retry) throws
1685+
const authProvider: AuthProvider = {
1686+
token: vi.fn(async () => 'token'),
1687+
onUnauthorized
1688+
};
1689+
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
1690+
const onerror = vi.fn();
1691+
transport.onerror = onerror;
1692+
1693+
await expect(transport.start()).rejects.toThrow('refresh failed');
1694+
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(2);
1695+
expect(onerror).toHaveBeenCalledTimes(1);
1696+
expect(onerror.mock.calls[0]![0].message).toBe('refresh failed');
1697+
});
16271698
});
16281699
});

0 commit comments

Comments
 (0)