site serve and view: pick a free port, and report a held port as taken #251
No reviewers
Labels
No labels
blocked
bug
decision
duplicate
enhancement
help wanted
invalid
landed?
question
ready-to-merge
stale
wontfix
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
allod/tools!251
Loading…
Reference in a new issue
No description provided.
Delete branch "agent/site-serve-free-port"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
allod site servenow serves a site that has no port in the registry, on a free port it picks, andallod site viewforwards whatever port the VM reports. A start is reported as working only when the preview itself holds the port, so another program on that port is no longer printed as the site's address.preview_portis served instead of refused. A port something else holds is refused with exit 1 before anything starts. A preview whose app listens on all addresses instead of 127.0.0.1 is no longer reported as served.--port, on a free port is started with the samesystemd-runcommand as before.--stopneeds no port. Exit codes keep their meaning.viewforwarding from a real hypervisor to a real VM. It is tested up to thessharguments it builds. That run is yours.Closes allod/tools#249
The issue's default was not enough
The issue proposes a check before the start and one retry, and leaves "accept a connection only when the listening socket belongs to a process of the unit" as a follow-up "unless the check before the start proves not enough". It was measured, and it is not enough. With the default alone, and another process taking the picked port before the app bound it:
The unit is still up while
nix runevaluates, so a connection answered by the other process was taken for the preview. This PR therefore carries both the check before the start and the socket-owner rule. The same race on this branch, at4d3e582:Exit 1, no address, no unit left, and two starts: one retry.
What changed
serve:--port, the registry'spreview_port, the port of the site's unit if it is running, then a port picked by binding 127.0.0.1 port 0 and releasing it.ALLOD_PREVIEW_PORTit was started with, read back withsystemctl --user show -p Environment. There is no state file; the port lasts exactly as long as the unit.preview_socket_linux.go): the listening socket for127.0.0.1:<port>in/proc/net/tcp, or for the IPv4-mapped loopback in/proc/net/tcp6, must be open in a process of the unit's control group.viewwith no port of its own passes none, reads the address the remote start printed, and forwards that port.preview_portthat would make it permanent. A start's standard output is the address line alone.docs/allod-site-preview.mdand both help texts say the same.Three behaviours beyond the issue's text:
ALLOD_PREVIEW_INTERFACEand listens on0.0.0.0or::ends at exit 3, still starting, where it used to be reported as served. The contract already required 127.0.0.1.--stopor--port, instead of getting a second picked port.serveexits 1 and stops the unit if this run started it.Risk
R3. The change touches how a systemd unit is started and judged, and makes the hypervisor act on a value a VM reports.
viewkeeps at most 128 bytes of what the VM prints, accepts exactly one line of exactlyhttp://127.0.0.1:<digits>with the port from 1024 to 65535, and handsssh -Lthe port as a formatted integer. A VM can therefore choose which loopback port at or above 1024 the hypervisor listens on, and nothing else. It could not before.serveexits 1 saying the owner cannot be read.serveneeds a user systemd manager before it gets that far.serve --vmits standard output, reach the hypervisor's terminal verbatim, control sequences included. That was so before this change and is left for an issue of its own.site_view.goandsite_serve.go, on different lines. Whichever merges second needs master merged into it first.Validation
Run by me on a dev VM. The tests,
go vet,gofmtandallod checkran at05ac91d, the head of this branch. The runs of the binary ran at4d3e582, which differs from the head in one test file only, against a throwaway site whose preview app islive-server:go test ./...,go test -tags site ./...,go test -tags secret ./...,go test -tags site,secret ./..., each with-count=1: all seven packagesokin each of the four runs.go vet ./...andgofmt -l .: no output.allod check:All 11 steps passed (checks: 9 built).It runs the Go tests inside a nix build sandbox, whereTestPreviewSocketOwnerReadsTheControlGroupskips: the sandbox names a control group whose directory it does not mount. That test runs and passes in the dev shell on the VM, so inside the repository check the socket-owner rule is not witnessed against a real control group.--port: exit 0,http://127.0.0.1:38811, the page answers 200, and the listener on that port is the unit'slive-server.servewhile running: the same address.--stop: exit 0, with no port given.socat, from the registry and from--port:port 18649 is already in use: ... was not started, exit 1, no unit.allod-preview-*unit, nosocat, nolive-server.Run by the implementing worker at
1b2badc, which differs from4d3e582in comments and one reflowed help line, and not repeated by me:--stop, thenserve: a new port is picked.::ffff:127.0.0.1answers a connection to 127.0.0.1 and appears only in/proc/net/tcp6.Not run:
viewfrom a second machine: thesshto the VM and thessh -Lforward./proc/net/tcp6; that branch is written and not exercised.Review
One reviewer from another model family (
gpt-5.6-sol) read the diff cold at1e496ca, followed by one fix round.viewbuffered the VM's output without limit; it now keeps 128 bytes./proc/net/tcp6is read too.Environmentvalue could be misread as the port; a line with a quote character is refused.--stop.sshargument or a shell other than as the parsed integer, and no case of another process's socket reported as the unit's.Comments were cut down at the owner's request before this PR was opened, as on allod/archetypes#120 and allod/tools#250.
05ac91d910toeb35256652Closing unmerged, at the owner's decision. Picking a port is dropped: a site is served on the port the registry gives it, or on
--port, and on nothing else. The reasoning is in the closing comment on allod/tools#249.The part worth keeping, refusing to start on a port something else holds, continues as allod/tools#252 on a new branch cut from master, so master does not carry nine commits that add the pick followed by more that remove it.
The measurements and the review recorded in this pull request's description stay valid for what they describe. The socket-owner rule built here is named in allod/tools#252 as the follow-up if a port is ever seen taken between the check and the app's bind.
Pull request closed