| 1 | # Findings on the relay spec, revision 2 |
| 2 | |
| 3 | Reviewed by **GPT-6 (Codex), 2026-09-07**, second pass, against revision 2 at `014339b`. Unedited apart from this note. Revision 3 records how each finding was taken. |
| 4 | |
| 5 | | Revision-1 finding | Disposition | Reason | |
| 6 | |---|---|---| |
| 7 | | R1-1 — JSON delivery | Closed | Standard padded base64 passes the existing config renderer. | |
| 8 | | R1-2 — Master selection | Closed | `kid` selects one master before Pion’s single integrity check. | |
| 9 | | R1-3 — Allocation ownership | Partial | Binding is specified, but its transport key remains ambiguous across UDP and TCP. | |
| 10 | | R1-4 — Active termination | Closed | Wrapped relay sockets support the proposed expiry and revocation mechanism. | |
| 11 | | R1-5 — Lease renewal | Closed | Independent sixty-second reads renew the lease; failed reads do not. | |
| 12 | | R1-6 — Aggregate concurrency | Partial | Aggregate CAS helps, but ordering and incomplete-operation recovery remain defective. | |
| 13 | | R1-7 — Master custody | Partial | The wider exposure is now explicit, but its departure from the parent remains a proposed decision. | |
| 14 | | R1-8 — Same-relay peers | Partial | Allowing the EIP removes the blanket refusal; the hairpin security argument is insufficient. | |
| 15 | | R1-9 — Operational limits | Partial | Account allocation limits and active byte-budget termination are added; aggregate bandwidth and rate bounds remain missing. | |
| 16 | | R1-10 — Rotation | Partial | Hot reload and durable retirement are specified; late minting still breaks the lifetime guarantee. | |
| 17 | | R1-11 — Minting inputs | Closed | `username_suffix`, raw-key decoding and byte encoding supply the required inputs. | |
| 18 | | R1-12 — Regions and ordering | Partial | Vocabulary and app-wide ordering are chosen; placement-to-AWS mapping is unspecified. | |
| 19 | | R1-13 — Repeated enable | Partial | Normal transitions preserve incarnation, but retries cannot repair interrupted delivery. | |
| 20 | | R1-14 — Status and delivery | Partial | Heartbeat freshness improves status; config adoption has no defined observation mechanism. | |
| 21 | | R1-15 — TLS certificates | Partial | Removing TLS avoids the issuance deadlock but leaves an unresolved parent-scope change. | |
| 22 | | R1-16 — Process activation | Closed | Initial start and platform-update restart are now explicit implementation requirements. | |
| 23 | | R1-17 — Metrics integration | Partial | Separate objects establish one writer, but measurement points and counter semantics remain incomplete. | |
| 24 | | R1-18 — Inter-region usage | Open | The required meter and account-usage gate are explicitly deferred without an approved parent amendment. | |
| 25 | | R1-19 — Memory priority | Partial | The ceiling is smaller and OOM bias explicit; neither establishes the claimed kill order. | |
| 26 | | R1-20 — Factual claims | Partial | The substantive corrections are made; a stale code citation and nonexistent section reference remain. | |
| 27 | |
| 28 | Reviewed commit `014339b1fd5295656595d0dddf0d370699a3976e`. Section and line references below refer to the [revision-2 spec][spec]. I independently reproduced all three cryptographic vector values. No files were changed and no live TURN or EC2 test was run. |
| 29 | |
| 30 | The requested Pion checks pass: [`RelayAddressGenerator` returns `net.PacketConn`][pion-generator], permitting a wrapper; a relay-socket [read error calls `DeleteAllocation`][pion-reader]. [`OnAllocationCreated` supplies the relay address before the reader starts][pion-created], so the implementation can associate a registered wrapper with its allocation at that callback. |
| 31 | |
| 32 | ## R2-1 — P1 — Aggregate CAS does not yet fence stale publication |
| 33 | |
| 34 | **Section:** §5, lines 319–328. |
| 35 | |
| 36 | The spec requires a listing and a conditional aggregate write, but does not require reading the aggregate’s ETag **before** starting that listing. This ordering is necessary: |
| 37 | |
| 38 | 1. Builder B reads A’s enabled record. |
| 39 | 2. Disable A publishes an aggregate without A. |
| 40 | 3. B reads that aggregate’s new ETag. |
| 41 | 4. B conditionally publishes its earlier listing, restoring A. |
| 42 | |
| 43 | Every operation follows the stated text, and the final CAS succeeds. |
| 44 | |
| 45 | Even with the correct ordering, “the aggregate can never carry an app whose record says disabled” is false. A crash after the per-app disable leaves the old aggregate until reconciliation, potentially an hour later. Successful sixty-second reads keep renewing that stale authority; the fifteen-minute lease does not bound this failure. |
| 46 | |
| 47 | **Correction:** Specify ETag read → complete listing → conditional publication, restarting the entire sequence on conflict. Define when revocation is complete, how pending revocation is exposed, and a recovery bound consistent with the serving guarantee. |
| 48 | |
| 49 | ## R2-2 — P2 — An interrupted enable can permanently omit credentials |
| 50 | |
| 51 | **Section:** §5, transition table and enable steps 4–6. |
| 52 | |
| 53 | Enable first records `enabled`, then writes config. If it stops between those writes, retrying the same enable takes the transition that expressly performs “no config write.” The reconciler repairs only the aggregate. The app can therefore become authorized indefinitely without ever receiving `CARLOS_RELAY`. |
| 54 | |
| 55 | Separate object CAS also does not fence stale config writers. An enable can pause before `appconfig.Apply`, a disable can finish removing credentials, and the old enable can subsequently restore them. Re-enable and rotation introduce equivalent ordering problems. |
| 56 | |
| 57 | **Evidence:** The specified sequence is at lines 335 and 353–360; [`appconfig.Apply`][apply] conditionally changes one bundle, not the relay record and bundle together. |
| 58 | |
| 59 | **Correction:** Record a durable operation version and incomplete phase. Reconcile both config and aggregate toward that version, prevent superseded operations from publishing config, and make repeated enable resume unfinished delivery. |
| 60 | |
| 61 | ## R2-3 — P2 — The owner-binding key can collide across transports |
| 62 | |
| 63 | **Section:** §3 owner binding; §4 verification step 3. |
| 64 | |
| 65 | The spec moves between “five-tuple” and “source address” without defining their mapping. A global map built from Pion’s event protocol and address strings is unsafe: Pion uses separate allocation managers for UDP and TCP listeners, but constructs the allocation tuple with `Protocol: allocation.UDP` even for TCP clients. UDP and TCP connections can have identical textual source and destination endpoints. |
| 66 | |
| 67 | The callbacks therefore permit two independent allocations to collide in the proposed owner map, causing false refusals, overwritten ownership or deletion of another allocation’s binding. |
| 68 | |
| 69 | **Evidence:** Pion’s [listener-specific managers][pion-server] and [`handleAllocateRequest` tuple construction][pion-turn]. |
| 70 | |
| 71 | **Correction:** Define the binding key using the actual client transport and typed source address, with listener scope where necessary. Do not use the event’s protocol string as the client transport. Define the bound identity fields explicitly and require deletion to remove only the binding belonging to that allocation. This is buildable with the specified listeners; it does not require abandoning the adapter. |
| 72 | |
| 73 | ## R2-4 — P2 — Retirement still contradicts the credential-lifetime guarantee |
| 74 | |
| 75 | **Section:** §2, lines 106–108; §4 master retirement; §5 rotation. |
| 76 | |
| 77 | Twenty-eight hours of overlap does not guarantee four hours of remaining validity for every old-key credential. An instance retaining the old config can mint at hour 27 with expiry hour 31. The master retires at hour 28, so subsequent authenticated requests fail before the promised expiry. |
| 78 | |
| 79 | Retirement also is not among §3’s active termination deadlines. Thus retirement can reject refreshes while existing traffic continues on that allocation. The spec must choose the intended behavior. |
| 80 | |
| 81 | The rollout has a second gap: heartbeat records identify authority and listeners, but not loaded masters. Updating a file is not an acknowledgement that every relay successfully loaded the new `kid` before the console distributes it. |
| 82 | |
| 83 | **Correction:** Define an issuer cutoff and maximum credential deadline, or begin the final lifetime allowance only after acknowledged issuer migration. Specify forced-retirement behavior and narrow the uninterrupted-lifetime guarantee accordingly. Include loaded `kid` and reload failure state in relay status before switching issuance. |
| 84 | |
| 85 | ## R2-5 — P2 — Socket counters do not measure the promised traffic |
| 86 | |
| 87 | **Section:** §3 metering, lines 221–233; §6 egress, lines 416–418. |
| 88 | |
| 89 | Relay-socket `WriteTo` measures client-to-peer payload. Relay-socket `ReadFrom` measures peer ingress **before** permission checks. Peer-to-client traffic leaves through a different socket, `TurnSocket`. |
| 90 | |
| 91 | Consequently: |
| 92 | |
| 93 | - The proposed `out` measurement misses return-leg egress. |
| 94 | - Counting raw relay reads against an app’s budget lets unsolicited packets consume that budget and terminate its allocations, although Pion never forwards those packets. |
| 95 | - The hourly object schema does not say whether `in`, `out` and `allocs` are minute deltas, hourly totals or process totals. It also does not specify how a restart preserves earlier measurements in the same hourly object. |
| 96 | |
| 97 | **Evidence:** Pion’s [relay read, permission checks and client writes][pion-reader]; the existing host writer’s [first-write merge behavior][metrics-writer] illustrates the restart issue that a separate object key alone does not solve. |
| 98 | |
| 99 | **Correction:** Define directions relative to the host, payload versus wire accounting, and the treatment of rejected ingress. Measure successful forwarding on both outbound legs. Specify counter intervals, allocation-count semantics, restart recovery and retry deduplication before assigning the schema to rollups. |
| 100 | |
| 101 | ## R2-6 — P2 — Config adoption cannot be inferred from the named API |
| 102 | |
| 103 | **Section:** §2 config delivery; §5 enable response, status and rotation. |
| 104 | |
| 105 | The spec asks `appconfig.Apply` for a resulting config generation and asks the instances API to establish that an instance restarted after adopting it. These are not the existing contracts: |
| 106 | |
| 107 | - [`Apply`][apply] returns an unset count. |
| 108 | - [`Generation`][generation] is a timestamp and nonce, not a numeric generation `N`. |
| 109 | - The [instances response][instances] contains release version, health, upstreams and serving boxes, without an applied-config identity or restart acknowledgement. |
| 110 | |
| 111 | A later restart alone would not prove which config the process loaded. |
| 112 | |
| 113 | TCP-address delivery is also overstated. [`SyncExec`][syncexec] delivers only when the bound bundle opts into `exec_delivery:"file"`; an unopted route receives nothing. |
| 114 | |
| 115 | **Correction:** Specify an additive config identity and per-instance acknowledgement of successful application/restart, carried through host status and the instances API. Define hibernating and failed-delivery states. Distinguish TCP routes awaiting external restart from routes with file delivery disabled. |
| 116 | |
| 117 | ## R2-7 — P2 — Account and box bandwidth and rate bounds remain absent |
| 118 | |
| 119 | **Section:** §3 limits, lines 208–220. |
| 120 | |
| 121 | The account cap bounds concurrent allocations, but not allocation churn or traffic. An account can spread requests across apps to multiply the 30-per-minute allowance and the 20 GB budget. The box likewise has no aggregate admission-rate or traffic bound. |
| 122 | |
| 123 | A rolling-hour byte allowance also permits a concentrated burst before termination; it does not establish bandwidth sharing with other apps or the edge. |
| 124 | |
| 125 | **Evidence:** The [parent’s policy][roadmap-policy] requires allocation, rate, bandwidth and lifetime bounds per app, account and box. Revision 2 supplies only some of those dimensions. |
| 126 | |
| 127 | **Correction:** Specify aggregate account and box rate/traffic limits, including burst behavior and enforcement on ongoing forwarding. Distinguish the rolling-hour allowance from the mechanism that bounds instantaneous traffic. |
| 128 | |
| 129 | ## R2-8 — P2 — The memory settings do not guarantee that the relay dies first |
| 130 | |
| 131 | **Section:** §3 hardening, lines 145–153; §7 flagship gate. |
| 132 | |
| 133 | `OOMScoreAdjust=800` is a bias added to a memory-dependent score, not a strict priority. A sufficiently large tenant with the existing `+500` adjustment can outrank a smaller relay. The repository permits a [1 GiB children tier][cgroup]. Linux’s [OOM scoring implementation](https://raw.githubusercontent.com/torvalds/linux/v6.12/mm/oom_kill.c) combines memory usage and the adjustment. |
| 134 | |
| 135 | The host budget also remains incomplete. The [edge ceiling][edge-unit] is 80% of physical memory; adding 256 MiB on a 2 GiB box leaves roughly 154 MiB beneath those ceilings for the console, other services and kernel. Their requirements are not budgeted. |
| 136 | |
| 137 | **Correction:** State a complete supported-box budget and the memory-pressure conditions under which relay-first shedding is required. Establish that behavior through an explicit shedding mechanism or a justified scoring envelope. A single successful load test cannot establish the unconditional ordering currently claimed. |
| 138 | |
| 139 | ## R2-9 — P2 — The EIP hairpin argument does not establish peer isolation |
| 140 | |
| 141 | **Section:** §3 peer policy, lines 197–207; §7 tests. |
| 142 | |
| 143 | The exception permits every UDP port on the EIP, not just live relay allocations. “The same security group” does not imply “nothing the public cannot reach”: rules can distinguish source addresses, and relayed traffic originates from the box’s address. The infrastructure already accepts source-restricted additional rules through [`extra_ingress`][infra]. AWS also documents [stateful connection tracking](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-connection-tracking.html), rather than a universal public-access predicate. |
| 144 | |
| 145 | This finding does **not** assert that EC2 hairpinning fails. The problem is that successful hairpinning would not prove the claimed isolation. The specified loopback test cannot establish the EC2 address translation and security-group behavior either. |
| 146 | |
| 147 | **Correction:** Restrict same-EIP forwarding to live relay allocation destinations using the wrapped socket’s destination-port visibility, or explicitly define and approve the broader public-service policy and its infrastructure constraints. Require the same-relay gate on the actual EIP deployment as well as loopback. |
| 148 | |
| 149 | ## R2-10 — P2 — Proposed deferrals are not yet an authoritative stage boundary |
| 150 | |
| 151 | **Section:** §§8–9 and the parent’s stage 4a contract. |
| 152 | |
| 153 | Revision 2 clearly labels proposed departures, which improves honesty, but it still declares the app contract fixed while the parent requires: |
| 154 | |
| 155 | | Parent requirement | Revision-2 replacement | |
| 156 | |---|---| |
| 157 | | TCP 3478 and TLS 5349; 443-only networks are the documented remaining transport gap | No TLS listener | |
| 158 | | Received, sent and inter-region bytes on the account’s usage line | Two byte counters in raw chunks; inter-region meter and usage view deferred | |
| 159 | | One-hour credential maximum | Four hours | |
| 160 | | Regional master custody with deployment-wide derived-key exposure | Deployment-wide master exposure through console secrets | |
| 161 | |
| 162 | **Evidence:** [Parent ingress and credentials][roadmap-ingress], [custody and policy][roadmap-policy], [metering][roadmap-meters], and [stage 4a acceptance gate][roadmap-gate]. Section 9 contains recommendations for Paul, not recorded decisions. |
| 163 | |
| 164 | **Correction:** Obtain and record the decisions, amend the parent and stage gate, and make the documents agree before freezing the implementation-plan inputs. If those decisions are not taken, retain the parent requirements. This is a contract-resolution requirement, not an objection to choosing a smaller stage. |
| 165 | |
| 166 | ## R2-11 — P2 — Placement ordering still lacks its conversion rule |
| 167 | |
| 168 | **Section:** §5 enable step 3, lines 348–352. |
| 169 | |
| 170 | The spec chooses AWS names for relay regions but still orders them using “the app’s placement region.” Placement values such as `ie` and `fleet/<name>` are not AWS region names. For multiple placements, the text also does not select which one determines the first relay. |
| 171 | |
| 172 | **Evidence:** [`edgesFile`][edgesfile] explicitly distinguishes and publishes `Placement` and AWS `Region`. |
| 173 | |
| 174 | **Correction:** Specify resolution through the published placement/region mapping, with deterministic behavior for multiple placements, missing mappings and local regions outside the allowed list. Alternatively, use console order unconditionally and remove the placement-first promise. |
| 175 | |
| 176 | ## R2-12 — P3 — Two references remain incorrect |
| 177 | |
| 178 | **Section:** §3 STUN; §5 enable step 5. |
| 179 | |
| 180 | The STUN paragraph refers to “§0,” which this spec does not contain. The `writeEnv` citation points to `internal/objstore/walk.go:554`, which discusses deletion concurrency; [`writeEnv` begins at line 602][writeenv]. |
| 181 | |
| 182 | **Correction:** Name the intended section and cite the function at its current location. |
| 183 | |
| 184 | **Verdict: request changes.** P1 and P2 problems remain. Aggregate publication can restore revoked authority under an ordering permitted by the text; delivery recovery, rotation, metering and status still need concrete contracts before the implementation plan. |
| 185 | |
| 186 | [spec]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-relay-stun-turn-design.md |
| 187 | [pion-generator]: /home/paulca/.claude/jobs/f1f6099c/tmp/gomodcache/github.com/pion/turn/v4@v4.1.4/server_config.go:20 |
| 188 | [pion-reader]: /home/paulca/.claude/jobs/f1f6099c/tmp/gomodcache/github.com/pion/turn/v4@v4.1.4/internal/allocation/allocation.go:301 |
| 189 | [pion-created]: /home/paulca/.claude/jobs/f1f6099c/tmp/gomodcache/github.com/pion/turn/v4@v4.1.4/internal/allocation/allocation_manager.go:140 |
| 190 | [pion-server]: /home/paulca/.claude/jobs/f1f6099c/tmp/gomodcache/github.com/pion/turn/v4@v4.1.4/server.go:77 |
| 191 | [pion-turn]: /home/paulca/.claude/jobs/f1f6099c/tmp/gomodcache/github.com/pion/turn/v4@v4.1.4/internal/server/turn.go:34 |
| 192 | [apply]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/generation.go:113 |
| 193 | [generation]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/generation.go:40 |
| 194 | [instances]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/console/apiinstances.go:132 |
| 195 | [syncexec]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/appconfig/syncexec.go:11 |
| 196 | [metrics-writer]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/metrics_loop.go:338 |
| 197 | [edge-unit]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/units/carlos-edge.service:57 |
| 198 | [cgroup]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/cgroup/cgroup.go:60 |
| 199 | [infra]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/infra/modules/region-host/main.tf:77 |
| 200 | [edgesfile]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/host/edgesfile.go:72 |
| 201 | [writeenv]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/internal/objstore/walk.go:602 |
| 202 | [roadmap-ingress]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:611 |
| 203 | [roadmap-policy]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:641 |
| 204 | [roadmap-meters]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:662 |
| 205 | [roadmap-gate]: /home/paulca/amadan.net/carlos/platform/.claude/worktrees/media-roadmap-spec/docs/superpowers/specs/2026-09-07-media-platform-roadmap-design.md:931 |
amadan