carlos / platform Public

Clone
git clone https://amadan.net/carlos/platform

Plain git — no account needed to clone.

Download

Download this file

1# Findings on the relay spec, revision 1
2
3Reviewed by **GPT-6 (Codex), 2026-09-07**, independently of the draft author, from the brief in `CODEX-REVIEW-BRIEF-RELAY.md`. Unedited apart from this note. Revision 2 of the spec records how each finding was taken.
4
5Reviewed commit `3d453c6b1f50b9c1fe8dc0a835e688e913e8c548`. Spec line references below refer to the [relay spec at that revision](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md). I independently reproduced all three cryptographic values in the test vector; they match. Pion findings use v4.1.4’s published API and source. No files were changed; no live TURN test was run.
6
7## R1-1 — P1 — The advertised JSON cannot pass through sealed config
8
9**Section:** §2, lines 44–57; §5, lines 241–246.
10
11`CARLOS_RELAY` contains JSON double quotes. The existing renderer rejects quotes in every value, including decrypted secrets. Writing it as `kind:"secret"` therefore does not solve delivery: the envelope can be stored, but rendering the app’s configuration fails. This blocks the contract Meet is supposed to implement.
12
13**Evidence:** [`ValidVarValue`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/materialize.go:23) rejects quotes; [`RenderEnv`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/materialize.go:49) decrypts secrets and applies that same check.
14
15**Correction:** Either specify a quote-free wire encoding and its app-side decoding, or explicitly extend config delivery to support JSON safely across both systemd and exec consumers. Test the complete seal → store → render → child-environment path before freezing §2.
16
17## R1-2 — P1 — `AuthHandler` cannot try the previous master
18
19**Section:** §4, verification step 4; §2’s rotation guarantee.
20
21The handler receives no password or request integrity value and returns exactly one key. Computing the current credential and returning its key ends the callback. Pion does not call it again with the previous master after integrity verification fails. Consequently, putting the new master first immediately rejects credentials derived from the old one.
22
23**Evidence:** Pion’s [`AuthHandler` signature](https://pkg.go.dev/github.com/pion/turn/v4#AuthHandler) returns one `[]byte`; [`authenticateRequest`](https://raw.githubusercontent.com/pion/turn/v4.1.4/internal/server/util.go) calls it once and performs one integrity check.
24
25**Correction:** Specify an implementable key-selection mechanism before declaring the username format fixed. Options include an authenticated key identifier in the credential contract or an explicit request-verification adapter supporting multiple candidate keys. Add interoperability tests exercising old and new credentials concurrently.
26
27## R1-3 — P1 — Authentication is not bound to the allocation’s owner
28
29**Section:** §3 authentication and metering; §4 verification.
30
31Pion v4.1.4 authenticates subsequent requests but locates their allocation by connection tuple without comparing the authenticated username to its original owner. A client can allocate using app A’s credential and then refresh or modify that allocation using app B’s valid credential on the same tuple. The proposed handler accepts B because B is enabled; nothing in the spec connects that decision to A’s allocation.
32
33This breaks the cross-app credential boundary and can separate the identity authorizing traffic from the identity being metered.
34
35**Evidence:** [`handleRefreshRequest`, `handleCreatePermissionRequest`, and `handleChannelBindRequest`](https://raw.githubusercontent.com/pion/turn/v4.1.4/internal/server/turn.go) use tuple-based lookup without an owner comparison. [RFC 8656 §21.3.3](https://www.rfc-editor.org/rfc/rfc8656.html#section-21.3.3) requires subsequent credentials to match the allocation’s original credentials.
36
37**Correction:** Bind each allocation immutably to its authenticated identity and enforce that binding on subsequent requests. Name the patched library or adapter that supplies this check. Test credential substitution on an existing tuple, rather than only changing fields in a new Allocate request.
38
39## R1-4 — P1 — Refusing refresh does not terminate expired traffic
40
41**Section:** §2, lines 75–81; §3 lifetime limits; §4 expiry verification.
42
43Rejecting an expired Refresh request does not delete the allocation. Existing ChannelData and Send indications bypass `AuthHandler`, so traffic can continue until existing allocation, permission, or channel state expires. The spec’s claim that the next refresh ends the relay is therefore false.
44
45The ten-minute allocation lifetime is also a default, not the stated cap: Pion accepts requested lifetimes below one hour, including values well above ten minutes.
46
47**Evidence:** Pion’s [request handlers](https://raw.githubusercontent.com/pion/turn/v4.1.4/internal/server/turn.go) return on authentication failure without deleting the allocation. Its [`allocationLifeTime`](https://raw.githubusercontent.com/pion/turn/v4.1.4/internal/server/util.go) accepts client-requested durations below one hour. The [roadmap’s stage 4a policy](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:650) requires established allocations to end at credential expiry.
48
49**Correction:** Give allocations an absolute credential deadline and an active termination mechanism independent of client refreshes. Specify how the implementation closes individual allocations for expiry, disable, region removal, and lease expiry. Enforce the ten-minute maximum explicitly. Test continued traffic after the deadline with no further authenticated requests.
50
51## R1-5 — P1 — The serving lease has no renewal mechanism
52
53**Section:** §3 authority record; §5 records.
54
55The console rewrites the authority record only on enable and disable. Relays fetch it only at startup or after a serial change. After fifteen quiet minutes, every relay must terminate its allocations and refuse new ones—even when the deployment is healthy.
56
57Measuring age since receipt does not fix the missing refresh path. Nor may an unrelated serial bump renew authority merely by causing an old record to be reread.
58
59**Evidence:** Spec lines 169–175 and 226–230 define those triggers. [`BumpSerial`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/release/serial.go:46) is explicitly a latency hint whose failure is tolerated, not a lease renewal.
60
61**Correction:** Define periodic authority revalidation independently of serial changes, the exact freshness clock, and what successful observation renews the lease. Failed reads must not renew it. Test both a healthy deployment with no writes for more than fifteen minutes and an outage that continues past the lease deadline.
62
63## R1-6 — P1 — Concurrent rebuilds can restore revoked authority
64
65**Section:** §5 records, enable, disable, and deletion.
66
67CAS protection on each app’s `relay.json` does not serialize replacement of the shared `control/relay.json`. An enable for B can read a snapshot containing A, pause, and overwrite the aggregate after A’s disable has removed A. The replacement can carry a fresh `written_at`, restoring A’s authority.
68
69The multi-object operation also has no recovery protocol. Failure after changing the app record but before publishing the aggregate leaves contradictory state indefinitely.
70
71**Evidence:** Spec lines 221–230 give CAS semantics only to the per-app record. Lines 240–259 describe sequential writes without aggregate fencing or a reconciliation loop. The existing [`appconfig.Apply`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/generation.go:113) guarantees one config-manifest write, not a transaction across these objects.
72
73**Correction:** Specify a fenced aggregate publisher that rebuilds after conflicts, plus durable reconciliation of incomplete operations. Include deletion and rotation in the same concurrency model. Test overlapping enable/disable operations, stale publishers, and failure after every durable write.
74
75## R1-7 — P1 — Console master custody contradicts the claimed exposure boundary
76
77**Section:** §4 region master; §5 rotation.
78
79The console holds every region’s masters as “sealed console secrets,” while the same paragraph says no master exists in the bucket, sealed or otherwise. If these use the existing sealed-config mechanism, both statements cannot be true: a compromised host can decrypt the console bundle and obtain every region’s master.
80
81That exposure is stronger than the roadmap’s conceded access to enabled apps’ derived keys. Masters also derive keys for future incarnations and future enables.
82
83**Evidence:** Spec lines 179–187. The [roadmap §1](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:52) explicitly describes deployment-wide decryption; [stage 4a’s custody statement](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:641) distinguishes regional masters from derived-key exposure.
84
85**Correction:** Name the console’s actual secret store, encryption authority, and readers. To preserve the stated boundary, its master copies must not use the deployment-wide sealed-config key. Keep the already-accepted derived-key exposure explicit.
86
87## R1-8 — P1 — The peer policy blocks two clients using the same relay
88
89**Section:** §3 peer policy; §7 flagship gate.
90
91Two clients forced to use TURN can receive relay candidates on the same region EIP at different allocation ports. Each then needs permission to reach the other’s candidate. The unconditional refusal of the relay’s own addresses rejects both.
92
93This affects the promised fallback when both clients cannot send UDP directly and connect to the relay over TCP or TLS. Falling through among URLs on the same server does not resolve it.
94
95**Evidence:** Spec lines 129–151 assign one public relay address and reject it as a peer. Pion’s [`PermissionHandler`](https://raw.githubusercontent.com/pion/turn/v4.1.3/server_config.go) receives a peer IP without its port, so that hook alone cannot distinguish a live relay allocation from another service on the box.
96
97**Correction:** Specify a safe relay-to-relay mechanism that preserves the prohibition on reaching arbitrary host services. An allocation-aware internal forwarding path needs an explicit design; an IP-only exception is insufficient. Add a test with both browsers forced to relay through the same region.
98
99## R1-9 — P2 — The operational limits do not bound bandwidth or accounts
100
101**Section:** §3 limits; §9 decisions 2 and 3.
102
103The 20 GB threshold refuses only new allocations. Existing allocations can keep transferring and refreshing after crossing it. One enabled app can therefore consume sustained bandwidth beyond the advertised budget.
104
105There is also no account limit. One account controlling 32 apps can fill all 2,048 allocation slots while every app remains within its individual cap.
106
107**Evidence:** Spec lines 153–160. The [roadmap’s policy](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:650) requires allocation, rate, bandwidth, and lifetime bounds per app, account, and box.
108
109**Correction:** Define ongoing traffic enforcement and aggregate account/box limits. Specify atomic admission across all listeners, failed-allocation accounting, and restart behavior for counters. Test an established allocation crossing the byte limit and one account spreading allocations across multiple apps.
110
111## R1-10 — P2 — Rotation drops calls and has no durable retirement deadline
112
113**Section:** §2 rotation promises; §5 rotation procedure.
114
115Restarting `carlos-relay.service` destroys its in-memory allocations and sockets. Even after fixing R1-2, step 1 drops calls rather than preserving them.
116
117The master list also contains no activation or retirement timestamp. “Previous for twenty-four hours” cannot survive process restarts reliably without additional state. Finally, an old derived key can mint a fresh four-hour credential late in the grace period; removing its master at hour 24 does not imply every such credential has expired.
118
119**Evidence:** Spec lines 87–91, 181, and 263–271. Pion’s [server lifecycle](https://raw.githubusercontent.com/pion/turn/v4.1.4/server.go) keeps allocation state inside the running server.
120
121**Correction:** Specify master reload without process replacement, durable key epochs and retirement deadlines, and acknowledgements for relay installation and app key adoption. Define how minting near retirement is bounded. Test uninterrupted traffic during rotation, delayed app delivery, partial rollout, and relay restart during grace.
122
123## R1-11 — P2 — The minting contract does not supply its identity inputs
124
125**Section:** §2 environment and minting.
126
127The app receives the regional keys and incarnation but must independently know the canonical account sqid and CARLOS app name. §2 does not specify where either comes from. An app’s own team identifier or application name is not necessarily its deployment identity.
128
129The encoding instructions should also explicitly require decoding the supplied base64 key into 32 raw bytes before HMAC.
130
131**Evidence:** Spec lines 44–73. The existing [agent-owned config variables](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/agentvars.go:19) do not supply these identities; the [exec backend’s injected variables](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/activator/backend_exec.go:660) do not supply them either.
132
133**Correction:** Deliver the canonical account/app identity, or an exact username suffix, as part of the contract. State the base64 alphabet, padding, key decoding, and username byte encoding explicitly. Supply app-side pseudocode using only documented inputs.
134
135## R1-12 — P2 — Region vocabulary and per-instance ordering are unresolved
136
137**Section:** §2 regional array; §5 enable; §6 console configuration.
138
139The example uses AWS region names, while CARLOS placement uses values such as `ie`. The spec requires the instance’s own region first but writes one ordered array into an app-wide config bundle. Two instances of one app in different regions cannot both receive their own region first through that operation alone.
140
141The master-variable template is also incomplete: literal substitution of `eu-west-1` produces a name containing hyphens, which the config API rejects.
142
143**Evidence:** [`edgesFile`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/edgesfile.go:72) explicitly separates AWS region from CARLOS placement. [`Apply`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/generation.go:113) writes the app’s default bundle. [`ValidVarName`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/appconfig.go:103) excludes hyphens.
144
145**Correction:** Choose one relay-region vocabulary, define its placement mapping and environment-variable normalization, and specify where per-instance ordering occurs. Test one app placed in two regions and the case where its local region is outside the allowed relay list.
146
147## R1-13 — P2 — Repeated enable contradicts the incarnation contract
148
149**Section:** §2 incarnation; §5 enable.
150
151§2 says incarnation changes only after disable followed by enable, and that region changes preserve it. Enable step 4 instead writes a fresh incarnation unconditionally. Retrying enable after a lost response can therefore invalidate live credentials. Using enable to change regions has the same problem.
152
153**Evidence:** Spec lines 83–85 versus 221–224 and 238–240.
154
155**Correction:** Define explicit state transitions: first enable, repeated enable, region update, repeated disable, and re-enable. Repeated enable must preserve the existing incarnation. Specify how removing a region terminates allocations there without rotating credentials in retained regions.
156
157## R1-14 — P2 — Status cannot establish delivery or current service health
158
159**Section:** §2 config restarts; §5 enable response and status.
160
161A successful config write does not prove that an instance received or loaded it. The proposed response nevertheless reports delivery and a restart. Some supported TCP-address routes receive files without ever being restarted by CARLOS; others are not eligible for delivery.
162
163The relay acknowledgement is also insufficient as specified. A dead relay’s last acknowledged `written_at` can equal the current record indefinitely. That establishes which record it once held, not whether it is alive, within lease, or listening on the advertised transports.
164
165**Evidence:** [`SyncExec`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/syncexec.go:11) documents the route exceptions. [`store.go`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/store.go:24) explicitly distinguishes recorded writes from runtime delivery.
166
167**Correction:** Report desired state, publication state, config generation, observed delivery, fresh relay heartbeat, lease state, and listener readiness separately. Define acknowledgement expiry and supported route shapes. The member command must expose incomplete enable and disable operations without requiring box access.
168
169## R1-15 — P2 — Allowing the hostname does not cause certificate issuance
170
171**Section:** §3 certificate for 5349.
172
173Adding a relay hostname to the edge’s allowed set does not initiate an ACME order. TURN clients connect to 5349, where no listener starts until a certificate exists, so they cannot trigger the edge’s 443 certificate path.
174
175The existing proactive certificate warmer does not automatically solve this: it enumerates routed/advertised hosts and excludes hosts under the apps domain. Issuance also belongs to the elected deployment issuer, which may be another box.
176
177**Evidence:** [`warmSet`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/edge/certwarm.go:351) and [`issuerHostPolicy`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/edge/certsync.go:191).
178
179**Correction:** Register relay names with the deployment-wide issuance and challenge machinery, explicitly trigger issuance and renewal, and define certificate propagation. Specify how 5349 starts when its first certificate arrives. Test from an empty cache with a different box holding the issuer role.
180
181## R1-16 — P2 — Installing the unit does not activate or update its process
182
183**Section:** §3 process.
184
185Adding a service to `enabledUnits` enables it for boot but does not start it. Bootstrap deliberately starts only timers and paths. Platform self-update uses the same arming plan and explicitly restarts only the edge.
186
187The relay can therefore remain inactive after installation or continue running the old executable after a platform update. Sharing the binary does not provide process adoption.
188
189**Evidence:** [`systemctlPlan`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/bootstrap.go:116), [`systemApplier.arm`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/systemapply.go:661), and [`restartEdge`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/systemapply.go:568).
190
191**Correction:** Specify initial activation, conditional enable/disable convergence, required directory creation, and update/rollback handling for the relay process. Include the staging/bootstrap-plan path, which cannot assume it contains the live box’s `host.env`. Test installation and update without rebooting.
192
193## R1-17 — P2 — The metrics integration lacks a compatible schema and single writer
194
195**Section:** §3 metering.
196
197Existing metrics samples have no per-app relay fields. More seriously, the relay is a separate process, while the host metrics writer retains its own hourly sample buffer. Independently appending relay samples to the same objects is unsafe: subsequent host flushes can replace them using the host’s buffered view.
198
199**Evidence:** [`metrics.Sample`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/metrics/sample.go:29) contains host and unit samples only. [`writeHourChunk`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/metrics_loop.go:338) merges existing data only on the writer’s first write for that hour.
200
201**Correction:** Define an additive relay sample schema and one writer per object. Either let the host sampler collect relay counters over a local interface or give the relay separate chunk keys. Specify authenticated allocation attribution, counter reset semantics, and updates to raw readers and rollups.
202
203## R1-18 — P2 — The inter-region meter and acceptance requirement were dropped
204
205**Section:** §3 metering; §6 egress cost; §7 testing; §8 exclusions.
206
207`relay_allocs` does not replace the roadmap’s third meter: inter-region bytes. The proposed samples cannot distinguish those bytes from other traffic. The flagship gate also reduces the roadmap’s account-usage requirement to unspecified meters moving, while deferring the usage view.
208
209**Evidence:** The [roadmap’s metering contract](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:662) requires received, sent, and inter-region bytes. Its [stage 4a gate](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:931) requires those meters on the account’s usage line.
210
211**Correction:** Restore the inter-region meter, define how traffic is classified and counted without double charging, and retain allocation count as an additional metric. Align the acceptance gate and stage boundary with the parent rather than silently weakening them.
212
213## R1-19 — P2 — The unit does not establish that the relay loses first
214
215**Section:** §3 hardening.
216
217The edge already permits 80% of physical memory; the proposed relay permits another 25%, before console and other services. Those independent ceilings allow host-wide memory pressure before either service reaches its own ceiling. `Nice` and `CPUWeight` do not determine which process loses under memory pressure.
218
219**Evidence:** The [edge unit](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/units/carlos-edge.service:57) explains its existing memory allocation. Systemd documents [memory ceilings and CPU weights](https://raw.githubusercontent.com/systemd/systemd/main/man/systemd.resource-control.xml) separately from [OOM victim adjustment](https://raw.githubusercontent.com/systemd/systemd/main/man/systemd.exec.xml).
220
221**Correction:** Budget memory across the shared box and specify the intended memory-pressure priority explicitly. Validate that priority under combined edge, tenant, and relay load on a `t4g.small`; the listed settings alone do not establish it.
222
223## R1-20 — P2 — Remaining factual claims and references need correction
224
225**Sections:** §2, §3, §4, and §9.
226
227The following defects are separate from the mechanism failures above:
228
229| Claim | Evidence | Correction |
230|---|---|---|
231| The vector is already checked into `internal/relay/vector_test.go` (§2:103). | `internal/relay` does not exist at the reviewed commit. | State that the test will be added. Preserve the verified numerical vector. |
232| `agent.go:1761` is the public-IP lookup (§3:134). | The lookup is [`readPublicIP`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/agent.go:1808); line 1761 is blank. | Cite the function and its IMDS leaf read. |
233| `agent.go:1304` is the fast serial poll (§3:172). | That location is restart-failure bookkeeping. [`fastConvergeLoop`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/agent.go:1418) documents the actual loop. | Correct the citation and specify the relay’s separate polling or IPC integration. |
234| RFC 8489 mandates MD5 with no alternative (§4:214–215). | [RFC 8489 §18.5.1](https://www.rfc-editor.org/rfc/rfc8489.html#section-18.5.1) defines both MD5 and SHA-256; MD5 is the legacy compatibility algorithm. | Attribute MD5 to the selected Pion compatibility mechanism, not an unconditional RFC mandate. |
235| Pion sends refreshes, and longer calls must restart ICE “before the hour is up” (§2:77–80). | The app contract hands credentials to a browser; that client controls refreshes. The same section specifies four hours. | Describe client-driven refreshes and use the actual credential deadline. Remove the unsupported promise that four hours covers every meeting. |
236
237**Verdict: request changes.** The app contract cannot be delivered through the current config path, and the proposed mechanisms do not yet establish allocation ownership, timely expiry, reliable revocation, or the claimed master-rotation behavior.