Fix detection of go2rtc process death and clarify logging#378
Fix detection of go2rtc process death and clarify logging#378matteius merged 1 commit intoopensensor:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves robustness of the embedded go2rtc process management by tightening executable discovery/validation and improving detection of unexpected process termination during startup.
Changes:
- Make PATH resolution stricter by requiring the candidate to be a regular file (not a directory) and returning an empty result when not found.
- Clarify logging for the “not found in PATH” case.
- Reap zombie children and re-check liveness during the API readiness loop to avoid treating a dead
go2rtcas running.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dpw13
force-pushed
the
fix/go2rtc-forking
branch
from
4a0e121 to
8493848
Compare
|
Good to re-review! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/video/go2rtc/go2rtc_process.c
Outdated
| // Reap any existing zombie children first | ||
| reap_zombie_children(); | ||
| reap_zombie_children(WAIT_ANY); | ||
|
|
There was a problem hiding this comment.
reap_zombie_children(WAIT_ANY) calls waitpid(-1, …) and will reap all exited children for the entire program, not just go2rtc. This can still interfere with other subsystems that expect to waitpid() their own child processes. Prefer reaping only the go2rtc PID(s) you created/managed here, or remove this global reap and rely on the targeted waits already used in the kill/wait flow.
There was a problem hiding this comment.
I'm making a judgement call to leave this specific instance in, as it's what the original code was doing. If I get a chance to review the shutdown of other subprocesses and threads, I may revisit this.
* clarify logging around binary not found * check that go2rtc binary is a real file before execl() * consolidate signalling processes and reaping children
dpw13
force-pushed
the
fix/go2rtc-forking
branch
from
8493848 to
9456ee5
Compare
|
🔥 |
This PR fixes a few issues:
killworks fine as long as you're reaping the child process beforehand. That wasn't being done so a zombiego2rtcprocess was considered alive../build/Release/bin/lightnvrfrom the root of the repo andgo2rtcis not installed, the current code finds thego2rtcdirectory and thinks that's a working executable. Check that the resolvedgo2rtcis a file and not a directory or something else.go2rtcinPATHseemed to indicate that it was found even when it wasn't. Clean up that code path and clarify the logging.go2rtclives long enough to pass the first check but dies after that, the code will continue attempting to connect even thoughgo2rtcis dead. Check that the process is still running in the reconnect loop.