From b658d19631900d5889dae49d05235d0ae68826e0 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 29 Jul 2026 08:48:50 +0000 Subject: [PATCH] fix(#533): harden OAuth recovery E2E readiness Co-Authored-By: OpenAI Codex Claude-Session: unavailable (OpenAI Codex) --- CHANGELOG.md | 6 +++++ playwright.config.js | 6 +++-- tests/e2e/oauth-document-recovery.spec.js | 3 +++ tests/e2e/oauth-document-recovery/index.html | 28 ++++++++++++++++---- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3f99980..2ee48b0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Fixed +- **OAuth document-recovery E2E runs no longer reuse an incompatible stale + harness server** (#533). Playwright now probes the fixture's server-only + config route before accepting an existing process, preventing misleading + ready-latch timeouts when an older static harness still owns the test port. + The fixture also drains every intercepted startup ClickHouse request before + arming its deliberate 401, so a late catalog success cannot mask auth loss. - **Dashboard variable option SQL can no longer pass Run with more than 1000 rows** (#496). The existing bounded probe still fetches one sentinel row, but Run now rejects that 1001-row response after column-shape validation and diff --git a/playwright.config.js b/playwright.config.js index 05bf5e5b..6e6f155b 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -14,10 +14,12 @@ export default defineConfig({ // modules (/src/**) as native ESM — no bundling, always current. The server // is build/e2e-serve.mjs (not a plain static server) because the tree is // mixed .js/.ts under ADR-0002: an imported `./x.js` whose module converted - // to `x.ts` must be found and type-stripped, or the fixture pages 404. + // to `x.ts` must be found and type-stripped, or the fixture pages 404. Probe + // a server-only route rather than a static page: otherwise Playwright can + // reuse an older harness which lacks routes required by the current suite. webServer: { command: 'node build/e2e-serve.mjs 5599', - url: 'http://127.0.0.1:5599/tests/e2e/editor.html', + url: 'http://127.0.0.1:5599/tests/e2e/oauth-document-recovery/config.json', reuseExistingServer: !process.env.CI, timeout: 30_000, }, diff --git a/tests/e2e/oauth-document-recovery.spec.js b/tests/e2e/oauth-document-recovery.spec.js index c7013fbb..73c72689 100644 --- a/tests/e2e/oauth-document-recovery.spec.js +++ b/tests/e2e/oauth-document-recovery.spec.js @@ -23,6 +23,9 @@ test.describe('OAuth document recovery redirect (#512)', () => { await page.goto(fixturePath); await page.waitForFunction(() => window.__oauthRecoveryReady === true); + expect(await page.evaluate(() => window.__oauthRecoveryInitialCatalog())).toMatchObject({ + started: true, settled: true, succeeded: true, pending: 0, + }); expect(await page.evaluate(() => window.__oauthRecoveryInitialDirtyGuard)).toBe(true); const authLossResponse = page.waitForResponse((response) => ( new URL(response.url()).pathname === fixtureChPath diff --git a/tests/e2e/oauth-document-recovery/index.html b/tests/e2e/oauth-document-recovery/index.html index 6f2d2d01..d997a051 100644 --- a/tests/e2e/oauth-document-recovery/index.html +++ b/tests/e2e/oauth-document-recovery/index.html @@ -49,7 +49,7 @@ const fixtureCh = `${fixtureRoot}/ch`; const versionSql = 'SELECT version() AS v, uptime() AS u FORMAT JSON'; const nativeFetch = window.fetch.bind(window); - const initialCatalog = { started: false, settled: false }; + const initialCatalog = { started: false, settled: false, succeeded: false, pending: 0 }; // The production session addresses its ClickHouse origin at the serving // origin. Keep that real client behavior, but send only THIS fixture's // POSTs to its dedicated fake endpoint; no other E2E page is intercepted. @@ -61,7 +61,9 @@ if (isInitialVersion) initialCatalog.started = true; if (isClickHousePost) url.pathname = fixtureCh; try { - return await nativeFetch(isClickHousePost ? url.href : input, init); + const response = await nativeFetch(isClickHousePost ? url.href : input, init); + if (isInitialVersion) initialCatalog.succeeded = response.ok; + return response; } finally { if (isInitialVersion) initialCatalog.settled = true; } @@ -93,6 +95,20 @@ } const app = createApp({ root: document.querySelector('#root'), fetch: fixtureFetch, broadcastChannel: () => null }); + // Track the complete catalog service operations, not only fetch(): fetch + // resolves at response headers, before JSON parsing and any dependent query + // batch. Readiness must wait through those later async boundaries too. + for (const name of ['loadSchema', 'loadReference', 'loadVersion']) { + const load = app.catalog[name].bind(app.catalog); + app.catalog[name] = async (...args) => { + initialCatalog.pending += 1; + try { + return await load(...args); + } finally { + initialCatalog.pending -= 1; + } + }; + } const summarize = () => app.state.tabs.value.map((tab) => ({ id: tab.id, doc: tab.doc, name: tab.name, sqlDraft: tab.sqlDraft, specText: tab.specText, dirtySql: tab.dirtySql, dirtySpec: tab.dirtySpec, editorMode: tab.editorMode, savedId: tab.savedId, @@ -199,6 +215,7 @@ } window.__oauthRecoveryState = () => summarize(); window.__oauthRecoveryWorkspaceKey = () => app.currentWorkspace?.key ?? null; + window.__oauthRecoveryInitialCatalog = () => ({ ...initialCatalog }); // The signed-in shell's retained inline login mount is established by the // initial surface effects. Wait for that real mount (not a timing guess) // before exposing the 401 trigger, so the test exercises `show()` on it. @@ -212,12 +229,13 @@ }); } // bootstrap intentionally starts catalog/version work in the background. - // Wait for its exact SQL request to complete AND for authedFetch to record - // the successful response before exposing the user-triggered 401; this + // Wait for its exact SQL request to succeed and every complete catalog + // service operation to settle before exposing the user-triggered 401; this // prevents a stale success from racing the first-contact auth-loss path. await new Promise((resolve) => { const waitForInitialCatalog = () => { - if (initialCatalog.started && initialCatalog.settled && app.conn.chCtx.authConfirmed) resolve(); + if (initialCatalog.started && initialCatalog.settled && initialCatalog.succeeded + && initialCatalog.pending === 0 && app.conn.chCtx.authConfirmed) resolve(); else requestAnimationFrame(waitForInitialCatalog); }; waitForInitialCatalog();