relay: the review follow-ups from #120
Four items from the relay implementation review, none of which blocked go-live. `carlos relay status` now names a retiring master and when it may be dropped. The relay reports each loaded master's retire_at in its heartbeat, the console carries it through the status reply, and the CLI prints it on the per-box line. Per box on purpose: a region is done with a key only once every relay there has stopped loading it, so the answer is not a single deployment-wide date. Two end-to-end tests spec §7 asks for and the branch never carried. The first covers CARLOS_RELAY_DENY_CIDRS, which is what holds the VPC boundary on a real box -- the built-in private ranges already had a test, the operator-configured ones had none, and they are ordinary public-looking space as far as the built-in list is concerned. The second drives a disable through the real Serve loop and its authority read rather than a direct EndWhere call, using a serial bump so the re-read happens in three seconds instead of a minute. That second test was wrong first: it retried Allocate and broke on any error, so a refusal for an unrelated reason (a per-app allocation cap) read exactly like the disable, and it passed with the app still enabled. It now waits on the heartbeat's own report of which record it holds, then makes one probe. The writer-lease refusal says to retry and roughly when. The commonest cause is not a second console but this one having restarted, leaving its own previous process's lease to expire. The spec's §5 record listing said config_generation; the implementation has always recorded issued_digest, the sha256 of the sealed envelope, on Record.IssuedDigest. Amended the spec to match the code. User-facing strings reviewed and approved with no edits.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
13 files changed,
+284
−4
apimode.go+3 −1docs/site/cli.md+4 −1docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md+5 −1internal/console/apirelay.go+13 −0internal/console/relayops.go+8 −1internal/relay/authority.go+5 −0internal/relay/masters.go+16 −0internal/relay/masters_test.go+23 −0internal/relay/serve.go+13 −0internal/relay/serve_test.go+127 −0internal/relay/server_test.go+34 −0relay.go+18 −0relay_test.go+15 −0
CI passed — run details
diff --git a/apimode.go b/apimode.go| index 16adcf4c7..7e8d0cd6f 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -2109,7 +2109,9 @@ type apiRelayRegion struct { |
| LeaseLive bool `json:"lease_live"` |
| Listeners []string `json:"listeners"` |
| Kids []string `json:"kids"` |
| - Stale bool `json:"stale"` |
| + // kid -> RFC3339 retire_at, for the loaded masters that carry one. |
| + Retiring map[string]string `json:"retiring,omitempty"` |
| + Stale bool `json:"stale"` |
| // The relay's own last two errors: why it stopped reading. Printed on a |
| // stale row, because the state without the reason is not actionable. |
| MastersError string `json:"masters_error,omitempty"` |
diff --git a/docs/site/cli.md b/docs/site/cli.md| index 1f638d8fc..3db937947 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -909,7 +909,10 @@ app holds ends on the relays' next authority read, within a minute. |
| TCP-address route it restarted by hand now runs the new key — `status` names |
| the routes waiting on that, since they are the one shape this platform |
| cannot restart for you. `rotate --region <r>` is the operator's verb after |
| -staging a new master; `keygen` prints one. `serve` is the unit's entry point |
| +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. |
| `keygen` prints one line — a key id, a key, and `-` for "no retirement date" |
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 bdc935a46..0c59d31c4 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,7 +393,11 @@ 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, config_generation, enabled_at, disabled_at}`. `op` is a |
| + 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 |
| 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 18d8c8fda..75db24f0d 100644 |
| --- a/internal/console/apirelay.go |
| +++ b/internal/console/apirelay.go |
| @@ -171,6 +171,13 @@ type apiRelayRegion struct { |
| LeaseLive bool `json:"lease_live"` // relay.Lease still armed to serve (15-minute ceiling) |
| Listeners []string `json:"listeners"` |
| Kids []string `json:"kids"` |
| + // Retiring is kid -> RFC3339 retire_at for the masters this box loaded |
| + // that carry one. It answers the operator's mid-rotation question — |
| + // when may the old key leave /etc/carlos/relay-masters — from the CLI, |
| + // per box, which is the only place the answer is true: a region is only |
| + // done with a key once EVERY relay there has stopped loading it |
| + // (discussion #120). |
| + Retiring map[string]string `json:"retiring,omitempty"` |
| // Stale mirrors relayRotate's own gate: a heartbeat whose last successful |
| // authority read is older than relayHeartbeatFresh (five minutes) cannot |
| // be read as current, so a dead relay cannot look alive by silence. |
| @@ -461,6 +468,12 @@ func (s *Server) relayReply(ctx context.Context, acct *Account, app string, rec |
| MastersError: hb.MastersError, |
| AuthorityError: hb.AuthorityError, |
| } |
| + for kid, at := range hb.Retiring { |
| + if row.Retiring == nil { |
| + row.Retiring = map[string]string{} |
| + } |
| + row.Retiring[kid] = at.UTC().Format(time.RFC3339) |
| + } |
| if hb.ReadAt.IsZero() { |
| row.HeartbeatAge = "never" |
| } else { |
diff --git a/internal/console/relayops.go b/internal/console/relayops.go| index 3c26d297f..719e196df 100644 |
| --- a/internal/console/relayops.go |
| +++ b/internal/console/relayops.go |
| @@ -123,8 +123,15 @@ var ( |
| "this deployment runs no relay (CARLOS_RELAY_REGIONS is empty) — ask the operator to arm a region before enabling it for an app") |
| errRelayNoSealKey = errors.New( |
| "this console has no secrets key (CARLOS_SECRETS_PUBKEY) — it cannot deliver a relay key, so it will not issue one") |
| + // The commonest cause is not another console at all: it is THIS console |
| + // having just restarted, leaving its own previous process's lease to |
| + // expire. That takes up to relayLeaseTTL, so the message says to retry |
| + // and names the wait rather than leaving an operator to guess whether |
| + // the deployment is wedged (discussion #120). |
| errRelayNotHeld = errors.New( |
| - "this console does not hold the relay writer lease — another console is driving the relay; the operation was not started") |
| + "this console does not hold the relay writer lease. another console is driving the relay, " + |
| + "or this one restarted and its previous lease has not expired yet. the operation was not started, " + |
| + "so retry in up to a minute") |
| errRelayNotEnabled = errors.New("the relay is not enabled for this app") |
| errRelayUndelivered = errors.New("a route has no config delivery") |
| // errRelayUnknownHost is `carlos relay adopted --host` naming something |
diff --git a/internal/relay/authority.go b/internal/relay/authority.go| index f827bce06..beae93c24 100644 |
| --- a/internal/relay/authority.go |
| +++ b/internal/relay/authority.go |
| @@ -80,6 +80,11 @@ type Heartbeat struct { |
| ReadAt time.Time `json:"read_at"` |
| Listeners []string `json:"listeners"` |
| Kids []string `json:"kids"` |
| + // Retiring is kid -> retire_at for the loaded masters that carry one, |
| + // so `carlos relay status` can tell an operator mid-rotation when the |
| + // old key may leave the box's masters file (discussion #120). Additive: |
| + // a relay older than this field simply reports none. |
| + Retiring map[string]time.Time `json:"retiring,omitempty"` |
| // The two "why is this record stale" fields: the last masters-file |
| // reload error and the last authority read error, either of which |
| // explains a ReadAt that has stopped moving. |
diff --git a/internal/relay/masters.go b/internal/relay/masters.go| index c8c1f7431..78e19ae30 100644 |
| --- a/internal/relay/masters.go |
| +++ b/internal/relay/masters.go |
| @@ -67,6 +67,22 @@ func (m *Masters) Lookup(kid string, now time.Time) (Master, bool) { |
| return mk, true |
| } |
| +// Retiring is the kids that carry a retire_at, and when it falls. An |
| +// operator mid-rotation reads it through `carlos relay status` to learn when |
| +// the old key may be dropped from /etc/carlos/relay-masters — the one fact |
| +// the CLI could not answer before (discussion #120). Kids with no retirement |
| +// 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 { |
| + out := map[string]time.Time{} |
| + for k, mk := range m.byKid { |
| + if !mk.RetireAt.IsZero() { |
| + out[k] = mk.RetireAt |
| + } |
| + } |
| + return out |
| +} |
| + |
| func (m *Masters) Kids() []string { |
| out := make([]string, 0, len(m.byKid)) |
| for k := range m.byKid { |
diff --git a/internal/relay/masters_test.go b/internal/relay/masters_test.go| index 1213e2fe5..2de1d203b 100644 |
| --- a/internal/relay/masters_test.go |
| +++ b/internal/relay/masters_test.go |
| @@ -100,3 +100,26 @@ func TestWatchMastersReloadsOnChange(t *testing.T) { |
| t.Fatal("no reload within three poll ticks") |
| } |
| } |
| + |
| +// Retiring is what `carlos relay status` turns into "when may this key leave |
| +// the box" (discussion #120). A master with no retirement date must be absent |
| +// rather than present with a zero time, or the CLI prints a retirement for |
| +// every key a box holds. |
| +func TestMastersRetiring(t *testing.T) { |
| + m, err := ParseMasters(strings.NewReader( |
| + "k0 " + base64.StdEncoding.EncodeToString(testMaster()) + " -\n" + |
| + "k1 " + base64.StdEncoding.EncodeToString(testMaster()) + " 2026-09-09T12:00:00Z\n")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + r := m.Retiring() |
| + if len(r) != 1 { |
| + t.Fatalf("Retiring() = %v, want just k1", r) |
| + } |
| + if got, want := r["k1"].UTC().Format(time.RFC3339), "2026-09-09T12:00:00Z"; got != want { |
| + t.Errorf("k1 retire_at = %s, want %s", got, want) |
| + } |
| + if _, ok := r["k0"]; ok { |
| + t.Error("k0 has no retirement date and must not appear") |
| + } |
| +} |
diff --git a/internal/relay/serve.go b/internal/relay/serve.go| index f855f51a5..50dacfff8 100644 |
| --- a/internal/relay/serve.go |
| +++ b/internal/relay/serve.go |
| @@ -309,6 +309,7 @@ func Serve(ctx context.Context, cfg Config, logf func(string, ...any)) error { |
| ReadAt: lease.Last(), // the last SUCCESSFUL read, never the wall clock |
| Listeners: listeners, |
| Kids: kidsOf(masters), |
| + Retiring: retiringOf(masters), |
| MastersError: mastersE, |
| } |
| if err != nil { |
| @@ -437,6 +438,18 @@ func kidsOf(m *Masters) []string { |
| return m.Kids() |
| } |
| +// retiringOf is Masters.Retiring with the same nil guard kidsOf carries. |
| +func retiringOf(m *Masters) map[string]time.Time { |
| + if m == nil { |
| + return nil |
| + } |
| + r := m.Retiring() |
| + if len(r) == 0 { |
| + return nil |
| + } |
| + return r |
| +} |
| + |
| // readPSI reads the `some avg10` percentage out of a pressure-stall file |
| // (/proc/pressure/memory): |
| // |
diff --git a/internal/relay/serve_test.go b/internal/relay/serve_test.go| index 565b42328..b50b64ae1 100644 |
| --- a/internal/relay/serve_test.go |
| +++ b/internal/relay/serve_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "net" |
| "os" |
| "path/filepath" |
| + "strconv" |
| "strings" |
| "sync" |
| "testing" |
| @@ -425,3 +426,129 @@ func waitHeartbeat(t *testing.T, store release.Store, box string) Heartbeat { |
| time.Sleep(20 * time.Millisecond) |
| } |
| } |
| + |
| +// heartbeatUDPPort pulls the udp listener's port out of a heartbeat, which is |
| +// the only place a test outside Serve can learn the port it bound. |
| +func heartbeatUDPPort(t *testing.T, hb Heartbeat) int { |
| + t.Helper() |
| + for _, l := range hb.Listeners { |
| + if p, ok := strings.CutPrefix(l, "udp:"); ok { |
| + n, err := strconv.Atoi(p) |
| + if err != nil { |
| + t.Fatalf("udp listener %q has no port: %v", l, err) |
| + } |
| + return n |
| + } |
| + } |
| + t.Fatalf("heartbeat lists no udp listener: %v", hb.Listeners) |
| + return 0 |
| +} |
| + |
| +// TestServeEndsAllocationsOnDisable is the end-to-end disable path through |
| +// the real Serve loop, which spec §7 asks for and the branch never carried |
| +// (discussion #120): a pion client holding a live allocation, an app removed |
| +// from the authority record, and the relay acting on it within one authority |
| +// read rather than on a direct EndWhere call. |
| +// |
| +// The serial is what makes it a test and not a minute-long wait: the relay |
| +// polls control/serial every three seconds and re-reads the authority record |
| +// when it moves, which is the same path a console-mediated disable takes on |
| +// a real box. |
| +func TestServeEndsAllocationsOnDisable(t *testing.T) { |
| + store := newMemStore(t) |
| + dir := t.TempDir() |
| + mastersPath := filepath.Join(dir, "relay-masters") |
| + if err := os.WriteFile(mastersPath, []byte("k1 "+b64(testMaster())+" -\n"), 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + ctx := context.Background() |
| + putAuthority := func(apps []AuthorityApp) time.Time { |
| + t.Helper() |
| + writtenAt := time.Now().UTC() |
| + body, err := json.Marshal(Authority{Apps: apps, WrittenAt: writtenAt}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + _, etag, _ := store.Get(ctx, AuthorityKey) |
| + if _, err := store.Put(ctx, AuthorityKey, body, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := release.BumpSerial(ctx, store, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + return writtenAt |
| + } |
| + putAuthority([]AuthorityApp{{Account: "abc", App: "meet", Incarnation: "0123456789abcdef", Regions: []string{"eu-west-1"}}}) |
| + |
| + cfg := Config{ |
| + Box: "box-disable", Region: "eu-west-1", MastersPath: mastersPath, |
| + PublicIP: net.ParseIP("127.0.0.1"), Store: store, Limits: DefaultLimits(), |
| + PressurePath: filepath.Join(dir, "no-pressure-here"), |
| + udp: "127.0.0.1:0", |
| + tcp: "127.0.0.1:0", |
| + } |
| + var logMu sync.Mutex |
| + quiet := false |
| + logf := func(format string, args ...any) { |
| + logMu.Lock() |
| + defer logMu.Unlock() |
| + if !quiet { |
| + t.Logf(format, args...) |
| + } |
| + } |
| + t.Cleanup(func() { logMu.Lock(); quiet = true; logMu.Unlock() }) |
| + |
| + runCtx, cancel := context.WithCancel(ctx) |
| + defer cancel() |
| + done := make(chan error, 1) |
| + go func() { done <- Serve(runCtx, cfg, logf) }() |
| + |
| + port := heartbeatUDPPort(t, waitHeartbeat(t, store, "box-disable")) |
| + username, password := creds(time.Now(), "abc", "meet", "0123456789abcdef") |
| + client := turnClient(t, &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: port}, username, password) |
| + relayConn, err := client.Allocate() |
| + if err != nil { |
| + t.Fatalf("allocate against a relay that has read an enabling record: %v", err) |
| + } |
| + defer relayConn.Close() |
| + |
| + // 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) |
| + |
| + // Wait on the relay's OWN report that it is holding the new record — |
| + // the heartbeat carries the WrittenAt of the authority copy it serves — |
| + // rather than on an allocation failing. Retrying Allocate until any |
| + // error appeared was the first shape of this test and it was not a test |
| + // at all: a refusal for an unrelated reason (a per-app allocation cap, |
| + // say) reads exactly like the disable, so the assertion passed with the |
| + // app still enabled. |
| + deadline := time.Now().Add(20 * time.Second) |
| + for { |
| + hb := waitHeartbeat(t, store, "box-disable") |
| + if !hb.WrittenAt.Before(disabledAt) { |
| + break |
| + } |
| + if time.Now().After(deadline) { |
| + t.Fatalf("the relay never picked up the record written at %s (last heartbeat holds %s)", disabledAt, hb.WrittenAt) |
| + } |
| + 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`. |
| + 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") |
| + } |
| + |
| + cancel() |
| + select { |
| + case err := <-done: |
| + if err != nil { |
| + t.Fatalf("Serve returned %v, want nil on a cancelled context", err) |
| + } |
| + case <-time.After(10 * time.Second): |
| + t.Fatal("Serve did not return after its context was cancelled") |
| + } |
| +} |
diff --git a/internal/relay/server_test.go b/internal/relay/server_test.go| index 043d53bee..a1e921b16 100644 |
| --- a/internal/relay/server_test.go |
| +++ b/internal/relay/server_test.go |
| @@ -657,3 +657,37 @@ func TestServerCloseKeepsReplacementBinding(t *testing.T) { |
| t.Fatal("the closed socket is still owned, so its admission never came back") |
| } |
| } |
| + |
| +// TestServerRefusesPermissionIntoConfiguredDenyCIDR covers the operator's |
| +// half of the peer policy, which the built-in private ranges do not reach: |
| +// CARLOS_RELAY_DENY_CIDRS carries the box's own VPC CIDR, and that CIDR is |
| +// ordinary public-looking space as far as the built-in list is concerned. |
| +// It is the one thing standing between a TURN allocation and the deployment's |
| +// private network, and until now only the built-in ranges had a test |
| +// (discussion #120). |
| +func TestServerRefusesPermissionIntoConfiguredDenyCIDR(t *testing.T) { |
| + // 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. |
| + _, vpc, err := net.ParseCIDR("198.51.100.0/24") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + s, _, now, _, _ := newTestServer(t, "127.0.0.1", nil, func(cfg *ServerConfig) { |
| + cfg.DenyCIDRs = append(cfg.DenyCIDRs, vpc) |
| + }) |
| + username, password := creds(now(), "abc", "meet", "0123456789abcdef") |
| + client := turnClient(t, s.UDPAddr(), username, password) |
| + if _, err := client.Allocate(); err != nil { |
| + t.Fatalf("allocate: %v", err) |
| + } |
| + if err := client.CreatePermission(&net.UDPAddr{IP: net.ParseIP("198.51.100.9"), Port: 1}); err == nil { |
| + t.Error("permission into the configured deny CIDR was granted — a TURN client could reach the VPC") |
| + } |
| + // The control: an address outside both the built-in list and the |
| + // configured range is still permitted, so the test is not passing |
| + // because permissions are broken outright. |
| + if err := client.CreatePermission(&net.UDPAddr{IP: net.ParseIP("203.0.113.9"), Port: 1}); err != nil { |
| + t.Errorf("permission to an ordinary public peer was refused: %v", err) |
| + } |
| +} |
diff --git a/relay.go b/relay.go| index f678ea8ca..442de8d6c 100644 |
| --- a/relay.go |
| +++ b/relay.go |
| @@ -33,6 +33,7 @@ import ( |
| "log" |
| "os" |
| "os/signal" |
| + "sort" |
| "strings" |
| "syscall" |
| @@ -402,6 +403,23 @@ func printRelayStatus(w io.Writer, res apiRelayReply) { |
| if len(rg.Kids) > 0 { |
| line += ", masters " + strings.Join(rg.Kids, ",") |
| } |
| + // A retiring master is the operator's cue, not the member's: it is |
| + // the only thing that says when the old key may leave this box's |
| + // /etc/carlos/relay-masters. Printed per box, because the region is |
| + // done with a key only once every relay there has stopped loading |
| + // it (discussion #120). |
| + if len(rg.Retiring) > 0 { |
| + kids := make([]string, 0, len(rg.Retiring)) |
| + for kid := range rg.Retiring { |
| + kids = append(kids, kid) |
| + } |
| + sort.Strings(kids) |
| + parts := make([]string, 0, len(kids)) |
| + for _, kid := range kids { |
| + parts = append(parts, kid+" after "+rg.Retiring[kid]) |
| + } |
| + line += ", retiring " + strings.Join(parts, ", ") |
| + } |
| switch { |
| case rg.Stale: |
| // STALE is the important word and it gets the warning colour: a |
diff --git a/relay_test.go b/relay_test.go| index 8fc848383..e06069306 100644 |
| --- a/relay_test.go |
| +++ b/relay_test.go |
| @@ -170,6 +170,21 @@ func TestPrintRelayStatus(t *testing.T) { |
| // an old record — the two words a member acts on. |
| wantNot: []string{"STALE", "older record", "disable pending"}, |
| }, |
| + { |
| + // The operator's mid-rotation question, and the only place the |
| + // CLI can answer it: when may the old key leave this box's |
| + // /etc/carlos/relay-masters (discussion #120). |
| + name: "a retiring master names when it may be dropped", |
| + res: healthy, |
| + modifier: func(r *apiRelayReply) { |
| + 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"}, |
| + }} |
| + }, |
| + want: []string{"masters k1,k2", "retiring k1 after 2026-09-09T12:00:00Z"}, |
| + }, |
| { |
| name: "stale names the relay's own reason", |
| res: healthy, |