Skip to content

fix(server): report the bound port; bind the port-sharing tests to ephemeral ports (#440, 1/2) - #556

Merged
Ark0N merged 2 commits into
Ark0N:masterfrom
Randalix:fix/test-ports
Oct 9, 2026
Merged

Ark0N merged 2 commits into
Ark0N:masterfrom
Randalix:fix/test-ports

Conversation

@Randalix

@Randalix Randalix commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Part 1 of 2 for #440, split the way you asked: the server change, the conversions for the four shared ports, and the guard. The mechanical sweep of the rest follows as its own PR once this shape has been reviewed.

Server (fix(server): report the port actually bound)

  1. After await this.app.listen(...), this.port takes the number from this.app.server.address(). A string address (a pipe) or null leaves it alone, no cast. Everything that reads this.port after listen now gets the real number: the banner (it printed :0), process.env.CODEMAN_API_URL, the docker bridge listener and the unauthenticated-bind warnings.
  2. The route context's port is a getter. You were right about the ordering: createRouteContext() runs from setupRoutes() before listen(), so the by-value port: this.port would have handed tunnelManager.start() a 0. One place copied the context by spreading it, registerCronRoutes(this.app, { ...ctx, cron }), which would have frozen port at 0 again. Nothing read it there (CronPort only declares cron), so it now gets { cron: this.cronService }. No route module destructures port from the context.
  3. get boundPort() on WebServer, readonly, for tests.

test/webserver-bound-port.test.ts builds the context before start(), like setupRoutes() does, and checks boundPort, ctx.port and CODEMAN_API_URL each against the socket's own address().port. With only the write-back line removed, all three fail.

Conversions

The four shared ports, both files of each: qr-auth / auth-security (3162), multiuser-auth / routes/ws-routes (3170), cod54-hook-event-auth / routes/voice-routes (3230, 3231). Every server in those six files now binds 0, including the other fixed ports they used (3160, 3161, 3163, 3171, 3220, 3221, 3232). The WebServer ones read boundPort. The raw Fastify and ws servers in the two route tests read server.address() after each listen, so a re-listen in beforeEach never reuses a stale number.

Two corrections to my issue. First, these pairs were not a race inside one run: both vitest configs set fileParallelism: false, so files run one after another. They collide between two runs on the same host (that's how I hit them, two worktrees) or with anything else on the port. Second, docker-self-update.test.ts never bound 3000. My scan counted the CODEMAN_PORT=3000 string in an env-parsing fixture, and nothing in the test tree binds 3000 on current master.

Guard

test/test-ports-guard.test.ts reads the test tree and fails on a WebServer whose port argument is anything but the literal 0. It recognises new WebServer(…), a class declared extends WebServer, and a destructured alias, because quick-start.test.ts gets its constructor as const { WebServer: TestWebServer } = await webServerModule, which a plain new WebServer( match misses. The 43 files that construct a WebServer with a non-zero port today are an explicit legacy list (four of them never call start(), so they don't actually bind). An entry whose file no longer matches fails, so PR 2 has to remove each name as it converts the file. I checked that a new file with new WebServer(3999, …) turns the guard red, with the file name and the fix in the message.

What it doesn't catch, written down in the file as well: a helper that takes the port as a parameter is checked at the helper, not its callers. test/mobile/helpers/server.ts is on the list, so the nine mobile tests calling createTestServer(PORT) aren't checked. It also misses import { WebServer as X } and new mod.WebServer(…), and the raw app.listen({ port: N }) / new WebSocketServer({ port: N }) sites, which belong to the sweep.

Verification

npm test: 9128 passed, 12 skipped, 4 failed, the same four that fail on a clean master checkout here and pass under LC_ALL=C (the box runs de_DE.UTF-8: test/git-clone.test.ts and two cases in test/routes/case-clone-routes.test.ts classify git's stderr, which comes back in German, and test/custom-model-run-menu-ui.test.ts expects 16,384 where toLocaleString() gives 16.384). typecheck, lint, format:check and check:frontend-syntax are green.

No changeset.

🤖 Generated with Claude Code

Randalix and others added 2 commits October 8, 2026 14:56
… usable

`port: 0` already bound an ephemeral port at the socket level, but `this.port`
stayed 0: the banner printed `:0`, CODEMAN_API_URL pointed panes at `:0`, the
docker bridge listener and the unauthenticated-bind warnings read 0, and the
route context's `port` was a by-value snapshot taken in setupRoutes(), before
listen() runs, so `tunnelManager.start(ctx.port, …)` would have been handed 0.

- After `app.listen()`, `this.port` takes the number from
  `this.app.server.address()` (string/null addresses are left alone).
- The route context exposes `port` as a getter, and the cron routes get only
  the `cron` their CronPort declares instead of a spread copy of the context.
- `get boundPort()`: the readonly accessor tests use instead of a private field.

test/webserver-bound-port.test.ts compares each reader against the socket's own
`address().port`; all three tests fail with only the write-back removed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…w fixed ports

Four ports were shared by two files each — 3162 (qr-auth / auth-security), 3170
(multiuser-auth / routes/ws-routes), 3230 and 3231 (cod54-hook-event-auth /
routes/voice-routes). Files run serially (`fileParallelism: false`), so the pairs
never met inside one run; they collide between two runs on one host, or with
anything else holding the port. All six files now bind port 0 and read the
number back (`boundPort` for WebServer, `server.address()` after each listen for
the raw Fastify / ws servers).

test/test-ports-guard.test.ts fails on a WebServer built under test/ whose port
argument is not the literal 0 — `new WebServer(…)`, a subclass, or a destructured
alias (`{ WebServer: T }`, as quick-start.test.ts does) — outside a legacy list of
the 43 files that construct one with a non-zero port today; the follow-up sweep
converts them. A converted file cannot stay listed. What it does not cover (helper
parameters, `import { WebServer as X }`, raw listen sites) is written down in it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Ark0N
Ark0N merged commit 0f5613b into Ark0N:master Oct 9, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Oct 9, 2026
Move test/sse-tile-grid-filter.test.ts to an ephemeral port. The release
added it with #561 on fixed port 3287, after #556 was cut, so it is not on
the guard's LEGACY_FIXED_PORT_FILES and test/test-ports-guard.test.ts failed
on the merged tree. It now builds new WebServer(0, ...) and its url() helper
reads server.boundPort (only ever called inside tests, after beforeAll).
Converting it is preferred over listing it, since the legacy list is
shrink-only.

Update the five docs that still told contributors to pick a unique fixed
port, which the new guard now rejects for any WebServer test: CLAUDE.md
(Adding Features and Testing), AGENTS.md, .github/CONTRIBUTING.md and the
wiki's Contributing page (mirrored to the public GitHub wiki). They now say
to bind port 0 and read boundPort (or address().port for a raw server), and
note that the mobile suite keeps its fixed ports for now, because
test/mobile/helpers/server.ts caches servers by port, so createTestServer(0)
from two callers would share one server.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 9, 2026
The Run dropdown scroll browser test built WebServer on the fixed port 3290,
which the static port guard (test/test-ports-guard.test.ts, from #556) rejects
for any file outside its shrink-only legacy list, so the CI gate failed on the
landing branch. The test now binds an ephemeral port with new WebServer(0, ...)
and navigates to server.boundPort, and its header records the new convention.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 9, 2026
Thirteen contributor PRs, each re-checked against its GitHub head, merged
with its own merge commit and landing fixes, reviewed, and gated together
(524 test files, 10194 tests): #559, #552, #550, #556, #551, #546, #542,
#540, #555, #543, #541, #502, #432.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Ark0N

Ark0N commented Oct 9, 2026

Copy link
Copy Markdown
Owner

Merged in 1.40.0, thanks @Randalix! boundPort plus the static guard is the right shape for #440: a test that needs a server can no longer quietly grab a fixed port. Applied on the way in (c2340ae): test/sse-tile-grid-filter.test.ts, which arrived with the tile grid after your branch was cut and used fixed port 3287, now binds port 0 instead of joining the shrink-only legacy list, and CLAUDE.md, AGENTS.md, CONTRIBUTING.md and the wiki's Contributing page now say to bind port 0 and read boundPort. The guard caught two more fixed-port tests during landing (#542's browser test and a new tile browser test), so it is already paying for itself.

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