-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix: add structured error code to SessionNotFoundError for session-closing retry #8884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
15f09a0
f7aa168
25c7e6a
afd1833
c6f5da5
85a8c36
1b9f37a
54f9fbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2764,7 +2764,10 @@ export function DaemonSessionProvider(props: DaemonSessionProviderProps) { | |
| autoReconnect && | ||
| loadingRequestedSession && | ||
| pendingLoad?.sessionId === restoreSessionId && | ||
| isClosingSessionLoadError(error) | ||
| isClosingSessionLoadError( | ||
| error, | ||
| !capabilities?.features.includes(CLIENT_IDENTITY_FEATURE), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] This gate is
The daemons this fallback exists for are the ones built between #8864 (merged 2026-08-10) and this PR: they emit the closing message but not the Two consequences: the branch is dead code that reads as intentional back-compat, and against an 8864-era daemon this WebUI actually loses a retry that worked before this PR (no
Simplest correct fix is to drop the second argument and match on This review was generated by QoderWork AI |
||
| ) | ||
| ) { | ||
| reconnectAttempt += 1; | ||
| const reconnectConfig = reconnectConfigRef.current; | ||
|
|
@@ -4729,13 +4732,18 @@ function isAuthFailureHttpError(error: unknown): boolean { | |
| return status !== undefined && AUTH_FAILURE_HTTP_STATUSES.has(status); | ||
| } | ||
|
|
||
| function isClosingSessionLoadError(error: unknown): boolean { | ||
| function isClosingSessionLoadError( | ||
| error: unknown, | ||
| allowLegacyMessage = false, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] This parameter is untested and was never reviewed.
No test covers it. All four cases touching this predicate ( Separately, the other caller at Also worth noting the message predicate was never complete: This review was generated by QoderWork AI |
||
| ): boolean { | ||
| if (!(error instanceof DaemonHttpError) || error.status !== 404) return false; | ||
| const body = isRecord(error.body) ? error.body : undefined; | ||
| return ( | ||
| typeof body?.['error'] === 'string' && | ||
| body['error'].endsWith( | ||
| 'The session is closing; retry after close completes', | ||
| ) | ||
| body?.['code'] === 'session_closing' || | ||
| (allowLegacyMessage && | ||
| typeof body?.['error'] === 'string' && | ||
| body['error'].endsWith( | ||
| 'The session is closing; retry after close completes', | ||
| )) | ||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.