Return error when trying to stop an unknown recording

The Nextcloud server expects a notification to be sent by the recording
server once a recording is stopped. However, if the recording server
does not know about certain recording the Nextcloud server did not
change the recording status, and the recording was kept active (even if
it was not).

To solve that now the recording server returns a 404 when trying to stop
an unknown recording (although not if the recording is being stopped),
and in that case the Nextcloud server updates the recording status to
"failed".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

lib/Exceptions/RecordingNotFoundException.php

@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2023 Daniel Calviño Sánchez <danxuliu@gmail.com>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\Talk\Exceptions;
+
+class RecordingNotFoundException extends \Exception {
+}

lib/Recording/BackendNotifier.php

@@ -24,11 +24,14 @@ declare(strict_types=1);
 
 namespace OCA\Talk\Recording;
 
+use GuzzleHttp\Exception\ClientException;
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Exception\ServerException;
 use OCA\Talk\Config;
+use OCA\Talk\Exceptions\RecordingNotFoundException;
 use OCA\Talk\Participant;
 use OCA\Talk\Room;
+use OCP\AppFramework\Http;
 use OCP\Http\Client\IClientService;
 use OCP\IURLGenerator;
 use OCP\Security\ISecureRandom;
@@ -168,10 +171,18 @@ class BackendNotifier {
 		}
 
 		$start = microtime(true);
-		$this->backendRequest($room, [
-			'type' => 'stop',
-			'stop' => $parameters,
-		]);
+		try {
+			$this->backendRequest($room, [
+				'type' => 'stop',
+				'stop' => $parameters,
+			]);
+		} catch (ClientException $e) {
+			if ($e->getResponse()->getStatusCode() === Http::STATUS_NOT_FOUND) {
+				throw new RecordingNotFoundException();
+			}
+
+			throw $e;
+		}
 		$duration = microtime(true) - $start;
 		$this->logger->debug('Send stop message: {token} ({duration})', [
 			'token' => $room->getToken(),

lib/Service/RecordingService.php

@@ -30,6 +30,7 @@ use OC\User\NoUserException;
 use OCA\Talk\Chat\ChatManager;
 use OCA\Talk\Config;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
+use OCA\Talk\Exceptions\RecordingNotFoundException;
 use OCA\Talk\Manager;
 use OCA\Talk\Participant;
 use OCA\Talk\Recording\BackendNotifier;
@@ -96,7 +97,14 @@ class RecordingService {
 			return;
 		}
 
-		$this->backendNotifier->stop($room, $participant);
+		try {
+			$this->backendNotifier->stop($room, $participant);
+		} catch (RecordingNotFoundException $e) {
+			// If the recording to be stopped is not known to the recording
+			// server it will never notify that the recording was stopped, so
+			// the status needs to be explicitly changed here.
+			$this->roomService->setCallRecording($room, Room::RECORDING_FAILED);
+		}
 	}
 
 	public function store(Room $room, string $owner, array $file): void {

psalm.xml

@@ -77,6 +77,7 @@
 		<file name="tests/stubs/oc_comments_comment.php" />
 		<file name="tests/stubs/oc_comments_manager.php" />
 		<file name="tests/stubs/oc_hooks_emitter.php" />
+		<file name="tests/stubs/GuzzleHttp_Exception_ClientException.php" />
 		<file name="tests/stubs/GuzzleHttp_Exception_ConnectException.php" />
 		<file name="tests/stubs/GuzzleHttp_Exception_ServerException.php" />
 		<file name="tests/stubs/Symfony_Component_EventDispatcher_GenericEvent.php" />

recording/docs/recording-api.md

@@ -70,3 +70,7 @@
     ```
 
     - `actor` is optional
+
+* Response:
+    - (Additional) Status code:
+        + `404 Not Found`: When there is no recording for the token.

recording/src/nextcloud/talk/recording/Server.py

@@ -28,7 +28,7 @@ import hmac
 from threading import Lock, Thread
 
 from flask import Flask, jsonify, request
-from werkzeug.exceptions import BadRequest, Forbidden
+from werkzeug.exceptions import BadRequest, Forbidden, NotFound
 
 from nextcloud.talk import recording
 from .Config import config
@@ -37,6 +37,7 @@ from .Service import RECORDING_STATUS_AUDIO_AND_VIDEO, Service
 app = Flask(__name__)
 
 services = {}
+servicesStopping = {}
 servicesLock = Lock()
 
 @app.route("/api/v1/welcome", methods=["GET"])
@@ -194,21 +195,52 @@ def stopRecording(backend, token, data):
 
     service = None
     with servicesLock:
+        if serviceId not in services and serviceId in servicesStopping:
+            app.logger.info(f"Trying to stop recording again: {backend} {token}")
+            return {}
+
         if serviceId not in services:
             app.logger.warning(f"Trying to stop unknown recording: {backend} {token}")
-            return {}
+            raise NotFound()
 
         service = services[serviceId]
 
         services.pop(serviceId)
 
+        servicesStopping[serviceId] = service
+
     app.logger.info(f"Stop recording: {backend} {token}")
 
-    serviceStopThread = Thread(target=service.stop, args=[actorType, actorId], daemon=True)
+    serviceStopThread = Thread(target=_stopRecordingService, args=[service, actorType, actorId], daemon=True)
     serviceStopThread.start()
 
     return {}
 
+def _stopRecordingService(service, actorType, actorId):
+    """
+    Helper function to stop a recording service.
+
+    The recording service will be removed from the list of services being
+    stopped once it is fully stopped.
+
+    :param service: the Service to stop.
+    """
+    serviceId = f'{service.backend}-{service.token}'
+
+    try:
+        service.stop(actorType, actorId)
+    except Exception as exception:
+        app.logger.exception(f"Failed to stop recording: {service.backend} {service.token}")
+    finally:
+        with servicesLock:
+            if serviceId not in servicesStopping:
+                # This should never happen.
+                app.logger.error(f"Recording stopped when not in the list of stopping services: {service.backend} {service.token}")
+
+                return
+
+            servicesStopping.pop(serviceId)
+
 # Despite this handler it seems that in some cases the geckodriver could have
 # been killed already when it is executed, which unfortunately prevents a proper
 # cleanup of the temporary files opened by the browser.

tests/stubs/GuzzleHttp_Exception_ClientException.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace GuzzleHttp\Exception;
+
+class ClientException extends \RuntimeException {
+	public function getResponse() {
+	}
+}