Skip to content

feat(start-sdk,start-os): retire hosts and bindings permanently - #3641

Open
helix-nine wants to merge 1 commit into
masterfrom
feat/retire-hosts-bindings
Open

feat(start-sdk,start-os): retire hosts and bindings permanently#3641
helix-nine wants to merge 1 commit into
masterfrom
feat/retire-hosts-bindings

Conversation

@helix-nine

@helix-nine helix-nine commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Why

setupInterfaces ends every pass by disabling each binding it did not just declare (clear_bindingsBindInfo::disable). That is the right default: it keeps the row, the external port number, and the user's per-address enable/disable and WAN opt-in choices, so a binding a package declares conditionally comes back at the address the user already bookmarked.

Nothing ever deletes one, and hosts were not removable at all. So a binding a package stops declaring for good:

  • keeps its external port claimed for as long as the service is installed — AvailablePorts is server-global and disable() never calls free. Only BindInfo::update (a re-bind of the same key) and uninstall ever reclaim.
  • keeps recomputing its addresses — Model<Host>::update_addresses has no enabled check, unlike NetServiceData::update, which does skip disabled bindings. So the datapath is down but the address book still advertises it.
  • still answers a dependent's getBridgeAddress with a 10.0.3.1:<port> that nothing listens on.

And a renamed host id leaves the whole Host behind — its public/private domains included — with no API able to remove it.

What changed

MultiHost.retire() and MultiHost.retirePort(), called from the up() of the version that stops binding:

migrations: {
  up: async ({ effects }) => {
    await sdk.MultiHost.of(effects, 'ui-multi').retire()
    await sdk.MultiHost.of(effects, 'api').retirePort(9090)
  },
  down: IMPOSSIBLE,
}

Both resolve false when there was nothing to remove, so a re-run after a restore is safe, and both throw if called inside a setupInterfaces callback, where the trailing disable sweep would make the result depend on statement order.

The part that needed care

Removing the DB node alone would have been worse than the status quo. Teardown is reconcile-driven through NetServiceData::update, and the sync loop iterates only hosts present in the database — so a removed host is never reconciled again. Of what a host holds, only the v4 forwards are refcounted and gc-scanned; gua_forwards (v6 DNAT + upstream pinhole), the vhost port_mappings (PCP/UPnP router leases) and dns_update's RFC 2136 records carry no refcount at all. Dropping a HostBinds entry leaks all three until the container exits.

NetServiceData::retire therefore reconciles the host against an empty Host first — which drives every branch of update to the "went away" side and runs the three reapers update owns and nothing else calls — and only then drops the entry. The sync loop prunes hosts absent from the database, retiring before updating so a port handed from a removed host to a surviving one in the same pass comes down before it is rebuilt.

That prune also fixes a pre-existing leak: NetService::drop spawns remove_all detached and can lose the race with uninstall's wholesale packageData/<id> delete, after which nothing could ever reclaim that package's datapath.

Related: reading the hosts map now distinguishes "no hosts" from "could not parse". It was de().unwrap_or_default(), which was harmless when an empty map just meant "update nothing" — with a prune it would have meant "retire everything".

Supporting fixes retirement depends on

  • remove_public_domain / remove_private_domain no longer upsert. They went through host_for, which ends in .upsert(host_id, || Ok(Host::new())), and unlike the address setters they have no or_not_found to roll the mutate back — so naming a host the service does not have added it as an empty entry that then had nothing to remove it. Reachable on released StartOS today with a typo'd host id; without this, an unrelated domain removal resurrects a host that was just retired. New host_for_existing mirrors host_for's server special-case exactly.
  • Uninstall's open-coded port math moved behind Host::release, shared with retirement so the two cannot drift. Behaviour-preserving; it is what the new unit tests exercise.

