Skip to content

fix(ci): replace fixed sleeps with active readiness polls to eliminate startup-timing flakes - #1091

Merged
kriszyp merged 6 commits into
mainfrom
kris/fix-ci-startup-timing
Jun 2, 2026
Merged

fix(ci): replace fixed sleeps with active readiness polls to eliminate startup-timing flakes#1091
kriszyp merged 6 commits into
mainfrom
kris/fix-ci-startup-timing

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

Three related CI flakes — all pure timing, all clear on re-run — caused by fixed-duration sleeps that don't hold on slow or contended CI runners:

  • ECONNREFUSED 9925 (Integration API Tests, Linux + Windows): The sleep 10 after harper.js start wasn't sufficient for Harper to bind port 9925 on slow runners. Replaced with TCP-connect poll loops (120s cap): nc -z loop on Linux, TcpClient loop in PowerShell on Windows. The install sleep 10 was also removed — it was cargo-culted; harper.js install is synchronous and doesn't need one.
  • Probe /CookieTest did not become ready within 60000ms (Integration Tests 3/4, Node.js v22): restartHttpWorkers in lifecycle.mjs uses a 2s per-probe timeout; under shard contention the cumulative retries exhausted the 60s default. Extended the readiness budget to 120s at all restartHttpWorkers / installAppComponent call sites in the affected test files.
  • Fixed-sleep restarts in restart.mjs (legacy API test path): restartWithTimeout and restartServiceHttpWorkersWithTimeout both used a raw setTimeout(timeout) — 45s for full restarts, 15s for http_workers restarts. Replaced with active TCP-connect polls (port 9925 for full restarts, port 9926 for http_workers restarts).
  • HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS: 120000 added to the Linux sharded integration-test job (Windows already had 180000) so startHarper's 'successfully started' deadline scales on slow runners.

Notes for reviewer

  • The restart.mjs TCP-connect probe for restartServiceHttpWorkersWithTimeout confirms the REST port (9926) is accepting connections, but does not wait for specific component routes to register. This is intentional — lifecycle.mjs::restartHttpWorkers (used by the modern .test.mjs suites) handles that probe; restart.mjs serves the legacy testSuite.mjs path where the component route isn't known to the callee.
  • The Windows PowerShell poll loop uses do/until with try/catch around TcpClient.Connect — moderately confident it's right, but worth a review eyeball since I can't run PowerShell locally.

Generated by Claude Sonnet 4.6.

…e startup-timing flakes

Three related CI failures were caused by fixed-duration sleeps that don't
hold on slow runners:

1. `ECONNREFUSED 9925` in API test jobs (Linux + Windows): the `sleep 10`
   after `harper.js start` was not enough time for Harper to bind port 9925
   on a contended runner. Replaced with a TCP-connect poll loop (nc on
   Linux, TcpClient on Windows) that exits as soon as port 9925 accepts
   connections, with a 120s hard cap.

2. "Probe /CookieTest did not become ready within 60000ms" in Integration
   Tests 3/4 (Node.js v22): `restartHttpWorkers` in lifecycle.mjs uses a
   2s per-probe timeout. Under shard contention the cumulative retries
   exhausted the 60s default budget before the route registered. Extended
   the readiness budget to 120s at every `restartHttpWorkers` /
   `installAppComponent` call site in the affected test files.

3. Fixed-sleep restarts in the legacy `restart.mjs` (restartWithTimeout and
   restartServiceHttpWorkersWithTimeout): replaced with active polls —
   TCP-connect on port 9925 for full restarts, TCP-connect on port 9926 for
   http_workers restarts — so they complete as soon as the server is
   reachable rather than burning a fixed 15–45s.

Also added `HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS: 120000` to the
Linux sharded integration-test job (Windows already had 180000) so
`startHarper`'s 'successfully started' wait scales on slow runners.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request replaces fixed timeouts with dynamic TCP port polling in the integration test suite to improve reliability on slow CI runners, alongside increasing several test timeouts to 120 seconds. The review feedback points out a potential issue in the new polling logic where connection attempts could hang indefinitely or leak resources, and suggests adding an explicit socket timeout and proper cleanup on error.

Comment on lines +18 to +24
const connected = await new Promise((resolve) => {
const socket = connect({ host, port }, () => {
socket.destroy();
resolve(true);
});
socket.on('error', () => resolve(false));
});

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.

medium

If a connection attempt hangs (e.g., due to firewall rules, routing issues, or slow host resolution), the connect call will block until the OS-level TCP connection timeout (which can be several minutes). This would bypass the deadlineMs check and cause the test runner to hang or time out globally.

Additionally, if an error occurs, the socket is not explicitly destroyed, which could lead to resource leaks.

Setting an explicit timeout on the socket and ensuring it is destroyed on error/timeout resolves these issues.

		const connected = await new Promise((resolve) => {
			const socket = connect({ host, port });
			socket.setTimeout(2000);
			socket.on('connect', () => {
				socket.destroy();
				resolve(true);
			});
			socket.on('timeout', () => {
				socket.destroy();
				resolve(false);
			});
			socket.on('error', () => {
				socket.destroy();
				resolve(false);
			});
		});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Already addressed: socket.setTimeout(2000) is set on every connection attempt (line 20 for waitForTcpPort, line 52 for waitForTcpPortClose), and all three handlers (connect, timeout, error) call socket.destroy() before resolving — no leak path. This was part of commit 215e10f5.

