carlos / platform Public

relay: make the #120 follow-up tests prove what they claim

Review findings on the previous commit. Two of them were tests that
passed against deliberately broken code.

TestServeEndsAllocationsOnDisable passed with Serve's
EndWhere(..., "authority") call deleted -- the only code that ends a live
allocation when the record drops the app. Its sole assertion was a fresh
Allocate, which proves admission refuses a NEW allocation and says nothing
about the live one, so the spec §7 test it exists to be was still missing.
It now carries a packet through the relay to a permitted loopback peer
while the app is enabled, and asserts nothing arrives after the disable.
That needs a peer the built-in policy would refuse, so Config gains an
unexported extraAllow beside the existing udp/tcp listen seams; the
advertised relay address is non-loopback, as on a real box, because a relay
IP equal to the peers does not forward.

Both wiring hops of the retiring display could be deleted with the whole
suite green: the heartbeat stopped reporting it and the console stopped
copying it, and only the two leaves were tested. The relay hop is now
covered by the heartbeat test (polled, because the masters watcher is a
goroutine and the first heartbeat can legitimately precede the file read)
and the console hop by a status-door test.

Also: skip the new permission test under -race, which it made unusable for
the whole package over an unlocked lifetimeTimer in pion/turn that nothing
here can fix; join the retiring list with the tight separator the masters
list uses, so its items cannot be misread as further clauses, and seed the
fixture with two kids out of order so the separator and the sort are both
asserted; correct the standing apiRelayReply comment, which read as a
blanket prohibition on reporting a retirement date; list issued_suffixes
and cutovers in the spec's record alongside issued_digest; make
Masters.Retiring nil-safe rather than relying on its one guarded caller.

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

Paul Campbell pushed by paul@keymail.dev 1 month ago 2f1b4d267a3acf34eb00318216ee71eb951b9918 parent 4845da6

12 files changed, +180 −29

docs/site/cli.md +1 −2
docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md +9 −5
internal/console/apirelay.go +6 −0
internal/console/apirelay_test.go +43 −1
internal/relay/masters.go +3 −0
internal/relay/race_off_test.go +6 −0
internal/relay/race_on_test.go +12 −0
internal/relay/serve.go +22 −14
internal/relay/serve_test.go +68 −4
internal/relay/server_test.go +3 −0
relay.go +1 −1
relay_test.go +6 −2

CI passed — run details

diff --git a/docs/site/cli.md b/docs/site/cli.md

index 3db937947..fc75af093 100644

--- a/docs/site/cli.md

+++ b/docs/site/cli.md

@@ -912,8 +912,7 @@ cannot restart for you. `rotate --region <r>` is the operator's verb after

staging a new master; `keygen` prints one. After a rotation, `status` names

any master that is retiring and the time it may be dropped, box by box, since

a region is done with a key only once every relay there has stopped loading

-it. `serve` is the unit's entry point

-and not a thing to run by hand.

+it. `serve` is the unit's entry point and not a thing to run by hand.

`keygen` prints one line — a key id, a key, and `-` for "no retirement date"

— for a region's master key. Stage that line in `/etc/carlos/relay-masters`

diff --git a/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md b/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md

index 0c59d31c4..087801786 100644

--- a/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md

+++ b/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md

@@ -393,11 +393,15 @@ ours. **(codex R1-20)**

- `apps/<acct>/<app>/relay.json`, CAS-guarded like `store.json`, is the

**source of truth** for the app: `{enabled, incarnation, regions,

- op, phase, issued_digest, enabled_at, disabled_at}`. `issued_digest`

- is the sha256 of the sealed envelope that was delivered, which is

- what `Record.IssuedDigest` records and what reconcile compares; an

- earlier draft of this spec called it `config_generation` and no

