carlos / platform Public

branch review: comments tell the truth about the two key forms

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 6f7cf77283cc677e31980319911a7422ca8b337b parent 084fdb0

6 files changed, +19 −20

internal/activator/backend_exec.go +5 −5
internal/cgroup/cgroup.go +3 −4
internal/console/metricsview.go +1 −1
internal/console/metricsview_test.go +0 −3
internal/host/metrics_probes.go +6 −5
internal/metrics/sample.go +4 −2

diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go

index 3673b1e02..a1918bc07 100644

--- a/internal/activator/backend_exec.go

+++ b/internal/activator/backend_exec.go

@@ -255,7 +255,7 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error {

if err != nil {

// WaitDelay reports on the SUCCESS path too: a seed that exits 0 after

// backgrounding something (a helper, a stray `&`) leaves the inherited

- // stderr pipe open, and Run then returns ErrWaitDelay INSTEAD of nil

+ // stderr pipe open, and Wait then returns ErrWaitDelay INSTEAD of nil

// even though the process exited 0. Treating that as a failure would

// fail the wake AND delete the database the seed had just correctly

// built — leaving the host fresh again next wake, i.e. permanently

@@ -301,7 +301,7 @@ func seedCreatedTheDB(statErr error) bool { return os.IsNotExist(statErr) }

// seedTimeout bounds a first-boot seed run — a wedged seed must fail the wake

// (which releases the lease) rather than sit on it. seedDrainGrace is how long

-// Run then waits for the killed seed's output pipes to close before giving up

+// Wait then waits for the killed seed's output pipes to close before giving up

// on them (cmd.WaitDelay). Both vars so tests can shrink them.

var (

seedTimeout = 2 * time.Minute

@@ -311,9 +311,9 @@ var (

// capBuffer keeps at most capBufferMax bytes of what's written to it and

// discards the rest, so quoting a child's stderr in an error can never turn a

// chatty seed into an unbounded string. Locked because it is read after

-// cmd.Run returns while os/exec's own copy goroutine may still be writing: on

-// a WaitDelay expiry (the wedged-seed path) Run returns without waiting for

-// that copy to finish.

+// the seed's Wait returns while os/exec's own copy goroutine may still be

+// writing: on a WaitDelay expiry (the wedged-seed path) Wait returns without

+// waiting for that copy to finish.

type capBuffer struct {

mu sync.Mutex

b []byte

diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go

index dfa02cce1..9752a0cf0 100644

--- a/internal/cgroup/cgroup.go

+++ b/internal/cgroup/cgroup.go

@@ -108,10 +108,9 @@ func Setup(logf func(string, ...any)) *Manager {

}

// SetupAt is the delegation dance against an explicit subtree root — the

-// testable core Setup wraps, and the constructor activator tests use over a

-// temp-dir fixture. Order matters: probe, move self to a leaf, THEN enable

-// controllers — cgroup v2 refuses subtree_control writes while the cgroup

-// itself still has member processes ("no internal processes").

+// testable core Setup wraps. Order matters: probe, move self to a leaf, THEN

+// enable controllers — cgroup v2 refuses subtree_control writes while the

+// cgroup itself still has member processes ("no internal processes").

func SetupAt(root string, logf func(string, ...any)) (*Manager, error) {

// 1. Probe live delegation: both control files must be writable by us.

// systemd chowns exactly these to User= when Delegate= is set; without

diff --git a/internal/console/metricsview.go b/internal/console/metricsview.go

index 45789388c..e11a11c09 100644

--- a/internal/console/metricsview.go

+++ b/internal/console/metricsview.go

@@ -656,7 +656,7 @@ func (m InstanceMetrics) Reporting() bool { return m.Box != "" }

// readings, Reporting false), never zeros and never silently dropped — an

// owner whose instance is asleep must still see it listed.

func (s *Server) OwnerAppMetrics(ctx context.Context, hosts []string) ([]InstanceMetrics, error) {

- // The allowlist, keyed by the one unit name each host can legally match.

+ // The allowlist, keyed by the unit names each host can legally match.

// Matching is exact-string on this map: a unit the caller did not ask

// for cannot be reached by any path below.

want := make(map[string]string, 2*len(hosts))

diff --git a/internal/console/metricsview_test.go b/internal/console/metricsview_test.go

index 73ec4a2b4..7acbadf1f 100644

--- a/internal/console/metricsview_test.go

+++ b/internal/console/metricsview_test.go

@@ -1294,9 +1294,6 @@ func TestOwnerAppMetricsSeesExecBackedInstances(t *testing.T) {

if got.SparkCPU == "" || got.SparkMem == "" {

t.Fatalf("sparklines empty: %+v", got)

}

- if got.CPUFrac != nil && *got.CPUFrac == 0.44 {

- t.Fatalf("row carries another tenant's exec reading: %+v", got)

- }

}

// TestOwnerAppMetricsExecAndUnitKeysNeverDoubleReport pins the spec's

diff --git a/internal/host/metrics_probes.go b/internal/host/metrics_probes.go

index 8ad11bd6e..30ec0a3bb 100644

--- a/internal/host/metrics_probes.go

+++ b/internal/host/metrics_probes.go

@@ -3,11 +3,12 @@

// a guessed zero. This file is thin OS glue — os.ReadFile and os.ReadDir

// wrapping the pure parsers in internal/metrics — and the readers are

// deliberately untested by the same convention as probes.go readers;

-// internal/metrics tests exercise the parsers with fixtures instead. The one

-// exception is listCarlosUnits: it is a directory WALK, not a single read, and

-// which directories it walks is a decision that has already been wrong once

-// (see its doc comment), so it takes its root as an argument and is pinned by

-// metrics_probes_test.go against a temp-dir fixture.

+// internal/metrics tests exercise the parsers with fixtures instead. The

+// exceptions are listCarlosUnits and listExecCgroups: they are directory

+// WALKs, not single reads, and which directories they walk is a decision that

+// has already been wrong once (see listCarlosUnits' doc comment), so they take

+// their roots as arguments and are pinned by metrics_probes_test.go against

+// temp-dir fixtures.

package host

import (

diff --git a/internal/metrics/sample.go b/internal/metrics/sample.go

index 99dcef827..da14784a0 100644

--- a/internal/metrics/sample.go

+++ b/internal/metrics/sample.go

@@ -27,8 +27,10 @@ type UnitSample struct {

}

// Sample is one point-in-time reading of a box: the host as a whole plus

-// every monitored unit, keyed by full unit name (e.g.

-// carlos-app@x.example.com.service).

+// every monitored unit, keyed by unit name: systemd unit names (e.g.

+// carlos-app@x.example.com.service) and delegated exec-child cgroup names

+// (e.g. carlos-exec@x.example.com — no .service suffix; written by the

+// sampler's exec-subtree scan since exec-child accounting).

type Sample struct {

At time.Time `json:"at"`

Host HostSample `json:"host"`