@claude

claude Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp and others added 3 commits June 1, 2026 11:38
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wait for port 9926 to close (old workers down) before polling for it
to reopen (new workers up). Prevents premature detection of old workers
still holding the port after restart_service http_workers.
Comment on lines +122 to +127
// Phase 1: wait for old workers to fully release the port (avoids premature
// detection of old workers still holding 9926 after the restart command).
await waitForTcpPortClose(host, restPort, Date.now() + 30_000);

// Phase 2: wait for new workers to bind and accept connections.
await waitForTcpPort(host, restPort, Date.now() + budget);

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.

Blocker: Phase 1 budget is independent of budget, so total wall time can reach 30s + budget.

waitForTcpPortClose uses a hardcoded 30 s deadline that is not subtracted from budget. Then Phase 2 starts a fresh Date.now() + budget clock. A call with the default restartHttpWorkersTimeout of 90 s can therefore stall for up to 120 s. On a slow runner where old workers are slow to release the port, the 30 s Phase-1 cap also throws prematurely with a misleading "did not close" error even though the new workers would eventually come up.

Suggested fix — share the remaining budget across both phases:

Suggested change
// Phase 1: wait for old workers to fully release the port (avoids premature
// detection of old workers still holding 9926 after the restart command).
await waitForTcpPortClose(host, restPort, Date.now() + 30_000);
// Phase 2: wait for new workers to bind and accept connections.
await waitForTcpPort(host, restPort, Date.now() + budget);
const deadline = Date.now() + budget;
// Phase 1: wait for old workers to fully release the port (avoids premature
// detection of old workers still holding 9926 after the restart command).
await waitForTcpPortClose(host, restPort, deadline);
// Phase 2: wait for new workers to bind and accept connections.
await waitForTcpPort(host, restPort, deadline);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Already fixed: const deadline = Date.now() + budget (line 131) is a single shared deadline passed to both waitForTcpPortClose (Phase 1) and waitForTcpPort (Phase 2) — the two phases race against the same clock. Fixed in commit 1cbc3057b.

Comment on lines +237 to +246
do {
Start-Sleep -Milliseconds 500
$ready = $false
try {
$tcp = New-Object System.Net.Sockets.TcpClient
$tcp.Connect('localhost', 9925)
$tcp.Close()
$ready = $true
} catch {}
} until ($ready -or (Get-Date) -gt $deadline)

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.

Blocker: TcpClient.Connect has no socket-level timeout on Windows — the deadline can be silently exceeded.

The synchronous $tcp.Connect('localhost', 9925) call has no timeout argument. If the OS queues the SYN without immediately accepting or refusing it (possible on a heavily loaded runner), this call blocks for the OS-level TCP timeout (potentially several minutes), ignoring $deadline. The Node.js path fixed this same issue (socket.setTimeout(2000) — commit 215e10f5), but the PowerShell path still lacks protection.

Use the asynchronous BeginConnect/EndConnect pattern with a WaitHandle and a 2 s timeout, or set the ReceiveTimeout/SendTimeout property before connecting:

Suggested change
do {
Start-Sleep -Milliseconds 500
$ready = $false
try {
$tcp = New-Object System.Net.Sockets.TcpClient
$tcp.Connect('localhost', 9925)
$tcp.Close()
$ready = $true
} catch {}
} until ($ready -or (Get-Date) -gt $deadline)
do {
Start-Sleep -Milliseconds 500
$ready = $false
try {
$tcp = New-Object System.Net.Sockets.TcpClient
$tcp.ReceiveTimeout = 2000
$tcp.SendTimeout = 2000
$connectTask = $tcp.ConnectAsync('localhost', 9925)
$null = $connectTask.Wait(2000)
if ($connectTask.IsCompletedSuccessfully) {
$tcp.Close()
$ready = $true
} else {
$tcp.Close()
}
} catch { try { $tcp.Close() } catch {} }
} until ($ready -or (Get-Date) -gt $deadline)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Already fixed: the PowerShell path uses $client.ConnectAsync('localhost', 9925).Wait(2000) (line 240) — ConnectAsync().Wait(2000) gives a 2-second per-attempt timeout, mirroring the Node.js socket.setTimeout(2000) fix from commit 215e10f5. Fixed in commit 1cbc3057b.

@kriszyp
kriszyp marked this pull request as ready for review June 1, 2026 18:45
@kriszyp
kriszyp marked this pull request as draft June 1, 2026 18:46
kriszyp added 2 commits June 1, 2026 12:47
- restartServiceHttpWorkersWithTimeout: share one deadline across
  close-phase and open-phase polls (fixes double-budget: 30s + budget)
- Windows CI poll: Replace Connect() with ConnectAsync().Wait(2000)
  to cap per-attempt TCP hang at 2s, matching Node.js socket.setTimeout
…thTimeout

On Windows the server may drop the TCP connection while shutting down
before the HTTP 200 response is fully delivered. Treat ECONNRESET,
ECONNREFUSED, ECONNABORTED, EPIPE, and AggregateError (dual-stack
refusal) as successful restart signals and proceed with the TCP poll.
@kriszyp
kriszyp marked this pull request as ready for review June 1, 2026 19:10
@kriszyp
kriszyp merged commit 41c1c4f into main Jun 2, 2026
48 checks passed
@kriszyp
kriszyp deleted the kris/fix-ci-startup-timing branch June 2, 2026 17:38
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