fix(port-check): cgroup-based same-env ownership + Bug B loopback ZFS boot docs (#176) - #176
Merged
Merged
Conversation
…ign squatter Reproduces Bug A: for multi-service apps the port-check calls `samohost_port_check_ok <port> <listener-unit>@<env>.service` for each listener. When the listener's unit is FAILED but the port is held by a sibling unit of the same env, `systemctl is-active` returns false and the check misclassifies the port as a foreign squatter, triggering `rm -f SAMOHOST_CADDY_SNIPPET` + Caddy reload — killing the preview vhost. Observed on samograph VM (2026-07-16): samograph-docs-add-robot-to-readme and other envs with all ports = 3111 (single-port allocation); the web@env unit holds 3111 while live@env unit fails with EADDRINUSE, and heal cycles delete the Caddy snippet on every pass (CF 525). New tests (f1–f7): - f1: sibling same-env unit in cgroup → must return 0 (own, not foreign) - f2: same unit in FAILED state → cgroup match → must return 0 - f3: foreign process cgroup → must reject (non-zero) - f4: generated function must declare env_name param - f5: generated function must call samohost_read_cgroup_for_pid - f6: call site must pass env name as 3rd arg - f7: is-active fast path must be preserved (backward compat) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t no longer classified as foreign squatter (Bug A) Root cause: `samohost_port_check_ok(port, unit)` used `systemctl is-active` as the sole ownership signal. For multi-service apps (samograph: web + live services), when `samograph-live@env.service` is FAILED but a sibling unit `samograph-web@env.service` holds the same port (both got port=3111 in a single-port allocation), the check returns false-positive "foreign squatter" → `rm -f SAMOHOST_CADDY_SNIPPET` + `caddy reload` → CF 525 on every heal cycle. Fix: 1. Add `samohost_read_cgroup_for_pid()` helper (reads /proc/<pid>/cgroup, stubbed in tests without real /proc access). 2. Extend `samohost_port_check_ok` with a third `env_name` argument. After the `is-active` fast path fails, iterate PIDs holding the port via `ss -tlnHp` and check `/proc/<pid>/cgroup` for `@<envName>.service`. Any process whose cgroup matches our env's suffix is classified as OURS (not foreign) — covers both: a failed same-env unit being restarted, and a sibling service that shares the same port value. 3. Update all call sites to pass `sq(t.name)` as the third argument. Backward-compat: `systemctl is-active` fast path preserved; `env_name` defaults to "" so callers that omit it get the old behaviour. Verified: 2056/2056 tests pass (0 regressions). Real-world: samograph VM had samograph-docs-add-robot-to-readme and samograph-docs-polish-user-facing-text vhosts deleted on every heal cycle; fix prevents the deletion path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… VMs Root cause: `dblab-loopback.service` (a per-VM artifact, not in samohost provisioning) declared `Before=zfs-import.target` but `zfs-import-cache.service` started independently before the loopback was set up, causing it to abort (signal=ABRT) — leaving the ZFS pool `dblab` unmounted on every reboot. Manual workaround (operator steps): 1. Rewrite `dblab-loopback.service` to explicitly declare `Before=zfs-import-cache.service` (not just `zfs-import.target`). 2. Add a drop-in `/etc/systemd/system/zfs-import-cache.service.d/10-dblab-loopback.conf` with `Requires=dblab-loopback.service` + `After=dblab-loopback.service`. This makes the ZFS import wait for the loop device to exist. Applied live on samograph VM 2026-07-16. Bug B is NOT a samohost-code bug — the unit is a per-VM artifact installed manually per the runbook. The platform gap (DBLab not auto-provisioned by cloud-init) is tracked in docs/stack/dblab.md. The fix must be baked into the future cloud-init module (PR #128 follow-up). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n; tighten f6 env-name assertion
samorev REQUEST_CHANGES — two blocking fixes:
1. awk portability (finding 1): the 3-arg match(\$0, /pid=([0-9]+)/, a) form
is a gawk extension. On mawk (Ubuntu's default awk alternative when gawk is
not installed, which it is not in the bootstrap list), it errors silently:
zero PIDs are extracted, the cgroup ownership check is skipped entirely, and
the function falls through to return 1 (foreign squatter) — reproducing the
exact bug this PR fixes. Replaced with:
ss -tlnHp | grep -F ":${port} " | grep -oP 'pid=\K[0-9]+'
GNU grep is always present on Ubuntu and the -oP / \K forms work correctly.
Added integration test f8 that runs the grep fragment against the host's
actual grep with a synthetic ss line and asserts both PIDs are extracted.
f8 also asserts the gawk 3-arg pattern is NOT present in the generated fn.
2. f6 too weak (finding 2): the old regex matched any 3-token call. New test
extracts every samohost_port_check_ok call line from the script, strips
quoting from the third positional token, and compares it to target().name
("field-record-1-feat-x") exactly. A blank, wrong, or missing env name
would now fail the test.
Also added inline comment at the ss -tlnHp block noting the grep -oP
portability rationale (non-blocking suggestion from samorev).
2057/2057 tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs surfaced during samograph preview recovery (2026-07-16).
Bug A — port-check deletes vhost for failed/sibling same-env unit (HIGH)
Root cause:
samohost_port_check_ok(port, unit)usedsystemctl is-activeas the sole ownership signal. For multi-service apps with a single-port allocation (where all listeners share one port value, e.g.PORT=WS_HUB_PORT=INGEST_PORT=3111), the web unit holds the port while the live unit is FAILED. The check sees: port IS held + target unit (live@env) is NOT active → misclassifies as foreign squatter → runsrm -f SAMOHOST_CADDY_SNIPPET+caddy reload→ CF 525 on every heal cycle.Fix:
samohost_read_cgroup_for_pid()helper (reads/proc/<pid>/cgroup, stubbed in tests).samohost_port_check_okwith a thirdenv_nameargument.systemctl is-activefast path fails, iterate PIDs holding the port viass -tlnHpand check/proc/<pid>/cgroupfor@<envName>.service. Any match = our env = not foreign.sq(t.name)as the third arg.Tests (TDD, RED → GREEN):
2056/2056 tests pass (0 regressions).
Bug B — dblab-loopback.service doesn't start on reboot (per-VM artifact)
Root cause:
dblab-loopback.serviceon the samograph cx23 VM declaredBefore=zfs-import.targetbutzfs-import-cache.servicestarted independently (racing the loopback setup) and aborted (signal=ABRT). Every reboot left the ZFS pool unmounted and all DBLab previews broken untilzpool import dblabwas run manually.Fix: This is a per-VM artifact not in samohost code. Added runbook steps to:
dblab-loopback.servicewithBefore=zfs-import-cache.service(not justzfs-import.target)zfs-import-cache.service.d/10-dblab-loopback.confwithRequires=dblab-loopback.serviceApplied live on samograph VM. Documented in
docs/dblab-install-runbook.md+docs/stack/dblab.md.Platform gap: DBLab is not auto-provisioned by samohost (known gap, docs/stack/dblab.md). This fix must be baked into the future cloud-init module (PR #128 follow-up).
Preview re-provisioning (done live)
After Bug B workaround + destroy-then-recreate of PR #111/112:
Note on PR #111/112 app-api + ws-hub: Both branches are missing
apps/app-api/server.tsandapps/ws-hub/dev-live-server.ts— these modules were added after the branches were cut. The web (Next.js) builds and serves 200. Alex must rebase these branches on main to get the full multi-service stack working.Root-cause analysis (Bug A)
The port-check was designed to prevent a foreign process from squatting on a port before the preview's systemd unit could bind it. The ownership signal (
systemctl is-active) is correct for the single-service case but insufficient for multi-service apps: when all listeners share one port value (legacy single-port allocation), ANY unit in the same env (web, live, etc.) is the legitimate owner — yet the check only tests the specific listener's unit.The cgroup-based fallback is the correct fix:
/proc/<pid>/cgroupis written by systemd for every spawned process and contains the unit name (@<envName>.service), readable without sudo. A truly foreign process has no samohost unit name in its cgroup.Commits
d13df04RED test: same-env sibling/failed unit must not be foreign squatter8435628GREEN fix: cgroup-based same-env ownership (Bug A)d8cb842docs: loopback ZFS boot ordering fix (Bug B)Test plan
bun test)🤖 Generated with Claude Code