fix: stop orphaning zms when a stream command fails refs ZoneMinder#5029

getStreamCmdResponse() responded to every ajax/stream.php failure the same way:
mint a fresh connkey and reload the img src. ajaxError() returns HTTP 200 with
result=Error, so these arrive in jQuery's done() rather than fail(), and all
twelve error paths in stream.php took that branch.

Only one of them means zms is gone. For the rest the process is still running
and streaming, and replacing the connkey makes it unaddressable: CMD_STOP,
CMD_QUIT and mode=single all then go to the new key, so nothing can reach the
old process and only SIGPIPE can stop it, which we know is unreliable. That is
why the reports of lingering zms after switching monitors were unaffected by
changes to what the stop path sends.

The timeout path made this routine rather than rare. On select() expiry
ajaxError is commented out, so the script carries on to socket_recvfrom() on a
now non-blocking socket. That returns false, and false == 0 under switch's loose
comparison, so a merely slow zms was reported as 'No data to read from socket'
and torn down.

stream.php now classifies each failure as no_socket, timeout, transient or
invalid, and sends it as 'reason'. The client restarts the stream only for
no_socket. A missing reason is still treated as fatal, so a php that predates
this keeps the old behaviour.

Before replacing the connkey the client now sends CMD_QUIT to the old one, so
the process we are about to lose track of is asked to exit. That is deliberately
not routed through streamCommand(): it must name its target explicitly, since
this.connKey is about to change, and its response must not feed back into
getStreamCmdResponse(), or a QUIT that also failed would re-enter the error path
and loop.

ajaxError() takes the classification as a third argument, named $reason because
$code is already the HTTP status, and only includes it when set, so the other
131 callers are unaffected.

Tests: tests/js covers the fatal/non-fatal decision including the no-reason
fallback, tests/php pins the classification mapping and the switch(false)
semantics the timeout branch depends on. Both verified to fail when the
behaviour is reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: connortechnology

tests/js/stream-error-reason.test.js

@@ -0,0 +1,63 @@
+'use strict';
+
+const assert = require('assert');
+const path = require('path');
+const ZM = require(path.join(__dirname, '../../web/js/MonitorStream.js'));
+
+let passed = 0;
+let failed = 0;
+function test(name, fn) {
+  try {
+    fn();
+    console.log('  ok ' + name);
+    passed++;
+  } catch (e) {
+    console.error('  FAIL ' + name);
+    console.error('    ' + e.message);
+    failed++;
+  }
+}
+
+console.log('streamErrorIsFatal');
+
+// Only a missing zms should tear the stream down. Restarting replaces the
+// connkey, which makes any zms still running unreachable, so treating a
+// recoverable failure as fatal is what orphaned the process.
+test('no_socket -> fatal, zms really is gone', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal('no_socket'), true);
+});
+
+test('timeout -> not fatal, zms is most likely alive and busy', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal('timeout'), false);
+});
+
+test('transient -> not fatal, the failure was local to php', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal('transient'), false);
+});
+
+test('invalid -> not fatal, restarting cannot fix a bad request', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal('invalid'), false);
+});
+
+// A php that predates the reason field sends no reason at all. Falling back to
+// the old always-restart behaviour keeps a mixed-version install working.
+test('missing reason -> fatal, preserves pre-reason behaviour', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal(undefined), true);
+});
+
+test('null reason -> fatal', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal(null), true);
+});
+
+test('empty reason -> fatal', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal(''), true);
+});
+
+// An unknown reason from a newer php should not be silently treated as fatal:
+// the conservative choice is to leave the stream alone and retry.
+test('unrecognised reason -> not fatal', () => {
+  assert.strictEqual(ZM.streamErrorIsFatal('something_new'), false);
+});
+
+console.log('\n' + passed + ' passed, ' + failed + ' failed');
+process.exit(failed ? 1 : 0);

tests/php/test_stream_error_reason.php

