Skip to content

Prevent stability ping values from freezing on long tests - #851

Merged
sstidl merged 3 commits into
masterfrom
copilot/stability-test-ping-value-freezes-fix
Sep 12, 2026
Merged

Prevent stability ping values from freezing on long tests#851
sstidl merged 3 commits into
masterfrom
copilot/stability-test-ping-value-freezes-fix

Conversation

Copilot AI commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Stability tests repeatedly read performance.getEntries() for ping precision. The browser resource-timing buffer can fill during long runs, leaving the worker to reuse a stale entry and report a frozen ping value.

  • Resource timing lifecycle
    • Clear resource timings after each Performance API timing read, including fallback/error paths.
    • Preserve the existing wall-clock timing fallback when the API is unavailable.
try {
  // Read latest resource timing entry
} catch (e) {
  // Use estimate
} finally {
  performance.clearResourceTimings();
}
  • Regression coverage
    • Add a browser-worker test that instruments clearResourceTimings() and verifies it runs after a successful ping.

Co-authored-by: sstidl <12804296+sstidl@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix ping value freezing issue after 60 seconds Prevent stability ping values from freezing on long tests Sep 12, 2026
Copilot AI requested a review from sstidl September 12, 2026 10:08
@sstidl
sstidl marked this pull request as ready for review September 12, 2026 10:10
Copilot AI lite review requested due to automatic review settings September 12, 2026 10:10
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Prevent frozen stability ping values by clearing resource timings

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Clear browser resource timings after every Performance API ping measurement.
• Add worker regression coverage verifying successful pings clear accumulated timing entries.
Diagram

sequenceDiagram
  actor P as Stability Page
  participant W as Stability Worker
  participant B as Ping Backend
  participant A as Performance API
  participant R as Ping Recorder
  P->>W: Start test
  W->>B: Send ping
  B-->>W: Complete response
  W->>A: Read latest timing
  A-->>W: Return timing entry
  W->>A: Clear timings
  W->>R: Record latency
Loading
High-Level Assessment

Clearing resource timings in a finally block immediately after each read is the least invasive and most reliable approach: it covers successful, fallback, and error paths while preserving wall-clock latency estimation. Clearing before the read would discard the current sample, while threshold-based cleanup would add unnecessary state and still permit stale entries.

Files changed (2) +43 / -0

Bug fix (1) +4 / -0
stability_worker.jsClear resource timings after each Performance API measurement +4/-0

Clear resource timings after each Performance API measurement

• Adds a finally block around resource timing lookup so the worker clears accumulated entries after every attempted read. Failures remain ignored, preserving the existing wall-clock timing fallback when the Performance API is unavailable.

stability_worker.js

Tests (1) +39 / -0
stability.spec.jsVerify stability pings clear browser resource timings +39/-0

Verify stability pings clear browser resource timings

• Loads and instruments the stability worker source in a browser test, intercepts a successful ping request, and waits for clearResourceTimings() to be called. The worker is terminated after the assertion to avoid leaking test resources.

tests/e2e/stability.spec.js

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. The new browser test always times out ✓ Resolved 🐞 Bug ≡ Correctness
Description
The route fulfillment at line 131 replaces the backend response with a headerless response, removing
the cross-origin permission normally emitted by empty.php. Because the blob worker runs under the
standalone origin while requesting the separate backend origin, its request reaches xhr.onerror
rather than the xhr.onload branch containing clearResourceTimings(), so the assertion rejects
after ten seconds.
Code

tests/e2e/stability.spec.js[131]

+    await page.route(`${baseUrls.backend}/empty.php?cors=true&r=*`, route => route.fulfill({ status: 200, body: "" }));
Evidence
The test loads its page from the standalone server and sends the worker ping to the backend server,
whose configured URLs use different ports and therefore different origins. The real empty.php
explicitly emits Access-Control-Allow-Origin: * when cors is present, but the added route
replaces that response with only a status and body; the worker only clears timings in its successful
xhr.onload path, while failed requests go through xhr.onerror without clearing them.

tests/e2e/helpers/env.js[1-5]
backend/empty.php[3-9]
stability_worker.js[174-203]
tests/e2e/stability.spec.js[129-160]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test fulfills a cross-origin worker request without the CORS response header that the real backend supplies, preventing the successful ping required to invoke `clearResourceTimings()`.
## Fix Focus Areas
- tests/e2e/stability.spec.js[129-160]
## Recommended Fix
Include `Access-Control-Allow-Origin: *` in the headers passed to `route.fulfill`, while preserving the successful status and empty body.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread tests/e2e/stability.spec.js Outdated

Copilot AI 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.

🟡 Changes recommended

The regression test’s cross-origin response lacks the required CORS header and may time out.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents stale ping measurements during long stability tests by clearing resource-timing entries after reads.

Changes:

  • Adds cleanup with fallback-safe timing behavior.
  • Adds browser-worker regression coverage for cleanup.
File summaries
File Description
tests/e2e/stability.spec.js Adds regression coverage for resource-timing cleanup.
stability_worker.js Clears resource timings after ping measurements.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/e2e/stability.spec.js Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

🟡 Changes recommended

The regression test’s cross-origin response lacks the required CORS header and may time out....

Fixed in 9eae35e by adding Access-Control-Allow-Origin: * to the mocked route.fulfill response while keeping status 200 and an empty body.

@sstidl
sstidl merged commit fa2e286 into master Sep 12, 2026
3 checks passed
@sstidl
sstidl deleted the copilot/stability-test-ping-value-freezes-fix branch September 12, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stability test: ping value freezes after ~60s due to unbounded performance.getEntries() buffer. Fix included

3 participants