Repository navigation
feat(srt): SRT relay addresses with a sticky id (srt+relay://) - #629
Conversation
|
This PR looks like part of a multi-PR dependency stack together with, in dependency order: Eyevinn/strom#1258 (Strom resolves Related: #628 (spec merged in #630) builds on this stack's relay-status cache, and its implementation waits for #629 to merge. |
… Strom SRT output URLs are stored with the passphrase encrypted at rest (#260), like source addresses, but only source addresses were decrypted in activateStromFlow. Output URLs went into srt_uri raw, so Strom got the encv1: ciphertext as the passphrase and the receiver could never decrypt the stream. Decrypt a copy of each mpegtssrt/efpsrt output URL before the output blocks are built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sources (srt/efp) and SRT outputs (mpegtssrt/efpsrt) accept
srt+relay://<id>[?params]: a session on the SRT relay (srt-splice) that
Strom fronts and resolves itself. Open Live never learns where the relay
is; it owns the id and the passphrase.
- srt+relay://auto asks the server for an id, like srt://:0 asks for a
port: a random 128-bit base64url id is generated once and stored. A
patch with auto keeps the id the doc already has, so the address the
outside party dials survives activation, restarts, flow regeneration
and Strom redeploys. An explicit id is stored as given.
- srtUrl() validates the relay form: id [A-Za-z0-9_-]{1,64}, the same
query charset and 10-79 passphrase rule, and rejects mode, streamid,
localaddress and localport (Strom sets them when it calls the relay).
The passphrase is encrypted at rest as for any SRT address.
- Relay addresses skip listener-port reservation and clash checks, and
are never declared as flow ports. Flow generation passes the
(decrypted) relay URI to srt_uri unchanged.
- StromClient.system.srtRelay() reads GET /api/srt-relay; the result is
cached ~30 s (404 from an older Strom = not configured; other failures
serve the last good answer).
- connect: outputs' dial-in address resolves relay addresses to
srt://<relay public>?streamid=<id> (source srt-relay, mode caller,
streamid), or uri null with reason srt-relay-not-configured /
srt-relay-public-unknown. port is now nullable. Sources get the same
connect field. Strom is only asked when a relay doc is in the reply.
- GET /api/v1/server-info adds srtRelay: { configured, public }.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…icit ids
A relay id is the streamid both callers use on the relay, which pairs
exactly two callers per id, so an id may be held by one document only.
Sources and outputs share one namespace.
- Create and patch refuse an id another source or output holds with 409
and a message naming that document. Like listener ports, create checks
again after the write: a clash undoes the write, then a generated
(auto) id is regenerated and retried, an explicit one is refused.
A doc's own id never counts against it.
- Explicit ids must be [A-Za-z0-9_-]{16,64}; a shorter one is a 400 that
says why (anyone who guesses the id can connect to the public relay
port). auto is unchanged and generates 22 characters.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y status Code-review follow-ups on the SRT relay branch: - One resolver, resolveSrtAddress() (next to resolveListenerAddress), and one scan, usedSrtBindings(), for listener ports and relay ids alike; the resolve/write/re-check/undo loop is shared by sources and outputs, create and update (services/srt-address-write.ts). relay-ids.ts is folded in. - PATCH resolves and checks the EFFECTIVE address whenever the address or the stream type changes, so a whip -> srt patch can no longer store a literal `auto` relay id or one another document holds. - PATCH re-checks after the write like POST, and undoes the write (restoring the previous document) on a clash. - The relay status is stale-while-revalidate: requests are served from cache and refreshed in the background, the cache is warmed at startup, and only a lookup with nothing cached waits (at most 3 s). - Strom unreachable is no longer "not configured": the status keeps the last known values with state unreachable/unknown; server-info carries `state`, and connect.reason says srt-relay-unreachable. - Create/patch to a relay address is refused with 422 when Strom positively reports no relay; accepted while Strom cannot be asked. - srtUrl() matches the srt+relay:// scheme case-insensitively, like the parser and Strom. - srt-connect uses one host:port parser for authored URIs and the relay's public address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c2d44ba to
c953e48
Compare
GStreamer's SRT elements in Strom match query keys and mode values
exactly and drop most problems silently: `PassPhrase=` or `mode=Caller`
is ignored (an address that looks encrypted runs unencrypted), a bad
escape or %00 sets nothing, and a raw '#' cuts the URI. Strom now
refuses such a URI when it builds the flow, so Open Live accepted it on
save and the production failed only at activation.
- lib/srt-uri-check.ts: checkSrtUriAsGStreamerReads, a port of Strom's
validate_srt_uri (types/src/srt_uri.rs): scheme exactly srt:// (or
srt+relay://), no raw '#', no malformed escape or %00, keys and mode
values in GStreamer's spelling (the message names it), known mode, no
key given twice, passphrase 10-80 decoded UTF-8 bytes, pbkeylen
0/16/24/32.
- srtUrl(url, { newValue }) runs it on a value being written: create,
and a PATCH that sends address/url. A PATCH without one (rename)
checks the stored address only by the older rules, so existing
documents keep working.
- The passphrase limit is 80 bytes (libsrt HAICRYPT_SECRET_MAX_SZ), not
79, and is measured decoded everywhere, so %20 counts as one byte. A
stored passphrase that passed the old rule (length as written) still
passes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
encodeURIComponent leaves these RFC 3986 unreserved/sub-delim characters unescaped, so the studio (and gateways, Companion) sent a passphrase such as "my!secret(2024)" as written, and the query allowlist answered 400 "Invalid SRT relay address format" with no hint at the cause. Both srt:// and srt+relay:// now share one allowlist that includes them. GStreamer and Strom pass them through unchanged, and the address only travels as JSON. Double quote, backslash, space and angle brackets stay refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A query key without '=' (srt://:5000?latency) crashes GStreamer 1.28.6 when the SRT element starts, and Strom now refuses such an srt_uri when it builds the flow. checkSrtUriAsGStreamerReads refuses it on save too, naming the key, for srt:// and srt+relay:// alike, so a new address never reaches Strom in that form. The message for a mode in the wrong case follows Strom's correction: GLib 2.88 and later read it ignoring case, older GLib ignores it, so the mode would differ between builds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…odes
Strom replaced GET /api/srt-relay (never released) with
GET /api/capabilities, which reports the SRT modes that can work, the
SRT relays by name with their public address, inbound reachability and
STROM_PORTS. Relay URIs gained a relay name: srt+relay://<id>[@<relay>].
- services/strom-capabilities.ts replaces srt-relay-status.ts: one
cached, non-blocking lookup of GET /api/capabilities, same timings
(30 s fresh, stale-while-revalidate, 5 s after a failure, 3 s first
wait, warmed at startup). State ok / unsupported (404: an older
Strom, every mode and no relay) / unreachable (last known values) /
unknown. Parsing fails open: a mode is off only when Strom says
false; malformed relays and port spans are dropped.
- Named relays: @<relay> is parsed, validated ([a-z0-9-]{1,32}) and
written (the default relay without @, as Strom writes it). `auto`
keeps the stored id and, without @, the stored relay, so renewing the
passphrase never moves the stream; auto@<relay> moves it, auto@default
moves it back. The dial-in address uses the named relay's public
address; connect gains `relay`, and reason `srt-relay-unknown` for a
name Strom does not report. A definite answer that the relay is not
there refuses the write with 422 and `code`
(srt-relay-not-configured | srt-relay-unknown) and `relays` (the
configured names). Relay ids stay unique across all relays.
- #628: server-info `srt { state, modes { listener, caller, rendezvous },
relays [{ name, public }], inboundReachable, ports }` replaces the
unreleased `srtRelay`. Modes are reported, never refused: a listener
or rendezvous address on a Strom that says it cannot work is stored
and gets connect.warning (srt-listener-unsupported /
srt-rendezvous-unsupported). Strom's own host is still resolved from
the authored host, SRT_PUBLIC_HOST, then the STROM_URL host. The
legacy port reservation is unchanged.
- docs/openapi.yaml documents the new fields, reasons and 422 body.
BREAKING CHANGE: server-info `srtRelay` is replaced by `srt` (only the
draft studio branch read it).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ADR-014 and the SRT direction spec were written before Strom settled on GET /api/capabilities with SRT modes and named relays, and before GET /api/srt-relay was dropped. Add a dated note on what #629 now implements where it differs (one endpoint, server-info `srt` shape, modes reported rather than refused, relay 422 codes) and rename the Strom env vars (STROM_SRT_RELAY_PUBLIC, STROM_SRT_RELAYS). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A patch with srt+relay://auto[@<relay>] on a document that already holds a relay address now keeps the stored (encrypted) passphrase, and the stored pbkeylen, when the query has no passphrase key. The passphrase protects the stream end to end and the relay never sees it, so moving to another relay no longer makes the operator type it again. An empty passphrase= removes it; passphrase=<value> sets it as before. An empty passphrase= is dropped from every relay address before it is stored, so it never reaches Strom. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review: feat(srt): SRT relay addresses with a sticky id (srt+relay://)
Verdict: REQUEST CHANGES
The code is solid and verified against Strom v0.8.2. CI is green, and locally on f4105d75 typecheck, build and vitest pass (184 files, 2233 tests). This PR also closes #628, so it is the first implementation of the approved #630 spec. It changes that spec's central decision (refuse with 422, now warn) and leaves the spec and ADR in their pre-implementation state. Neither the team-lead nor a maintainer has signed off on the change. The fixes are mostly documentation and one sign-off.
Prior findings
Sources: the #630 team-lead approval (resolutions 1-14), #628's proposal and test cases, the triage comments on #628 (2026-11-11 to 2026-11-13), and the stack-flag comment on this PR. This PR had no earlier reviews or inline comments.
| # | Finding (source) | Status | Evidence |
|---|---|---|---|
| 1 | API refuses create, or a PATCH to, a direction Strom positively reports as unsupported, with 422 srt-direction-unsupported + direction (#628 proposal 3 and test case "creating a listener via the API is refused"; #630 res. 3, 4, 6) |
OPEN | Not implemented. f554d14 replaces it with connect.warning (src/lib/srt-connect.ts:175-202, src/routes/status.ts:30-31 "never enforced by the API"). Triage on #628 (2026-11-13 12:18Z, 15:18Z, 16:17Z) required a team-lead sign-off on this amendment. There is none on #628, #629 or #630, and no human has waived it. |
| 2 | #629's relay 422 adopts the same code/direction scheme (#630 res. 3) |
OPEN | It uses code: srt-relay-not-configured | srt-relay-unknown + relays, with no direction (src/services/srt-address-write.ts:22-29). This is a reasonable change, but it is part of the same unapproved amendment. |
| 3 | Unreachable/unknown capabilities set connect.warning: srt-capabilities-unknown (#630 res. 4) |
OPEN | Not implemented: SrtConnectWarning has only the two mode warnings (src/lib/srt-connect.ts:45). Unreachable warns from last-known modes, unknown never warns. The amendment note does not mention this change. |
| 4 | Listener port outside Strom's ports: warn, not refuse (#630 res. 9) |
OPEN | There is no such warning. srt.ports is exposed (src/routes/status.ts:42), but nothing compares a listener port against it. The amendment does not mention this either. |
| 5 | srtDirection: host + mode=listener is a listener, hostless mode=caller maps to null, one unit test per case (#630 res. 13) |
OPEN | Hostless mode=caller maps to 'listener' (src/lib/srt-connect.ts:186-191, test src/__tests__/srt-relay.test.ts:416). That may be the correct GStreamer reading, but it differs from the approved rule and the amendment does not record it. |
| 6 | Flip ADR-014 from Proposed to Accepted in the first implementation PR (#630 res. 14; #628 triage 11-12, 11-13 ×2) | OPEN | docs/decisions/ADR-014-strom-reported-srt-capabilities.md:4 still says **Status**: Proposed. |
| 7 | Update the spec header: Status Implemented, Strom dependency 0.8.2, drop the stale strom#1258 references, and fold resolutions 11-14 in, including the §2 and §8-item-11 sequencing wording (#628 triage 11-12, 11-13 08:18Z, 16:17Z) | OPEN | docs/specs/srt-direction-capabilities.md:4 still says Status: Draft (… pre-implementation). :7 says strom#1257 is "open, no PR yet" (it is closed; #1260 shipped). :8-10 say #629 is a draft and depends on strom#1258 "open, ready for review" (it was closed without merging, superseded by #1262). :74 and :451 still have "or rebases onto it" / "starts from #629's branch state". The new note at :13-14 cites the Strom branches feat/capabilities / feat/srt-relay-v2 instead of strom#1260/#1262 and v0.8.2. |
| 8 | server-info keeps srtPorts, srtPortState, srtRelay (#630 res. 2) |
FIXED (srtPorts/srtPortState) | src/routes/status.ts:44 (3251e31). srtRelay never reached main (origin/main:src/routes/status.ts:27), so removing it breaks nothing released. Its replacement by srt is covered by the sign-off in row 1. |
| 9 | connect.warning does not depend on uri and can appear next to reason (#630 res. 5) |
FIXED | src/lib/srt-connect.ts:172-177 (3251e31) adds the warning to any resolved direct connect, including the uri: null + reason cases. |
| 10 | Existing records stay listed and still activate (#630 res. 7; #628 test case 4) | FIXED | The mode is never enforced. Flow generation passes the URI through unchanged (src/lib/flow-generator.ts:1219-1225, src/__tests__/srt-relay-routes.test.ts:357). |
| 11 | Older Strom: 404 means unsupported with every mode, other failures mean unreachable, parsing fails open (#630 res. 8; #628 proposal 5) |
FIXED | src/services/strom-capabilities.ts:80-81,126-135,151-152 (3251e31), with tests in src/__tests__/strom-capabilities.test.ts. |
| 12 | Sequencing: implement only after #629 merges, not stacked on it (#630 res. 11) | OPEN (resolved by row 1's sign-off) | Instead, #628 is implemented inside #629 (3251e31). The 16:17Z triage accepts reviewing #629 as the implementation, but only an agent wrote that. Record it in the same team-lead sign-off. |
| 13 | Stack hold: drafts depend on unmerged strom#1258 (stack-flag comment, 2026-11-11) | FIXED | The PR left draft at 15:24Z. strom#1260/#1262 merged, and v0.8.2 was released 2026-11-13T15:52Z. The PR body now cites #1260/#1261/#1262. |
| 14 | PR body still said the Strom branches had no PR numbers (#628 triage 15:18Z) | FIXED | The PR body was edited (not a commit) and now cites Eyevinn/strom#1260, #1261, #1262. |
Critical issues (must fix before merge)
- SPEC DIVERGENCE, not signed off (rows 1-5, 12). The approved spec and ADR-014 require a 422
srt-direction-unsupported. This PR warns instead. ADR-014 explicitly rejected that option (ADR-014…md:80-81: "Hide only in the UI, no API refusal. Rejected: the API would accept addresses the UI says do not work, so gateways and Companion would still create undialable listeners"). The amendment justifies the change in one clause, "since the operator may know better". It does not answer the rejected-alternative argument: gateways and Companion can still create undialable listeners, and the only signal is aconnect.warningthey may not read. It also does not list rows 3, 4 and 5 as deviations. To fix, do one of the following:- (a) Implement the 422 as specified.
- (b) Get an explicit team-lead (or maintainer) sign-off comment that accepts the amendment, covering rows 1-5 and 12. Then rewrite ADR-014 so that Decision 4 and the "Hide only in the UI" alternative match the shipped behaviour, with the reason for reversing it, for example: the operator may have a port forward Strom does not know about, and
STROM_INBOUND_REACHABLE=falseis a deployment hint, not proof. Also list the droppedsrt-capabilities-unknownwarning, the droppedportswarning and the changed hostless-mode=callermapping.
- Stale spec header and ADR status (rows 6, 7). Spec
:4→Status: Implemented (#629). Spec:7-10→ strom#1260/#1262 merged, Strom ≥ 0.8.2, #629/#259 no longer drafts, strom#1258 closed and superseded. Spec:74,:451→ drop the "rebases onto it" sequencing. Spec note:13-14→ cite strom#1260/#1262 and v0.8.2 instead of branch names. ADR-014:4→ Accepted (amended 2026-11-13). ADR-014:7→ drop "strom#1257 (open, not yet implemented)". ADR-014 title → it currently says directions are "refused only on a definite answer", which is now true only for relays. §3 "Status: proposed, not yet implemented upstream" (:81) and the env-var table "Listener hidden and refused" (:399) contradict the shipped behaviour. Either mark them superseded inline or fix them, because "sections below are kept as the original design" is easy to miss.
Warnings (should fix)
-
docs/specs/srt-direction-capabilities.md:66-67still describesStromClient.system.srtRelay()/GET /api/srt-relayas the current baseline. That endpoint was removed, so mark it as removed.
Security (checked, no blockers)
- URL validation.
srt+relay://has no host, so there is no SSRF surface. Plainsrt://keeps the private/loopback/link-local check (src/lib/url-validation.ts:425-428,447-449). The relay id is limited to[A-Za-z0-9_-]{16,64}for explicit ids and 128-bit random forauto(src/lib/srt-relay.ts:67,115-116). The relay name is limited to[a-z0-9-]{1,32}.mode,streamid,localaddressandlocalportare refused on relay addresses (keys are decoded byURLSearchParams, so%73treamidis caught too). The query allowlist grows by! * ' ( ) ~. The address travels only as JSON, never into a shell or HTML, and",\, space and<>stay refused. - New-value check.
checkSrtUriAsGStreamerReadsis a port of Strom'svalidate_srt_uri. It is applied on create and on a PATCH that sends the address. Stored addresses keep the lenient rule, so nothing existing is blocked. The removed checks are all relaxations: the passphrase maximum goes from 79 to 80 bytes, length is now measured decoded with the legacy raw length still accepted, and the allowlist widens. Each relaxation is justified against libsrt and GStreamer. - Passphrase handling. The
autoPATCH merge keeps the storedencv1:value verbatim.encryptPassphraseskips already-encrypted values (src/lib/srt-passphrase-crypto.ts:185), so it is not double-wrapped. An emptypassphrase=is stripped before storage.connect.urinever includes the passphrase, and the responses still mask it (maskSrtPassphraseinroutes/sources.tsandroutes/outputs.ts:200). - Separate fix (cbf8152). SRT output URLs were sent to Strom still encrypted. This PR fixes that by decrypting a copy before flow generation (
src/lib/flow-generator.ts:1219-1225), and the change is covered byflow-generator-output-passphrase.test.ts. This is a real bug fix. - Known limitation. Relay ids are unique only within one Open Live instance. This is documented in the PR body and acceptable.
What was verified
- The Strom contract is real and was not guessed: Eyevinn/strom
types/src/capabilities.rsat v0.8.2 hasnetwork { inbound_reachable, ports: Option<Vec<PortSpan{first,last}>> },protocols.srt { modes { listener, caller, rendezvous }, relays [{ name, public }] }andwebrtc { ice_servers, ice_transport_policy }. This matchesStromCapabilitiesResponseinsrc/lib/strom.ts. Strom v0.8.2 was published 2026-11-13T15:52:21Z. - CI
build-and-testpasses. Locally onf4105d75:pnpm install --frozen-lockfile,pnpm run typecheck,pnpm run buildandnpx vitest run(184 files, 2233 tests) all pass. The repo has no lint script. - The PATCH path resolves the effective address on an address or stream-type change, and re-checks and undoes the write on a clash (
src/routes/sources.ts,src/services/srt-address-write.ts). A rename is not re-checked. - Every commit has a
Co-Authored-Bytrailer, and the branch isfeat/srt-relay, notmain. - No new dependencies.
|
team-lead sign-off on the #629 spec amendment (daily-backlog-pr Phase 2, 2026-11-13) This covers the spec divergence in the eyevinn-pr-reviewer review at
Also accepted (studio review nit): a stored listener record whose Strom reports Still required before merge
open-live-studio#259 has matching work: the |
Add connect.warning `srt-port-outside-strom-ports` for a plain srt:// listener whose port is outside every span Strom reports in network.ports (last-known while Strom is unreachable), when no legacy srtPorts reservation is active. `srt-listener-unsupported` wins when Listener itself is unsupported. #630 resolution 9, required by the team-lead sign-off on #629. Tests cover the span checks, the reservation and null-ports cases, and that create (201) and PATCH (200) on sources and outputs carry connect.warning. openapi documents the new value on all four routes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spec: status Implemented, Strom >= 0.8.2 (strom#1260/#1262), stale strom#1257/#1258 and draft references fixed, /api/srt-relay marked removed, superseded sections marked inline, #630 resolutions 11-14 folded in with the team-lead sign-off decisions. ADR-014: Accepted (amended 2026-11-13); Decision 4 and the "Hide only in the UI" alternative rewritten to record why modes are reported and only relays are refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
daily-backlog-pr Phase 3: first re-review after the REQUEST_CHANGES verdict, on the new head with the fix commits pushed by the Phase 2 re-entry. |
There was a problem hiding this comment.
PR Review: feat(srt): SRT relay addresses with a sticky id (srt+relay://), re-review at 69df078b
Verdict: REQUEST CHANGES
The code is ready. The row-4 port warning (8199b73) is correct and tested, the name srt-port-outside-strom-ports matches open-live-studio#259 (4b2431af), and every condition in the team-lead sign-off is met. The one thing still blocking is documentation. 69df078 adds "passages that the amendment supersedes are marked inline" to the spec, but the spec's acceptance tests (§13), the studio 422 handling (§6), the sequence diagram (§8) and the /api/srt-relay poller design (§3.2, §4.1) still describe the refuse-with-422 / srt-capabilities-unknown design and are not marked. The spec is now Status: Implemented, and §13 is the issue's test list made concrete, so a tester working from it would assert the opposite of what ships. The fix is mechanical and docs only.
Prior findings
Sources: the eyevinn-pr-reviewer review at f4105d75 (rows 1-14, two Critical issues, one Warning), the team-lead sign-off on this PR (comment 6859992148, agent team), the stack-flag and re-review-cycle comments on this PR, and the six triage comments on #628 (2026-11-11 to 2026-11-13). There are no inline review comments, and no human has commented on #628 or #629 apart from the agent-posted triage. Rows 1, 2, 3, 5 and 12 are FIXED because the earlier review named option (b) as the fix: "an explicit team-lead (or maintainer) sign-off comment that accepts the amendment, covering rows 1-5 and 12. Then rewrite ADR-014 …". Both halves of that fix are present.
| # | Finding (source) | Status | Evidence |
|---|---|---|---|
| 1 | 422 srt-direction-unsupported for a direction Strom reports as unsupported (#628 proposal 3 and test case; #630 res. 3, 4, 6) |
FIXED (option (b)) | Accepted with conditions in sign-off 6859992148. ADR-014 Decision 4 has been rewritten (ADR-014…md:48-65), and the rejected alternative has been reversed, with reasons (:95-113). Condition (i): create/PATCH responses carry connect.warning, tested for sources and outputs (src/__tests__/srt-relay-routes.test.ts:363,389,397, plus the earlier POST source test). Condition (ii): Eyevinn/open-live-ingest#23 is open. Condition (iii): ADR-014 has been rewritten (69df078). |
| 2 | Relay 422 adopts the code/direction scheme (#630 res. 3) |
FIXED (option (b)) | Accepted in the sign-off. Recorded in ADR-014…md:56-61 and spec :47-49 (69df078). |
| 3 | srt-capabilities-unknown warning (#630 res. 4) |
FIXED (option (b)) | Accepted (dropped) in the sign-off. Recorded in ADR-014…md:108-110 and spec :50-52 (69df078). |
| 4 | Warn on a listener port outside Strom's ports (#630 res. 9; the sign-off made it required) |
FIXED | 8199b73: srtPortWarning (src/lib/srt-connect.ts:226-233) is chained after the mode warning (:193, so an unsupported mode wins). Port 0 is skipped. When the legacy reservation is reserved, the warning is suppressed (src/services/strom-capabilities.ts:205-212) because that path already refuses with its own 422 (src/services/listener-ports.ts:182-183). While Strom is unreachable it uses the last-known ports, and unsupported/unknown give ports: null, so there is no warning. Unit tests are at src/__tests__/srt-relay.test.ts:413-459, route tests for create 201 and PATCH 200 on sources and outputs at srt-relay-routes.test.ts:372-417, and the reservation flag test at strom-capabilities.test.ts:208-222. openapi documents the enum and all four routes. The name matches studio#259 (srt-port-outside-strom-ports in src/lib/api.ts and its tests). |
| 5 | Hostless mode=caller maps to null (#630 res. 13) |
FIXED (option (b)) | Accepted in the sign-off (it maps to listener). Recorded in ADR-014…md:62-64,110-112 and spec :60-62. The unit test is present. |
| 6 | ADR-014 Proposed → Accepted (#630 res. 14; #628 triage) | FIXED | ADR-014…md:4 now reads Accepted (amended 2026-11-13). The title, the :7 dependency line and the Risks line have been updated (69df078). |
| 7 | Spec header: Status Implemented, Strom 0.8.2, stale strom#1257/#1258 and draft references, sequencing wording :74/:451, note cites branches (#628 triage; earlier review Critical 2) |
FIXED (header items) | Spec :4 is Implemented, :6-17 list Strom ≥ 0.8.2, strom#1260/#1262, and #1258 closed and superseded, and :20 cites strom#1260/#1262 and v0.8.2. The "rebases onto it" / "starts from #629's branch state" wording has been replaced (:115-116, :510-511). The §3 status line and the env-var table were also fixed (:118-123, :455). |
| 7b | Every remaining passage that contradicts the shipped behaviour is marked superseded inline or fixed (earlier review Critical 2: "Either mark them superseded inline or fix them, because 'sections below are kept as the original design' is easy to miss"; sign-off "still required" item 2, "superseded sections") | OPEN | §3 :118, §4.4 :237, §5.2 :317, §5.3 :353-356 and §11 items 3/4 are marked. These are not, although :33-34 now says they are: §13 test cases :544 T1 (→ 422 srt-direction-unsupported/listener), :547 T4 (srt-listener-port-outside-strom-ports, the old name), :548 T5 (PATCH caller→listener on another record → 422), :549 T6 (connect.warning:"srt-capabilities-unknown"). §6 studio :391-392 (handle 422 srt-direction-unsupported) and the Edit-dialog bullet (not supported by this Strom, changed by the sign-off's studio decision). §8 sequence diagram :419-420 (GET /api/srt-relay), :434-436 (the 422 branch). §3.2 parser :155-157 and §4.1 poller :170-176 (fetches GET /api/srt-relay in parallel), plus §4.2 :225-227 (/api/srt-relay authoritative). The §2 table rows at :108 and :111 still say draft PR. |
| W | Spec :66-67 describes GET /api/srt-relay as the current baseline (earlier review Warning) |
FIXED | The §2 table row (:107) is marked removed, and :106 is marked replaced by strom-capabilities.ts (69df078). The design sections that still use the endpoint are covered by row 7b. |
| 8 | server-info keeps srtPorts, srtPortState (#630 res. 2) |
FIXED | src/routes/status.ts:44, unchanged since 3251e31. |
| 9 | connect.warning does not depend on uri (#630 res. 5) |
FIXED | src/lib/srt-connect.ts:190-194. The port warning uses the same path. |
| 10 | Existing records stay listed and still activate (#630 res. 7; #628 test case 4) | FIXED | Nothing is enforced. Flow generation decrypts and passes the address through (src/lib/flow-generator.ts:279, srt-relay-routes.test.ts). |
| 11 | Older Strom: 404 → unsupported, other failures → unreachable, fail open (#630 res. 8; #628 proposal 5) |
FIXED | src/services/strom-capabilities.ts (3251e31), unchanged. Tests are in strom-capabilities.test.ts. |
| 12 | Sequencing: implement only after #629 merges (#630 res. 11) | FIXED (option (b)) | Accepted in the sign-off. Recorded in spec :55-57, :115-116 and :510-511 (69df078). |
| 13 | Stack hold on unmerged strom#1258 (stack-flag comment 2026-11-11) | FIXED | strom#1260/#1262 merged and v0.8.2 was released. The PR is not a draft. |
| 14 | PR body said the Strom branches had no PR numbers (#628 triage 15:18Z) | FIXED | The PR body cites strom#1260, #1261 and #1262. |
Critical issues (must fix before merge)
-
The spec still contradicts the shipped behaviour in unmarked sections (row 7b). Mark each of these superseded inline (or fix it), as was done for §4.4/§5.2:
- §13 T1 →
201withconnect.warning: srt-listener-unsupported. - §13 T4 →
srt-port-outside-strom-ports. - §13 T5 → drop the
→ 422on PATCH caller→listener (now200+ warning). - §13 T6 →
201, no warning, or a warning from the last-known modes. - §6 → the
422 srt-direction-unsupportedbullet and the Edit-dialog label (the sign-off's studio decision). - §8 → the
/api/srt-relayfetch and the 422altbranch. - §3.2 and §4.1/§4.2 →
/api/srt-relayis gone, and there is one fetch ofGET /api/capabilities. - §2
:108,:111→draft PR→ in #629.
This is docs only. No code or test change is needed.
- §13 T1 →
Warnings (should fix)
- None new. The minor items below can go in the same docs commit.
Nits (optional)
- ADR-014 Decision 3 (
:46-47) still listsserver-info.srt { listener, caller, relay, ports, state }and "srtRelaystays". The amended header (:11-12) covers it, but an inline "(amended)" marker like Decision 4's would be consistent.
Security (checked, no change since f4105d75)
- 8199b73 only reads Strom's port spans and the reservation state, and only adds an optional response field. It adds no new input, no new route and no persistence. The earlier security notes (URL validation, relay id/name limits, passphrase handling, output-passphrase decryption fix) still apply. Those files are unchanged in the two new commits.
What was verified
- Head
69df078bcaaf6fffea8d3c7cac2d6dd21f9f9412. The new commits are 8199b73 (code) and 69df078 (docs). Both are byeyevinn-developer[bot]onfeat/srt-relaywith aCo-Authored-Bytrailer, as are all 12 commits on the branch. - Locally on the head:
corepack pnpm install --frozen-lockfile,pnpm run typecheck,pnpm run buildandnpx vitest runall pass (184 files, 2246 tests, up from 2233). The repo has no lint script. - CI:
build-and-testpasses (run 43472751905). StromCapabilities.portsexists, andgetPortReservation()and the 422 for a port outside the reservation exist as cited. Nothing is fabricated.- The warning name matches open-live-studio#259 at
4b2431af(type,srtConnectWarningText, tests). This PR and #259 must merge together, #629 first. - Eyevinn/open-live-ingest#23 ("Surface Open Live's connect.warning after registering an SRT source") exists and is open.
- No new dependencies.
|
daily-backlog-pr Phase 3: the first re-review at Still open (row 7b: docs only).
Optional, in the same commit: ADR-014 Decision 3 ( Merge order is unchanged: this PR first, then the |
Bring §13 T1/T3-T7, the §6 studio bullets, the §8 sequence diagram, §3.2, §4.1-§4.3, §5.1, §9 and the §2 table rows in line with what #629 ships: one fetch of GET /api/capabilities, modes reported on connect.warning (srt-listener-unsupported, srt-port-outside-strom-ports), no srt-direction-unsupported 422 and no srt-capabilities-unknown. Mark ADR-014 Decisions 1-3 as amended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Final-cycle docs fix pushed as
Optional: ADR-014 Decision 3 is marked (Amended 2026-11-13) with the shipped |
There was a problem hiding this comment.
PR Review: feat(srt): SRT relay addresses with a sticky id (srt+relay://), final verification at acef6547
Verdict: APPROVE
This is the final-verification pass, judged only against the final-cycle list (comment 6860232060): points 1-8 plus the optional ADR-014 marker. The only new commit, acef6547, is docs-only: it touches only docs/specs/srt-direction-capabilities.md and docs/decisions/ADR-014-strom-reported-srt-capabilities.md, with no src/ or test changes. Every point is addressed in the file, and every new technical claim matches the code on the head.
Prior findings
No human waived anything, so there are no DROPPED rows.
| # | Finding (source) | Status | Evidence |
|---|---|---|---|
| 1 | 422 srt-direction-unsupported for an unsupported direction (#630 res. 3/4/6) |
FIXED (option (b)) | Accepted with conditions in team-lead sign-off 6859992148; ADR-014 Decision 4 rewritten in 69df078; src/lib/srt-connect.ts:191-193. |
| 2 | Relay 422 code/direction scheme (#630 res. 3) |
FIXED (option (b)) | Accepted in the sign-off; relayRefusal() src/services/listener-ports.ts:285-305; spec §2 :108 matches (acef654). |
| 3 | srt-capabilities-unknown warning (#630 res. 4) |
FIXED (option (b)) | Dropped per sign-off; spec §11 item 4 :536, §13 T6 :605. |
| 4 | Port outside Strom's ports warning (#630 res. 9; required by sign-off) |
FIXED | 8199b73: srtPortWarning, src/lib/srt-connect.ts:221-230. |
| 5 | Hostless mode=caller → null (#630 res. 13) |
FIXED (option (b)) | Accepted in sign-off; srtAddressMode src/lib/srt-connect.ts:202-208; spec §4.2 :219-221. |
| 6 | ADR-014 Proposed → Accepted | FIXED | 69df078, ADR-014…md:4. |
| 7 | Spec header, status, Strom 0.8.2, sequencing wording | FIXED | 69df078, spec :4-20. |
| 7b | Every passage contradicting shipped behaviour marked inline or fixed (final-cycle list 1-8) | FIXED | acef654, see table below. |
| W | Spec described GET /api/srt-relay as current baseline |
FIXED | 69df078, spec §2 :107. |
| 8 | server-info keeps srtPorts, srtPortState (#630 res. 2) |
FIXED | src/routes/status.ts:44. |
| 9 | connect.warning independent of uri (#630 res. 5) |
FIXED | src/lib/srt-connect.ts:191-193. |
| 10 | Existing records stay listed and still activate (#630 res. 7) | FIXED | Nothing enforced; flow generation unchanged. |
| 11 | Older Strom: 404 → unsupported, others → unreachable, parse fails open (#630 res. 8) |
FIXED | src/services/strom-capabilities.ts:146-165. |
| 12 | Sequencing: implement after #629 merges (#630 res. 11) | FIXED (option (b)) | Accepted in sign-off; spec :55-57. |
| 13 | Stack hold on unmerged strom#1258 | FIXED | strom#1260/#1262 merged; v0.8.2 released. |
| 14 | PR body cited no Strom PR numbers | FIXED | Body cites strom#1260, #1261, #1262. |
Final-cycle list, point by point (at acef6547)
| Pt | Required | Status | Where |
|---|---|---|---|
| 1 | §13 T1 → 201 with srt-listener-unsupported |
FIXED | :600, marked amended. |
| 2 | §13 T4 → srt-port-outside-strom-ports |
FIXED | :603. |
| 3 | §13 T5 → PATCH caller→listener 200 + warning | FIXED | :604. |
| 4 | §13 T6 → 201, no srt-capabilities-unknown |
FIXED | :605; matches strom-capabilities.ts:80-82,155-161. |
| 5 | §6 422 bullet and Edit-dialog label | FIXED | :423-435. |
| 6 | §8 diagram: no /api/srt-relay, no 422 alt branch |
FIXED | :450-492. |
| 7 | §3.2, §4.1, §4.2: one fetch of /api/capabilities |
FIXED | :151, :177-185, :219, :238,253. |
| 8 | §2 draft PR → in #629 |
FIXED | :108, :111. |
| opt | ADR-014 Decision 3 "(amended)" | FIXED | ADR-014…md:48-51. |
Edits beyond the list (§5.1, §9, §11, §12) were checked for accuracy only and match the code.
Critical issues
None.
Warnings
None.
Nits (optional)
- §13 T4: the 422 applies only to a port outside an active reservation; a port inside it is 201 with no warning. Not blocking.
CI: build-and-test green on acef6547. Merge order: this PR first, then the server-info.srt check on staging, then Eyevinn/open-live-studio#259.
Needs Strom 0.8.2 (Eyevinn/strom#1260 capabilities, #1261, #1262 named SRT relays, all merged). Merge together with the studio, Eyevinn/open-live-studio#259. Also covers #628.
What
SRT sources and outputs can be reached through an SRT relay (srt-splice) on one public UDP port, instead of a listener port per source or output. Strom dials the relay from inside, and the outside party dials it with a streamid. Strom may have several relays, each with its own name.
Address scheme
srt+relay://<id>[@<relay>][?query]inSourceDoc.address(srt/efp) andOutputDoc.url(mpegtssrt/efpsrt). It is passed to Strom'ssrt_uriunchanged, and Strom resolves it.@<relay>names one of the relays Strom reports. Names are[a-z0-9-], 1–32 characters, and are validated on save. Without@the address uses the relaydefault, as in Strom, and Open Live writes the default relay without@.Sticky id:
srt+relay://auto[@<relay>]means "generate an id for me", likesrt://:0for ports. Open Live generates a 128-bit id once and stores it.autokeeps the stored id. Open Live owns the id because it creates a new Strom flow on every activation.autowithout@also keeps the stored relay, so renewing the passphrase (srt+relay://auto?passphrase=…) never moves the stream to another relay.auto@<relay>moves the stored id to that relay, andauto@defaultmoves it back to the default relay.Passphrase on an
autoPATCH of a document that already holds a relay address:passphrasekey: the stored passphrase (still encrypted, not re-encrypted) and the storedpbkeylenare kept. A relay switch therefore keeps the passphrase: it protects the stream end to end and the relay never sees it, so there is no reason to make the operator type it again.passphrase=(empty): the passphrase is removed. An emptypassphrase=is never stored or sent to Strom.passphrase=<value>: sets it, as before.Rules for ids:
No port reservation for relay addresses. The legacy port broker is unchanged.
Strom capabilities come from Strom
GET /api/capabilities. It replacesGET /api/srt-relay, which Strom removed before it was ever released. Open Live caches the answer and refreshes it in the background, so a request never waits on Strom (30 s fresh, 5 s after a failure, warmed at startup).stateisok,unsupported,unreachableorunknown.unsupportedmeans a 404 from an older Strom. Every mode is offered as before, and there is no relay.unreachablekeeps the last known values.API:
GET /api/v1/server-infonow hassrt. It replaces the unreleasedsrtRelay.true(Only offer the SRT directions the Strom instance supports #628), and offers "Via relay" only whenrelaysis non-empty, with a relay picker when there is more than one.API:
connect(the dial-in address) is on SRT sources and outputs.srt://<that relay's public>?streamid=<id>. It never includes the passphrase, and it hasrelay: "<name>".reason(only whenuriis null) can besrt-relay-not-configured,srt-relay-unknown(Strom does not report that name),srt-relay-unreachableorsrt-relay-public-unknown.warningfield:srt-listener-unsupportedorsrt-rendezvous-unsupported, when Strom reports that the mode the address needs cannot work.SRT modes are reported, never refused. The operator may know better. A listener address on a Strom with
listener: falseis still stored, and it gets the warning.SRT_PUBLIC_HOST, then theSTROM_URLhost. Strom deliberately reports no public host.Relay refusals: a relay address gets 422 when Strom definitely does not have the relay it names. The body is
{ error, statusCode, code, relays }.code: "srt-relay-not-configured"when Strom has no relays at all, or is an older Strom.code: "srt-relay-unknown"when Strom has no relay of that name.relayslists the names Strom has.connect.reasonsays why the dial-in address is unknown.One resolver (
resolveSrtAddress,usedSrtBindings,writeWithSrtAddress) handles listener ports and relay ids in all four handlers.SRT addresses are checked at save the way GStreamer reads them (
lib/srt-uri-check.ts, a port of Strom'svalidate_srt_uri), on create and on a PATCH that sendsaddress/url. These get a 400 that names the right spelling:modein the wrong case=(new:?latencycrashes GStreamer 1.28.6, and Strom now refuses it)%00#pbkeylenother than 0/16/24/32A stored address is not checked again, so renaming an older document keeps working.
Passphrase limit is 10–80 bytes (libsrt), measured decoded.
! * ' ( ) ~are accepted in the query.Separate fix (own commit, cbf8152): SRT output passphrases were sent to Strom still encrypted (
encv1:...), because outputs, unlike sources, were not decrypted before flow generation.docs/openapi.yamldocuments all of the above. ADR-014 and the Only offer the SRT directions the Strom instance supports #628 spec have a dated note on how Strom's API landed.Strom configuration (for reference)
STROM_SRT_RELAYis nowSTROM_SRT_RELAY_PUBLIC(plusSTROM_SRT_RELAY_INTERNAL, andSTROM_SRT_RELAYS=name=public[|internal],...for named relays).STROM_INBOUND_REACHABLE=falseturns off listener and rendezvous inmodes. See Stromdocs/SRT_RELAY.md.Verified
tsc --noEmitpasses,vitestpasses: 184 files, 2233 tests (keep on switch, remove with empty value, set new, same-relay no-op, route tests with real encryption).pnpm buildpasses.@relayparse and write, dial-in per relay, sticky renewal keeping the relay, the 422 codes, mode warnings and theserver-info.srtshape.server-info.srtwith both relays. Media through Strom relay blocks was verified end to end on both relays (ffmpeg → relay → Strom input → encoder → Strom output → relay → ffprobe, encrypted H.264/AAC), driven through the Strom API, not through Open Live.Known limitations
auto) ids are 128-bit random, so this needs two explicit ids chosen alike. It is unlikely in practice, and it is not handled.srt-relay-unknown. A passphrase renewal withautoon that document is then refused with 422 until another relay is chosen (auto@<name>).Related
GET /api/capabilities), #1261 (caller output queue), #1262 (named SRT relays; supersedes #1258'sGET /api/srt-relay)server-info.srt.modesandconnect.warning)🤖 Generated with Claude Code