From a002e766cac7ed06579d4e5801468d734be1cd32 Mon Sep 17 00:00:00 2001 From: pabloFuente Date: Mon, 3 Nov 2025 22:45:05 +0100 Subject: [PATCH] openvidu-server: fix missing recordingStatusChanged failed event --- .../ComposedQuickStartRecordingService.java | 21 +++++--- .../service/ComposedRecordingService.java | 26 ++++++++-- .../recording/service/RecordingService.java | 5 ++ .../server/utils/CustomFileManager.java | 4 ++ .../server/utils/LocalCustomFileManager.java | 12 ++++- .../test/e2e/OpenViduTestAppE2eTest.java | 50 ++++++------------- 6 files changed, 68 insertions(+), 50 deletions(-) diff --git a/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedQuickStartRecordingService.java b/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedQuickStartRecordingService.java index 7c3386efb..85b753fe5 100644 --- a/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedQuickStartRecordingService.java +++ b/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedQuickStartRecordingService.java @@ -102,7 +102,7 @@ public class ComposedQuickStartRecordingService extends ComposedRecordingService this.sessionsContainers.put(session.getSessionId(), containerId); try { - this.waitForVideoFileNotEmpty(recording); + this.waitForVideoFileNotEmpty(recording, session); } catch (Exception e) { this.cleanRecordingMaps(recording); throw this.failStartRecording(session, recording, @@ -133,13 +133,18 @@ public class ComposedQuickStartRecordingService extends ComposedRecordingService recording.getId()); } - try { - dockerManager.runCommandInContainerSync(recording.getRecordingProperties().mediaNode(), containerId, - "./composed_quick_start.sh --stop-recording", 10); - } catch (IOException e1) { - log.error("Error stopping COMPOSED_QUICK_START recording {}: {}", recording.getId(), e1.getMessage()); - failRecordingCompletion(recording, containerId, true, - new OpenViduException(Code.RECORDING_COMPLETION_ERROR_CODE, e1.getMessage())); + if (containerId != null) { + try { + dockerManager.runCommandInContainerSync(recording.getRecordingProperties().mediaNode(), containerId, + "./composed_quick_start.sh --stop-recording", 10); + } catch (IOException e1) { + log.error("Error stopping COMPOSED_QUICK_START recording {}: {}", recording.getId(), e1.getMessage()); + failRecordingCompletion(recording, containerId, true, + new OpenViduException(Code.RECORDING_COMPLETION_ERROR_CODE, e1.getMessage())); + } + } else { + log.warn("No container ID found for session {} when stopping recording {}. Container may have already been cleaned up or never created.", + recording.getSessionId(), recording.getId()); } if (this.openviduConfig.isRecordingComposedExternal()) { diff --git a/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedRecordingService.java b/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedRecordingService.java index e8f2639f3..00353416a 100644 --- a/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedRecordingService.java +++ b/openvidu-server/src/main/java/io/openvidu/server/recording/service/ComposedRecordingService.java @@ -175,7 +175,7 @@ public class ComposedRecordingService extends RecordingService { this.sessionsContainers.put(session.getSessionId(), containerId); try { - this.waitForVideoFileNotEmpty(recording); + this.waitForVideoFileNotEmpty(recording, session); } catch (Exception e) { this.cleanRecordingMaps(recording); throw this.failStartRecording(session, recording, @@ -397,9 +397,22 @@ public class ComposedRecordingService extends RecordingService { } protected void updateRecordingAttributes(Recording recording) { + String infoFilePath = this.openviduConfig.getOpenViduRecordingPath() + + recording.getId() + "/" + recording.getId() + RecordingService.COMPOSED_INFO_FILE_EXTENSION; + + // Check if info file exists before trying to process it + java.nio.file.Path path = java.nio.file.Paths.get(infoFilePath); + if (!java.nio.file.Files.exists(path)) { + log.warn("Recording info file does not exist at {}. Recording {} may have failed to start properly.", + infoFilePath, recording.getId()); + recording.setStatus(io.openvidu.java.client.Recording.Status.failed); + // Don't return early - we need to continue so that sealRecordingMetadataFileAsReady() + // gets called in downloadComposedRecording(), which triggers the webhook + return; + } + try { - RecordingInfoUtils infoUtils = new RecordingInfoUtils(this.openviduConfig.getOpenViduRecordingPath() - + recording.getId() + "/" + recording.getId() + RecordingService.COMPOSED_INFO_FILE_EXTENSION); + RecordingInfoUtils infoUtils = new RecordingInfoUtils(infoFilePath); if (!infoUtils.hasVideo()) { log.error("COMPOSED recording {} with hasVideo=true has not video track", recording.getId()); @@ -421,11 +434,14 @@ public class ComposedRecordingService extends RecordingService { } } - protected void waitForVideoFileNotEmpty(Recording recording) throws Exception { + protected void waitForVideoFileNotEmpty(Recording recording, Session session) throws Exception { final String VIDEO_FILE = this.openviduConfig .getOpenViduRecordingPath(recording.getRecordingProperties().mediaNode()) + recording.getId() + "/" + recording.getName() + RecordingService.COMPOSED_RECORDING_EXTENSION; - this.fileManager.waitForFileToExistAndNotEmpty(recording.getRecordingProperties().mediaNode(), VIDEO_FILE); + + // Check if session was closed while we're waiting for the video file + fileManager.waitForFileToExistAndNotEmpty(recording.getRecordingProperties().mediaNode(), VIDEO_FILE, + () -> !session.isClosed()); log.info("File {} exists and is not empty", VIDEO_FILE); } diff --git a/openvidu-server/src/main/java/io/openvidu/server/recording/service/RecordingService.java b/openvidu-server/src/main/java/io/openvidu/server/recording/service/RecordingService.java index 5e8868929..2def148aa 100644 --- a/openvidu-server/src/main/java/io/openvidu/server/recording/service/RecordingService.java +++ b/openvidu-server/src/main/java/io/openvidu/server/recording/service/RecordingService.java @@ -201,6 +201,11 @@ public abstract class RecordingService { this.recordingManager.startingRecordings.remove(recording.getId()); this.recordingManager.sessionsRecordingsStarting.remove(session.getSessionId()); + + // Send "failed" webhook event + this.cdr.recordRecordingStatusChanged(recording, null, System.currentTimeMillis(), + io.openvidu.java.client.Recording.Status.failed); + this.stopRecording(session, recording, null); return new OpenViduException(Code.RECORDING_START_ERROR_CODE, errorMessage); } diff --git a/openvidu-server/src/main/java/io/openvidu/server/utils/CustomFileManager.java b/openvidu-server/src/main/java/io/openvidu/server/utils/CustomFileManager.java index 73f6768d1..55803acb7 100644 --- a/openvidu-server/src/main/java/io/openvidu/server/utils/CustomFileManager.java +++ b/openvidu-server/src/main/java/io/openvidu/server/utils/CustomFileManager.java @@ -23,6 +23,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStreamWriter; import java.util.Properties; +import java.util.function.BooleanSupplier; import org.apache.commons.io.FileUtils; import org.slf4j.Logger; @@ -123,6 +124,9 @@ public abstract class CustomFileManager { public abstract void waitForFileToExistAndNotEmpty(String mediaNodeId, String absolutePathToFile) throws Exception; + public abstract void waitForFileToExistAndNotEmpty(String mediaNodeId, String absolutePathToFile, + BooleanSupplier shouldContinueWaiting) throws Exception; + public abstract int maxSecondsWaitForFile(); } diff --git a/openvidu-server/src/main/java/io/openvidu/server/utils/LocalCustomFileManager.java b/openvidu-server/src/main/java/io/openvidu/server/utils/LocalCustomFileManager.java index 5e45fb50d..a97a9f4f6 100644 --- a/openvidu-server/src/main/java/io/openvidu/server/utils/LocalCustomFileManager.java +++ b/openvidu-server/src/main/java/io/openvidu/server/utils/LocalCustomFileManager.java @@ -1,11 +1,18 @@ package io.openvidu.server.utils; import java.io.File; +import java.util.function.BooleanSupplier; public class LocalCustomFileManager extends CustomFileManager { @Override public void waitForFileToExistAndNotEmpty(String mediaNodeId, String absolutePathToFile) throws Exception { + waitForFileToExistAndNotEmpty(mediaNodeId, absolutePathToFile, () -> true); + } + + @Override + public void waitForFileToExistAndNotEmpty(String mediaNodeId, String absolutePathToFile, + BooleanSupplier shouldContinueWaiting) throws Exception { // Check 10 times per seconds int MAX_SECONDS_WAIT = this.maxSecondsWaitForFile(); @@ -14,7 +21,7 @@ public class LocalCustomFileManager extends CustomFileManager { int i = 0; boolean arePresent = fileExistsAndHasBytes(absolutePathToFile); - while (!arePresent && i < LIMIT) { + while (!arePresent && i < LIMIT && shouldContinueWaiting.getAsBoolean()) { try { Thread.sleep(MILLISECONDS_INTERVAL_WAIT); arePresent = fileExistsAndHasBytes(absolutePathToFile); @@ -23,6 +30,9 @@ public class LocalCustomFileManager extends CustomFileManager { throw new Exception("Interrupted exception while waiting for file " + absolutePathToFile + " to exist"); } } + if (!shouldContinueWaiting.getAsBoolean()) { + throw new Exception("Recording was stopped while waiting for file " + absolutePathToFile); + } if (!arePresent) { throw new Exception("File " + absolutePathToFile + " does not exist and hasn't been created in " + MAX_SECONDS_WAIT + " seconds"); diff --git a/openvidu-test-e2e/src/test/java/io/openvidu/test/e2e/OpenViduTestAppE2eTest.java b/openvidu-test-e2e/src/test/java/io/openvidu/test/e2e/OpenViduTestAppE2eTest.java index 62b55c7c8..288b6be88 100644 --- a/openvidu-test-e2e/src/test/java/io/openvidu/test/e2e/OpenViduTestAppE2eTest.java +++ b/openvidu-test-e2e/src/test/java/io/openvidu/test/e2e/OpenViduTestAppE2eTest.java @@ -1523,44 +1523,22 @@ public class OpenViduTestAppE2eTest extends AbstractOpenViduTestappE2eTest { user.getEventManager().waitUntilEventReaches("streamPlaying", 4); checkDockerContainerRunning("openvidu/openvidu-recording", 1); - OV.fetch(); - session = OV.getActiveSessions().get(0); - session.close(); + OV.fetch(); + session = OV.getActiveSessions().get(0); + session.close(); - event = CustomWebhook.waitForEvent("recordingStatusChanged", 1); - if ("stopped".equals(event.get("status").getAsString())) { - // Recording hasn't had time to start. Should trigger stopped, started, failed - event = CustomWebhook.waitForEvent("recordingStatusChanged", 5); // started - Assertions.assertEquals("started", event.get("status").getAsString(), - "Wrong status in recordingStatusChanged event"); - try { - // Wait first for a failed event - event = CustomWebhook.waitForEvent("recordingStatusChanged", 10); // failed - Assertions.assertEquals("failed", event.get("status").getAsString(), - "Wrong status in recordingStatusChanged event"); - Assertions.assertEquals(Recording.Status.failed, OV.getRecording(sessionName + "~2").getStatus(), - "Wrong recording status"); - } catch (Exception e) { - // If the failed event is not received, it's because the session has been destroyed - // before the recording started - // Check for sessionDestroyed event - event = CustomWebhook.waitForEvent("sessionDestroyed", 5); - } - } else { - // Recording did have time to start. Should trigger started, stopped, ready - event = CustomWebhook.waitForEvent("recordingStatusChanged", 5); // started - Assertions.assertEquals("stopped", event.get("status").getAsString(), - "Wrong status in recordingStatusChanged event"); - event = CustomWebhook.waitForEvent("recordingStatusChanged", 10); // failed - Assertions.assertEquals("ready", event.get("status").getAsString(), - "Wrong status in recordingStatusChanged event"); - Assertions.assertEquals(Recording.Status.ready, OV.getRecording(sessionName + "~2").getStatus(), - "Wrong recording status"); - } + // Recording hasn't had time to start. Should trigger stopped, then failed immediately + event = CustomWebhook.waitForEvent("recordingStatusChanged", 1); // stopped + Assertions.assertEquals("stopped", event.get("status").getAsString(), + "Wrong status in recordingStatusChanged event"); + + event = CustomWebhook.waitForEvent("recordingStatusChanged", 5); // failed + Assertions.assertEquals("failed", event.get("status").getAsString(), + "Wrong status in recordingStatusChanged event"); + Assertions.assertEquals(Recording.Status.failed, OV.getRecording(sessionName + "~2").getStatus(), + "Wrong recording status"); - checkDockerContainerRunning("openvidu/openvidu-recording", 0); - - } finally { + checkDockerContainerRunning("openvidu/openvidu-recording", 0); } finally { CustomWebhook.shutDown(); } }