Live output retarget: stop a bad destination taking the socket, or the craft, down - #122
Merged
Conversation
The destination fill ran AFTER the open/reuse block and closed the fd when it failed -- including on the REUSE path, where the transport type was unchanged and the socket was working. Nothing here resolves names (the fill is inet_pton only), so `outgoing.server=udp://somehost:5600` took a live output down on EVERY backend that calls this. #280 guarded the CV610 caller. This fixes the function, which is the shared one, so Star6E and Maruko get it too -- and the guard in the CV610 caller stays, because it also covers that backend's ring branch, which never reaches this function at all. Filling a local and committing only on success also makes the whole call all-or-nothing: no path now leaves the destination updated but the socket closed, or the reverse. Device-verified on BOTH SigmaStar backends -- each temporarily moved onto udp:// for the run, since a craft on frame-shm:// refuses a live retarget before this code is reached, then restored: Maruko (.233) Star6E (.232) good retarget ok, packetsSent 10372 -> 11995 ok, 15170 -> 16866 bad hostname URI REFUSED, config unchanged, REFUSED, config packets 13659 -> 14754 unchanged, 18504 -> 19585 retarget again after ok ok The third arm is the one worth having: it shows the socket is not merely still open after a refusal but still usable. Host coverage pins the rest of the contract -- fresh open, same-type reuse keeping the fd, the close+reopen type change, and the destination being left untouched on refusal. Mutation-checked: restoring the old ordering fails cfg_bad_uri_kept_handle, cfg_bad_uri_fd_still_open and cfg_bad_uri_dst_untouched, and nothing else. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng it (#283) The CV610 retarget guard declared a throwaway sockaddr_storage and a socklen_t purely to ask a yes/no question, and threw both away. That is the only place the idiom appears, and it would have to be copied verbatim by any future caller that commits or respawns on a URI without going through output_socket_configure(). output_socket_destination_is_usable() gives it a name and a home next to the fill it wraps. Ring URIs answer 0 -- they carry no sockaddr, and their validity belongs to whoever creates the ring. No behaviour change; this is the same call the guard already made. Device-verified on the CV610 bench anyway, because the call site moved: bad hostname URI refused, config still frame-shm://venc_frame, frames 3523 -> 3833, pid 1054 unchanged (no respawn) good ring URI still accepted -- the helper rejects ring types, so this arm is what shows the caller's type check still lets them through Host coverage: good udp, good unix, bad host, NULL, and a ring URI. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured on all three benches: one unusable `outgoing.server` in the config brought the whole pipeline up -- sensor, ISP, VPSS, encoder, JPEG, sidecar, even the HTTP server on Maruko -- and then tore all of it down and exited. No video AND no API, so correcting a one-line config error needed ssh, which a craft in the field does not have. The API can no longer persist such a URI (#280, #282), but a config written by an older build, a hand edit or a restore still can, and the failure is total. Now the output comes up inert instead: loud ERROR naming the URI, socket left unopened, daemon runs, API reachable. `outgoing.server` is live-settable, so the operator fixes it remotely -- verified end to end on all three, from the inert state to packets flowing, with no restart: cv610 active:false -> set udp:// -> active:true, packetsSent 4892 -> 8521 star6e active:false -> set udp:// -> active:true, packetsSent 2273 -> 3894 maruko active:false -> set udp:// -> active:true, packetsSent 7092 -> 12427 `active:false` matters as much as the recovery: the inert state is reported honestly rather than as a live output with silent drops. The asymmetry with the live retarget is deliberate. A retarget still REFUSES a bad URI, because there the craft has a working output to lose; at boot it has none either way. Placement: the policy sits in each backend's startup orchestration, not in the output modules. star6e_output_init() and maruko_output_init() still return -1 for a URI they cannot use -- that is their contract, and a test already pinned it. The callers tear the output down (which leaves socket_handle -1 and the destination zeroed) and carry on. Every send path already refuses a negative handle, so frames are counted as drops. Maruko needed one more change, and only the bench found it: maruko_pipeline_run() refused to start at all without an output, so the pipeline came up, the API came up, and then the run loop returned -1 and the daemon exited anyway. Star6E and CV610 have no such precondition. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoProtect live output and keep pipelines reachable on invalid destinations
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Type retarget reopens live descriptor
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+2063
to
+2065
| /* Type change still closes and reopens. */ | ||
| CHECK("cfg_type_change", output_socket_configure(&handle, &dst, &dst_len, | ||
| &transport, &unix_uri, 1, 0, &connected) == 0); |
There was a problem hiding this comment.
1. Type retarget reopens live descriptor 📎 Requirement gap ☼ Reliability
The new test explicitly preserves in-place close-and-reopen behavior for UDP↔Unix retargets. Producers can consequently send through a closed or newly reused descriptor while completing an in-progress frame.
Agent Prompt
## Issue description
Live UDP↔Unix retargets close and reopen a descriptor that producer threads may still be using.
## Issue Context
Commit the requested configuration and report reinitialization as pending, but defer descriptor replacement to the backend restart/reinitialization path. Apply the same behavior across all three backends.
## Fix Focus Areas
- src/output_socket.c[225-253]
- src/cv610_runtime.c[1270-1290]
- src/star6e_output.c[1163-1199]
- src/maruko_output.c[225-263]
- tests/test_star6e_output.c[2063-2067]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+233
to
+234
| *dst = new_dst; | ||
| *dst_len = new_len; |
There was a problem hiding this comment.
2. Connected udp can split frames 📎 Requirement gap ☼ Reliability
The retarget path commits a new destination and then calls connect() on the existing connected-UDP socket without synchronizing with an access-unit send. Datagrams from one access unit can therefore be divided between the old and new receivers.
Agent Prompt
## Issue description
Connected-UDP retargeting changes the kernel destination while an access unit may still be in progress.
## Issue Context
Ensure every datagram of an access unit uses one destination, either by moving destination selection into the per-frame snapshot or by avoiding connected UDP for retargetable outputs.
## Fix Focus Areas
- src/output_socket.c[233-253]
- src/output_socket.c[293-300]
- src/cv610_runtime.c[1464-1513]
- src/star6e_output.c[731-871]
- src/maruko_output.c[276-449]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 task
The review caught a real one in the boot-path change, and it is worse on Star6E than reported. Both teardowns reset transport POLICY along with transport state -- Maruko clears allow_unix_encoder_stall, Star6E clears that AND requested_connected_udp -- and the live retarget reads those fields back out of the output. So a craft that started inert and was then recovered by the operator would silently come up with a DIFFERENT policy than the config asks for: unconnected UDP on Star6E, and no Unix stall allowance on either. Nothing would have reported it. Both are now re-seeded from the same source init reads. Also from the review: the new startup behaviour was undocumented, and the release metadata was not bumped. VERSION 0.82.0, HISTORY entry, and the contract now describes what a craft does with a destination it cannot bring up -- non-fatal at boot with the output inert and active:false, refused on a live change, with the asymmetry explained. contract_version stays 0.29.0: no field, endpoint or payload changed, only behaviour. Not taken, and already tracked in OpenIPC#121: the type-retarget descriptor reopen and the connected-UDP frame split. Both are bounded to one dropped or split access unit during an operator-initiated retarget, and the fix for the first changes observable API behaviour, so doing it on one backend diverges it from the other two. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three commits, follow-up to #120, closing the shared half of #121 plus a worse failure found while testing it. All device-verified on CV610, Star6E and Maruko.
1.
output_socket_configure()must not destroy a working socketThe destination fill ran after the socket open/reuse and closed the fd when it failed — including on the reuse path, where the transport type was unchanged and the socket was working. Nothing here resolves names (the fill is
inet_ptononly), sooutgoing.server=udp://somehost:5600took a live output down on every backend that calls this.Filling a local and committing only on success also makes the call all-or-nothing: no path now leaves the destination updated but the socket closed, or the reverse.
The third arm shows the socket is not merely still open after a refusal, but still usable.
2. A shared helper for the question, instead of open-coding it
output_socket_destination_is_usable()replaces a throwawaysockaddr_storagedeclared purely to ask a yes/no question. Ring URIs answer 0 — they carry no sockaddr, and their validity belongs to whoever creates the ring. It exists for callers that commit or respawn on a URI without reachingconfigure(), which is the class of bug in (3).3. A bad
outgoing.servermust not brick the craft at bootMeasured on all three: one unusable URI in the config brought the whole pipeline up — sensor, ISP, VPSS, encoder, JPEG, sidecar, even the HTTP server on Maruko — then tore it all down and exited. No video and no API, so fixing a one-line config error needed ssh, which a craft in the field does not have.
The output now comes up inert: loud ERROR, socket unopened, daemon runs, API reachable. Recovery verified end to end, no restart:
active:falseactive:true, packetsSent 4892 → 8521active:falseactive:true, packetsSent 2273 → 3894active:falseactive:true, packetsSent 7092 → 12427active:falsematters as much as the recovery: the inert state is reported honestly rather than as a live output silently dropping every frame.A live retarget still refuses a bad URI — there the craft has a working output to lose. At boot it has none either way.
The policy sits in each backend's startup orchestration, not in the output modules:
star6e_output_init()andmaruko_output_init()still return-1for a URI they cannot use, which is their contract and which an existing test already pinned. Maruko needed one extra change that only the bench found —maruko_pipeline_run()refused to start without an output, so the daemon exited anyway despite the rest working.Still open in #121
Gap A (
connectedUdpputs the destination in the kernel socket, outside the seqlock snapshot) and Gap B (a live transport-type change closes and reopens the fd). Both are bounded to a single dropped or split access unit during an operator-initiated retarget. Gap B has a clean fix, but it changes observable API behaviour, so doing it on one backend diverges it from the other two — that is the decision the issue holds.Validation
make buildclean on all three backends;make verify;make test-werror0 failurestest_output_socket_configure_is_all_or_nothing— fresh open, same-type reuse keeping the fd, close+reopen type change, destination untouched on refusal, plus the helper. Mutation-checked: the old ordering fails exactly three arms🤖 Generated with Claude Code
https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB