Skip to content

TB-964: fail fast on remote errors, close the tunnel on SIGINT/SIGTERM - #21

Open
jochen-testingbot wants to merge 1 commit into
mainfrom
fix/tb-964-fail-fast-tunnel-cleanup
Open

jochen-testingbot wants to merge 1 commit into
mainfrom
fix/tb-964-fail-fast-tunnel-cleanup

Conversation

@jochen-testingbot

Copy link
Copy Markdown
Contributor

Fixes https://testingbot-team.atlassian.net/browse/TB-964 (observed with 1.1.5 while running the Cypress tutorial).

Problems

  1. No failure reporting. When the remote Cypress run errors (Cypress 13 rejecting cypress.json, or "Cypress did not respond after 20 seconds"), the CLI polled until it was killed (900 s and 420 s in the audit). There were two causes:
    • getStatus() returned FAILED, but nothing handled it.
    • When the grid couldn't start the session, the hub hands back an error… session id. The web API then keeps that run READY, so the CLI only stops at its 30-minute READY cap. (Server side: CypressRunJob lacks the error-prefixed session guard that Espresso, XCUITest and Maestro have. That is noted on the ticket and not fixed here.)
  2. Tunnel leak. Ctrl-C left the tunnel READY server-side:
    • The handlers were registered only after archiving, so they missed the tunnel-start window.
    • SIGTERM wasn't handled.
    • exitHandler never called process.exit() when a tunnel was configured but not open.
  3. Crash. Tunnel.stop() dereferenced this.tunnel while it was undefined: TypeError: Cannot read properties of undefined (reading 'close').

Changes

  • Poller:
    • New static failure(response). A run counts as a terminal failure when its status is FAILED or unknown (the run's errors are included in the message), or when its test.sessionId starts with error.
    • check() rejects on the first such poll and marks the spinner failed.
    • API errors inside the interval are caught, so there are no unhandled rejections. Five consecutive errors reject.
    • The poll interval is injectable, for the tests.
  • Tunnel: stop() is idempotent and resolves when there is no tunnel. New isActive().
  • RunProject:
    • registerCloseHandlers() runs before the tunnel starts.
    • It handles SIGINT (exit 130), SIGTERM (exit 143), SIGUSR1/2, uncaughtException and unhandledRejection.
    • exitHandler(code) is idempotent: it stops the remote job, closes the tunnel if one is open, and always exits.
    • On a poll failure the CLI also calls stop_project, then closes the tunnel and exits 1 (as before, via finally).
  • npm test: node --test over test/*.test.js, against the compiled lib/. tsc || true because main already has type errors in src/index.ts (yargs typings) and the io.connect call. Those are pre-existing and not touched here.

Not done here:

  • Replacing request with fetch (also listed on the ticket) is a separate change. It needs engines raised to Node ≥ 18.
  • No version bump and no npm publish.

Verification

npm test
# tests 9   # pass 9   # fail 0

The tests cover:

  • FAILED with errors rejects with the message.
  • An error… session rejects on the first poll (in under 1 s, not 30 min).
  • DONE resolves.
  • A single API error is retried; five in a row reject.
  • Tunnel.stop() without a tunnel doesn't throw, and closes an open tunnel exactly once.
  • SIGINT exits 130 and SIGTERM exits 143, each closing the tunnel.
  • exitHandler exits even when the tunnel never opened.

Negative control: the same test file against main's compiled lib hangs on the first FAILED-run test and is killed by the 60 s timeout.

Live check (real tunnel on the shared account, Java 23):

  1. A small script builds a RunProject from lib/, starts the real tunnel and registers the handlers.
  2. It sends itself SIGINT once "Tunnel is ready" prints. Result: exit 130 after 7 s.
  3. GET /v1/tunnel/list → [] (it was also [] before the run).

- Poller: a FAILED (or unknown) run, or a run whose session id starts with
  'error' (the grid could not start Cypress), now rejects with the remote
  error instead of polling until killed. API errors no longer escape the
  interval as unhandled rejections; five in a row give up.
- Tunnel.stop() is idempotent and a no-op without a tunnel (it crashed with
  "Cannot read properties of undefined (reading 'close')").
- Signal handlers are registered before the tunnel starts, cover SIGTERM,
  and always close the tunnel and exit (130/143). On a remote failure the
  CLI also stops the remote project.
- node:test tests (npm test).
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.

1 participant