Skip to content

Fix orphaned zms processes accumulating on Montage page tab hide/show#4706

Open
Simpler1 wants to merge 2 commits intoZoneMinder:masterfrom
Simpler1:patch-5
Open

Fix orphaned zms processes accumulating on Montage page tab hide/show#4706
Simpler1 wants to merge 2 commits intoZoneMinder:masterfrom
Simpler1:patch-5

Conversation

@Simpler1
Copy link
Copy Markdown
Contributor

When the Montage page tab is hidden for more than 15 seconds, the onvisibilitychange handler calls monitors[i].stop() on all active streams. When the tab becomes visible again, startVisibleMonitors() detects monitor.started === false and calls start(), spawning new zms processes with new connKey values.

The problem is that stop() sends CMD_STOP to the ZoneMinder API, which does not terminate the zms process. The old zms process continues running with no client consuming its output, no timeout to kill it, and no mechanism to ever clean it up. Each hide/show cycle leaves 3 more orphaned zms processes (one per monitor).

Over time these accumulate until the server exhausts resources, at which point new zms requests begin returning 504 Gateway Timeout errors and the Montage page stops streaming entirely.

This is particularly impactful for users who run the Montage page continuously (e.g. for alarm sound notifications), where the browser's tab visibility API will hide/show the tab frequently due to normal computer use.

Simpler1 and others added 2 commits March 12, 2026 12:10
When the Montage page tab is hidden for more than 15 seconds, the onvisibilitychange handler calls monitors[i].stop() on all active streams. When the tab becomes visible again, startVisibleMonitors() detects monitor.started === false and calls start(), spawning new zms processes with new connKey values.

The problem is that stop() sends CMD_STOP to the ZoneMinder API, which does not terminate the zms process. The old zms process continues running with no client consuming its output, no timeout to kill it, and no mechanism to ever clean it up. Each hide/show cycle leaves 3 more orphaned zms processes (one per monitor).

Over time these accumulate until the server exhausts resources, at which point new zms requests begin returning 504 Gateway Timeout errors and the Montage page stops streaming entirely.

This is particularly impactful for users who run the Montage page continuously (e.g. for alarm sound notifications), where the browser's tab visibility API will hide/show the tab frequently due to normal computer use.
// Use kill() instead of stop() to send CMD_QUIT and terminate the zms
// process. stop() only sends CMD_STOP which leaves zms running, causing
// orphaned processes to accumulate each time the tab is hidden/shown.
monitorStream.kill();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You didn't read my comment above. hide/show shouldn't kill zms. If you kill it then everything stops. No audio alerts, etc. We just want to stop/pause video streaming.

Alternatively we could switch to monitor status polling instead of working through zms..

@connortechnology
Copy link
Copy Markdown
Member

The issue lies elsewhere. stop() should not alter connKey so that we can later send a play and can continue to poll for monitor status updates.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses orphaned zms processes created by tab hide/show cycles by switching the tab-visibility shutdown behavior from stop() (CMD_STOP) to kill() (CMD_QUIT) in classic-skin views, so zms terminates instead of lingering without a consumer.

Changes:

  • Update Zone view tab-visibility handler to call monitors[i].kill() after 15s hidden.
  • Update Watch view tab-visibility handler to call monitorStream.kill() when hidden while playing.
  • Update Montage view tab-visibility handler to call monitors[i].kill() after 15s hidden.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
web/skins/classic/views/js/zone.js Use kill() on tab hidden timeout instead of stop() to prevent orphaned zms processes.
web/skins/classic/views/js/watch.js Use kill() on tab hidden timeout instead of stop() to prevent orphaned zms processes.
web/skins/classic/views/js/montage.js Use kill() on tab hidden timeout instead of stop() to prevent orphaned zms processes.
Comments suppressed due to low confidence (1)

