agent: address a USB camera by its identity, not its boot order - #1853
agent: address a USB camera by its identity, not its boot order#1853chrisdok43 wants to merge 3 commits into
Conversation
A camera's /dev/videoN number is assigned in USB enumeration order, so it is a
property of the boot rather than of the camera. A reboot re-ordered the two
cameras on one of our devices and every caller that had pinned a number was
then addressing the other one.
The failure mode is why this is worth fixing rather than documenting: opening
the wrong camera SUCCEEDS. It streamed a valid picture from the wrong sensor,
reported no error anywhere, and went unnoticed for hours precisely because
nothing looked broken.
ListVideoDevices offered no way out. `id` and `path` are the enumeration order;
`name` and `driver` identify the MODEL, so they cannot separate two cameras of
the same type -- which is a normal install, not a corner case.
VideoDevice already solves this for network cameras (`mac`, "stable identity").
This gives local cameras the equivalent, from the two names udev already
publishes:
by_id vendor + product + serial survives a reboot AND a move to another
port; collides only for two same-model
cameras sharing a factory serial
by_path the USB port topology survives a reboot but NOT a move; the
only thing that separates identical
cameras
Both are reported so a caller can pick the property it needs. Only by_id is
resolvable in StreamVideo: pinning a stream to a physical port is a decision a
caller should make deliberately rather than inherit.
StreamVideoRequest.device_by_id wins over device_id and is resolved at REQUEST
time, which is the point -- the number is looked up when the stream opens
instead of being baked into a config that outlives the boot it was written on.
Three deliberate choices, each with a test:
* a name that matches nothing RAISES rather than falling back to device_id.
Falling back would stream whatever the kernel put at that number and tell
nobody, which is the exact bug this removes. The error lists the by-ids the
agent can actually see, because the caller may not be able to log into the
device to find out.
* matching is EXACT, not a prefix. "..._SN1-video-index0" must not match
"..._SN10-video-index0" and hand back one of them silently.
* a missing /dev/v4l is not an error. CSI and network cameras have no entry,
and losing the enrichment beats failing enumeration -- the numbers still
work, they are simply not stable.
Absent device_by_id, behaviour is unchanged; a device with no /dev/v4l
enumerates exactly as before, which is pinned by its own test.
The tests reproduce the incident rather than the helper: the same two real
camera names resolve correctly both before and after the enumeration swap.
Reintroducing the silent fallback fails TestStreamDeviceIDRefusesAnUnknownName.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KmLkEMDTdpMPuCPmRZs6px
AI Security ReviewNote Automated security review from Claude. Apply, adapt, silence with Input coverage: 7/7 changed files; 32,878/32,878 bytes reviewed; diff SHA-256 No security findings. |
`--by-id` takes the name `camera list` now reports and sends it as
StreamVideoRequest.device_by_id, which the agent resolves at request time.
Without it the new field is unreachable from the CLI, so the identity is
reported and cannot be used.
`--id` keeps working and its help now says what it actually is -- the boot
order. Passing both is an error rather than a silent precedence rule: they name
the camera two different ways and picking one for the caller is how you end up
streaming something nobody asked for.
Verified on hardware (jetson-orin-nano, Logitech C920):
--by-id <the camera> 5.76 MB of H.264
--by-id <absent cam> 0 bytes, and the error names the two by-ids the agent
can see rather than falling back to device 0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KmLkEMDTdpMPuCPmRZs6px
Verified end to end on hardwareTested on a jetson-orin-nano ( Before — the premise, reproduced{ "name": "HD Pro Webcam C920",
"path": "/dev/video0",
"transport": 1,
"driver": "uvcvideo" }Nothing there identifies the unit: What udev had all along: After{ "name": "HD Pro Webcam C920",
"path": "/dev/video0",
"transport": 1,
"driver": "uvcvideo",
"by_id": "usb-046d_HD_Pro_Webcam_C920_762B1F5F-video-index0",
"by_path": "platform-3610000.usb-usb-0:2.1:1.0-video-index0" }Note Streaming by nameAnd the case the whole change exists forAsking for a camera that is not plugged in: It refuses and says what is actually there, instead of streaming One addition since the first push
Note for reviewersAn old CLI against a patched agent shows no new fields — the CLI's generated proto has no |
Physical validation: port move + rebootSame device (jetson-orin-nano, Logitech C920). The camera was physically moved to a different USB port, then the box was rebooted (
The practical check: the That second line is the point. The identifier this PR resolves on survived; the one it deliberately does not resolve on broke exactly as the proto comment claims. That was an assertion when I wrote it and is now measured. What this does not showIt does not reproduce the original outage. With a single camera the numbering never shuffles, so Reproducing the swap needs a second camera on the same box so a reboot has something to race with. Worth doing if someone has a spare, but the design property it would exercise is the one already shown above. |
#1817 landed and touches the same files. Three conflicts, all additive on both sides -- the two changes are independent and simply arrived at the same lines: * StreamVideoRequest: #1817 takes field 5 (`VideoCodec codec`), this branch takes field 6 (`device_by_id`). No collision; field 6 was chosen when this branch opened precisely because #1817 had already claimed 5. * camera.go: #1817 adds `raw` / `Codec`, this branch adds `byID` / `DeviceById`. Both kept. * The generated .pb.go was regenerated from the merged proto rather than hand-resolved, and the unrelated version-comment churn `make proto` produces was reverted so the diff stays to the video service. Verified: gofmt, go vet, go build ./go/... clean; agent/services and cli/commands both pass, including #1817's raw-frame tests; the 12 tests added by this branch still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmLkEMDTdpMPuCPmRZs6px
Swift E2E ReviewNo Swift E2E review issues were generated for this run. Report artifact: swift-e2e-tests.gh33589683864.run.0001 |
The failure reproduced on hardware, with two identical camerasEarlier testing used one camera, which can never renumber — so the original outage stayed unreproduced. A second camera has since been added to the bench box, and it is the hard case: two Logitech C920s, same model, different serials. Everything a caller could match on is identical: One of them also reports no Then the cameras were replugged in a different orderNothing moved physically — both stayed in the same USB sockets ( The numbers traded places. And the consequence, measured: A config pinned to Every scheme has now failed except by_id
And it settles the same-model questionThis box cannot be served by matching on the sysfs |
Closes WDY-2774.
The problem
A camera's
/dev/videoNnumber is assigned in USB enumeration order, so it belongs to the boot, not to the camera. A reboot re-ordered the two cameras on one of our devices, and every caller that had pinned a number was then addressing the other one.The failure mode is what makes this worth fixing rather than documenting: opening the wrong camera succeeds. It streamed a valid picture from the wrong sensor, reported no error anywhere, and went unnoticed for hours precisely because nothing looked broken.
ListVideoDevicesoffered no way out:id,pathname,driverTwo matching cameras on one box is a normal install, not a corner case.
The change
VideoDevicealready solves this for network cameras:string mac = 9; // stable identity for network cameras; empty for local camerasThis gives local cameras the equivalent, from the two names udev already publishes:
Both are reported so a caller can pick the property it needs. Only
by_idis resolvable inStreamVideo— pinning a stream to a physical port is a decision a caller should make deliberately rather than inherit.StreamVideoRequest.device_by_idwins overdevice_idand is resolved at request time. That's the point: the number is looked up when the stream opens, instead of being baked into a config that outlives the boot it was written on.Three deliberate choices, each with a test
device_id. Falling back would stream whatever the kernel put at that number and tell nobody — the exact bug this removes. The error lists the by-ids the agent can actually see, because the caller may not be able to log into the device to find out...._SN1-video-index0must not match..._SN10-video-index0and silently hand back one of them./dev/v4lis not an error. CSI and network cameras have no entry, and losing the enrichment beats failing enumeration — the numbers still work, they're just not stable.Scope
Absent
device_by_id, behaviour is unchanged. A device with no/dev/v4lenumerates exactly as before, pinned by its own test.Tests
They reproduce the incident, not just the helper — the same two real camera names from the affected device resolve correctly both before and after the enumeration swap:
Plus: by-path reported separately, missing directories, a dangling symlink not costing healthy cameras their identity, exact-vs-prefix matching, and the unchanged path.
Reintroducing the silent fallback fails
TestStreamDeviceIDRefusesAnUnknownName; I checked.Verification
gofmtclean,go vetclean,go build ./go/...cleango test ./go/internal/agent/services/— passes, 12 new testsgo test ./go/...— 82 packages ok. Two failures, both pre-existing and environmental, verified by running them on cleanmain:TestBuildCmd_MultiService_BuildsFromManifestOnly— fails onmaintoo (macOS/private/varvs/varsymlink)TestSampleGPUFallsBackToTegrastatsForOrin— passes 4/4 in isolation; only flakes under full-suite load, and this change doesn't touchhoststatsprotocis v7.35.1, the repo's committed output is v7.34.0), so I reverted the unrelated ones. Worth noting the repo's generated headers are already mixed —v5.28.3,v7.34.0,v1.36.11-develall appear.Not in this PR
A
by-path-based resolve for the same-model/shared-serial case. Reporting it is free and informative; resolving by it needs a policy call about pinning streams to ports.🤖 Generated with Claude Code
https://claude.ai/code/session_01KmLkEMDTdpMPuCPmRZs6px