- implementation ever carried that name. **(discussion #120)** `op` is a

+ op, phase, issued_digest, issued_suffixes, cutovers, enabled_at,

+ disabled_at}`. `issued_digest` is the sha256 of the sealed envelope

+ that was delivered, which is what `Record.IssuedDigest` records and

+ what reconcile compares; an earlier draft of this spec called it

+ `config_generation` and no implementation ever carried that name.

+ `issued_suffixes` (region to the username suffix last issued) and

+ `cutovers` (the per-route member acknowledgements) are the other two

+ fields `relay.Record` has always carried and this listing has always

+ omitted. **(discussion #120)** `op` is a

counter bumped by every operation; `phase` is one of `config`,

`aggregate`, `done` and records how far the operation got. The

incarnation is sixteen hex characters drawn at random at first

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

index 75db24f0d..b2d40fbb0 100644

--- a/internal/console/apirelay.go

+++ b/internal/console/apirelay.go

@@ -221,6 +221,12 @@ type apiRelayReply struct {

// retirement date on an app's status would mean either inventing state the

// console does not keep or recomputing it from a clock, which would tell a

// member a retirement deadline that drifts every time they ask.

+ //

+ // apiRelayRegion.Retiring is NOT that value and is not a softening of this

+ // rule (discussion #120). It is the retire_at a relay actually loaded out

+ // of its own masters file, reported per box and echoed verbatim: state the

+ // operator wrote, not state this console computed, so it cannot drift

+ // between two reads.

}

// apiRelayRotateReply is what `carlos relay rotate` answers with — a region

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

index f54ab92c5..b5762f374 100644

--- a/internal/console/apirelay_test.go

+++ b/internal/console/apirelay_test.go

@@ -36,11 +36,18 @@ func relayURL(fx *consoleFixture, app string) string {

// with its last SUCCESSFUL authority read `age` in the past — the clock

// status reads, and the one a dead relay cannot move.

func seedHeartbeat(t *testing.T, fx *consoleFixture, box, region string, age time.Duration, kids ...string) {

+ t.Helper()

+ seedHeartbeatRetiring(t, fx, box, region, age, nil, kids...)

+// seedHeartbeatRetiring is seedHeartbeat with the retiring map set, for the

+// one test that covers the hop from a relay's heartbeat to the status reply.

+func seedHeartbeatRetiring(t *testing.T, fx *consoleFixture, box, region string, age time.Duration, retiring map[string]time.Time, kids ...string) {

t.Helper()

now := time.Now()

hb := relay.Heartbeat{

Box: box, Region: region, ReadAt: now.Add(-age), WrittenAt: now.Add(-age),

- Listeners: []string{"udp:3478", "tcp:3478"}, Kids: kids,

+ Listeners: []string{"udp:3478", "tcp:3478"}, Kids: kids, Retiring: retiring,

}

if err := relay.WriteHeartbeat(context.Background(), fx.rel, hb); err != nil {

t.Fatalf("seed heartbeat: %v", err)

@@ -741,3 +748,38 @@ func TestAPIRelayStatusNeverAdoptsOnEmptyIssuedSuffixes(t *testing.T) {

t.Fatalf("delivery = %q, an empty-IssuedSuffixes record must never read as adopted", got)

}

+// TestAPIRelayStatusCarriesRetiringMasters covers the hop from a relay's

+// heartbeat to the status reply (discussion #120). Without it the console

+// half of the retiring display can be deleted with the whole suite green,

+// and an operator mid-rotation silently reads "nothing is retiring" — which

+// is indistinguishable from the true answer and is the wrong one to act on.

+func TestAPIRelayStatusCarriesRetiringMasters(t *testing.T) {

+ fx, _ := relayFixture(t, "code-relay-retiring")

+ token := cliToken(t, fx, "alice@keymail.dev", "code-relay-retiring-2")

+ if status, out := postJSON(t, relayURL(fx, "titogo"), map[string]any{}, token); status != http.StatusOK {

+ t.Fatalf("enable = %d %v, want 200", status, out)

+ }

+ retire := time.Date(2026, 9, 9, 12, 0, 0, 0, time.UTC)

+ seedHeartbeatRetiring(t, fx, "box1", "eu-west-1", time.Minute,

+ map[string]time.Time{"k1": retire}, "k1", "k2")

+ status, out := getJSON(t, relayURL(fx, "titogo"), token)

+ if status != http.StatusOK {

+ t.Fatalf("status = %d %v, want 200", status, out)

+ }

+ row := regionRow(t, out, 0)

+ got, ok := row["retiring"].(map[string]any)

+ if !ok {

+ t.Fatalf("region row carries no retiring map: %#v", row)

+ }

+ if got["k1"] != retire.Format(time.RFC3339) {

+ t.Errorf("retiring[k1] = %v, want %s", got["k1"], retire.Format(time.RFC3339))

+ }

+ // k2 is loaded but carries no retirement date, so it must be absent

+ // rather than present with a zero time.

+ if _, ok := got["k2"]; ok {

+ t.Errorf("k2 has no retirement date and must not appear: %#v", got)

+ }

diff --git a/internal/relay/masters.go b/internal/relay/masters.go

index 78e19ae30..12886dbd8 100644

--- a/internal/relay/masters.go

+++ b/internal/relay/masters.go

@@ -74,6 +74,9 @@ func (m *Masters) Lookup(kid string, now time.Time) (Master, bool) {

// date are absent rather than zero-valued, so "nothing is retiring" is an

// empty map and not a map of zero times.

func (m *Masters) Retiring() map[string]time.Time {

+ if m == nil {

+ return nil

+ }

out := map[string]time.Time{}

for k, mk := range m.byKid {

if !mk.RetireAt.IsZero() {

diff --git a/internal/relay/race_off_test.go b/internal/relay/race_off_test.go

new file mode 100644

index 000000000..472c943f7

--- /dev/null

+++ b/internal/relay/race_off_test.go

@@ -0,0 +1,6 @@

+//go:build !race

+package relay

+// raceDetector is false in an ordinary build. See race_on_test.go.

+const raceDetector = false

diff --git a/internal/relay/race_on_test.go b/internal/relay/race_on_test.go

new file mode 100644

index 000000000..5ce1e39f7

--- /dev/null

+++ b/internal/relay/race_on_test.go

@@ -0,0 +1,12 @@

+//go:build race

+package relay

+// raceDetector lets one test skip itself under -race. The race it would

+// report is upstream, in pion/turn: Allocation.AddPermission publishes a

+// permission under permissionsLock and then calls perms.start(), which

+// writes p.lifetimeTimer with no lock, while Allocation.Close reads that

+// field from the packet-handler goroutine. Nothing in this package can fix

+// it, and leaving it unskipped costs the whole package its -race coverage,

+// which is worth more than this one test (discussion #120).

+const raceDetector = true

diff --git a/internal/relay/serve.go b/internal/relay/serve.go

index 50dacfff8..9c3aaae33 100644

--- a/internal/relay/serve.go

+++ b/internal/relay/serve.go

@@ -124,6 +124,13 @@ type Config struct {

// test seam and nothing else: production is :3478 on both, per spec §3,

// and no env may move them. Empty means the constants above.

udp, tcp string

+ // extraAllow is the same kind of seam, for the same reason. A relay on a

+ // real box never permits a loopback peer, and an end-to-end test that

+ // carries traffic between two sockets on this machine has nowhere else

+ // to put them. Unexported so no env and no operator can widen the peer

+ // policy; ServerConfig.ExtraAllow is the reviewed, production-facing one.

+ extraAllow []*net.IPNet

}

// ConfigFromEnv builds a Config from the unit's environment. imdsIP and

@@ -242,20 +249,21 @@ func Serve(ctx context.Context, cfg Config, logf func(string, ...any)) error {

tcp = relayTCPAddr

}

srv, err := NewServer(ServerConfig{

- Region: cfg.Region,

- RelayIP: cfg.PublicIP,

- UDP: udp,

- TCP: tcp,

- MinPort: 49152,

- MaxPort: 65535,

- DenyCIDRs: cfg.DenyCIDRs,

- Limits: cfg.Limits,

- Now: now,

- Log: logf,

- Meter: meter,

- Masters: func() *Masters { mu.Lock(); defer mu.Unlock(); return masters },

- Authority: func() *Authority { mu.Lock(); defer mu.Unlock(); return auth },

- Lease: func(t time.Time) bool { mu.Lock(); defer mu.Unlock(); return lease.Live(t) },

+ Region: cfg.Region,

+ RelayIP: cfg.PublicIP,

+ UDP: udp,

+ TCP: tcp,

+ MinPort: 49152,

+ MaxPort: 65535,

+ DenyCIDRs: cfg.DenyCIDRs,

+ ExtraAllow: cfg.extraAllow,

+ Limits: cfg.Limits,

+ Now: now,

+ Log: logf,

+ Meter: meter,

+ Masters: func() *Masters { mu.Lock(); defer mu.Unlock(); return masters },

+ Authority: func() *Authority { mu.Lock(); defer mu.Unlock(); return auth },

+ Lease: func(t time.Time) bool { mu.Lock(); defer mu.Unlock(); return lease.Live(t) },

})

if err != nil {

return err

diff --git a/internal/relay/serve_test.go b/internal/relay/serve_test.go

index b50b64ae1..6008e1ed1 100644

--- a/internal/relay/serve_test.go

+++ b/internal/relay/serve_test.go

@@ -189,7 +189,11 @@ func TestServeWritesHeartbeatAndStops(t *testing.T) {

store := newMemStore(t)

dir := t.TempDir()

masters := filepath.Join(dir, "relay-masters")

- if err := os.WriteFile(masters, []byte("k1 "+b64(testMaster())+" -\n"), 0o640); err != nil {

+ // k2 carries a retirement date, so this fixture also covers the hop that

+ // puts it on the heartbeat for `carlos relay status` (discussion #120).

+ if err := os.WriteFile(masters, []byte(

+ "k1 "+b64(testMaster())+" -\n"+

+ "k2 "+b64(testMaster())+" 2026-09-09T12:00:00Z\n"), 0o640); err != nil {

t.Fatal(err)

}

cfg := Config{

@@ -228,6 +232,28 @@ func TestServeWritesHeartbeatAndStops(t *testing.T) {

if hb.ReadAt.IsZero() {

t.Error("heartbeat carries no read time, so the lease clock is invisible")

}

+ // Without this the relay half of the retiring display can be deleted with

+ // the whole suite green, and an operator mid-rotation silently reads

+ // "nothing is retiring". Polled rather than asserted on the heartbeat

+ // above: the masters watcher is a goroutine, so the FIRST heartbeat can

+ // legitimately be written before the file has been read.

+ deadline := time.Now().Add(10 * time.Second)

+ for {

+ hb := waitHeartbeat(t, store, "box-serve")

+ if got, ok := hb.Retiring["k2"]; ok {

+ if want := "2026-09-09T12:00:00Z"; got.UTC().Format(time.RFC3339) != want {

+ t.Errorf("k2 retire_at = %s, want %s", got.UTC().Format(time.RFC3339), want)

+ }

+ if _, ok := hb.Retiring["k1"]; ok {

+ t.Error("k1 has no retirement date and must not be reported as retiring")

+ }

+ break

+ }

+ if time.Now().After(deadline) {

+ t.Fatalf("no heartbeat ever carried k2's retirement (last: %+v)", hb.Retiring)

+ }

+ time.Sleep(50 * time.Millisecond)

+ }

cancel()

select {

@@ -482,10 +508,14 @@ func TestServeEndsAllocationsOnDisable(t *testing.T) {

cfg := Config{

Box: "box-disable", Region: "eu-west-1", MastersPath: mastersPath,

- PublicIP: net.ParseIP("127.0.0.1"), Store: store, Limits: DefaultLimits(),

+ // The advertised relay address is deliberately NOT loopback, as on a

+ // real box: the peers are on loopback and a relay IP equal to them

+ // does not forward (TestServerRelaysBothLegs makes the same choice).

+ PublicIP: net.ParseIP("203.0.113.7"), Store: store, Limits: DefaultLimits(),

PressurePath: filepath.Join(dir, "no-pressure-here"),

udp: "127.0.0.1:0",

tcp: "127.0.0.1:0",

+ extraAllow: mustCIDRs("127.0.0.0/8"),

}

var logMu sync.Mutex

quiet := false

@@ -512,6 +542,28 @@ func TestServeEndsAllocationsOnDisable(t *testing.T) {

}

defer relayConn.Close()

+ // A peer on loopback, permitted through the extraAllow seam, and one

+ // packet through the relay to prove the allocation carries traffic while

+ // the app IS enabled. Without this the "no traffic after" check below

+ // would pass against a relay that never forwarded anything at all.

+ peer, err := net.ListenPacket("udp4", "127.0.0.1:0")

+ if err != nil {

+ t.Fatal(err)

+ }

+ defer peer.Close()

+ peerAddr := peer.LocalAddr().(*net.UDPAddr)

+ if err := client.CreatePermission(peerAddr); err != nil {

+ t.Fatalf("permission for a loopback peer: %v", err)

+ }

+ if _, err := relayConn.WriteTo([]byte("before"), peerAddr); err != nil {

+ t.Fatalf("write through the relay: %v", err)

+ }

+ buf := make([]byte, 64)

+ peer.SetReadDeadline(time.Now().Add(5 * time.Second))

+ if n, _, err := peer.ReadFrom(buf); err != nil || string(buf[:n]) != "before" {

+ t.Fatalf("the relay did not carry traffic while enabled: n=%d err=%v", n, err)

+ }

// The disable: the same record without the app, and a serial bump so the

// relay re-reads inside three seconds rather than at the next minute.

disabledAt := putAuthority(nil)

@@ -535,8 +587,20 @@ func TestServeEndsAllocationsOnDisable(t *testing.T) {

time.Sleep(100 * time.Millisecond)

}

- // Now, and only now, one probe: a credential that was good a moment ago

- // must be refused, which is what a member sees after `relay disable`.

+ // The allocation taken before the disable must stop carrying traffic.

+ // This is the assertion the test is named for, and the first version of

+ // it did not make: it only re-Allocated, which proves admission refuses

+ // a NEW allocation and says nothing about the live one. Deleting Serve's

+ // EndWhere(..., "authority") call left that version green.

+ if _, err := relayConn.WriteTo([]byte("after"), peerAddr); err == nil {

+ peer.SetReadDeadline(time.Now().Add(time.Second))

+ if n, _, err := peer.ReadFrom(buf); err == nil {

+ t.Errorf("the relay forwarded %q after the app was disabled: the allocation did not end", buf[:n])

+ }

+ // And a credential that was good a moment ago must be refused, which is

+ // what a member sees after `relay disable`.

probe := turnClient(t, &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: port}, username, password)

if _, err := probe.Allocate(); err == nil {

t.Error("the relay allocated for an app the authority record no longer carries")

diff --git a/internal/relay/server_test.go b/internal/relay/server_test.go

index a1e921b16..0b69675e4 100644

--- a/internal/relay/server_test.go

+++ b/internal/relay/server_test.go

@@ -666,6 +666,9 @@ func TestServerCloseKeepsReplacementBinding(t *testing.T) {

// private network, and until now only the built-in ranges had a test

// (discussion #120).

func TestServerRefusesPermissionIntoConfiguredDenyCIDR(t *testing.T) {

+ if raceDetector {

+ t.Skip("pion/turn races on Permission.lifetimeTimer between AddPermission and Close; see race_on_test.go")

+ }

// 198.51.100.0/24 stands in for the VPC: globally routable as far as

// the built-in private list is concerned, denied only because the

// operator said so.

diff --git a/relay.go b/relay.go

index 442de8d6c..b9c8492fb 100644

--- a/relay.go

+++ b/relay.go

@@ -418,7 +418,7 @@ func printRelayStatus(w io.Writer, res apiRelayReply) {

for _, kid := range kids {

parts = append(parts, kid+" after "+rg.Retiring[kid])

}

- line += ", retiring " + strings.Join(parts, ", ")

+ line += ", retiring " + strings.Join(parts, ",")

}

switch {

case rg.Stale:

diff --git a/relay_test.go b/relay_test.go

index e06069306..3d98f753f 100644

--- a/relay_test.go

+++ b/relay_test.go

@@ -180,10 +180,14 @@ func TestPrintRelayStatus(t *testing.T) {

r.RegionStatus = []apiRelayRegion{{

Region: "eu-west-1", Box: "relay-1", HeartbeatAge: "12s", Current: true,

LeaseLive: true, Listeners: []string{"udp:3478"}, Kids: []string{"k1", "k2"},

- Retiring: map[string]string{"k1": "2026-09-09T12:00:00Z"},

+ Retiring: map[string]string{"k2": "2026-09-10T12:00:00Z", "k1": "2026-09-09T12:00:00Z"},

}}

- want: []string{"masters k1,k2", "retiring k1 after 2026-09-09T12:00:00Z"},

+ // Two of them, deliberately seeded out of order: the kids are

+ // sorted, and the retiring clause uses the same tight separator

+ // the masters list does, so its items cannot be misread as

+ // further clauses of the line.

+ want: []string{"masters k1,k2", "retiring k1 after 2026-09-09T12:00:00Z,k2 after 2026-09-10T12:00:00Z"},

{

name: "stale names the relay's own reason",