web/skins/classic/views/js/zone.js:807

  • TimerHideShow is not cleared when the tab becomes visible again. If the user hides the tab for <15s and then returns, the pending timeout can still fire and kill the monitors while the page is visible. Clear/cancel TimerHideShow in the visible branch before restarting streams (consistent with the pattern used in other views).
  } else {
    //Start monitors when show page
    for (let i = 0, length = monitorData.length; i < length; i++) {
      if (!monitors[i].started) {
        monitors[i].start();
      }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +796 to +798
// Use kill() instead of stop() to send CMD_QUIT and terminate the zms
// process. stop() only sends CMD_STOP which leaves zms running, causing
// orphaned processes to accumulate each time the tab is hidden/shown.
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

This visibility handler now calls kill(), but MonitorStream.kill() currently sets started=false before calling stop(), which makes stop() return early and skip its normal cleanup (e.g., clearing intervals/resetting activePlayer). Since this code relies on restarting monitors after the tab becomes visible, consider updating kill()/stop() so kill() performs full local cleanup while still avoiding sending CMD_STOP after CMD_QUIT.

Suggested change
// Use kill() instead of stop() to send CMD_QUIT and terminate the zms
// process. stop() only sends CMD_STOP which leaves zms running, causing
// orphaned processes to accumulate each time the tab is hidden/shown.
// First perform a normal stop to run local cleanup (clear intervals,
// reset activePlayer, etc.) and send CMD_STOP, then call kill() to
// send CMD_QUIT and terminate the zms process. This avoids sending
// CMD_STOP after CMD_QUIT while still ensuring full cleanup.
if (monitors[i].started) {
monitors[i].stop();
}

Copilot uses AI. Check for mistakes.
Comment on lines 1046 to +1049
//closing should kill, hiding should stop/pause
for (let i = 0, length = monitors.length; i < length; i++) {
// Stop instead of pause because we don't want buffering in zms
monitors[i].stop();
monitors[i].kill();
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The inline comment says “closing should kill, hiding should stop/pause”, but the hidden-tab timeout now calls kill() (not stop/pause). Please update the comment to match the current behavior (and rationale) so future changes don’t reintroduce the issue.

Copilot uses AI. Check for mistakes.
for (let i = 0, length = monitors.length; i < length; i++) {
// Stop instead of pause because we don't want buffering in zms
monitors[i].stop();
monitors[i].kill();
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Switching to kill() here has the same cleanup concern as in other views: MonitorStream.kill() sets started=false before calling stop(), causing stop() to return early and skip clearing timers/resetting activePlayer. Because Montage restarts streams on visibility (via startVisibleMonitors()), this can leave stale state and interfere with a clean restart. Consider adjusting kill()/stop() so kill() both sends CMD_QUIT and still executes the full local teardown path.

Suggested change
monitors[i].kill();
monitors[i].stop();

Copilot uses AI. Check for mistakes.
Comment on lines +1405 to +1407
// Use kill() instead of stop() to send CMD_QUIT and terminate the zms
// process. stop() only sends CMD_STOP which leaves zms running, causing
// orphaned processes to accumulate each time the tab is hidden/shown.
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Using kill() here relies on MonitorStream.kill() performing a full teardown so monitorStream.start() works correctly when the tab becomes visible again. Currently kill() sets started=false before calling stop(), and stop() returns early when started is false, which skips parts of the normal cleanup (timers/activePlayer). Consider fixing kill()/stop() so the QUIT path still runs the complete local cleanup needed for reliable hide/show restart.

Suggested change
// Use kill() instead of stop() to send CMD_QUIT and terminate the zms
// process. stop() only sends CMD_STOP which leaves zms running, causing
// orphaned processes to accumulate each time the tab is hidden/shown.
// First run the normal stop() cleanup while started is true, then use kill()
// to send CMD_QUIT and terminate the zms process. stop() only sends CMD_STOP
// which leaves zms running, causing orphaned processes to accumulate each time
// the tab is hidden/shown if we don't also call kill().
monitorStream.stop();

Copilot uses AI. Check for mistakes.
@IgorA100
Copy link
Copy Markdown
Contributor

stop() should not alter connKey

stop() doesn't change connKey
In MonitorStream.js, when starting a stream, there's a condition called if (this.activePlayer == 'zms') {, but at that point, activePlayer will be empty because we haven't specified it anywhere. We only end up in this place where the above condition is located if we couldn't previously determine which player will be used.
I'm still not entirely clear on these conditions:

    if (this.activePlayer == 'zms') {
...
    } else if (-1 != stream.src.indexOf('mode=paused')) {
...
    } else {
...
    }

@IgorA100
Copy link
Copy Markdown
Contributor

If you replace
if (this.started) this.streamCommand(CMD_STOP);
with
stream.src = stream.src.replace(/mode=jpeg/i, 'mode=single');
in MonitorStream.js, the ZMS process is killed and then, in start(), it is started with a new ConKey.
But I don't know why Isaac decided to use streamCommand(CMD_STOP) instead of mode=single. Perhaps this is to prevent the ZMS process from being killed?

P.S. The check for if (this.started) is unnecessary, since we already have a similar check earlier in the code.
P.S. 2 I tried to execute CMD_PLAY after CMD_STOP without changing ConKey, but the video stream does not start and a new ZMS process is created.

@connortechnology
Copy link
Copy Markdown
Member

The idea is: pause stops play, but buffers in ram so you can step backwards, etc. It is meant to be short term pause. STOP stops play, but does not buffer. It also does not exit, because it is assumed that there will be a subsequent. command. It should also continue to send keep alive frames to keep the stream open. QUIT means quit outright. The reason I don't like editing the url, is that it kills the image stream (hopefully). If zms doesn't die immediately, and we don't change the connkey, the new stream will hang. So we need a new connKey. The issue is really just latency. At the end of the day, we are polishing a turd. We need to move to a websocket based player.

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.

4 participants