OCPBUGS-105789: Recover expired Playwright sessions via auto re-login #16954
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| import type { Frame, Page, TestInfo } from '@playwright/test'; | ||
|
|
||
| import { | ||
| isOnLoginPage, | ||
| performLogin, | ||
| resolveCredentialsForStorageState, | ||
| saveStorageState, | ||
| } from '../setup/login-helper'; | ||
|
|
||
| /** | ||
| * Guards against re-entrant / concurrent re-login attempts on the same page. | ||
| */ | ||
| const reloginInProgress = new WeakMap<Page, Promise<void>>(); | ||
|
|
||
| /** | ||
| * The last app route each page navigated to before any auth redirect. Used to | ||
| * send the page back where the test intended to be after re-authenticating, | ||
| * since by the time recovery runs the page URL is the OAuth/login page. | ||
| */ | ||
| const lastAppUrl = new WeakMap<Page, string>(); | ||
|
|
||
| function storageStatePath(testInfo: TestInfo): string | undefined { | ||
| const state = testInfo.project.use.storageState; | ||
| return typeof state === 'string' ? state : undefined; | ||
| } | ||
|
|
||
| /** | ||
| * True for OAuth server / console login URLs — the pages a session-expiry | ||
| * redirect passes through. These are never the route a test wants to resume at. | ||
| * | ||
| * Matches against the URL pathname and anchors on known auth path prefixes so | ||
| * console resource routes that merely contain "auth" or "login" as a segment | ||
| * (e.g. a Secret named "auth") aren't misclassified. | ||
| */ | ||
| function isAuthUrl(url: string): boolean { | ||
| let pathname: string; | ||
| try { | ||
| pathname = new URL(url).pathname; | ||
| } catch { | ||
| return false; | ||
| } | ||
| return /^\/(oauth2?|login|auth)(\/|$)/.test(pathname); | ||
| } | ||
|
|
||
| /** | ||
| * Detects when a navigation has landed on the OAuth login page — meaning the | ||
| * session snapshot in storageState has expired — and transparently | ||
| * re-authenticates the current persona, refreshing the stored session so | ||
| * subsequent tests reuse the fresh state. After re-login, navigates back to | ||
| * the route the test was heading to so deep-link tests resume in place. | ||
| * | ||
| * The setup projects establish the session once; long runs can outlive the | ||
| * OAuth token, after which every navigation silently redirects to the login | ||
| * page and tests hang waiting for elements that never appear. This fixture is | ||
| * the self-healing recovery for that case. | ||
| */ | ||
| export async function recoverSessionIfExpired( | ||
| page: Page, | ||
| testInfo: TestInfo, | ||
| detectTimeoutMs = 0, | ||
| ): Promise<boolean> { | ||
| // If a re-login is already running (or being set up) for this page, await it | ||
| // rather than starting a second one. | ||
| const existing = reloginInProgress.get(page); | ||
| if (existing) { | ||
| await existing; | ||
| return true; | ||
| } | ||
|
|
||
| // Claim the re-login slot synchronously — before any await — so concurrent | ||
| // navigation events can't both pass the check above and launch duplicate | ||
| // logins. The deferred is published now and settled in the finally below; | ||
| // keeping every early return and rejection inside the try guarantees the | ||
| // slot is always released (a stuck claim would block all future recovery). | ||
| // The claim resolves on success and rejects on failure so joiners (the | ||
| // `existing` branch above) observe the same outcome instead of a false success. | ||
| let release!: () => void; | ||
| let fail!: (error: unknown) => void; | ||
| const claim = new Promise<void>((resolve, reject) => { | ||
| release = resolve; | ||
| fail = reject; | ||
| }); | ||
| // A rejected claim that nobody awaits is an unhandled rejection; attach a | ||
| // no-op catch to the stored copy so only explicit awaiters see the error. | ||
| claim.catch(() => {}); | ||
| reloginInProgress.set(page, claim); | ||
|
Comment on lines
+108
to
+119
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Propagate the actual recovery result to joiners. The shared Use Proposed fix- let release!: () => void;
+ let release!: (recovered: boolean) => void;
...
- const claim = new Promise<void>((resolve, reject) => {
+ const claim = new Promise<boolean>((resolve, reject) => {
...
if (existing) {
- await existing;
- return true;
+ return existing;
}
...
- release();
+ release(false);
...
- release();
+ release(false);
...
- release();
+ release(true);Also update the Also applies to: 117-118 🤖 Prompt for AI Agents |
||
|
|
||
| try { | ||
| if (!(await isOnLoginPage(page, detectTimeoutMs))) { | ||
| release(); | ||
| return false; | ||
| } | ||
|
|
||
| const statePath = storageStatePath(testInfo); | ||
| const credentials = statePath ? resolveCredentialsForStorageState(statePath) : null; | ||
| if (!credentials) { | ||
| // No credentials to recover with (e.g. auth disabled or dev creds unset). | ||
| release(); | ||
| return false; | ||
| } | ||
|
|
||
| const intendedUrl = lastAppUrl.get(page); | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `[auth] Session expired for "${testInfo.titlePath.join(' > ')}"; re-authenticating.`, | ||
| ); | ||
| await performLogin(page, credentials.username, credentials.password, credentials.idpName); | ||
| if (statePath) { | ||
| await saveStorageState(page, statePath); | ||
| } | ||
| // Restore the route the test was navigating to before the redirect, so | ||
| // deep-link tests resume where they expected rather than on the console | ||
| // home page that performLogin lands on. | ||
| if (intendedUrl && intendedUrl !== page.url()) { | ||
| await page.goto(intendedUrl, { waitUntil: 'domcontentloaded' }); | ||
| } | ||
| release(); | ||
| return true; | ||
| } catch (error) { | ||
| // Propagate the failure to every awaiter of this claim. | ||
| fail(error); | ||
| throw error; | ||
| } finally { | ||
| reloginInProgress.delete(page); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * True while a re-login is running (or being set up) for the page. The page.goto | ||
| * override consults this to skip the recovery step for recovery-owned | ||
| * navigations — performLogin and the route-restoration goto both call the | ||
| * overridden page.goto, and re-running recovery there would deadlock the | ||
| * override on the very claim it is nested inside. | ||
| */ | ||
| export function isRecoveryInProgress(page: Page): boolean { | ||
| return reloginInProgress.has(page); | ||
| } | ||
|
|
||
| /** | ||
| * Awaits any session recovery currently in progress for the page (no-op if | ||
| * none). Call this after a navigation so a test action doesn't race an | ||
| * in-flight re-login triggered by that same navigation. | ||
| */ | ||
| export async function awaitSessionRecovery(page: Page): Promise<void> { | ||
| const inProgress = reloginInProgress.get(page); | ||
| if (inProgress) { | ||
| await inProgress.catch(() => { | ||
| /* best-effort — recovery errors are surfaced by the failing test action */ | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Attaches a main-frame navigation listener that re-authenticates whenever a | ||
| * navigation lands on the login page. Returns a disposer to detach it. | ||
| */ | ||
| export function attachSessionRecovery(page: Page, testInfo: TestInfo): () => void { | ||
| const handler = (frame: Frame) => { | ||
| if (frame !== page.mainFrame()) { | ||
| return; | ||
| } | ||
| const url = frame.url(); | ||
| const onAuthUrl = isAuthUrl(url); | ||
| // Remember the most recent non-auth route so recovery can return to it. | ||
| if (url && url !== 'about:blank' && !onAuthUrl) { | ||
| lastAppUrl.set(page, url); | ||
| } | ||
| // On a normal (non-auth) navigation, detect the login page instantly so the | ||
| // hot path adds no latency. When we land on an auth URL the session likely | ||
| // expired but the login form may still be rendering, so give it a bounded | ||
| // window to appear before deciding recovery isn't needed. | ||
| // Fire-and-forget: recovery guards its own re-entrancy. Swallow errors so a | ||
| // transient navigation event doesn't reject an unrelated step. | ||
| void recoverSessionIfExpired(page, testInfo, onAuthUrl ? 5_000 : 0).catch(() => { | ||
| /* best-effort recovery */ | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }; | ||
| page.on('framenavigated', handler); | ||
| return () => page.off('framenavigated', handler); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: openshift/console
Length of output: 6073
🏁 Script executed:
Repository: openshift/console
Length of output: 22794
🏁 Script executed:
Repository: openshift/console
Length of output: 16321
🌐 Web query:
Playwright storageState({ path }) implementation atomic file write behavior💡 Result:
The Playwright
storageState({ path })method does not implement atomic file write behavior [1][2]. When this method is called, Playwright serializes the browser context's state (cookies, local storage, and optionally IndexedDB) and writes it directly to the specified path using standard file system operations [1]. Internally, Playwright uses Node.js'sfs.promises.writeFileto save the state as a JSON file [1][3][4]. This process creates or overwrites the file directly [1][2]. Because it does not use an atomic pattern—such as writing to a temporary file and then renaming it—there is a possibility that a process reading the file simultaneously could encounter a partially written or corrupted state if the write operation is interrupted [1][2]. Additionally, the method does not provide built-in protections against race conditions or concurrent writes to the same path [2]. If multiple test workers or processes attempt to callstorageStatewith the same output path simultaneously, their writes may interleave or cause conflicts [2]. To mitigate these risks in environments where concurrency is a concern, it is recommended to: 1. Use unique paths for different workers or test runs (e.g., by utilizingtest.info().outputPath()) [2]. 2. Avoid sharing the same state file across concurrent processes that might write to it [2]. 3. Treat the file as an immutable artifact once it has been generated by a setup process, ensuring that subsequent test workers only perform read operations [2].Citations:
Serialize storage-state writes by
statePath.WORKERScan run multiple workers in one project, but all admin workers usekubeadmin.jsonand all developer workers usedeveloper.json.saveStorageStatewrites directly without a lock or atomic replacement. Concurrent recovery can corrupt or overwrite the shared state. Use a lock keyed bystatePathor assign each worker a separate state file.🤖 Prompt for AI Agents