Fix Speedtest worker lifecycle cleanup

[sstidl]

Summary

• terminate workers and clear polling timers when a run completes or aborts
• guard asynchronous messages and callbacks from stale runs
• add end-to-end coverage for completion, abort, and restart lifecycle handling

Testing

• npm test -- --runInBand (project reports no automated test suite configured)

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix Speedtest worker lifecycle cleanup and stale-run guards

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Terminate workers and clear polling resources after completion or abort.
• Isolate runs to reject stale messages, callbacks, and abort timeouts.
• Cover completion, forced abort, restart, and stale-event behavior with Playwright.

Diagram

graph TD
  Start["start()"] --> Active["Active run"] --> Message{"Current run?"}
  Message -->|"terminal"| Finish["finish()"] --> Cleanup["Resource cleanup"] --> Onend["onend()"]
  Message -->|"stale"| Ignore["Ignore event"]
  Active -->|"abort()"| Grace["Abort grace"] -->|"response or timeout"| Finish
  Onend -->|"restart"| Start

Loading

High-Level Assessment

The per-run lifecycle object with identity guards is appropriate for this API because it keeps worker and timer ownership explicit without changing the public interface. A global generation counter or separate lifecycle class would add indirection without materially improving correctness for the current scope.

Files changed (2) +290 / -20

Bug fix (1) +66 / -20

speedtest.jsMake worker completion and abort cleanup run-safe +66/-20

Make worker completion and abort cleanup run-safe

• Introduces per-run worker and timer ownership with an idempotent finish path that clears intervals, cancels abort timeouts, terminates workers, and releases references. Guards messages and polling callbacks from stale runs, adds forced abort completion after one second, and permits safe restart from 'onend'.

speedtest.js

Tests (1) +224 / -0

worker-lifecycle.spec.jsAdd end-to-end worker lifecycle regression coverage +224/-0

Add end-to-end worker lifecycle regression coverage

• Adds Playwright tests using a fake Worker to verify normal cleanup, restart after completion or abort, synchronous restart from 'onend', forced abort timeout behavior, single completion notification, and rejection of delayed events from prior runs.

tests/e2e/worker-lifecycle.spec.js

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. Aborted runs can lose telemetry ✓ Resolved 🐞 Bug ≡ Correctness

Description

finish immediately calls worker.terminate() when the first status response reports state 5, even
though the worker starts aborted-run telemetry asynchronously and does not await it before setting
that state. With full or debug telemetry enabled, the next 200 ms poll—or the new one-second
fallback—can kill the worker while its telemetry request is in flight, contrary to the documented
promise to send timing data for aborted tests.

Code

speedtest.js[350]

+      worker.terminate();

Evidence

The main thread terminates the worker as soon as finish runs, and both terminal status messages
and the abort fallback reach that path. The worker's abort handler starts sendTelemetry but
immediately sets state 5, while sendTelemetry completes through later XMLHttpRequest callbacks;
the documentation explicitly defines full telemetry as including aborted tests.

speedtest.js[339-350]
speedtest.js[378-384]
speedtest.js[417-421]
speedtest_worker.js[247-255]
speedtest_worker.js[692-714]
doc.md[432-436]

Agent prompt

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

## Issue description
Aborted runs report terminal state before their asynchronous telemetry request completes, allowing the main thread to terminate the worker and cancel that request.
## Fix Focus Areas
- speedtest.js[339-350]
- speedtest.js[417-421]
- speedtest_worker.js[247-255]
- speedtest_worker.js[692-714]
## Recommended Fix
Add an abort-completion handshake so the worker reports terminal completion only after aborted-run telemetry succeeds or fails, and coordinate the forced-cleanup timeout so it does not preempt an acknowledged telemetry upload while still recovering from an unresponsive worker.

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

---

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 ↗

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Pull request overview

Fixes Speedtest worker lifecycle cleanup across completion, abort, restart, and stale asynchronous events.

Changes:

• Tracks and cleans up workers, timers, and abort timeouts.
• Guards against stale callbacks and worker messages.
• Adds lifecycle E2E coverage and updates dependencies.

File summaries

File	Description
tests/e2e/worker-lifecycle.spec.js	Tests completion, abort, restart, and stale-event handling.
speedtest.js	Implements worker and timer lifecycle management.
package-lock.json	Updates transitive dependency versions.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

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