Notes for review

  • retire() deletes domains the user added, with no notification (per @dr-bonez). The guide and both TSDocs instead tell package authors to name a retired host in their release notes, and the StartOS user docs say a domain can be removed by a service update and to check those release notes.
  • The SDK's OSVersion floor moves 0.4.00.4.0.2 (per @dr-bonez). 0.4.0.2 is the release carrying these effects, so the registry never offers a package whose migration calls retire() to a server that would answer Method not found. One consumer — setupManifest.ts:68, osVersion: manifest.osVersion ?? OSVersion — so this is the whole mechanism. Root package.json is already 0.4.0.2 and projects/start-os/Cargo.toml is 0.4.0-rev.2, so the floor names a version this branch actually ships. The per-function "requires 0.4.0.2" caveats came back out of the guide and the TSDocs, since the floor now guarantees it.
  • Promise<boolean> rather than the write-effect family's Promise<null>, so a mistyped id is at least visible. Happy to make it Promise<null> if the deviation isn't worth it.
  • Both changelogs add under existing unreleased headings (## 2.0.10, ## [0.4.0.2]); no tag exists for either, so no manifest bump.
  • getServicePortForward softened to Option<NetInfo> / Promise<NetInfo | null> (per @dr-bonez). It is the one host effect with no callback, so throwing was the worst available answer for the one caller class that cannot react; it also could not distinguish "no such binding" from "the host is gone", and already reported a disabled binding's stale ports. Zero behavioural call sites in the monorepo, the SDK, or the package fleet, and free while 2.0.10 is untagged. It now goes through host_for_existing too, so a read no longer upserts (harmless before only because it ran against a peek() snapshot).

Docs

New ## Retiring a Host or Binding in the packaging guide, mirroring #3626's structure — what makes one orphaned, whose job clearing it is, the migration pattern, why neither StartOS nor the SDK can infer it, the failure modes, and cleaning up after the fact. Cross-linked both ways with #3626's "Retiring a replay key", since they are the same shape: package-created state that outlives the release which stopped creating it. Also recipe-version-migrations.md, service-to-service.md, and a note in the StartOS user docs about a domain being removed with its interface.

Testing

cargo check -p start-core; cargo test -p start-core --lib (557 passed, incl. 3 new release_* tests covering both ports, non-interference, and the inclusive range span at the u16 boundary); make start-core-ts-bindings-check; tsc for start-core / start-sdk / container-runtime; make start-sdk-test (86, incl. 2 new MultiHost.retire/retirePort forwarding tests) and the setupInterfaces guard tests; make container-runtime-test (12); prettier clean.

Two tests guard the retire path specifically:

  • only_hosts_the_database_dropped_are_retired — the prune's selection, extracted as retired_hosts so it can be exercised directly (the file already sets that precedent with ssl_vhost_public_v4). Covers the four cases that matter: nothing dropped, one dropped, a host the database gained that the datapath hasn't built yet, and the first pass after a restart, when binds is empty and must retire nothing.
  • host_binds_holds_only_what_update_reconciles — destructures HostBinds exhaustively, so adding a field fails to compile until someone has looked at whether update tears it down. That is the regression this design is actually exposed to: retiring works by reconciling against an empty Host, which is a teardown only while every resource HostBinds holds is driven by the desired set computed from that host — and gua_forwards, the vhost port maps and the DNS records carry no refcount, so nothing else would reap a new one. I verified the guard bites rather than assuming it: adding a fifth field fails with E0027: pattern does not mention field.

Still not covered, and honestly not unit-testable: that the apply phase, given empty desired maps, issues the actual unforward6 / reconcile_port_maps / vhost.gc / dns_update.gc calls against a live kernel. That needs a NetController with real nft, vhost and DNS controllers. The refcount-and-gc layer underneath it does have coverage (forward.rs 6 tests, vhost.rs 8), so the untested seam is narrow — but it is real, and a VM run is what would close it.

@helix-nine
helix-nine force-pushed the feat/retire-hosts-bindings branch from 5b4c1e0 to ad3e2cb Compare August 7, 2026 18:56
@helix-nine

Copy link
Copy Markdown
Contributor Author

Floor raised: OSVersion 0.4.00.4.0.2 (StartSdk.ts:80).

That is the whole mechanism — the only consumer is setupManifest.ts:68 (osVersion: manifest.osVersion ?? OSVersion), so every package built with 2.0.10 now declares 0.4.0.2 and the registry won't offer it to a server that would answer Method not found. Checked that the floor names a version this branch actually ships: root package.json is 0.4.0.2 and projects/start-os/Cargo.toml is 0.4.0-rev.2.

Also rewrote the ### Changed changelog entry, which still described the old 0.4.0 bump, retitled the heading to ## 2.0.10 — StartOS 0.4.0.2, and dropped the per-function "requires 0.4.0.2" caveats from the guide and both TSDocs — redundant now the floor guarantees it.

tsc and the 86 SDK tests still green.

@helix-nine
helix-nine force-pushed the feat/retire-hosts-bindings branch from ad3e2cb to 0b2754a Compare August 7, 2026 19:08
@helix-nine

Copy link
Copy Markdown
Contributor Author

Both applied.

Notification dropped. Removed the notify call from retire_host, the two i18n keys (×5 locales), and the now-unused notifications import. The BTreeSet domain-dedup went with it — it only existed to keep a split-DNS domain from being named twice in that message. Reworded the four places that promised a notification: the guide and MultiHost.retire's TSDoc now tell package authors to name a retired host in their release notes, and the StartOS user docs say a domain can be removed by a service update and to check those notes.

getServicePortForward softened to Option<NetInfo> / Promise<NetInfo | null>. It also now goes through host_for_existing, so a read no longer upserts — harmless before only because it ran against a peek() snapshot that got discarded. host_for was its last user in bind.rs, so the import moved rather than being left dead.

Green: 557 Rust, 86 SDK, 12 container-runtime, tsc ×3, bindings-check, prettier. No unused-import warnings from either touched file.

@helix-nine
helix-nine force-pushed the feat/retire-hosts-bindings branch from 0b2754a to 5f08cdc Compare August 7, 2026 19:33
@helix-nine

Copy link
Copy Markdown
Contributor Author

Added two, and they're aimed at different halves of the claim.

only_hosts_the_database_dropped_are_retired — pulled the prune's selection out as retired_hosts so it can be exercised directly, following the precedent ssl_vhost_public_v4 already set in that file. Covers nothing-dropped, one-dropped, a host the database gained that the datapath hasn't built yet, and the first pass after a restart where binds is empty and must retire nothing.

host_binds_holds_only_what_update_reconciles — this is the one I actually care about. Retiring works by reconciling against an empty Host, which is a teardown only while every resource HostBinds holds is driven by the desired set update computes from that host. gua_forwards, the vhost port maps and the DNS records carry no refcount, so a new field added without a teardown branch would leak silently and no runtime test would catch it. The test destructures HostBinds exhaustively, so adding a field fails to compile until someone has looked at it. I checked that it bites rather than assuming — added a fifth field and got E0027: pattern does not mention field, pointing straight at the test.

What I still can't test in-repo: that the apply phase, handed empty desired maps, issues the real unforward6 / reconcile_port_maps / vhost.gc / dns_update.gc against a live kernel — that needs a NetController with real nft/vhost/DNS controllers. The refcount-and-gc layer under it is covered (forward.rs 6 tests, vhost.rs 8), so the gap is narrow, but it's real. Say the word if you want me to take a VM and close it.

559 Rust tests green (was 557).

@helix-nine

Copy link
Copy Markdown
Contributor Author

We shouldn't — I cargo-culted it from bind/bindRange and it doesn't survive checking. Dropped from both retire wrappers.

What I found tracing it:

  • The server ignores it entirely. No handler takes a stack field, and none of the effect params structs set deny_unknown_fields, so serde drops it on arrival. Nothing in start-core reads a stack param at all.
  • rpcRound doesn't read it back either. On failure it builds a fresh new Error(${message}@${method}) — the caller's stack is not attached to the rejection.
  • Its one actual reader is incidental: the error branch logs utils.asError({ method, params, error }), and params still holds stack, so it lands in the container's log. That's a real breadcrumb — but it argues the stack belongs at that console.error call, not in the payload we serialize and put on the socket.
  • Only 4 call sites had it, and bind/bindRange predate the monorepo (they arrive in the reorg commit 950be49dd as a pure move), so it's inherited rather than deliberate.

Left bind/bindRange alone since they're pre-existing and out of scope here — happy to strip them, or move the stack into the log call so the breadcrumb survives without the wire cost, in a follow-up if you want either.

tsc and the 12 container-runtime tests still green.

@helix-nine
helix-nine force-pushed the feat/retire-hosts-bindings branch from dbc0558 to 49b3a9b Compare August 7, 2026 20:27
@helix-nine

Copy link
Copy Markdown
Contributor Author

Done — bind and bindRange no longer send one either. Those were the only two left: grep -rn 'stack:' --include=*.ts projects shared-libs now returns nothing outside node_modules/dist/tests.

I audited every remaining call stack in the tree rather than grepping for the one pattern, and the other four are all genuinely consumed where they are taken, so I left them:

site why it stays
RpcListener.ts:138, :187 debug: error?.stack on an RPC error responserpcRound reads it back (res.error.data?.debug), appends it to the message and logs Debug: …. This is the one stack that legitimately crosses the socket, and it goes the other direction.
EffectCreator.ts:124, :414 passed straight into a local console.warn for the out-of-context warnings. Never leaves the process.
SystemForEmbassy/index.ts:173 console.info({ passedErrorStack: … }) in the legacy v1 adapter. Local, and it does print it — flagging it as arguable debug cruft, but it uses what it captures, and it is unrelated to this PR.

Also re-confirmed the Rust side has no consumer at all: no stack field on any params struct, and no deny_unknown_fields anywhere under service/effects/, so both fields were being parsed and discarded on arrival.

No changelog entry — the field never reached a handler, so there is no observable change for a user or a package author.

Green: 559 Rust, 12 container-runtime, tsc, prettier.

dr-bonez
dr-bonez previously approved these changes Aug 11, 2026
@dr-bonez

Copy link
Copy Markdown
Member

@helix-nine rebase

`setupInterfaces` ends every pass by disabling each binding it did not just
declare. Disabling is the right default — it keeps the row, the external port
number and the user's per-address choices, so a conditionally-declared binding
returns at the address they bookmarked.

Nothing ever deletes one. A binding a package stops declaring for good keeps
its external port claimed for as long as the service is installed (the pool is
server-global and `disable()` never frees), keeps recomputing its addresses
(`update_addresses` does not consult `enabled`), and still answers
`getBridgeAddress` with a `10.0.3.1:<port>` nothing listens on. Hosts were not
removable at all, so a renamed host id left its domains behind for good.

`MultiHost.retire()` and `MultiHost.retirePort()` remove them, from the `up()`
of the version that stops binding. Both are idempotent, so a re-run after a
restore is safe, and both throw if called during a `setupInterfaces` pass,
where the trailing disable sweep would make the result depend on statement
order.

Removing the DB node alone would have been worse than the status quo. Teardown
is reconcile-driven through `NetServiceData::update`, and of what a host holds
only the v4 forwards are refcounted and gc-scanned — `gua_forwards`, the vhost
port mappings (PCP/UPnP router leases) and `dns_update`'s RFC 2136 records are
not. Dropping a `HostBinds` entry leaks all three. `NetServiceData::retire`
therefore reconciles the host against an empty `Host` first, which drives every
branch of `update` to the "went away" side and runs the reapers `update` owns,
and only then drops the entry. The sync loop prunes hosts absent from the
database — and treats a hosts map it could not read as no information rather
than as every host having gone away.

The SDK's minimum StartOS version moves to `0.4.0.2`, the release carrying
these effects, so the registry never offers a package whose migration calls
`retire()` to a server that would answer `Method not found`.

`bind` and `bindRange` stop sending a caller stack in their RPC params, and
the retire wrappers never start. Nothing read it: no handler takes a `stack`
field and no params struct denies unknown fields, so serde dropped it on
arrival. These were the only two call sites in the tree that put a stack in a
payload; every other stack in the container runtime is consumed where it is
taken, either by a local `console.warn` or as the `debug` field of an RPC error
response, which `rpcRound` does read back.

Two tests guard the retire path. `retired_hosts` is extracted so the prune's
selection can be exercised directly, and `HostBinds` is destructured
exhaustively in a test so adding a resource to it fails to compile until
someone has looked at whether `update` tears it down — several of its fields
carry no refcount, so nothing else would reap them.

`getServicePortForward` resolves `null` rather than throwing when the binding
does not exist. It is the one host effect with no `callback`, so a caller
cannot react to a change, and it already reported a disabled binding's stale
ports — which retiring makes an observable difference.

Two supporting fixes retirement depends on:

- `remove_public_domain` / `remove_private_domain` went through `host_for`,
  which upserts, so naming a host the service does not have added it as an
  empty entry that then had nothing to remove it. They now take
  `host_for_existing` and no-op.
- Uninstall's open-coded port math moved behind `Host::release`, shared with
  retirement so the two cannot drift.
@helix-nine

Copy link
Copy Markdown
Contributor Author

Rebased onto 909a6f020, now 14 commits ahead of the old base. Heads up that this dismissed your approval — the repo ruleset has dismiss_stale_reviews_on_push: true, so a rebase can't avoid it.

One conflict, in projects/start-os/CHANGELOG.md: your image-checksum entry landed in the same ### Fixed slot as mine. Kept both.

Three of the new commits touch net/service code, so I checked for semantic conflicts rather than trusting the clean rebase — no file overlap with this branch, and nothing that interacts badly. Worth noting though that #3639 is the same bug one level up: it fixes the interface record that a dormant binding leaves behind, and its commit message names the cause exactly — "the old binding is kept dormant by design — its external port and address book survive for a future rebind — but the interface record exported from it lingered as well." That is the disable-vs-retire gap this PR is about. The two compose: retiring the old binding is the clean version of the case #3639 has to clean up after, since a retired binding leaves no record to dedupe. Jitsi moving its Web UI 80 → 8000 is exactly the worked example in the new docs section.

Re-verified everything on the new base rather than assuming the rebase was inert:

  • cargo check -p start-core clean; cargo test -p start-core --lib 569 passed (was 559 — master added 10)
  • my six tests all still pass by name: the three release_* port tests, only_hosts_the_database_dropped_are_retired, host_binds_holds_only_what_update_reconciles, plus the pre-existing ssl_vhost_public_v4 one
  • make start-core-ts-bindings-check clean, tsc green for start-core / start-sdk / container-runtime
  • make start-sdk-test 86, make container-runtime-test 12
  • spot-checked the substance survived: both effects registered, the prune still routed through retired_hosts, getServicePortForward still Option<NetInfo>, floor still 0.4.0.2, and zero stack: payloads left in the tree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants