test(web): pin the active-server retention the callback recovery depends on
Review round 4 raised that the callback catch leaves `activeServerId` set: the disconnect there emits only on a status change, and a rejected token exchange throws inside `completeOAuthFlow` before the reconnect, so the client is still at its initial "disconnected" and the listener that would clear it never fires. That is accurate, and clearing it is nonetheless wrong. "Authorize again" on the re-auth banner hands `clearServerOAuthState` the live client only when the banner's server is the active one; releasing it here passes `null` instead and the stale tokens are never cleared from the client holding them, which is the one thing that recovery exists to do. Confirmed by trying it — the existing lost-authorization-state test fails on `clearOAuthTokens` never being called. So the retention is now asserted rather than incidental, with the reason at both the code and the test, and the InspectorView double surfaces `activeServer` so a later cleanup of the apparent leak fails loudly instead of quietly breaking the recovery. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
45f50be9f8
commit
f0720e4ed6
2 changed files with 24 additions and 2 deletions
|
|
@ -468,6 +468,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
readResourceState?: { status?: string };
|
||||
currentLogLevel?: string;
|
||||
activeTab?: string;
|
||||
activeServer?: string;
|
||||
erroredServerId?: string;
|
||||
initializeResult?: { serverInfo: { name: string; version: string } };
|
||||
onActiveTabChange: (tab: string) => void;
|
||||
|
|
@ -549,6 +550,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
<span data-testid="errored-server">
|
||||
{props.erroredServerId ?? "none"}
|
||||
</span>
|
||||
<span data-testid="active-server">{props.activeServer ?? "none"}</span>
|
||||
<button onClick={() => props.onActiveTabChange("Servers")}>
|
||||
switch-servers-tab
|
||||
</button>
|
||||
|
|
@ -2604,8 +2606,15 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => {
|
|||
await waitFor(() =>
|
||||
expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
|
||||
);
|
||||
// `resumeAfterOAuth` carries the reconnect, which can reject while holding
|
||||
// the status at "connecting", so the attempt is torn down explicitly.
|
||||
// The active server is deliberately retained. "Authorize again" on the
|
||||
// re-auth banner only hands `clearServerOAuthState` the live client when
|
||||
// the banner's server is the active one, so releasing it here would leave
|
||||
// the stale tokens on the client that holds them. Asserted so a later
|
||||
// "cleanup" of the apparent leak fails here rather than silently breaking
|
||||
// the recovery.
|
||||
expect(screen.getByTestId("active-server")).toHaveTextContent("A");
|
||||
// The teardown still runs, for the case where the reconnect is what
|
||||
// rejected and the status really is left at "connecting".
|
||||
const client = clientInstances[0] as EventTarget & {
|
||||
disconnect: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -2909,6 +2909,19 @@ function App() {
|
|||
// active-server lock is never released. Before the EMA guard, since a
|
||||
// stuck session is worth clearing whichever way the error classifies.
|
||||
await client.disconnect().catch(() => {});
|
||||
// `activeServerId` is deliberately NOT cleared here, even though the
|
||||
// disconnect above often cannot announce itself: it emits only on a
|
||||
// status *change*, and the commonest failure — a rejected token
|
||||
// exchange — throws inside `completeOAuthFlow` before the reconnect
|
||||
// runs, so this freshly built client is still at its initial
|
||||
// `"disconnected"` and the listener that would clear it never fires.
|
||||
//
|
||||
// That looks like a leak and is not. The next step after a callback
|
||||
// failure is the re-auth banner below, and its "Authorize again" hands
|
||||
// `clearServerOAuthState` the live client only when the banner's server
|
||||
// *is* the active one. Releasing it here would pass `null` instead, and
|
||||
// the stale tokens would never be cleared from the client that holds
|
||||
// them — the one thing that recovery exists to do.
|
||||
if (isEmaClientNotConfiguredError(err)) {
|
||||
notifications.show({
|
||||
title: `Cannot connect to "${server.name}"`,
|
||||
|
|
|
|||
Loading…
Reference in a new issue