@@ -0,0 +1,86 @@
+<?php
+// Regression test for the failure classification in web/ajax/stream.php.
+//
+// The client restarts a stream only for a 'no_socket' failure, because
+// restarting replaces the connkey and leaves any zms still running unreachable.
+// A failure classified too harshly therefore orphans a live zms process; one
+// classified too leniently leaves a dead stream on screen.
+//
+// This parses the classification out of stream.php rather than executing it:
+// the script talks to unix sockets and a running zms, so it cannot be driven
+// directly. What is worth pinning down is the mapping itself.
+//
+// Run as: php tests/php/test_stream_error_reason.php
+
+$failures = 0;
+$passes = 0;
+
+function check($name, $got, $want) {
+  global $failures, $passes;
+  if ($got === $want) {
+    $passes++;
+    echo "  ok $name\n";
+  } else {
+    $failures++;
+    echo "  FAIL $name\n";
+    echo "    got:  " . var_export($got, true) . "\n";
+    echo "    want: " . var_export($want, true) . "\n";
+  }
+}
+
+$path = __DIR__ . '/../../web/ajax/stream.php';
+$src = file_get_contents($path);
+if ($src === false) {
+  echo "Cannot read $path\n";
+  exit(1);
+}
+
+// Every ajaxError() call must carry a classification, otherwise the client
+// falls back to treating it as fatal and we are back to orphaning zms.
+$calls = preg_match_all('/^\s*ajaxError\(/m', $src, $m);
+$classified = preg_match_all('/STREAM_ERR_[A-Z_]+/', $src, $m2);
+echo "ajaxError classification\n";
+check('every ajaxError call is classified',
+  $calls > 0 && $classified >= $calls, true);
+
+// The four classes the client distinguishes.
+foreach (array('NO_SOCKET' => 'no_socket', 'TIMEOUT' => 'timeout',
+               'TRANSIENT' => 'transient', 'INVALID' => 'invalid') as $const => $value) {
+  check("STREAM_ERR_$const is defined as '$value'",
+    (bool)preg_match("/define\('STREAM_ERR_$const',\s*'$value'\)/", $src), true);
+}
+
+// A missing socket, and a send to a socket with no listener, are the only
+// cases that mean zms is gone. These are the ones that may restart the stream.
+echo "\nfatal classification\n";
+check('missing socket file is no_socket',
+  (bool)preg_match('/does not exist.*?STREAM_ERR_NO_SOCKET/s', $src), true);
+check('socket_sendto failure is no_socket',
+  (bool)preg_match('/socket_sendto\(.*?STREAM_ERR_NO_SOCKET/s', $src), true);
+
+// A timeout must NOT be reported as a generic failure: zms is most likely
+// alive, and restarting it is what orphaned the process.
+echo "\nnon-fatal classification\n";
+check('select timeout is reported as timeout, not transient',
+  (bool)preg_match('/\$select_timed_out\s*\)\s*\{\s*ajaxError\(.*?STREAM_ERR_TIMEOUT/s', $src), true);
+check('socket_create failure is transient',
+  (bool)preg_match('/socket_create\(\).*?STREAM_ERR_TRANSIENT/s', $src), true);
+check('socket_bind failure is transient',
+  (bool)preg_match('/socket_bind\(.*?STREAM_ERR_TRANSIENT/s', $src), true);
+check('bad request is invalid',
+  (bool)preg_match('/No connkey or no command.*?STREAM_ERR_INVALID/s', $src), true);
+
+// socket_recvfrom() returns false on failure, and false == 0 under switch's
+// loose comparison, so a timed-out select lands on `case 0` rather than -1.
+// If this ever stopped holding, the timeout branch would silently never run.
+echo "\nphp switch semantics the timeout branch relies on\n";
+$matched = null;
+switch (false) {
+  case -1: $matched = -1; break;
+  case 0: $matched = 0; break;
+  default: $matched = 'default'; break;
+}
+check('switch(false) matches case 0, not case -1', $matched, 0);
+
+echo "\n$passes passed, $failures failed\n";
+exit($failures ? 1 : 0);

web/ajax/stream.php

@@ -7,8 +7,26 @@
 define('MSG_TIMEOUT', ZM_WEB_AJAX_TIMEOUT/2);
 define('MSG_DATA_SIZE', 4+256);
 
