ReviewOS

also looking at this

stacks/ts-cloud

fix(deploy): allocate site ports per box, so a second attach cannot collide

#170
Merged glennmichael123 wants to merge fix/attach-port-allocation into main
4 files +643 -1

Closes #168. Unblocks #167's first operation.

Correction to the issue's premise, first

I wrote #168 saying the second attach "cannot bind" and surfaces "as a service that will not start". That is wrong, and the truth is worse. assertPortsAreFree in stacks records what actually happens:

Two processes CAN bind the same port here: ts-cloud's units do not set exclusive binding, so the kernel load-balances between them instead of failing. Nothing errors, both services look healthy, and each domain serves the other tenant's site on roughly half its requests.

That is not hypothetical: predicthq.org answered with a storefront's pages for a day and a half. So this is a silent traffic-mixing bug, not a startup failure, which makes plan-time allocation more valuable rather than less.

And detection already exists, which I also missed when filing. assertPortsAreFree (stacks, buddy deploy) diffs the ports this project wants against the box's live listeners over SSH, resolves the owning systemd unit, skips this project's own units, and stops the deploy. Live listeners are better evidence than declared config, and that check stays the last line of defence.

What it cannot do is avoid the clash. Its remedy is to exit and tell the operator to go pick free ports in config/cloud.ts by hand, which is precisely the acceptance criterion #168 asks to remove. This PR is the other half: deciding the ports before anything ships, from data a plan already has.

The bug

Every app generated from the Stacks cloud template declares the same loopback ports, and validateDeploymentConfig only ever sees one project's sites:

loghq/config/cloud.ts     port: 3022 (app)   port: 3023 (api)
bughq/config/cloud.ts     port: 3022 (app)   port: 3023 (api)

The box already knew

No new bookkeeping. Each project's deploy writes its own rpx registry fragment under /etc/rpx/sites.d, and every server-app route records its upstream as host:port. The fragments are already a complete port registry; nothing read them.

occupiedHostPorts()fragments to a port -> owning slug map
allocateSitePorts()keeps a declared port when free, else walks to the next
buildHostSitePortsScript() / parseHostSiteFragments()the one step that must touch the box, split so both halves unit-test without a server

Two details that are easy to get wrong, both pinned by tests:

  • The deploying project's own fragment must be skipped. It is already on the box from the previous deploy, so counting it makes an attached app's second deploy conflict with its first and never succeed again. (assertPortsAreFree has the same insight, via unit.startsWith(slug-).)
  • A declared port is kept whenever it is free. A box with no co-tenants allocates nothing, ports stay stable across deploys, and a colliding template app lands on 3024 rather than an arbitrary high port.

validateDeploymentConfig gains an optional occupiedPorts map and reports a co-tenant collision separately from a same-config one, because the remedy differs: the operator cannot see the other project's config, so the message names the project holding the port. Omitting the option validates exactly as before, so every existing caller is untouched.

Acceptance criteria

  • A port conflict is reported at plan time, naming both owners
  • Attaching a second template app succeeds without either app editing a port by hand (allocateSitePorts)
  • Port assignments are recorded where the next attach can see them (the fragments already were that record, now readable)
  • --dry-run shows the ports each site will occupy

What I deliberately did not wire, and why

The only caller of validateDeploymentConfig is bin/commands/deploy.ts:559, which runs before any driver exists, so it has no host connection and cannot read the fragments yet. Two options existed and I took neither blindly:

  1. Probing inside attachToComputeInfrastructure would break hetzner-driver.test.ts, which exercises attach with a mocked client and no SSH.
  2. A best-effort probe would reintroduce the exact failure being reported: a silent skip that lets the collision through.

So this lands the layer with full coverage and leaves the single occupiedPorts call site to the attach/CLI surface in #167 and stacksjs/stacks#2342, where a plan and a host connection both legitimately exist. Better a tested seam than a probe that fails open.

Verification

  • 27 new tests, including the issue's exact case: two untouched template apps on one box produce two plan-time errors naming bughq, and allocation puts the second on 3024/3025
  • Every load-bearing assertion mutation-checked: dropping the self-skip, the co-tenant lookup, the last-colon parse, or the walk-up each fail the suite
  • Full suite 4022 pass, 0 fail; typecheck clean; lint 0 errors 0 warnings
  • Ports parse on the last colon so a bracketed IPv6 upstream reads correctly

Note for reviewers on a fresh clone: build packages/core and packages/ts-cloud first, or @ts-cloud/core resolves to a stale dist/ and unrelated suites fail on missing exports.

4 changed files on the files tab, with 0 review threads.