Repository navigation
Conversation
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment — the fix is correct and the new test is a real guard, but that test has not run in CI. Check (Linux) went red on a racy AES67 test in an earlier binary, and cargo test stopped there. Re-run the failed job and this becomes an Approve on the same head.
Requested changes
- None to the diff. A maintainer re-runs the failed job:
gh run rerun 36672596978 --failed. What is needed is a log linereading_a_missing_pad_does_not_create_it ... ok. At this head the log has no line fromenum_property_read_testat all.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Both reads no longer request a pad | CONFIRMED |
backend/src/gst/pipeline/properties.rs:420 — .static_pad(pad_name) and :457, each followed only by ok_or_else(PadNotFound). update_pad_property keeps its fallback at :376 |
| No in-repo caller relied on the read creating a pad | CONFIRMED |
The only frontend caller reads sink_{i} for each editor input (frontend/src/compositor_editor/api_sync.rs). builtin.compositor requests those pads at build, via backend/src/blocks/builtin/compositor.rs:260 — info!("Requesting {} GL mixer sink pads in NULL state", num_inputs); (and the CPU equivalent). The PATCH handler reads back only after update_pad_property, which still creates the pad |
| The test fails on revert | CONFIRMED |
backend/tests/enum_property_read_test.rs:182 — assert_eq!(sinks, vec!["sink_0"], "a read requested pads on the mixer");. With the fallback restored, request_pad_simple("sink_5") adds a pad and this assert fails |
static_pad finds existing request pads |
EXTERNAL |
gst_element_get_static_pad searches every pad on the element |
| Covering test executed in CI | UNVERIFIED |
Check (Linux) job 109750646362: test result: FAILED in drop_on_latency_test. enum_property_read_test and pipeline_lifecycle_test never ran |
The red check is not this diff's. aes67_input_sets_drop_on_latency_on_the_sdpdemux_rtpbin panicked with rtpbin rtpbin0 inside sdpdemux has drop-on-latency=false. The same base, 7ac260c, passed CI on main, and nothing in this diff reaches AES67. SPECULATIVE (not verified): it is a race in the test. The test polls sdpdemux's children every 20 ms and reads the property as soon as an rtpbin appears. But the block sets that property from connect_element_added (backend/src/blocks/builtin/aes67.rs:240 — element.set_property("drop-on-latency", true);), which upstream emits after the child is already listed. A poll that lands between the two sees false. That deserves its own issue. I am noting it here because it will redden other PRs too.
Diagnosis — Right fix at the right layer: a GET should not mutate the pipeline. Coverage is ABSOLUTE for the two read paths.
Radius — LOCAL: two getters, one endpoint. A client that probes a missing pad now gets an error instead of a new empty pad. That error is HTTP 400, although the #[utoipa::path] documents 404 for "pad not found". The mismatch is older than this PR (every error maps to BAD_REQUEST in get_pad_properties) and only becomes easier to hit here. It would be a small follow-up.
Overlaps — none. #879 touches properties.rs and #803/#806 touch api/flows.rs, but none of them touches the pad-read paths.
Confidence: HIGH
4084a76 to
5d9117c
Compare
srperens
left a comment
There was a problem hiding this comment.
Supersedes my review at 4084a76 (Comment). Re-reviewed at 5d9117c. This head is a rebase onto main with no change to the PR's own work: a two-dot git diff 4084a76 5d9117c on the PR's two files is empty. The verdict moves to Approve. The only blocking row was that the covering test had not run, and it has now run green.
Verdict: Approve — the fix is correct, the guard fails on revert, and the guard and the lifecycle canary both executed green at this head.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Both reads no longer request a pad | CONFIRMED |
backend/src/gst/pipeline/properties.rs:420 — .static_pad(pad_name) and :457. The diff is unchanged since my review of 2026-09-30, where this row was CONFIRMED. |
| No in-repo caller relied on the read creating a pad | CONFIRMED |
CONFIRMED in my review of 2026-09-30. The rebase onto 34008fe/769d969/f4fd0a9 adds no pad-property reader. |
| The test fails on revert | CONFIRMED |
backend/tests/enum_property_read_test.rs:182 — assert_eq!(sinks, vec!["sink_0"], "a read requested pads on the mixer"); |
static_pad finds existing request pads |
EXTERNAL |
gst_element_get_static_pad searches every pad on the element. |
| Covering test executed in CI | CONFIRMED |
Run 36726458748 at 5d9117c, Check (Linux) job 109924763356: test reading_a_missing_pad_does_not_create_it ... ok. All three pipeline_lifecycle_test tests are ok. The log has 46 test result: ok lines and no FAILED. |
The earlier red check — #923 (f4fd0a9) rewrote the AES67 test that had raced the block's connect_element_added handler, and it is in this base. aes67_input_sets_drop_on_latency_on_the_sdpdemux_rtpbin ... ok in the same job.
Radius — LOCAL, unchanged. The small follow-up from my last review still stands: a missing pad answers HTTP 400 where #[utoipa::path] documents 404. That mismatch is older than this PR.
Overlaps — none. #879 still edits backend/src/gst/pipeline/properties.rs, but only in the live-update setter, adding the HTML Input URL path. It does not touch the two pad readers.
Confidence: HIGH
…lper Eyevinn#935 moved build_manager onto common::manager, after Eyevinn#922 added a test that still passed it a registry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GET .../pads/{pad}/properties (and the single-property read) fell back
to request_pad_simple when the pad did not exist, so a read created an
unlinked request pad that stayed on the element for the life of the
flow. A client probing sink_0..sink_10 left six input-less pads on a
live compositor. static_pad already finds request pads that exist, so
a read of a missing pad now returns PadNotFound.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d9117c to
d358ca7
Compare
srperens
left a comment
There was a problem hiding this comment.
This supersedes my review at 5d9117c (Approve, since dismissed). The verdict stands.
Verdict: Approve. The new head d358ca7 amends the single commit on the same base (34008fe). The product change is byte-identical. The only delta is in the guard test, which now builds its PipelineManager inline with fully qualified paths instead of calling build_manager.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Both reads no longer request a pad | CONFIRMED |
CONFIRMED in my reviews of 2026-09-30 and 2026-10-01, and unchanged since: backend/src/gst/pipeline/properties.rs:420 — .static_pad(pad_name), and the same at :457 |
| The test still fails on revert | CONFIRMED |
The assertion is unchanged: backend/tests/enum_property_read_test.rs:194 — assert_eq!(sinks, vec!["sink_0"], "a read requested pads on the mixer"); |
| The inline construction survives #935 in either merge order | CONFIRMED |
#935 drops this file's BlockRegistry, EventBroadcaster and NamedTempFile imports and changes build_manager's signature. The new test uses none of them unqualified: backend/tests/enum_property_read_test.rs:167 — let registry = strom::blocks::BlockRegistry::new(temp_file.path()); |
| Nothing merged since relies on a read creating a pad | CONFIRMED |
The branch is 48 commits behind main, and the trial merge onto origin/main is clean. Main's only change to properties.rs is the HTML Input live-URL setter and a string-read mapping, neither of them a pad reader. No line added on main calls get_pad_property or get_pad_properties |
static_pad finds existing request pads |
EXTERNAL |
Assumes gst_element_get_static_pad searches every pad on the element, as stated before |
Radius. LOCAL, unchanged. The setter still requests a pad (backend/src/gst/pipeline/properties.rs:376 — let pad = if let Some(p) = element.static_pad(pad_name) {, falling back to request_pad_simple). That is the intended asymmetry: only reads became side-effect free.
Overlaps
- #935 (and draft #936, carrying the same hunks) edits
backend/tests/enum_property_read_test.rs. There is a textual conflict in the imports only. The amend was made so that either order compiles, and the third claim row confirms it does. No design decision is involved. - #928 (property endpoints answer 400 where 404 is documented) is the follow-up from my earlier review. It is older than this PR and does not need to land with it.
Tests & CI. At d358ca7 these passed: Check (Linux), Build (Linux x86_64/ARM64), Check & Build (WASM), API Contract Check and Static JS Tests. macOS and Windows were skipping, and nothing here is platform-specific. In Check (Linux) job 110566458159, reading_a_missing_pad_does_not_create_it ... ok and aes67_input_sets_drop_on_latency_on_the_sdpdemux_rtpbin ... ok. All three pipeline_lifecycle_test canaries are ok, and the log has no FAILED or panicked. That run predates main's 48 commits. My only evidence for the merged tree is the static trial merge above.
Confidence: HIGH
Description
GET /api/flows/{id}/elements/{el}/pads/{pad}/propertiesand the single-property read fell back torequest_pad_simplewhen the pad did not exist. A read therefore created an unlinked request pad, which stayed on the element for the life of the flow. On a production rig, one client probingsink_0..sink_10left six input-less pads on the live vision-mixer compositor.static_padalready finds request pads that exist, so the fallback only ever fired for missing pads. The two getters now returnPadNotFoundinstead.update_pad_propertykeeps its fallback, because a write that creates a pad is at least deliberate.Impact is cleanup rather than a live fault. With the same six stray pads added to a running copy of the production flow, a 200 s trial with five publishers and seven viewers showed no frame loss on the GPU compositor. The stray pads were ruled out as the cause of the video stalls investigated alongside this (those are #921).
Callers checked: the compositor editor reads only pads of inputs it already knows about, and Open Live's
getPadis called the same way.Related Issue
None.
Type of Change
Checklist
cargo fmt --allcargo clippy --workspace --all-targets -- -D warnings(pre-commit hook)cargo test --workspace: onlyenum_property_read_testwas runTests run
cargo test --test enum_property_read_test: all 3 pass, including the newreading_a_missing_pad_does_not_create_it.properties.rsrestored, the new test fails and the other two pass.🤖 Generated with Claude Code