+/* Failure classes reported to the client as 'reason'.  The client needs to
+ * tell "zms is gone, start a new one" apart from "this particular exchange
+ * failed", because restarting the stream replaces the connkey and makes any
+ * still-running zms unreachable.
+ *
+ *   no_socket - zms is not listening: it never started, or it has exited.
+ *               The only class where restarting the stream is the right answer.
+ *   timeout   - the command went out but no reply came back in time.  zms is
+ *               most likely alive and busy.
+ *   transient - a failure local to this php process (socket setup, a short
+ *               read).  Says nothing at all about zms.
+ *   invalid   - bad request or an unexpected reply.  Retrying will not help.
+ */
+define('STREAM_ERR_NO_SOCKET', 'no_socket');
+define('STREAM_ERR_TIMEOUT', 'timeout');
+define('STREAM_ERR_TRANSIENT', 'transient');
+define('STREAM_ERR_INVALID', 'invalid');
+
 if ( !($_REQUEST['connkey'] && $_REQUEST['command']) ) {
-  ajaxError('No connkey or no command in stream ajax');
+  ajaxError('No connkey or no command in stream ajax', HTTP_STATUS_OK, STREAM_ERR_INVALID);
 }
 
 @mkdir(ZM_PATH_SOCKS);
@@ -38,13 +56,13 @@
 
 if (!($socket = @socket_create(AF_UNIX, SOCK_DGRAM, 0))) {
   if ($semaphore) sem_release($semaphore);
-  ajaxError('socket_create() failed: '.socket_strerror(socket_last_error()));
+  ajaxError('socket_create() failed: '.socket_strerror(socket_last_error()), HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
 }
 
 $localSocketFile = ZM_PATH_SOCKS.'/zms-'.$connkey.'w.sock';
 if (!socket_bind($socket, $localSocketFile)) {
   if ($semaphore) sem_release($semaphore);
-  ajaxError("socket_bind( $localSocketFile ) failed: ".socket_strerror(socket_last_error()));
+  ajaxError("socket_bind( $localSocketFile ) failed: ".socket_strerror(socket_last_error()), HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
 }
 
 switch ($_REQUEST['command']) {
@@ -99,51 +117,62 @@
 
 if (!file_exists($remSockFile)) {
   if ($semaphore) sem_release($semaphore);
-  ajaxError("Socket $remSockFile does not exist.  This file is created by zms, and since it does not exist, either zms did not run, or zms exited early.  Please check your zms logs and ensure that CGI is enabled in apache and check that the PATH_ZMS is set correctly.  Make sure that ZM is actually recording.  If you are trying to view a live stream and the capture process (zmc) is not running then zms will exit. Please go to http://zoneminder.readthedocs.io/en/latest/faq.html#why-can-t-i-see-streamed-images-when-i-can-see-stills-in-the-zone-window-etc for more information.");
+  ajaxError("Socket $remSockFile does not exist.  This file is created by zms, and since it does not exist, either zms did not run, or zms exited early.  Please check your zms logs and ensure that CGI is enabled in apache and check that the PATH_ZMS is set correctly.  Make sure that ZM is actually recording.  If you are trying to view a live stream and the capture process (zmc) is not running then zms will exit. Please go to http://zoneminder.readthedocs.io/en/latest/faq.html#why-can-t-i-see-streamed-images-when-i-can-see-stills-in-the-zone-window-etc for more information.", HTTP_STATUS_OK, STREAM_ERR_NO_SOCKET);
 } else {
   if (!@socket_sendto($socket, $msg, strlen($msg), 0, $remSockFile)) {
     if ($semaphore) sem_release($semaphore);
-    ajaxError("socket_sendto( $remSockFile ) failed: ".socket_strerror(socket_last_error()));
+    ajaxError("socket_sendto( $remSockFile ) failed: ".socket_strerror(socket_last_error()), HTTP_STATUS_OK, STREAM_ERR_NO_SOCKET);
   }
 }
 
 $rSockets = array($socket);
 $wSockets = NULL;
 $eSockets = NULL;
+$select_timed_out = false;
 
 $timeout = MSG_TIMEOUT - ( time() - $start_time );
 
 $numSockets = socket_select($rSockets, $wSockets, $eSockets, intval($timeout/1000), ($timeout%1000)*1000);
 
 if ( $numSockets === false ) {
   if ($semaphore) sem_release($semaphore);
-  ajaxError('socket_select failed: '.socket_strerror(socket_last_error()));
+  ajaxError('socket_select failed: '.socket_strerror(socket_last_error()), HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
 } else if ( $numSockets < 0 ) {
   if ($semaphore) sem_release($semaphore);
-  ajaxError("Socket closed $remSockFile");
+  ajaxError("Socket closed $remSockFile", HTTP_STATUS_OK, STREAM_ERR_NO_SOCKET);
 } else if ( $numSockets == 0 ) {
   ZM\Error("Timed out waiting for msg $remSockFile after waiting $timeout milliseconds");
   socket_set_nonblock($socket);
+  // Not an error on its own: the socket is now non-blocking, so the recvfrom
+  // below returns immediately and reports this as a timeout rather than
+  // pretending zms had nothing to say.
+  $select_timed_out = true;
   #ajaxError("Timed out waiting for msg $remSockFile");
 } else if ( $numSockets > 0 ) {
   if ( count($rSockets) != 1 ) {
     if ($semaphore) sem_release($semaphore);
-    ajaxError('Bogus return from select, '.count($rSockets).' sockets available');
+    ajaxError('Bogus return from select, '.count($rSockets).' sockets available', HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
   }
 }
 
 $nbytes = @socket_recvfrom($socket, $msg, MSG_DATA_SIZE, 0, $remSockFile);
 if ($semaphore) sem_release($semaphore);
 switch ($nbytes) {
 case -1 :
-  ajaxError("socket_recvfrom( $remSockFile ) failed: ".socket_strerror(socket_last_error()));
+  ajaxError("socket_recvfrom( $remSockFile ) failed: ".socket_strerror(socket_last_error()), HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
   break;
 case 0 :
-  ajaxError('No data to read from socket');
+  // socket_recvfrom() returns false on error, and false == 0 under switch's
+  // loose comparison, so a timed-out select lands here rather than on -1.
+  if ($select_timed_out) {
+    ajaxError("Timed out waiting for msg $remSockFile after waiting $timeout milliseconds",
+      HTTP_STATUS_OK, STREAM_ERR_TIMEOUT);
+  }
+  ajaxError('No data to read from socket', HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
   break;
 default :
   if ( $nbytes != MSG_DATA_SIZE ) {
-    ajaxError("Got unexpected message size, got $nbytes, expected ".MSG_DATA_SIZE);
+    ajaxError("Got unexpected message size, got $nbytes, expected ".MSG_DATA_SIZE, HTTP_STATUS_OK, STREAM_ERR_TRANSIENT);
   }
   break;
 }
@@ -197,9 +226,9 @@
   ajaxResponse(array('status'=>$data));
   break;
 default :
-  ajaxError('Unexpected received message type '.$data['type']);
+  ajaxError('Unexpected received message type '.$data['type'], HTTP_STATUS_OK, STREAM_ERR_INVALID);
 }
-ajaxError('Unrecognised action or insufficient permissions in ajax/stream');
+ajaxError('Unrecognised action or insufficient permissions in ajax/stream', HTTP_STATUS_OK, STREAM_ERR_INVALID);
 
 function ajaxCleanup() {
   global $socket, $localSocketFile;

web/includes/functions.php

@@ -1791,13 +1791,20 @@ function jsonDecode($value) {
 define('HTTP_STATUS_BAD_REQUEST', 400);
 define('HTTP_STATUS_FORBIDDEN', 403);
 
-function ajaxError($message, $code=HTTP_STATUS_OK) {
+/* $reason is an optional machine-readable classification of the failure, for
+ * callers that need to react differently to different errors instead of
+ * parsing $message.  It is named $reason rather than $code because $code is
+ * already taken by the HTTP status.  Included in the response only when set,
+ * so existing callers and their clients are unaffected.
+ */
+function ajaxError($message, $code=HTTP_STATUS_OK, $reason=null) {
   $backTrace = debug_backtrace();
   ZM\Debug($message.' from '.print_r($backTrace, true));
   if ( function_exists('ajaxCleanup') )
     ajaxCleanup();
   if ( $code == HTTP_STATUS_OK ) {
     $response = array('result'=>'Error', 'message'=>$message);
+    if ($reason) $response['reason'] = $reason;
     header('Content-type: application/json');
     exit(jsonEncode($response));
   }

web/js/MonitorStream.js

@@ -2,6 +2,21 @@
 var janus = null;
 const streaming = [];
 
+/* Does this ajax/stream.php failure mean the zms behind our connkey is gone?
+ *
+ * Only then is it right to tear the stream down and start a new one, because
+ * doing so replaces the connkey and leaves any still-running zms unaddressable.
+ * A slow reply or a socket problem local to php says nothing about zms, and
+ * restarting on those is what left processes behind.
+ *
+ * An absent reason is treated as fatal so that a php that predates the reason
+ * field keeps the older behaviour.
+ */
+function streamErrorIsFatal(reason) {
+  if (!reason) return true;
+  return reason == 'no_socket';
+}
+
 function MonitorStream(monitorData) {
   this.id = monitorData.id;
   this.name = monitorData.name;
@@ -1389,11 +1404,23 @@ function MonitorStream(monitorData) {
     } else {
       if (!this.started) return;
       console.error(respObj.message);
+
+      // Only a zms that is actually gone justifies tearing the stream down;
+      // see streamErrorIsFatal().
+      if (!streamErrorIsFatal(respObj.reason)) {
+        console.log('Not reloading stream for '+respObj.reason+' error, will retry on the next poll');
+        return;
+      }
+
       // Try to reload the image stream.
       console.log('Reloading stream: ' + stream.src);
-      // Instead of changing rand, perhaps we should be changing connKey.
       let src = (-1 != stream.src.indexOf('rand=')) ? stream.src.replace(/rand=\d+/i, 'rand='+Math.floor((Math.random() * 1000000) )) : stream.src+'&rand='+Math.floor((Math.random() * 1000000));
       src = src.replace(/auth=\w+/i, 'auth='+auth_hash);
+      /* Make the old zms exit before we stop being able to address it.  Once
+       * the connkey is replaced nothing can reach the old process, so if it
+       * missed SIGPIPE it would linger and keep streaming forever.
+       */
+      this.quitConnKey(this.connKey);
       this.streamCmdParms.connkey = this.statusCmdParms.connkey = this.connKey = this.genConnKey();
       src = src.replace(/connkey=\d+/i, 'connkey='+this.connKey);
       stream.src = '';
@@ -1592,6 +1619,28 @@ function MonitorStream(monitorData) {
     }
   };
 
+  /* Tell the zms behind a specific connkey to exit.
+   *
+   * Deliberately not routed through streamCommand()/streamCmdReq():
+   *   - those send to this.connKey at request time, and the caller here is
+   *     about to replace it, so the QUIT has to name its target explicitly;
+   *   - their response is fed back into getStreamCmdResponse(), and this is
+   *     called from that function's error path.  A QUIT that also failed would
+   *     re-enter the error path, quit again, and loop.
+   * The outcome is ignored on purpose: this is best effort, and there is
+   * nothing useful to do if the process is already gone.
+   */
+  this.quitConnKey = function(connkey) {
+    if (!connkey) return;
+    const params = Object.assign({}, this.streamCmdParms, {command: CMD_QUIT, connkey: connkey});
+    jQuery.ajaxQueue({
+      url: this.url + (auth_relay?'?'+auth_relay:''),
+      xhrFields: {withCredentials: true},
+      data: params,
+      dataType: 'json'
+    });
+  };
+
   this.streamCommand = function(command) {
     if (!this.started) {
       console.log('Not sending command, stream not started', command);
@@ -2615,3 +2664,7 @@ function appendMseBuffer(packet, context) {
     context.restart(context.currentChannelStream, 1000);
   }
 }
+
+if (typeof module !== 'undefined' && module.exports) {
+  module.exports = {streamErrorIsFatal};
+}