Skip to content

fix(api): reading a pad's properties no longer requests the pad - #922

Open
wagenet wants to merge 1 commit into
Eyevinn:mainfrom
wagenet:wagenet/pad-get-no-request
Open

wagenet wants to merge 1 commit into
Eyevinn:mainfrom
wagenet:wagenet/pad-get-no-request

Conversation

@wagenet

@wagenet wagenet commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Description

GET /api/flows/{id}/elements/{el}/pads/{pad}/properties and the single-property read fell back to request_pad_simple when 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 probing sink_0..sink_10 left six input-less pads on the live vision-mixer compositor.

static_pad already finds request pads that exist, so the fallback only ever fired for missing pads. The two getters now return PadNotFound instead. update_pad_property keeps 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 getPad is called the same way.

Related Issue

None.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have run cargo fmt --all
  • I have run cargo clippy --workspace --all-targets -- -D warnings (pre-commit hook)
  • I have run cargo test --workspace: only enum_property_read_test was run
  • I have updated documentation as needed (none needed)
  • I have added tests for new functionality

Tests run

  • cargo test --test enum_property_read_test: all 3 pass, including the new reading_a_missing_pad_does_not_create_it.
  • Revert check: with properties.rs restored, the new test fails and the other two pass.

🤖 Generated with Claude Code

@wagenet
wagenet marked this pull request as ready for review September 30, 2026 05:18

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. None to the diff. A maintainer re-runs the failed job: gh run rerun 36672596978 --failed. What is needed is a log line reading_a_missing_pad_does_not_create_it ... ok. At this head the log has no line from enum_property_read_test at 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

srperens
srperens previously approved these changes Oct 1, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

wagenet added a commit to wagenet/strom that referenced this pull request Oct 1, 2026
…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>
@wagenet
wagenet force-pushed the wagenet/pad-get-no-request branch from 5d9117c to d358ca7 Compare October 1, 2026 20:20
@srperens srperens mentioned this pull request Oct 6, 2026
3 of 9 tasks

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants