Skip to content

test: cover browser recovery from a stale keep-alive socket - #34592

Open
jennifer-shehane wants to merge 2 commits into
developfrom
claude/chromium-socket-recovery-test-agg2oe
Open

test: cover browser recovery from a stale keep-alive socket#34592
jennifer-shehane wants to merge 2 commits into
developfrom
claude/chromium-socket-recovery-test-agg2oe

Conversation

@jennifer-shehane

@jennifer-shehane jennifer-shehane commented Aug 13, 2026

Copy link
Copy Markdown
Member

Additional details

With CYPRESS_INTERNAL_DISABLE_PROXY=1 browser traffic no longer goes through the Node MITM proxy — Cypress pauses requests via CDP Fetch and lets Chrome make them. Recovering from a request written to a dead pooled keep-alive socket therefore stops being Cypress's job and becomes the browser's, which leaves Cypress with an implicit dependency on Chromium's network stack that nothing tested.

1. New test for that dependency. network_error_stale_keepalive.cy.js runs warm/race fetch pairs against an origin that answers the first request on a connection and meets any request on a reused socket with a FIN and no response. It asserts three things:

  • the raced fetch still gets a 200 (the browser recovered);
  • the origin actually killed at least one reused socket, via a stats route — so a change in connection pooling cannot quietly turn this into a test that never races anything;
  • cy.intercept fired exactly once per logical request — this is the fragile property, and what would catch a Chromium change or a Cypress change that moved interception above the retry layer.

It is gated on CYPRESS_INTERNAL_DISABLE_PROXY === '1', mirroring non_proxied_spec.ts and cookies_spec.ts, so it activates when the flag flips.

2. Removed the skipped Google Chrome context (9 tests). These could not simply be unskipped — they were bit-rotted. launchBrowser calls the private chrome._getArgs(browser) with one argument while that helper now takes (browser, options, port), so every test threw TypeError: Cannot read properties of undefined (reading 'userAgent') before a browser launched. They also drove a raw browser rather than Cypress, so a failure described Chrome rather than anything Cypress guarantees. Removed along with the helpers, controllers and routes only they reached.

3. Wired up network_error_handling.cy.js, which was orphaned. It holds 9 Cypress-level tests (cy.visit/cy.request/subresource retries) and nothing in the repo referenced it, so it never ran — while seven harness routes existed solely to serve it. It now runs, with expectedExitCode: 2 for the two fails after retrying 5x tests that fail by design.

Its <form> body case only holds while Cypress proxies the traffic: with the proxy disabled the browser owns the retry and will not replay a non-idempotent request, so it times out. That case moved to its own spec which the harness selects by proxy mode. This is worth a reviewer's attention — the behavior change looks intentional, but nothing currently asserts either way, and if it is intentional a positive assertion pinning the new contract (a POST is not replayed when unproxied) would be a good follow-up.

Net effect: −9 dead tests, +9 live ones, 8 of which run in both proxy modes.

Steps to test

cd system-tests
CYPRESS_INTERNAL_DISABLE_PROXY=1 node ./scripts/run.js --glob-in-dir="test" network_error_handling
node ./scripts/run.js --glob-in-dir="test" network_error_handling

Results observed locally:

with flag without flag
Passing 6 6
Pending 2 (SNI + gated form test) 1 (stale keep-alive)
Failing 0 1

The one failure without the flag is does not connect to the upstream proxy for the SNI server request, which fails identically on develop in the same environment — a sandbox networking limitation interfering with DebugProxy, not a regression from this change.

Determinism of the socket-kill assertion was checked separately: instrumenting the stats route showed killedOnReusedSocket=5 on 5 consecutive runs, never varying, against a threshold of 1.

One caveat on verification: no Google Chrome build was installable in the sandbox, so the new test was exercised against Chromium 141.0.7390.37 rather than branded Chrome. Same version and network stack, but CI will be the first run on the exact binary.

How has the user experience changed?

No change — test-only.

PR Tasks

  • [na] Is there an associated issue with maintainer approval for PR submission?
  • Have tests been added/updated?
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

Generated by Claude Code


Note

Low Risk
Changes are limited to system-test specs, harness routes, and conditional skips; no product runtime code is modified.

Overview
Test-only refresh of network error system tests around proxy vs. direct-browser (CYPRESS_INTERNAL_DISABLE_PROXY) behavior.

Stale keep-alive: Adds network_error_stale_keepalive.cy.js and harness routes that simulate a FIN on a reused pooled socket. The spec checks the browser still gets 200, the server actually killed a reused socket, and cy.intercept only sees one event per logical request (no double-reporting of browser-level retries). That suite runs only when the internal proxy is disabled.

Previously orphaned Cypress retries: network_error_handling.cy.js is now executed from the harness (including the two tests that are expected to fail after max retries). The form POST body replay case moves to network_error_form_retry.cy.js and runs only when the Node MITM proxy is enabled, since body replay on reset is a proxy behavior.

Cleanup: Removes the skipped raw-Chrome launchBrowser context and related dead routes/controllers (proxy-invalid hosts, mid-response resets, etc.).

Reviewed by Cursor Bugbot for commit 5a5257e. Bugbot is set up for automated code reviews on this repo. Configure here.

claude added 2 commits August 13, 2026 14:25
With the MITM proxy disabled the browser reaches the origin directly, so
recovering a request written to a socket the origin has already closed is the
browser's job rather than Cypress's. Pin what Cypress depends on there: the
request succeeds, and a retry the browser performs below the interception layer
is not reported to the test as a second request.

The origin also counts the sockets it kills, so a change in connection pooling
cannot quietly turn this into a test that never races anything.

Refs: #24716

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XrvQo3bxCQTAM7vDx1m3Py
The `Google Chrome` context could not be unskipped: it calls the private
`chrome._getArgs(browser)` with one argument while that helper now takes
`(browser, options, port)`, so every test in it threw on `options.userAgent`
before a browser launched. It also drove a raw browser rather than Cypress, so a
failure there described Chrome rather than anything Cypress guarantees. Remove
it along with the helpers, controllers and routes only it reached.

`network_error_handling.cy.js` covers the Cypress-level contract instead — visit,
request and subresource retries — but nothing ever ran it. Wire it up. Its
`<form>` body case only holds while Cypress proxies the traffic, since the
browser will not replay a non-idempotent request, so that case moves to its own
spec the harness selects by proxy mode.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XrvQo3bxCQTAM7vDx1m3Py
@jennifer-shehane jennifer-shehane self-assigned this Aug 13, 2026
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.

2 participants