Skip to content

OCPBUGS-105789: Recover expired Playwright sessions via auto re-login - #16954

Open
rhamilto wants to merge 7 commits into
openshift:mainfrom
rhamilto:OCPBUGS-105789
Open

OCPBUGS-105789: Recover expired Playwright sessions via auto re-login#16954
rhamilto wants to merge 7 commits into
openshift:mainfrom
rhamilto:OCPBUGS-105789

Conversation

@rhamilto

@rhamilto rhamilto commented Aug 11, 2026

Copy link
Copy Markdown
Member

Analysis / Root cause:

The Playwright e2e suite authenticates exactly once, in the setup projects (admin-auth.setup.ts, developer-auth.setup.ts), and freezes the session into a storageState snapshot. Every test project loads that static snapshot (playwright.config.ts) and nothing ever re-authenticates — performLogin is only referenced by the two setup files.

When the OpenShift OAuth token expires during a long run, subsequent page.goto() calls silently redirect to the login page, and tests hang waiting for elements (e.g. user-dropdown-toggle, #page-sidebar) that never appear, failing with generic toBeVisible/test-timeout errors. This showed up as widespread login-page hangs in CI.

Jira: https://redhat.atlassian.net/browse/OCPBUGS-105789

Solution description:

Add a self-healing auth fixture that overrides Playwright's built-in page fixture:

  • e2e/fixtures/auth-fixture.ts (new) — attaches a main-frame framenavigated listener that detects when a navigation lands on the login page and transparently re-runs performLogin for the active persona, then re-saves storageState so later tests reuse the fresh session. After re-login it restores the route the test was navigating to, so deep-link tests resume where they intended rather than on the console home page. Concurrency is handled by claiming the re-login slot synchronously (closing a TOCTOU window where two navigation events could each start a login), and recovery-owned navigations bypass the page.goto override so they don't deadlock on their own in-progress claim. A failed re-login rejects the shared claim so any joining caller observes the failure instead of a false success. It logs a warning when recovery fires so future occurrences are visible in CI logs.
  • e2e/setup/login-helper.ts — adds isOnLoginPage() and resolveCredentialsForStorageState() (dispatches to the existing getAdminCredentials()/getDeveloperCredentials() helpers based on the project's storage-state filename). isOnLoginPage() supports an instantaneous check on the hot path and a bounded wait for still-rendering OAuth redirects; it uses a non-waiting isVisible() for the zero-timeout case because Playwright treats waitFor({ timeout: 0 }) as "wait forever".
  • e2e/fixtures/index.ts — wires recovery into the shared page fixture and wraps page.goto so callers await any recovery the navigation itself triggered. Since every spec imports test from here, coverage is suite-wide with no per-test changes.
  • e2e/tests/console/app/session-recovery.spec.ts (new) — regression test pinning the login-page detection primitive the recovery flow depends on. It renders markup via data: URLs so no cluster is required.

Auth-disabled clusters and unset developer credentials cause recovery to no-op safely.

Depends on #16953 (OCPBUGS-105609), which refactors the same setup/login helpers this change builds on. Until #16953 merges, this PR's diff will also include those changes; once it merges, only this change remains. Please merge #16953 first.

Screenshots / screen recording:

Test setup:

Requires a cluster whose OAuth access-token lifetime is shorter than the full e2e run so the session expires mid-run; the recovery path then re-authenticates transparently. The session-recovery.spec.ts detection tests run without a cluster.

Test cases:

  • Full Playwright e2e run outlasting the OAuth token: post-expiry tests recover instead of hanging on the login page.
  • Admin and developer personas each re-authenticate with the correct credentials.
  • Deep-link test whose navigation triggers re-login resumes on its intended route.
  • Auth-disabled cluster: recovery no-ops, tests unaffected.
  • Unset developer credentials: recovery no-ops without error.
  • session-recovery.spec.ts: login-page detection by data-test-id, by username/password inputs, non-detection of an authenticated app page, bounded wait for a slow-rendering login form, and awaitSessionRecovery no-op when idle.

Browser conformance:

  • Chrome

Additional info:

Test-infrastructure only — no product/runtime code changes.

Reviewers and assignees:

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved recovery from expired sessions with automatic re-authentication and restoration of the intended page.
    • Increased reliability for Web Terminal loading, navigation, visibility checks, and configuration actions.
  • Tests

    • Added coverage for session recovery, login detection, delayed authentication, and deep-link preservation.
    • Improved authentication setup using configured credentials.
    • Expanded coverage for slower Web Terminal scenarios with extended timeouts and retry handling.

rhamilto and others added 2 commits August 11, 2026 13:51
Addresses multiple sources of flakiness in the web terminal Playwright
e2e tests:

- warmupSPA now detects the login page and re-authenticates instead of
  retrying until timeout when the auth session has expired
- waitForTerminalIconVisible replaced manual retry loop with toPass()
  to prevent "page closed" errors when the test timeout fires mid-reload
- waitForTerminalWindow default timeout increased from 60s to 120s for
  slow DevWorkspace provisioning
- navigateToWebTerminalConfig uses longer timeouts for the actions menu
  and Web Terminal tab which load dynamically
- All web terminal tests marked test.slow() to triple the default timeout
- Extracted getBaseURL, getAdminCredentials, getDeveloperCredentials into
  login-helper.ts to eliminate duplication across auth setup files and
  warmupSPA

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Playwright e2e suite authenticates once in the setup projects and
freezes the session into a storageState snapshot that every test reuses.
Nothing re-authenticates, so when the OpenShift OAuth token expires during
a long run, navigations silently redirect to the login page and tests hang
waiting for elements that never appear.

Add a self-healing auth fixture that overrides the built-in page fixture:
a main-frame navigation listener detects when a navigation lands on the
login page and transparently re-runs performLogin for the active persona
(resolved from the project storageState path), then refreshes the stored
session so later tests reuse the fresh state. Re-entrancy is guarded so
the login flow's own navigations don't recurse.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 11, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@rhamilto: This pull request references Jira Issue OCPBUGS-105789, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Analysis / Root cause:

The Playwright e2e suite authenticates exactly once, in the setup projects (admin-auth.setup.ts, developer-auth.setup.ts), and freezes the session into a storageState snapshot. Every test project loads that static snapshot (playwright.config.ts) and nothing ever re-authenticates — performLogin is only referenced by the two setup files.

When the OpenShift OAuth token expires during a long run, subsequent page.goto() calls silently redirect to the login page, and tests hang waiting for elements (e.g. user-dropdown-toggle, #page-sidebar) that never appear, failing with generic toBeVisible/test-timeout errors. This showed up as widespread login-page hangs in CI.

Jira: https://redhat.atlassian.net/browse/OCPBUGS-105789

Solution description:

Add a self-healing auth fixture that overrides Playwright's built-in page fixture:

  • e2e/fixtures/auth-fixture.ts (new) — attaches a main-frame framenavigated listener that detects when a navigation lands on the login page and transparently re-runs performLogin for the active persona, then re-saves storageState so later tests reuse the fresh session. Re-entrancy is guarded via a WeakMap so the login flow's own navigations don't recurse, and it logs a warning when recovery fires so future occurrences are visible in CI logs.
  • e2e/setup/login-helper.ts — adds isOnLoginPage() and resolveCredentialsForStorageState() (dispatches to the existing getAdminCredentials()/getDeveloperCredentials() helpers based on the project's storage-state filename).
  • e2e/fixtures/index.ts — wires recovery into the shared page fixture. Since every spec imports test from here, coverage is suite-wide with no per-test changes.

Auth-disabled clusters and unset developer credentials cause recovery to no-op safely.

Depends on #16953 (OCPBUGS-105609), which refactors the same setup/login helpers this change builds on. Until #16953 merges, this PR's diff will also include those changes; once it merges, only this change remains. Please merge #16953 first.

Screenshots / screen recording:

Test setup:

Requires a cluster whose OAuth access-token lifetime is shorter than the full e2e run so the session expires mid-run; the recovery path then re-authenticates transparently.

Test cases:

  • Full Playwright e2e run outlasting the OAuth token: post-expiry tests recover instead of hanging on the login page.
  • Admin and developer personas each re-authenticate with the correct credentials.
  • Auth-disabled cluster: recovery no-ops, tests unaffected.
  • Unset developer credentials: recovery no-ops without error.

Browser conformance:

  • Chrome

Additional info:

Test-infrastructure only — no product/runtime code changes.

Reviewers and assignees:

🤖 Generated with Claude Code

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@rhamilto

Copy link
Copy Markdown
Member Author

/hold for #16953 to merge first

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR centralizes Playwright authentication, adds session recovery after login redirects, integrates recovery into page navigation, increases Web Terminal wait and retry limits, and marks affected tests as slow.

Changes

E2E reliability

Layer / File(s) Summary
Shared authentication contracts
frontend/e2e/setup/login-helper.ts, frontend/e2e/setup/admin-auth.setup.ts, frontend/e2e/setup/developer-auth.setup.ts
Shared helpers resolve the console URL, credentials, storage-state personas, and login-page status. Authentication setup uses the shared login flow.
Session recovery and fixture integration
frontend/e2e/fixtures/auth-fixture.ts, frontend/e2e/fixtures/index.ts, frontend/e2e/tests/console/app/session-recovery.spec.ts
Session recovery tracks intended URLs, detects login redirects, prevents concurrent recovery, refreshes storage state, restores routes, and coordinates page navigation. Tests cover login detection, bounded waiting, synchronization, and deep-link preservation.
Web Terminal timing and test execution
frontend/e2e/pages/web-terminal-config-page.ts, frontend/e2e/pages/web-terminal-page.ts, frontend/e2e/tests/webterminal/...
Page actions use longer and retryable waits. Affected tests are marked slow, and an unused import is removed.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as Playwright test
  participant Fixture as page fixture
  participant Recovery as session recovery
  participant Login as login helper
  participant Page as Playwright Page
  Test->>Fixture: navigate with page.goto
  Fixture->>Recovery: record URL and check expiration
  Recovery->>Page: detect login redirect
  Recovery->>Login: resolve credentials
  Login-->>Recovery: return credentials
  Recovery->>Page: performLogin and save storage state
  Recovery->>Page: restore intended URL
  Fixture-->>Test: return navigation response
Loading

Suggested labels: verified, component/core

Suggested reviewers: fsgreco, leo6leo

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Jira issue and the main change: automatic recovery of expired Playwright sessions through re-login.
Description check ✅ Passed The description covers root cause, solution, testing, browser coverage, dependencies, and scope, with sufficient detail for review.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The PR adds Playwright test titles only; the diff contains no Ginkgo It, Describe, Context, or When titles, and the added titles use static text.
Test Structure And Quality ✅ Passed The PR changes frontend TypeScript Playwright fixtures and specs. The added test uses test.describe/test and expect; no changed file contains Ginkgo It, Eventually, BeforeEach, or AfterEach code.
Microshift Test Compatibility ✅ Passed The PR adds Playwright TypeScript tests using test.describe/test, not new Ginkgo e2e tests; therefore the MicroShift API compatibility check does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The full PR diff contains only frontend/e2e TypeScript files and no Go/Ginkgo tests; added coverage uses Playwright test.describe/test and has no multi-node or HA assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed The PR diff contains only frontend e2e TypeScript fixtures, helpers, page objects, and tests; it adds no deployment manifests, operator/controller code, or scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The PR diff changes only 12 frontend/e2e TypeScript files, with no Go or OTE binary changes; its sole console.warn is not stdout from an OTE process-level setup.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR adds only frontend TypeScript Playwright tests, not Ginkgo tests. The new test uses data:text/html pages and example URL strings without IPv4 assumptions or external network requests.
No-Weak-Crypto ✅ Passed The PR diff adds authentication recovery and timeout logic only; searches found no MD5, SHA1, DES, RC4, Blowfish, ECB, crypto APIs, custom crypto, or secret comparisons.
Container-Privileges ✅ Passed The PR changes only 12 TypeScript files. The diff contains no container/Kubernetes manifests or matches for privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation.
No-Sensitive-Data-In-Logs ✅ Passed The PR adds one warning with only the static test title path; it does not log passwords, tokens, URLs, hostnames, session IDs, or customer data.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2026
@openshift-ci
openshift-ci Bot requested review from Leo6Leo and fsgreco August 11, 2026 19:21
@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@rhamilto: This pull request references Jira Issue OCPBUGS-105789, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

Analysis / Root cause:

The Playwright e2e suite authenticates exactly once, in the setup projects (admin-auth.setup.ts, developer-auth.setup.ts), and freezes the session into a storageState snapshot. Every test project loads that static snapshot (playwright.config.ts) and nothing ever re-authenticates — performLogin is only referenced by the two setup files.

When the OpenShift OAuth token expires during a long run, subsequent page.goto() calls silently redirect to the login page, and tests hang waiting for elements (e.g. user-dropdown-toggle, #page-sidebar) that never appear, failing with generic toBeVisible/test-timeout errors. This showed up as widespread login-page hangs in CI.

Jira: https://redhat.atlassian.net/browse/OCPBUGS-105789

Solution description:

Add a self-healing auth fixture that overrides Playwright's built-in page fixture:

  • e2e/fixtures/auth-fixture.ts (new) — attaches a main-frame framenavigated listener that detects when a navigation lands on the login page and transparently re-runs performLogin for the active persona, then re-saves storageState so later tests reuse the fresh session. Re-entrancy is guarded via a WeakMap so the login flow's own navigations don't recurse, and it logs a warning when recovery fires so future occurrences are visible in CI logs.
  • e2e/setup/login-helper.ts — adds isOnLoginPage() and resolveCredentialsForStorageState() (dispatches to the existing getAdminCredentials()/getDeveloperCredentials() helpers based on the project's storage-state filename).
  • e2e/fixtures/index.ts — wires recovery into the shared page fixture. Since every spec imports test from here, coverage is suite-wide with no per-test changes.

Auth-disabled clusters and unset developer credentials cause recovery to no-op safely.

Depends on #16953 (OCPBUGS-105609), which refactors the same setup/login helpers this change builds on. Until #16953 merges, this PR's diff will also include those changes; once it merges, only this change remains. Please merge #16953 first.

Screenshots / screen recording:

Test setup:

Requires a cluster whose OAuth access-token lifetime is shorter than the full e2e run so the session expires mid-run; the recovery path then re-authenticates transparently.

Test cases:

  • Full Playwright e2e run outlasting the OAuth token: post-expiry tests recover instead of hanging on the login page.
  • Admin and developer personas each re-authenticate with the correct credentials.
  • Auth-disabled cluster: recovery no-ops, tests unaffected.
  • Unset developer credentials: recovery no-ops without error.

Browser conformance:

  • Chrome

Additional info:

Test-infrastructure only — no product/runtime code changes.

Reviewers and assignees:

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

  • Improved end-to-end test recovery when sessions expire, including automatic re-authentication and session refresh.

  • Increased resilience for Web Terminal loading, navigation, and visibility checks.

  • Tests

  • Updated authentication setup to use configured credentials consistently.

  • Allowed longer execution times for slower Web Terminal scenarios and configuration tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/e2e/fixtures/auth-fixture.ts`:
- Around line 81-85: Update attachSessionRecovery and the navigation flow used
by performLogin so session recovery is awaited rather than started
fire-and-forget. Wrap navigation to preserve the original target, recover an
expired session, and retry that target before allowing subsequent test actions
to continue; add a regression test covering an expired-session redirect.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c0fb6ea2-b79d-45c4-bf5a-1aff73050158

📥 Commits

Reviewing files that changed from the base of the PR and between a9c6d3b and 0862123.

📒 Files selected for processing (11)
  • frontend/e2e/fixtures/auth-fixture.ts
  • frontend/e2e/fixtures/index.ts
  • frontend/e2e/pages/web-terminal-config-page.ts
  • frontend/e2e/pages/web-terminal-page.ts
  • frontend/e2e/setup/admin-auth.setup.ts
  • frontend/e2e/setup/developer-auth.setup.ts
  • frontend/e2e/setup/login-helper.ts
  • frontend/e2e/tests/webterminal/developer/web-terminal-basic.spec.ts
  • frontend/e2e/tests/webterminal/developer/web-terminal-devuser.spec.ts
  • frontend/e2e/tests/webterminal/web-terminal-admin.spec.ts
  • frontend/e2e/tests/webterminal/web-terminal-config.spec.ts

Comment thread frontend/e2e/fixtures/auth-fixture.ts
- Restore the intended route after re-login so deep-link tests resume where
  they were headed instead of on the console home page (CodeRabbit).
- Detect the login page with a bounded wait on auth-URL navigations so a
  still-rendering OAuth login form isn't missed, while keeping normal
  navigations on an instantaneous check to avoid per-navigation latency
  (CodeRabbit + timing race).
- Claim the re-login slot synchronously to close a TOCTOU window where two
  concurrent navigation events could each launch a login.
- Hoist the Frame type import and clarify the proactive-check comment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rhamilto

Copy link
Copy Markdown
Member Author

/pipeline required

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-gcp-console
/test e2e-playwright

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/e2e/fixtures/auth-fixture.ts`:
- Around line 31-32: Update isAuthUrl to parse the URL and evaluate only its
normalized pathname, avoiding matches from query strings or unrelated URL text.
Anchor the authentication-path regex to the complete pathname shape with ^ and
$, while preserving recognition of OAuth, OAuth2, login, and auth routes.
- Around line 60-68: Update the authentication recovery flow around
saveStorageState to serialize storage-state writes by statePath. Use a shared
lock keyed by each statePath so concurrent workers targeting kubeadmin.json or
developer.json cannot write simultaneously, and ensure the lock is released on
every success and failure path; preserve the existing re-login coordination
behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b870fd4e-cb46-41f2-92bd-70e5cc86b2e9

📥 Commits

Reviewing files that changed from the base of the PR and between 0862123 and 9be4cc2.

📒 Files selected for processing (3)
  • frontend/e2e/fixtures/auth-fixture.ts
  • frontend/e2e/fixtures/index.ts
  • frontend/e2e/setup/login-helper.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/e2e/fixtures/index.ts
  • frontend/e2e/setup/login-helper.ts

Comment thread frontend/e2e/fixtures/auth-fixture.ts Outdated
Comment on lines +31 to +32
function isAuthUrl(url: string): boolean {
return /\/oauth\/|\/oauth2\/|\/login(\/|$|\?)|\/auth\//.test(url);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Anchor authentication URL matching to the pathname.

The current expression matches /oauth/ and /auth/ anywhere in the full URL. A normal application route or query value can then be classified as an auth URL. This prevents lastAppUrl updates and can start unnecessary recovery.

Parse the URL, normalize its pathname, and match the complete auth-path shape.

Proposed fix
 function isAuthUrl(url: string): boolean {
-  return /\/oauth\/|\/oauth2\/|\/login(\/|$|\?)|\/auth\//.test(url);
+  const pathname = new URL(url).pathname.normalize('NFC');
+  return /^\/(?:oauth2?|login|auth)(?:\/.*)?$/.test(pathname);
 }

As per path instructions, “Normalize Unicode and anchor regexes (^$); watch for ReDoS.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function isAuthUrl(url: string): boolean {
return /\/oauth\/|\/oauth2\/|\/login(\/|$|\?)|\/auth\//.test(url);
function isAuthUrl(url: string): boolean {
const pathname = new URL(url).pathname.normalize('NFC');
return /^\/(?:oauth2?|login|auth)(?:\/.*)?$/.test(pathname);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/e2e/fixtures/auth-fixture.ts` around lines 31 - 32, Update isAuthUrl
to parse the URL and evaluate only its normalized pathname, avoiding matches
from query strings or unrelated URL text. Anchor the authentication-path regex
to the complete pathname shape with ^ and $, while preserving recognition of
OAuth, OAuth2, login, and auth routes.

Source: Path instructions

Comment on lines +60 to +68
// 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 once we know whether a
// recovery is actually needed; if not, we release the slot immediately.
let release!: () => void;
const claim = new Promise<void>((resolve) => {
release = resolve;
});
reloginInProgress.set(page, claim);

Copy link
Copy Markdown
Contributor

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:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- storage-state configuration ---'
fd -i 'playwright.config.*|package.json' frontend -x sh -c \
  'printf "\n--- %s ---\n" "$1"; rg -n -C 3 "storageState|workers|fullyParallel" "$1" || true' sh {}

printf '%s\n' '--- storage-state persistence implementation ---'
rg -n -C 5 --glob '*.ts' \
  'function saveStorageState|const saveStorageState|saveStorageState\(|storageState\(\{.*path' \
  frontend/e2e

Repository: openshift/console

Length of output: 6073


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- playwright projects and storage-state paths ---'
sed -n '1,190p' frontend/playwright.config.ts

printf '%s\n' '--- auth fixture and fixture scopes ---'
sed -n '1,150p' frontend/e2e/fixtures/auth-fixture.ts
sed -n '1,180p' frontend/e2e/fixtures/index.ts

printf '%s\n' '--- storage-state path definitions and save helper ---'
rg -n -C 8 --glob '*.ts' \
  'adminStorageState|developerStorageState|STORAGE_STATE_DIR|storageState\(\{ path|saveStorageState' frontend/e2e frontend/playwright.config.ts

Repository: openshift/console

Length of output: 22794


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Playwright version ---'
rg -n -C 2 '"`@playwright/test`"|"playwright"' frontend/package.json package.json frontend/yarn.lock yarn.lock pnpm-lock.yaml package-lock.json 2>/dev/null || true

printf '%s\n' '--- repository-level synchronization around auth-state writes ---'
rg -n -C 4 --glob '!node_modules/**' \
  'kubeadmin\.json|developer\.json|storageState\(\{ *path|saveStorageState|lockfile|proper-lockfile|mkdir.*lock|writeFile.*storage' .

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's fs.promises.writeFile to 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 call storageState with 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 utilizing test.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.

WORKERS can run multiple workers in one project, but all admin workers use kubeadmin.json and all developer workers use developer.json. saveStorageState writes directly without a lock or atomic replacement. Concurrent recovery can corrupt or overwrite the shared state. Use a lock keyed by statePath or assign each worker a separate state file.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/e2e/fixtures/auth-fixture.ts` around lines 60 - 68, Update the
authentication recovery flow around saveStorageState to serialize storage-state
writes by statePath. Use a shared lock keyed by each statePath so concurrent
workers targeting kubeadmin.json or developer.json cannot write simultaneously,
and ensure the lock is released on every success and failure path; preserve the
existing re-login coordination behavior.

The prior review pass introduced a bounded-wait path in isOnLoginPage that
called locator.waitFor({ timeout: timeoutMs }). Playwright treats timeout: 0
as "wait forever" rather than "check instantaneously", so the hot-path
callers that pass 0 would have blocked every navigation indefinitely,
deadlocking the whole suite.

Guard the zero case with a non-waiting isVisible() check and only fall back
to waitFor when a bounded window is explicitly requested.

Add e2e/tests/console/app/session-recovery.spec.ts, which pins the login-page
detection primitive the recovery flow depends on. It renders markup via
data: URLs so no cluster is required, and it caught the timeout:0 deadlock.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rhamilto

Copy link
Copy Markdown
Member Author

/pipeline required

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-gcp-console
/test e2e-playwright

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/e2e/fixtures/auth-fixture.ts`:
- Around line 108-112: Update the navigation wrapper around
recoverSessionIfExpired so recovery-initiated navigations are marked before
invoking the original page.goto, allowing the wrapper to bypass recovery for
those calls and avoid waiting on its own claim. Apply the same marker/skip
behavior to navigations within performLogin, and add a regression test covering
route restoration through the wrapped page fixture when intendedUrl is set.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6efc1919-1349-4967-96bf-e12c4a4f23d1

📥 Commits

Reviewing files that changed from the base of the PR and between 9be4cc2 and d34792e.

📒 Files selected for processing (4)
  • frontend/e2e/fixtures/auth-fixture.ts
  • frontend/e2e/fixtures/index.ts
  • frontend/e2e/setup/login-helper.ts
  • frontend/e2e/tests/console/app/session-recovery.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/e2e/fixtures/index.ts
  • frontend/e2e/setup/login-helper.ts

Comment thread frontend/e2e/fixtures/auth-fixture.ts
Address two issues from the pre-push review:

- The page.goto override drove recovery, but recovery itself navigates
  through that same overridden goto (performLogin's initial goto and the
  route-restoration goto). Re-entering recovery there would await the very
  claim promise it is nested inside, deadlocking the override. Add an
  isRecoveryInProgress guard so recovery-owned navigations pass straight
  through to the original goto.

- The shared recovery claim resolved even when performLogin threw, so a
  caller that joined an in-flight recovery (the reloginInProgress branch)
  saw a false success. Reject the claim on failure so every awaiter
  observes the same outcome, with a no-op catch on the stored copy to
  avoid an unhandled rejection when nobody joins.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rhamilto

Copy link
Copy Markdown
Member Author

/pipeline required
/label tide/merge-method-squash

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-gcp-console
/test e2e-playwright

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/e2e/fixtures/auth-fixture.ts`:
- Around line 75-86: Update the reloginInProgress map and shared claim in
recoverSessionIfExpired to use Promise<boolean>, return the existing claim
directly to joiners, resolve false when the login page is absent or credentials
are unavailable, and resolve true only after successful recovery.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 95f85d2f-bd3e-4e0e-8124-8ca14e446085

📥 Commits

Reviewing files that changed from the base of the PR and between d34792e and bd0600d.

📒 Files selected for processing (2)
  • frontend/e2e/fixtures/auth-fixture.ts
  • frontend/e2e/fixtures/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/e2e/fixtures/index.ts

Comment on lines +75 to +86
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Propagate the actual recovery result to joiners.

The shared claim is Promise<void>, but recoverSessionIfExpired returns Promise<boolean>. A caller that joins at Lines 64-67 always receives true. The owner resolves the claim without recovery when the login page is not detected or credentials are unavailable.

Use Promise<boolean> for the claim. Resolve false on both no-op paths and true after successful recovery. Return the existing claim to joiners.

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 reloginInProgress map value type to Promise<boolean>.

Also applies to: 117-118

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/e2e/fixtures/auth-fixture.ts` around lines 75 - 86, Update the
reloginInProgress map and shared claim in recoverSessionIfExpired to use
Promise<boolean>, return the existing claim directly to joiners, resolve false
when the login page is absent or credentials are unavailable, and resolve true
only after successful recovery.

…links

Address the second-round pre-push review:

- awaitSessionRecovery no longer swallows recovery rejections. A re-login
  failure now propagates through the page.goto override so the active test
  fails with the original error instead of silently proceeding on the login
  page.

- Record the intended destination in the page.goto override before
  navigating. When a deep link immediately redirects to the login page, the
  framenavigated handler may only ever observe the auth URL, so relying on
  it alone would restore a stale route; capturing the target up front makes
  recovery return to where the test was headed.

Add a regression test covering deep-link recording: an app URL is
remembered, and a subsequent auth-URL or about:blank navigation does not
overwrite it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saveStorageState wrote directly to the destination file. Playwright workers
are separate processes, so two workers recovering the same persona could
interleave writes to the same storageState file, leaving another worker
loading that file to observe half-written, invalid JSON.

Write to a per-process temp file and rename it into place. A same-directory
rename is atomic, so a concurrent reader always sees a complete file. Clean
up the temp file if the write fails.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rhamilto

Copy link
Copy Markdown
Member Author

/pipeline required
/test backend

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-gcp-console
/test e2e-playwright

@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@rhamilto: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-playwright a504855 link false /test e2e-playwright
ci/prow/backend a504855 link true /test backend

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@rhamilto

Copy link
Copy Markdown
Member Author

This may be overkill in light of #16911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants