ReviewOS

stacks/rpx

Tests leak a listener on :48080 and an untracked SiteSupervisor

#2271
Closed chris opened this 10 days ago ·
10 days ago

Summary

Two test files leave real resources running past their own end. Distinct from the mock leakage in #2270 (this is unclosed resources, not unrestored mocks), same root consequence: state that outlives the file in a shared bun test process.

1. A real HTTP server left bound to :48080

packages/rpx/test/start-single-proxy-production-certs.test.ts:42

The test spies createSharedProxyServer and startServer, but not startHttpRedirectServer. So Start.startProxies({ httpPort: 48080, … }) reaches src/start.ts:1295-1301, calls .listen(48080) and registers it via activeServers.add (src/start.ts:710). The only afterEach (lines 22-26) just rms the temp dir — Start.cleanup() is never called, so the listener and its fd are held for the remainder of the process.

The silent trap: a later startProxies({ httpPort: 48080 }) sees isPortInUse === true at src/start.ts:1295, skips starting its own redirect server, and its redirect/ACME assertions then talk to this file's leftover.

This is already observable locally — a full-suite run prints:

[error] Shared HTTPS proxy failed to bind :48443; not exiting

It harms nothing on macOS only because this file sorts last (#47). One readdir or hash difference away from landing before websocket-proxy.test.ts or any startProxies caller.

Fix: stop the server in an afterEach (call Start.cleanup(), or capture and close the returned server).

2. An untracked SiteSupervisor

packages/rpx/test/site-supervisor.test.ts:267

The SiteSupervisor built in 'stopAll signals every running site' is the only one of eight never pushed to supervisors (compare lines 70, 95, 129, 155, 188, 217, 247), so the afterEach at line 55 cannot reach it. Its 30s setInterval reaper and readiness loop are cleaned up only on the success path.

If that test throws or times out, a live 30s interval plus a readiness loop survive into later files, and the loop eventually emits a log.warn attributed to whatever file is running then.

Fix: supervisors.push(...) it like the other seven.

Note

Item 2 interacts with the shutdown bug filed separately — the stray warn that surfaces under another file's name has its own cause in src/site-supervisor.ts.

10 days ago

Closing: one of the two items was real and is fixed in #2274; the other was my false positive.

Item 2 — untracked SiteSupervisor — real, fixed. The supervisor in 'stopAll signals every running site' was the only one of eight never pushed to supervisors, so the afterEach could not reach its 30s reaper or readiness loop. Now registered, and deliberately before the first await that can reject — pushed after, a throwing onRequest would still leak it.

Item 1 — the :48080 listener — false positive. It does not leak.

My original evidence was a flawed probe: it spawned bun test <file> as a subprocess and then tried to bind the port. Of course it was free — the child process had exited, taking the listener with it. That tells you nothing about the leak, which only matters within one bun test process, across files.

Probing it correctly — appending a probe test file to the same process and having it bind :48080:

BEFORE (main's version of the file, then probe, same process)   2 pass / 0 fail
AFTER  (fixed version, then probe)                              2 pass / 0 fail

The port is free either way, so startProxies is not leaving that redirect server bound. I had already written a Start.cleanup({ hosts: false, certs: false }) into the file's afterEach to drain it, and removed it again rather than ship a fix for a bug that does not exist — particularly one with global reach, since Start.cleanup drains activeServers for the whole process and would happily tear down listeners another test file owns.

That file did have a real problem, but it was the #2270 one: createSharedSpy.mockRestore() / startServerSpy.mockRestore() sat as the last two statements of the it, so any assertion above them throwing left startServer a permanent no-op and createSharedProxyServer permanently returning null. That is fixed in #2274 by moving the restore into afterEach.

The [error] Shared HTTPS proxy failed to bind :48443 line in local runs is unrelated and pre-existing — it is an HTTPS-port bind, not :48080, and the count is identical before and after (6 occurrences either way).

Sign in to comment on this issue.