fix: stop orphaned zms processes accumulating, without killing streams on tab hide - #5120
fix: stop orphaned zms processes accumulating, without killing streams on tab hide#5120connortechnology wants to merge 3 commits into
Conversation
A zms in the stopped state could never exit. The branch handling it slept and continued without writing anything, so the socket was never touched and a client that had gone away was never discovered - no write, no EPIPE, no SIGPIPE. The continue also skipped the ttl check at the bottom of the loop, so even a deadline set by the caller did not apply. The only way out was an explicit CMD_QUIT, and any stream that missed one stayed until the machine was restarted. That is what accumulates on the montage page: the tab is hidden, the browser side sends CMD_STOP, and if the stream is later replaced rather than resumed, nothing can address the old process again and it sleeps forever. Hold the last captured frame on entering the stopped state and re-send it every five seconds, the way the paused state already does, and apply ttl here too. The frame is what makes a departed client detectable; it also keeps the connection open for the resume that stopped is meant to allow, which is what the state was documented to do. Where nothing has been captured yet there is no frame to hold, so a text frame is sent instead - the write matters more than what is in it. setLastViewed is deliberately still not called: capture and decoding should not be held active for a stream that is not playing. refs #4706 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
select_zms() mints a fresh connkey whenever it has to rebuild the stream src, without telling the process the old connkey belonged to that it is finished. Once the key is replaced nothing can reach that process again: no CMD_QUIT can be delivered, and a stopped one will not notice on its own. getStreamCmdResponse() already learned this - its reload path calls quitConnKey() first, with a comment saying why - but the path every ordinary start() takes did not. quitConnKey had exactly two references in the file: its own definition and that one use. refs #4706 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b04b8eb to
0363e90
Compare
|
Tested on a live instance and dropped one of the three commits as a result. Before/after, two monitors on a montage page, one hide/show cycle each time:
The keepalive works as intended — What I got wrong: the third commit made hide/show resume the stopped @IgorA100 said exactly this on #4706 and I dismissed it as the client re-creating the stream. It wasn't. Resuming on an existing connkey needs the command delivery path investigated first, which is a separate piece of work and not a |
select_zms() has a branch that resumes an existing zms with CMD_PLAY rather than rebuilding the stream, but three things stopped it ever working. Every one of them showed up only when the page was driven for real. stop() ends by clearing activePlayer, which is the only thing that branch tests, so after any stop it was unreachable and start() fell through to "new src, new connkey" - a second zms, the first left running and no longer addressable. That is the montage page's accumulating processes: hide the tab, the handler stops each stream, and returning replaces rather than resumes. The same happens when a monitor is scrolled out of view and back. stop() now remembers what it shut down, and select_zms() resumes on that as well as on activePlayer, provided we still hold the connkey to address it. srcAuthCurrent required a non-empty zmAuth.hash, which is '' whenever auth is off or the relay carries no hash. There is nothing that can go stale in that case, so the src is as current as it will ever be; requiring a hash sent every such install down the rebuild path for no reason. streamCommand() drops anything sent while !started, and started is not set until the end of select_zms(), so the resume issued its CMD_PLAY into nothing and the stream stayed stopped. Resuming after a pause worked only because pause() leaves started set. It is now set before the command goes out. Finally, the rebuild path clears the "Loading..." info block from img_onload, which cannot fire on a resume because src never changes. Without clearing it, a stream that had in fact resumed sat behind that block and its still image and looked frozen - the fault that made this look unfixable at first. restart() is excluded from all of it: it is the error path, whatever failed may be that very zms, and a broken img is not repaired by CMD_PLAY. Verified on a live montage page with two monitors: across two hide/show cycles the same two zms processes are kept - no orphans, no respawn - and both pictures are live afterwards, with the zms status reporting stopped=0 and ~15 fps. refs #4706 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Update: the resume commit is back, and now actually works. My earlier comment said resuming a stopped Three things were preventing the
With those fixed the stream status came back Live result on a 2-monitor montage page, two hide/show cycles:
So hide/show now costs nothing to return from: no new process, no restart latency, and the status polling that drives alarm sounds is never interrupted. @IgorA100 — your observation on #4706 that |
Fixes the orphaned
zmsaccumulation reported in #4706, without killing streams on tab hide — which @connortechnology ruled out there, since killing takes the status polling and alarm sounds down with it.Reproduced and verified on a live instance. Two monitors on a montage page, one simulated tab hide/show cycle each time:
zms+ stockMonitorStream.jsThe pre-fix run is exactly the accumulation #4706 describes. With the branch the process ids do not change at all across the cycles, and both pictures are live afterwards.
The two defects
A stopped
zmscould never exit (src/zm_monitorstream.cpp:670). The stopped branch slept andcontinued without writing anything, so the socket was never touched and a client that had gone away was never discovered: no write, noEPIPE, noSIGPIPE. Thecontinuealso skipped thettlcheck at the bottom of the loop, so even a caller's deadline did not apply. The only way out was an explicitCMD_QUIT, and a stream that missed one stayed until the machine was restarted. This is why they accumulate rather than merely appear.The last captured frame is now held on entering the stopped state and re-sent every five seconds, as the paused state already does, and
ttlapplies here too. Confirmed in the logs of a running stream:setLastViewedis deliberately still not called: capture and decoding should not be held active for a stream that is not playing.select_zms()replaced the connkey without quitting the process it addressed (web/js/MonitorStream.js). Once the key is gone nothing can reach thatzmsagain, so noCMD_QUITcan ever be delivered to it.getStreamCmdResponse()already learned this — its reload path callsquitConnKey()first, with a comment saying exactly why — but the path every ordinarystart()takes did not.quitConnKeyhad precisely two references in the file: its own definition and that one use.Resuming instead of rebuilding
The third commit makes the hide/show path resume the stopped
zmswithCMD_PLAYrather than build a new one — whatCMD_STOPis designed for, and what the discussion on #4706 assumed should work. It did not, for three separate reasons, none of which were visible from reading:stop()clearsactivePlayer, which is the only thing the resume branch tests, so after a stop it was unreachable.srcAuthCurrentrequired a non-emptyzmAuth.hash, which is''whenever auth is off — so those installs always rebuilt, for no reason, since there is no hash that can go stale.streamCommand()drops anything sent while!started, andstartedis not set until the end ofselect_zms(). The resume issued itsCMD_PLAYinto nothing. Resuming after a pause worked only becausepause()leavesstartedset.With those fixed the
zmsstatus reportsstopped: 0and ~15 fps — and the picture still looked frozen, because the rebuild path clears the"Loading..."info block fromimg_onload, which cannot fire whensrcnever changes. The resumed stream was playing underneath its own overlay. Clearing the block on the resume path is the last piece.@IgorA100 reported on #4706 that a
CMD_PLAYafterCMD_STOPdoes not restart the video. That was correct, and points 3 and 4 above are why.Testing
zmsbuilds clean, no new warnings; full C++ suite 138/138 pass; ESLint clean onMonitorStream.js.zmsdebug logs.stopped: 0and ~15 fps from the stream status query, and the burnt-in camera timestamp advancing in the browser afterwards.Relationship to #4706
Supersedes it. Same diagnosis of the symptom, different mechanism: nothing is killed on tab hide, and the process that does get replaced is now told to quit rather than left behind.
🤖 Generated with Claude Code