feat(start-sdk,start-os): retire hosts and bindings permanently - #3641
feat(start-sdk,start-os): retire hosts and bindings permanently#3641helix-nine wants to merge 1 commit into
Conversation
5b4c1e0 to
ad3e2cb
Compare
|
Floor raised: That is the whole mechanism — the only consumer is Also rewrote the tsc and the 86 SDK tests still green. |
ad3e2cb to
0b2754a
Compare
|
Both applied. Notification dropped. Removed the
Green: 557 Rust, 86 SDK, 12 container-runtime, tsc ×3, bindings-check, prettier. No unused-import warnings from either touched file. |
0b2754a to
5f08cdc
Compare
|
Added two, and they're aimed at different halves of the claim.
What I still can't test in-repo: that the apply phase, handed empty desired maps, issues the real 559 Rust tests green (was 557). |
5f08cdc to
dbc0558
Compare
|
We shouldn't — I cargo-culted it from What I found tracing it:
Left tsc and the 12 container-runtime tests still green. |
dbc0558 to
49b3a9b
Compare
|
Done — 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:
Also re-confirmed the Rust side has no consumer at all: no 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. |
|
@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.
49b3a9b to
737f1c5
Compare
|
Rebased onto One conflict, in 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:
|
Why
setupInterfacesends every pass by disabling each binding it did not just declare (clear_bindings→BindInfo::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:
AvailablePortsis server-global anddisable()never callsfree. OnlyBindInfo::update(a re-bind of the same key) and uninstall ever reclaim.Model<Host>::update_addresseshas noenabledcheck, unlikeNetServiceData::update, which does skip disabled bindings. So the datapath is down but the address book still advertises it.getBridgeAddresswith a10.0.3.1:<port>that nothing listens on.And a renamed host id leaves the whole
Hostbehind — its public/private domains included — with no API able to remove it.What changed
MultiHost.retire()andMultiHost.retirePort(), called from theup()of the version that stops binding:Both resolve
falsewhen there was nothing to remove, so a re-run after a restore is safe, and both throw if called inside asetupInterfacescallback, 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 vhostport_mappings(PCP/UPnP router leases) anddns_update's RFC 2136 records carry no refcount at all. Dropping aHostBindsentry leaks all three until the container exits.NetServiceData::retiretherefore reconciles the host against an emptyHostfirst — which drives every branch ofupdateto the "went away" side and runs the three reapersupdateowns 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::dropspawnsremove_alldetached and can lose the race with uninstall's wholesalepackageData/<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_domainno longer upsert. They went throughhost_for, which ends in.upsert(host_id, || Ok(Host::new())), and unlike the address setters they have noor_not_foundto 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. Newhost_for_existingmirrorshost_for's server special-case exactly.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.OSVersionfloor moves0.4.0→0.4.0.2(per @dr-bonez).0.4.0.2is the release carrying these effects, so the registry never offers a package whose migration callsretire()to a server that would answerMethod not found. One consumer —setupManifest.ts:68,osVersion: manifest.osVersion ?? OSVersion— so this is the whole mechanism. Rootpackage.jsonis already0.4.0.2andprojects/start-os/Cargo.tomlis0.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'sPromise<null>, so a mistyped id is at least visible. Happy to make itPromise<null>if the deviation isn't worth it.## 2.0.10,## [0.4.0.2]); no tag exists for either, so no manifest bump.getServicePortForwardsoftened toOption<NetInfo>/Promise<NetInfo | null>(per @dr-bonez). It is the one host effect with nocallback, 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 throughhost_for_existingtoo, so a read no longer upserts (harmless before only because it ran against apeek()snapshot).Docs
New
## Retiring a Host or Bindingin 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. Alsorecipe-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 newrelease_*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 newMultiHost.retire/retirePortforwarding 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 asretired_hostsso it can be exercised directly (the file already sets that precedent withssl_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, whenbindsis empty and must retire nothing.host_binds_holds_only_what_update_reconciles— destructuresHostBindsexhaustively, so adding a field fails to compile until someone has looked at whetherupdatetears it down. That is the regression this design is actually exposed to: retiring works by reconciling against an emptyHost, which is a teardown only while every resourceHostBindsholds is driven by the desired set computed from that host — andgua_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 withE0027: 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.gccalls against a live kernel. That needs aNetControllerwith real nft, vhost and DNS controllers. The refcount-and-gc layer underneath it does have coverage (forward.rs6 tests,vhost.rs8), so the untested seam is narrow — but it is real, and a VM run is what would close it.