amadan
host: fix round 1 — null unit-backed sockets, path.Join the lease prefix
Review confirmed a critical bug: listeningProbe's raw /proc/net/unix read reports "awake" forever for systemd-socket-activated (registry.BackingUnit) hibernating tenants, because the carlos-app@.socket unit holds the kernel listener open at all times regardless of whether the paired service is actually running. A confidently wrong reading where the status page's commitment demands an honest null. Fixed by extracting the decision into two pure, independently-testable functions in probes.go: unitBackedSockets(routes) collects the socket paths belonging to Backing==registry.BackingUnit routes, and wrapListeningProbe(unitBacked, probe) returns nil for any of those sockets before ever consulting the kernel table, passing exec-backed sockets through unchanged (where the kernel-table reading IS reliable). statusLoop now lists routes once per tick specifically to build this set before constructing status.Probes, wiring Listening as wrapListeningProbe(unitBackedSockets(routes), listeningProbe) instead of the raw probe. Real fix (asking systemd directly) is future work, noted in the doc comment rather than guessed at. New TestWrapListeningProbeNullsUnitBackedSockets in host_test.go proves the seam directly and confirms the one-level-up observable effect: because snapshot.go's CollectBox nulls the whole box's Awake/Asleep pair the instant any hibernating route's Listening call returns nil, a box with even one unit-backed hibernating tenant now correctly reports its ENTIRE awake/asleep split as unknown, not just that one route's contribution. Minor: replaced the lease's `prefix+"/status-lease"` string concat with path.Join(prefix, "status-lease"), matching activator.go's Config.hibernatePrefix convention and guarding a trailing-slash CARLOS_STATUS_PREFIX from producing a doubled "//status-lease" key.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+123
−13
internal/host/agent.go+20 −2internal/host/host_test.go+47 −0internal/host/probes.go+56 −11
diff --git a/internal/host/agent.go b/internal/host/agent.go| index a892dfb25..b170ff6e8 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -44,6 +44,7 @@ import ( |
| "math/rand/v2" |
| "os" |
| "os/exec" |
| + "path" |
| "path/filepath" |
| "strings" |
| "time" |
| @@ -213,12 +214,25 @@ func statusLoop(reg *registry.Registry) { |
| time.Sleep(delay) |
| delay = 5*time.Minute + rand.N(time.Minute) |
| + // Routes are listed here (not left to statusTick's own reg.List()) |
| + // because the Listening probe needs to know which sockets are |
| + // systemd-socket-activated BEFORE it's built — see |
| + // wrapListeningProbe's doc comment. statusTick lists again for |
| + // itself since it stays store/lease-agnostic and self-contained; |
| + // one extra List() per 5-minute tick is not worth threading routes |
| + // through the pure function's signature for. |
| + routes, err := reg.List() |
| + if err != nil { |
| + log.Printf("agent: status tick: list routes: %v", err) |
| + continue |
| + } |
| + |
| p := status.Probes{ |
| MachineID: readMachineID(), |
| Region: boxRegion, |
| MemFrac: readMemFrac(), |
| DiskFrac: readDiskFrac(), |
| - Listening: listeningProbe, |
| + Listening: wrapListeningProbe(unitBackedSockets(routes), listeningProbe), |
| // LagSecs stays nil: litestream's per-DB replication lag isn't |
| // surfaced up to the agent process anywhere yet — the |
| // replication machinery (internal/lease's Replicator) lives |
| @@ -230,7 +244,11 @@ func statusLoop(reg *registry.Registry) { |
| // a gap to paper over now. |
| } |
| - held, acqErr := lease.Acquire(ctx, bucket, prefix+"/status-lease", region) |
| + // path.Join (an S3 key, not a filesystem path — matches |
| + // activator.go's Config.hibernatePrefix convention) rather than a |
| + // bare "+" concat: guards against a trailing-slash |
| + // CARLOS_STATUS_PREFIX producing a doubled "//status-lease" key. |
| + held, acqErr := lease.Acquire(ctx, bucket, path.Join(prefix, "status-lease"), region) |
| publish := acqErr == nil |
| if acqErr != nil { |
| var heldErr *lease.HeldError |
diff --git a/internal/host/host_test.go b/internal/host/host_test.go| index d31937844..3c0851bbb 100644 |
| --- a/internal/host/host_test.go |
| +++ b/internal/host/host_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "path/filepath" |
| "strings" |
| "testing" |
| + "time" |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -213,6 +214,52 @@ func TestStatusTickWritesBoxAndPublishesOnlyWhenAsked(t *testing.T) { |
| } |
| } |
| +// TestWrapListeningProbeNullsUnitBackedSockets is fix round 1's regression |
| +// test: listeningProbe's raw /proc/net/unix read is confidently WRONG for |
| +// systemd-socket-activated (registry.BackingUnit) routes, since the |
| +// carlos-app@.socket unit holds the kernel-level listener open at all times |
| +// regardless of whether the paired service is actually awake — so a raw |
| +// probe would report "awake" forever for a sleeping unit-backed tenant. This |
| +// exercises the pure seam (unitBackedSockets + wrapListeningProbe) statusLoop |
| +// wires together, with a stand-in raw probe standing in for |
| +// /proc/net/unix's "yes, bound" answer on both sockets — exactly what a real |
| +// kernel-table read would say, since the unit's socket is always bound. |
| +func TestWrapListeningProbeNullsUnitBackedSockets(t *testing.T) { |
| + routes := []registry.Route{ |
| + {Host: "exec-app", Kind: "instance", Hibernate: true, Socket: "/run/carlos/exec-app.sock"}, |
| + {Host: "unit-app", Kind: "instance", Hibernate: true, Socket: "/run/carlos/apps/unit-app.sock", Backing: registry.BackingUnit}, |
| + } |
| + |
| + // rawAlwaysListening stands in for listeningProbe: both sockets read as |
| + // bound/listening, which is the honest kernel-table answer for BOTH |
| + // routes here (the unit-backed one's systemd .socket unit never closes |
| + // its listener) — the wrapper's job is to know that answer can't be |
| + // trusted for the unit-backed one. |
| + rawAlwaysListening := func(string) *bool { v := true; return &v } |
| + wrapped := wrapListeningProbe(unitBackedSockets(routes), rawAlwaysListening) |
| + |
| + if got := wrapped(routes[0].Socket); got == nil || !*got { |
| + t.Errorf("exec-backed socket must pass through to the raw probe unchanged, got %v", got) |
| + } |
| + if got := wrapped(routes[1].Socket); got != nil { |
| + t.Errorf("unit-backed socket must report nil (unknown), got %v — its .socket unit's always-open listener can't distinguish awake from asleep", got) |
| + } |
| + |
| + // One level up, the observable effect on a published snapshot: CollectBox |
| + // nulls the WHOLE box's Awake/Asleep pair the instant any hibernating |
| + // route's Listening call returns nil (snapshot.go's partial-knowledge |
| + // rule — "some known, some not" publishes as null, never a partial |
| + // count). So even though the exec-backed route's own state IS known |
| + // here, the box-wide totals still come back nil because the unit-backed |
| + // route's is not. That's the correct, if blunt, behavior this fix |
| + // produces: a box carrying even one systemd-socket-activated hibernating |
| + // tenant can't yet publish a trustworthy awake/asleep split at all. |
| + b := status.CollectBox(routes, status.Probes{MachineID: "m", Listening: wrapped}, time.Now()) |
| + if b.Routes.Awake != nil || b.Routes.Asleep != nil { |
| + t.Errorf("box awake=%v asleep=%v, want both nil: one unit-backed hibernating route must null the whole box's counts, not just its own", b.Routes.Awake, b.Routes.Asleep) |
| + } |
| +} |
| + |
| func TestBootstrapIdempotent(t *testing.T) { |
| root := t.TempDir() |
| if err := bootstrap(root); err != nil { |
diff --git a/internal/host/probes.go b/internal/host/probes.go| index eb03b5433..ea5d93ff2 100644 |
| --- a/internal/host/probes.go |
| +++ b/internal/host/probes.go |
| @@ -16,6 +16,8 @@ import ( |
| "strconv" |
| "strings" |
| "syscall" |
| + |
| + "github.com/carlosframework/platform/internal/registry" |
| ) |
| // readMachineID reads /etc/machine-id, trimmed. Unreadable (missing file, |
| @@ -95,17 +97,23 @@ func readDiskFrac() *float64 { |
| return &frac |
| } |
| -// listeningProbe answers status.Probes.Listening by reading /proc/net/unix |
| -// — the kernel's own table of bound unix-domain sockets — rather than |
| -// dialing sock. Dialing would itself be a connection attempt on a |
| -// hibernating tenant's socket, which is exactly the event that wakes it: the |
| -// probe would falsify the very thing it's trying to observe. Presence of |
| -// sock's path in the table is the awake signal (an exec-backed hibernating |
| -// route's socket only exists in the kernel while the spawned child holds it |
| -// open; asleep, the child is gone and the entry is gone with it); absence is |
| -// asleep. Only a failure to read the table at all -> nil ("unknown"), which |
| -// is what makes CollectBox null the whole Awake/Asleep pair for this box |
| -// rather than reporting a guess. |
| +// listeningProbe reads /proc/net/unix — the kernel's own table of bound |
| +// unix-domain sockets — rather than dialing sock. Dialing would itself be a |
| +// connection attempt on a hibernating tenant's socket, which is exactly the |
| +// event that wakes it: the probe would falsify the very thing it's trying to |
| +// observe. Presence of sock's path in the table is the awake signal, absence |
| +// is asleep; only a failure to read the table at all -> nil ("unknown"). |
| +// |
| +// This reading is correct ONLY for exec-backed routes, where the socket only |
| +// exists in the kernel while the spawned child holds it open — asleep, the |
| +// child is gone and the entry is gone with it. It is CONFIDENTLY WRONG for |
| +// systemd-socket-activated (registry.BackingUnit) routes: the carlos-app@.socket |
| +// unit holds the listener open in the kernel table at all times, awake or |
| +// asleep, specifically so systemd can catch the first connection and spawn |
| +// the service — so this function alone would report "awake" forever for a |
| +// sleeping unit-backed tenant. Callers MUST route unit-backed sockets through |
| +// wrapListeningProbe below rather than calling this directly; statusLoop is |
| +// the only caller and does exactly that. |
| func listeningProbe(sock string) *bool { |
| f, err := os.Open("/proc/net/unix") |
| if err != nil { |
| @@ -133,3 +141,40 @@ func listeningProbe(sock string) *bool { |
| } |
| return &found |
| } |
| + |
| +// unitBackedSockets returns the set of socket paths belonging to routes |
| +// whose Backing is registry.BackingUnit — the systemd-socket-activated |
| +// whole-app tenants that make listeningProbe's kernel-table reading |
| +// unreliable (see its doc comment). Pure and routes-only so it and |
| +// wrapListeningProbe below are each independently testable at the seam |
| +// host_test.go exercises, with no /proc read anywhere near the test. |
| +func unitBackedSockets(routes []registry.Route) map[string]bool { |
| + set := map[string]bool{} |
| + for _, r := range routes { |
| + if r.Backing == registry.BackingUnit { |
| + set[r.Socket] = true |
| + } |
| + } |
| + return set |
| +} |
| + |
| +// wrapListeningProbe wraps probe so a unit-backed socket (per unitBacked) |
| +// always reports nil — unknown — instead of whatever probe would say. |
| +// systemd's .socket unit holds a unit-backed tenant's listener open in the |
| +// kernel table at ALL times, awake or asleep; a raw kernel-table read can |
| +// therefore never tell the two states apart for these routes, and reporting |
| +// "awake" from that alone would be a confidently wrong answer where the |
| +// status page's commitment demands an honest null. This stays true until the |
| +// agent learns to ask systemd itself (e.g. `systemctl is-active |
| +// carlos-app@<host>.service`) — a real fix, not implemented here, tracked as |
| +// future work rather than guessed at. Exec-backed sockets (not in the set) |
| +// pass straight through to probe unchanged, since listeningProbe's |
| +// kernel-table reading IS reliable for them. |
| +func wrapListeningProbe(unitBacked map[string]bool, probe func(string) *bool) func(string) *bool { |
| + return func(sock string) *bool { |
| + if unitBacked[sock] { |
| + return nil |
| + } |
| + return probe(sock) |
| + } |
| +} |