From 1a7095ab2ab2c0cfcc76c8d969b41c68c53e2a9a Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 25 Sep 2026 17:45:51 -0400 Subject: [PATCH 01/24] feat(dgw): list session recording logs separately in the manifest A .slog pushed for a session that already has a recording is now listed in a new `logs` field of recording.json, named `log-{n}.slog`. The `files` list keeps holding recordings only, so released players and streamers keep working. A log-only session (AD Console) still lists its .slog in `files`. The `logs` field is omitted when empty, so existing manifests stay the same. The session ZIP download now includes the logs. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 39 +++- devolutions-gateway/src/recording.rs | 260 ++++++++++++++++++++++++--- 2 files changed, 276 insertions(+), 23 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 5212eb095..48489e92a 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -633,6 +633,8 @@ where #[serde(rename_all = "camelCase")] struct RecordingZipManifest { files: Vec, + #[serde(default)] + logs: Vec, } #[derive(Debug, Deserialize)] @@ -695,8 +697,8 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + logs: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +struct JrecLog { + file_name: String, } impl JrecManifest { + fn has_recording(&self) -> bool { + self.files.iter().any(|file| { + Utf8Path::new(&file.file_name) + .extension() + .and_then(RecordingFileType::from_extension) + .is_some_and(|file_type| file_type != RecordingFileType::SessionRecordingLog) + }) + } + fn read_from_file(path: impl AsRef) -> anyhow::Result { let json = std::fs::read(path)?; let manifest = serde_json::from_slice(&json)?; @@ -241,6 +258,7 @@ struct OnGoingRecording { manifest_path: Utf8PathBuf, session_must_be_recorded: bool, disconnected_ttl: Duration, + pushing_log: bool, } enum RecordingManagerMessage { @@ -519,29 +537,39 @@ impl RecordingManagerTask { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let (manifest, recording_file) = if recording_path.exists() { + let (manifest, recording_file, pushing_log) = if recording_path.exists() { debug!(path = %recording_path, "Recording directory already exists"); let mut existing_manifest = JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")?; - let next_file_idx = existing_manifest.files.len(); - let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); + let pushing_log = file_type == RecordingFileType::SessionRecordingLog && existing_manifest.has_recording(); - let file_name = format!("recording-{next_file_idx}.{}", file_type.extension()); - let recording_file = recording_path.join(&file_name); + let file_name = if pushing_log { + let file_name = format!("log-{}.{}", existing_manifest.logs.len(), file_type.extension()); + existing_manifest.logs.push(JrecLog { + file_name: file_name.clone(), + }); + file_name + } else { + let next_file_idx = existing_manifest.files.len(); + let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); + let file_name = format!("recording-{next_file_idx}.{}", file_type.extension()); + existing_manifest.files.push(JrecFile { + start_time, + duration: 0, + file_name: file_name.clone(), + }); + file_name + }; - existing_manifest.files.push(JrecFile { - start_time, - duration: 0, - file_name, - }); + let recording_file = recording_path.join(&file_name); existing_manifest .save_to_file(&manifest_path) .context("override existing manifest")?; - (existing_manifest, recording_file) + (existing_manifest, recording_file, pushing_log) } else { debug!(path = %recording_path, "Create recording directory"); @@ -564,13 +592,14 @@ impl RecordingManagerTask { start_time, duration: 0, files: vec![first_file], + logs: Vec::new(), }; initial_manifest .save_to_file(&manifest_path) .context("write initial manifest to disk")?; - (initial_manifest, recording_file) + (initial_manifest, recording_file, false) }; let active_recording_count = self.rx.active_recordings.insert(id); @@ -596,6 +625,7 @@ impl RecordingManagerTask { manifest_path, session_must_be_recorded, disconnected_ttl, + pushing_log, }, ); let ongoing_recording_count = self.ongoing_recordings.len(); @@ -625,20 +655,31 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - let current_file = ongoing - .manifest - .files - .last_mut() - .context("no recording file (this is a bug)")?; - current_file.duration = end_time - current_file.start_time; + let current_file_name = if ongoing.pushing_log { + &ongoing + .manifest + .logs + .last() + .context("no log file (this is a bug)")? + .file_name + } else { + let current_file = ongoing + .manifest + .files + .last_mut() + .context("no recording file (this is a bug)")?; + current_file.duration = end_time - current_file.start_time; - ongoing.manifest.duration = end_time - ongoing.manifest.start_time; + ongoing.manifest.duration = end_time - ongoing.manifest.start_time; + + ¤t_file.file_name + }; let recording_file_path = ongoing .manifest_path .parent() .expect("a parent") - .join(¤t_file.file_name); + .join(current_file_name); debug!(path = %ongoing.manifest_path, "Write updated manifest to disk"); @@ -964,3 +1005,180 @@ async fn remux(input_path: Utf8PathBuf) { Ok(()) } } + +#[cfg(test)] +mod tests { + use devolutions_gateway_task::ShutdownHandle; + use serde_json::json; + + use super::*; + + const MASTER_MANIFEST: &str = r#"{ + "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", + "startTime": 1, + "duration": 5, + "files": [ + { + "fileName": "recording-0.webm", + "startTime": 1, + "duration": 5 + } + ] +}"#; + + struct Harness { + _dir: tempfile::TempDir, + recordings_path: Utf8PathBuf, + sender: RecordingMessageSender, + _shutdown_handle: ShutdownHandle, + } + + impl Harness { + fn start() -> Self { + let dir = tempfile::tempdir().expect("temp dir"); + let recordings_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); + let (sender, receiver) = recording_message_channel(); + let (session_manager_handle, _) = crate::session::session_manager_channel(); + let (job_queue_handle, _) = JobQueueHandle::new(); + let task = RecordingManagerTask::new( + receiver, + recordings_path.clone(), + session_manager_handle, + job_queue_handle, + ); + let (shutdown_handle, shutdown_signal) = ShutdownHandle::new(); + tokio::spawn(recording_manager_task(task, shutdown_signal)); + + Self { + _dir: dir, + recordings_path, + sender, + _shutdown_handle: shutdown_handle, + } + } + + fn manifest_path(&self, id: Uuid) -> Utf8PathBuf { + self.recordings_path.join(id.to_string()).join("recording.json") + } + + fn write_manifest(&self, id: Uuid, json: &str) { + std::fs::create_dir_all(self.recordings_path.join(id.to_string())).expect("create recording dir"); + std::fs::write(self.manifest_path(id), json).expect("write manifest"); + } + + fn read_manifest(&self, id: Uuid) -> serde_json::Value { + serde_json::from_slice(&std::fs::read(self.manifest_path(id)).expect("read manifest")) + .expect("parse manifest") + } + + async fn connect(&self, id: Uuid, file_type: RecordingFileType) -> String { + let path = self + .sender + .connect(id, file_type, Duration::ZERO) + .await + .expect("connect"); + path.file_name().expect("file name").to_owned() + } + + async fn disconnect(&self, id: Uuid) { + self.sender.disconnect(id).await.expect("disconnect"); + // Messages are processed in order, so this waits for the disconnect to be handled. + self.sender.get_count().await.expect("sync with manager"); + } + + async fn push(&self, id: Uuid, file_type: RecordingFileType) -> String { + let file_name = self.connect(id, file_type).await; + self.disconnect(id).await; + file_name + } + } + + #[tokio::test] + async fn log_only_session_lists_log_in_files() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + + let file_name = harness.push(id, RecordingFileType::SessionRecordingLog).await; + + assert_eq!(file_name, "recording-0.slog"); + let manifest = harness.read_manifest(id); + assert_eq!(manifest["files"][0]["fileName"], "recording-0.slog"); + assert!(manifest.get("logs").is_none()); + } + + #[tokio::test] + async fn log_pushed_after_recording_goes_to_logs() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + + harness.push(id, RecordingFileType::WebM).await; + let first_log = harness.push(id, RecordingFileType::SessionRecordingLog).await; + let second_log = harness.push(id, RecordingFileType::SessionRecordingLog).await; + + assert_eq!(first_log, "log-0.slog"); + assert_eq!(second_log, "log-1.slog"); + let manifest = harness.read_manifest(id); + let files = manifest["files"].as_array().expect("files"); + assert_eq!(files.len(), 1); + assert_eq!(files[0]["fileName"], "recording-0.webm"); + assert_eq!( + manifest["logs"], + json!([{ "fileName": "log-0.slog" }, { "fileName": "log-1.slog" }]) + ); + } + + #[tokio::test] + async fn log_push_keeps_recording_durations() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.write_manifest(id, MASTER_MANIFEST); + + harness.push(id, RecordingFileType::SessionRecordingLog).await; + + let manifest = harness.read_manifest(id); + assert_eq!(manifest["duration"], 5); + assert_eq!(manifest["files"][0]["duration"], 5); + } + + #[tokio::test] + async fn recording_reconnect_keeps_logs() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.push(id, RecordingFileType::WebM).await; + harness.push(id, RecordingFileType::SessionRecordingLog).await; + + let file_name = harness.connect(id, RecordingFileType::WebM).await; + assert_eq!(file_name, "recording-1.webm"); + assert_eq!(harness.read_manifest(id)["logs"], json!([{ "fileName": "log-0.slog" }])); + + harness.disconnect(id).await; + let manifest = harness.read_manifest(id); + assert_eq!(manifest["files"][1]["fileName"], "recording-1.webm"); + assert_eq!(manifest["logs"], json!([{ "fileName": "log-0.slog" }])); + } + + #[tokio::test] + async fn shadow_file_list_excludes_logs() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.push(id, RecordingFileType::WebM).await; + harness.connect(id, RecordingFileType::SessionRecordingLog).await; + + let files = harness.sender.list_files(id).await.expect("list files"); + + let file_names: Vec<_> = files.iter().filter_map(|path| path.file_name()).collect(); + assert_eq!(file_names, ["recording-0.webm"]); + } + + #[test] + fn manifest_without_logs_round_trips_byte_for_byte() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("recording.json"); + std::fs::write(&path, MASTER_MANIFEST).expect("write manifest"); + + let manifest = JrecManifest::read_from_file(&path).expect("read manifest"); + manifest.save_to_file(&path).expect("save manifest"); + + assert_eq!(std::fs::read_to_string(&path).expect("read back"), MASTER_MANIFEST); + } +} From f51e5c920ebb433d253ad4ed636f913bf33f453d Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Sun, 27 Sep 2026 13:30:51 -0400 Subject: [PATCH 02/24] fix(dgw): refuse /shadow while only a session log is pushed A post-session .slog push kept the session "connected", so /shadow streamed the finished recording instead of refusing like master did. /shadow now closes with "streaming ended" when the ongoing push is a log, and log chunks no longer wake /shadow streamers. The ongoing push now tracks which manifest entry it writes to (recording or log, by index) instead of a bool, which removes the "no log file" bug branch. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 13 +- devolutions-gateway/src/recording.rs | 199 +++++++++++++++++++-------- 2 files changed, 153 insertions(+), 59 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 48489e92a..8944cdda8 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -994,9 +994,16 @@ async fn shadow_recording( return close_with_error(ws, StreamerCloseCode::InternalError); }; - let Ok(recording_files) = recordings.list_files(id).await else { - warn!(%id, "Shadow recording rejected: failed to list recording files"); - return close_with_error(ws, StreamerCloseCode::InternalError); + let recording_files = match recordings.list_files(id).await { + Ok(Some(recording_files)) => recording_files, + Ok(None) => { + debug!(%id, "Shadow recording rejected: only a Log is being pushed"); + return close_with_error(ws, StreamerCloseCode::StreamingEnded); + } + Err(_) => { + warn!(%id, "Shadow recording rejected: failed to list recording files"); + return close_with_error(ws, StreamerCloseCode::InternalError); + } }; let Some(recording_path) = recording_files.last() else { diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 458a29a2c..b5c67731d 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -42,6 +42,7 @@ struct JrecManifest { start_time: i64, duration: i64, files: Vec, + /// Append-only, like `files`: artifact names and `CurrentArtifact` indices are derived from positions. #[serde(default, skip_serializing_if = "Vec::is_empty")] logs: Vec, } @@ -123,8 +124,8 @@ where } }; - let recording_file = match recordings.connect(session_id, file_type, disconnected_ttl).await { - Ok(recording_file) => recording_file, + let (recording_file, artifact) = match recordings.connect(session_id, file_type, disconnected_ttl).await { + Ok(connected) => connected, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); client_stream.shutdown().await.context("shutdown")?; @@ -161,7 +162,10 @@ where loop { tokio::select! { _ = flush_signal.notified() => { - recordings.new_chunk_appended(session_id)?; + // Log data is not media, so it must not wake `/shadow` streamers. + if let CurrentArtifact::Recording(_) = artifact { + recordings.new_chunk_appended(session_id)?; + } }, _ = shutdown_signal_clone.wait() => { break; @@ -251,6 +255,12 @@ pub enum OnGoingRecordingState { LastSeen { timestamp: i64 }, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum CurrentArtifact { + Recording(usize), + Log(usize), +} + #[derive(Debug, Clone)] struct OnGoingRecording { state: OnGoingRecordingState, @@ -258,7 +268,7 @@ struct OnGoingRecording { manifest_path: Utf8PathBuf, session_must_be_recorded: bool, disconnected_ttl: Duration, - pushing_log: bool, + artifact: CurrentArtifact, } enum RecordingManagerMessage { @@ -266,7 +276,7 @@ enum RecordingManagerMessage { id: Uuid, file_type: RecordingFileType, disconnected_ttl: Duration, - channel: oneshot::Sender, + channel: oneshot::Sender<(Utf8PathBuf, CurrentArtifact)>, }, Disconnect { id: Uuid, @@ -277,7 +287,7 @@ enum RecordingManagerMessage { }, ListFiles { id: Uuid, - channel: oneshot::Sender>, + channel: oneshot::Sender>>, }, GetCount { channel: oneshot::Sender, @@ -342,7 +352,7 @@ impl RecordingMessageSender { id: Uuid, file_type: RecordingFileType, disconnected_ttl: Duration, - ) -> anyhow::Result { + ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { @@ -429,7 +439,8 @@ impl RecordingMessageSender { Ok(rx.await?) } - pub(crate) async fn list_files(&self, recording_id: Uuid) -> anyhow::Result> { + /// Returns `None` when the ongoing push is a Log: there is no Recording to stream. + pub(crate) async fn list_files(&self, recording_id: Uuid) -> anyhow::Result>> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::ListFiles { @@ -525,7 +536,7 @@ impl RecordingManagerTask { id: Uuid, file_type: RecordingFileType, disconnected_ttl: Duration, - ) -> anyhow::Result { + ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { const LENGTH_WARNING_THRESHOLD: usize = 1000; if let Some(ongoing) = self.ongoing_recordings.get(&id) @@ -537,31 +548,31 @@ impl RecordingManagerTask { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let (manifest, recording_file, pushing_log) = if recording_path.exists() { + let (manifest, recording_file, artifact) = if recording_path.exists() { debug!(path = %recording_path, "Recording directory already exists"); let mut existing_manifest = JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")?; - let pushing_log = file_type == RecordingFileType::SessionRecordingLog && existing_manifest.has_recording(); - - let file_name = if pushing_log { - let file_name = format!("log-{}.{}", existing_manifest.logs.len(), file_type.extension()); - existing_manifest.logs.push(JrecLog { - file_name: file_name.clone(), - }); - file_name - } else { - let next_file_idx = existing_manifest.files.len(); - let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - let file_name = format!("recording-{next_file_idx}.{}", file_type.extension()); - existing_manifest.files.push(JrecFile { - start_time, - duration: 0, - file_name: file_name.clone(), - }); - file_name - }; + let (file_name, artifact) = + if file_type == RecordingFileType::SessionRecordingLog && existing_manifest.has_recording() { + let idx = existing_manifest.logs.len(); + let file_name = format!("log-{idx}.{}", file_type.extension()); + existing_manifest.logs.push(JrecLog { + file_name: file_name.clone(), + }); + (file_name, CurrentArtifact::Log(idx)) + } else { + let idx = existing_manifest.files.len(); + let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); + let file_name = format!("recording-{idx}.{}", file_type.extension()); + existing_manifest.files.push(JrecFile { + start_time, + duration: 0, + file_name: file_name.clone(), + }); + (file_name, CurrentArtifact::Recording(idx)) + }; let recording_file = recording_path.join(&file_name); @@ -569,7 +580,7 @@ impl RecordingManagerTask { .save_to_file(&manifest_path) .context("override existing manifest")?; - (existing_manifest, recording_file, pushing_log) + (existing_manifest, recording_file, artifact) } else { debug!(path = %recording_path, "Create recording directory"); @@ -599,7 +610,7 @@ impl RecordingManagerTask { .save_to_file(&manifest_path) .context("write initial manifest to disk")?; - (initial_manifest, recording_file, false) + (initial_manifest, recording_file, CurrentArtifact::Recording(0)) }; let active_recording_count = self.rx.active_recordings.insert(id); @@ -625,7 +636,7 @@ impl RecordingManagerTask { manifest_path, session_must_be_recorded, disconnected_ttl, - pushing_log, + artifact, }, ); let ongoing_recording_count = self.ongoing_recordings.len(); @@ -639,7 +650,7 @@ impl RecordingManagerTask { ); } - Ok(recording_file) + Ok((recording_file, artifact)) } async fn handle_disconnect(&mut self, id: Uuid) -> anyhow::Result<()> { @@ -655,24 +666,16 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - let current_file_name = if ongoing.pushing_log { - &ongoing - .manifest - .logs - .last() - .context("no log file (this is a bug)")? - .file_name - } else { - let current_file = ongoing - .manifest - .files - .last_mut() - .context("no recording file (this is a bug)")?; - current_file.duration = end_time - current_file.start_time; + let current_file_name = match ongoing.artifact { + CurrentArtifact::Recording(idx) => { + let current_file = &mut ongoing.manifest.files[idx]; + current_file.duration = end_time - current_file.start_time; - ongoing.manifest.duration = end_time - ongoing.manifest.start_time; + ongoing.manifest.duration = end_time - ongoing.manifest.start_time; - ¤t_file.file_name + ¤t_file.file_name + } + CurrentArtifact::Log(idx) => &ongoing.manifest.logs[idx].file_name, }; let recording_file_path = ongoing @@ -837,8 +840,8 @@ async fn recording_manager_task( match msg { RecordingManagerMessage::Connect { id, file_type, disconnected_ttl, channel } => { match manager.handle_connect(id, file_type, disconnected_ttl).await { - Ok(recording_file) => { - let _ = channel.send(recording_file); + Ok(connected) => { + let _ = channel.send(connected); } Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } @@ -890,6 +893,9 @@ async fn recording_manager_task( }, RecordingManagerMessage::ListFiles { id, channel } => { match manager.ongoing_recordings.get(&id) { + Some(recording) if matches!(recording.artifact, CurrentArtifact::Log(_)) => { + let _ = channel.send(None); + } Some(recording) => { let recordings_folder = recording.manifest_path.parent().expect("a parent"); @@ -900,7 +906,7 @@ async fn recording_manager_task( .map(|file| recordings_folder.join(&file.file_name)) .collect(); - let _ = channel.send(files); + let _ = channel.send(Some(files)); } None => { warn!(%id, "No recording found for provided ID"); @@ -1072,7 +1078,7 @@ mod tests { } async fn connect(&self, id: Uuid, file_type: RecordingFileType) -> String { - let path = self + let (path, _) = self .sender .connect(id, file_type, Duration::ZERO) .await @@ -1080,6 +1086,51 @@ mod tests { path.file_name().expect("file name").to_owned() } + async fn client_push_wakes_streamers(&self, id: Uuid, file_type: RecordingFileType) -> bool { + let claims = serde_json::from_value(json!({ + "jet_aid": id, + "jet_rop": "push", + "exp": i64::MAX, + "jti": Uuid::new_v4(), + })) + .expect("claims"); + let (mut client, server) = io::duplex(1024); + let (shutdown_handle, shutdown_signal) = ShutdownHandle::new(); + let push = tokio::spawn( + ClientPush::builder() + .recordings(self.sender.clone()) + .claims(claims) + .client_stream(server) + .file_type(file_type) + .session_id(id) + .shutdown_signal(shutdown_signal) + .build() + .run(), + ); + + let (tx, mut woken) = oneshot::channel(); + self.sender.add_new_chunk_listener(id, tx); + + // The push flushes, and so signals, whenever it runs out of input. + let woke = tokio::time::timeout(Duration::from_secs(2), async { + loop { + client.write_all(b"chunk").await.expect("write chunk"); + tokio::select! { + _ = &mut woken => break, + () = tokio::time::sleep(Duration::from_millis(20)) => {} + } + } + }) + .await + .is_ok(); + + drop(client); + push.await.expect("join push").expect("push"); + self.sender.get_count().await.expect("sync with manager"); + drop(shutdown_handle); + woke + } + async fn disconnect(&self, id: Uuid) { self.sender.disconnect(id).await.expect("disconnect"); // Messages are processed in order, so this waits for the disconnect to be handled. @@ -1158,18 +1209,54 @@ mod tests { } #[tokio::test] - async fn shadow_file_list_excludes_logs() { + async fn shadow_streams_ongoing_recording() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, RecordingFileType::WebM).await; - harness.connect(id, RecordingFileType::SessionRecordingLog).await; + harness.connect(id, RecordingFileType::WebM).await; let files = harness.sender.list_files(id).await.expect("list files"); + let files = files.expect("a Recording is being pushed"); let file_names: Vec<_> = files.iter().filter_map(|path| path.file_name()).collect(); assert_eq!(file_names, ["recording-0.webm"]); } + #[tokio::test] + async fn shadow_refuses_while_only_a_log_is_pushed() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.push(id, RecordingFileType::WebM).await; + harness.connect(id, RecordingFileType::SessionRecordingLog).await; + + let files = harness.sender.list_files(id).await.expect("list files"); + + assert!(files.is_none()); + } + + #[tokio::test] + async fn log_chunks_do_not_wake_streamers() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.push(id, RecordingFileType::WebM).await; + + assert!( + !harness + .client_push_wakes_streamers(id, RecordingFileType::SessionRecordingLog) + .await + ); + } + + #[tokio::test] + async fn log_only_session_slog_chunks_wake_streamers() { + let harness = Harness::start(); + + assert!( + harness + .client_push_wakes_streamers(Uuid::new_v4(), RecordingFileType::SessionRecordingLog) + .await + ); + } + #[test] fn manifest_without_logs_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); From 796f4ff76f61effcc402a7626a6de0ff9d6f57c4 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Sun, 27 Sep 2026 13:30:52 -0400 Subject: [PATCH 03/24] refactor(dgw): use artifact naming for the session ZIP plan The ZIP now holds logs as well as recordings, so "clip" no longer fits. The public endpoint doc is unchanged, so the OpenAPI output is too. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 34 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 8944cdda8..842fd56bc 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -36,7 +36,7 @@ use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. const ZIP_CHUNK_SIZE: usize = 64 * 1024; -/// Maximum files in a session ZIP (`recording.json` + clips). +/// Maximum files in a session ZIP (`recording.json` + every Artifact listed, files and logs). /// /// Reconnect windows only mint a small number of clips per session in practice; /// this bound blocks pathological manifests without rejecting normal multi-clip packages. @@ -660,21 +660,21 @@ fn recording_file_content_type(path: &Utf8Path) -> &'static str { /// Immutable package membership for one download attempt. /// -/// `manifest_bytes` are the exact `recording.json` contents used to derive `clip_names`, -/// so the archived manifest cannot drift from the clips included in the ZIP. +/// `manifest_bytes` are the exact `recording.json` contents used to derive `artifact_names`, +/// so the archived manifest cannot drift from the Artifacts included in the ZIP. #[derive(Debug, Clone)] struct RecordingZipPlan { manifest_bytes: Vec, - clip_names: Vec, + artifact_names: Vec, } impl RecordingZipPlan { fn entry_count(&self) -> usize { - 1 /* recording.json */ + self.clip_names.len() + 1 /* recording.json */ + self.artifact_names.len() } } -/// Snapshots `recording.json` and the clip files it references at call time. +/// Snapshots `recording.json` and every Artifact it lists (files and logs) at call time. async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result { let manifest_path = recording_dir.join("recording.json"); let manifest_bytes = tokio::fs::read(&manifest_path).await.map_err(|error| { @@ -697,7 +697,7 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result Result Result Result<(), HttpError> { let mut total_bytes = u64::try_from(plan.manifest_bytes.len()).unwrap_or(u64::MAX); - for file_name in &plan.clip_names { + for file_name in &plan.artifact_names { let path = recording_dir.join(file_name); let metadata = tokio::fs::metadata(&path).await.map_err(|error| { if error.kind() == io::ErrorKind::NotFound { @@ -867,7 +867,7 @@ fn build_recording_zip_archive( zip.write_all(&plan.manifest_bytes) .context("write recording.json ZIP entry")?; - for file_name in &plan.clip_names { + for file_name in &plan.artifact_names { if cancel.load(Ordering::Relaxed) { return Err(anyhow::Error::new(RecordingZipCancelled)); } @@ -1095,7 +1095,7 @@ mod tests { .unwrap_or_else(|error| panic!("snapshot plan: {error}")); assert_eq!(plan.manifest_bytes, manifest_bytes); assert_eq!( - plan.clip_names, + plan.artifact_names, vec!["recording-0.webm".to_owned(), "recording-1.webm".to_owned()] ); } @@ -1130,7 +1130,7 @@ mod tests { let plan = snapshot_recording_zip_plan(&dir_path) .await .unwrap_or_else(|error| panic!("snapshot plan: {error}")); - assert_eq!(plan.clip_names, ["recording-0.webm", "log-0.slog", "log-1.slog"]); + assert_eq!(plan.artifact_names, ["recording-0.webm", "log-0.slog", "log-1.slog"]); } #[tokio::test] @@ -1304,7 +1304,7 @@ mod tests { let plan = RecordingZipPlan { manifest_bytes: b"{}".to_vec(), - clip_names: vec!["missing-clip.webm".to_owned()], + artifact_names: vec!["missing-clip.webm".to_owned()], }; let (_shutdown_handle, shutdown_signal) = devolutions_gateway_task::ShutdownHandle::new(); let error = recording_zip_body(dir_path, plan, Uuid::nil(), shutdown_signal) @@ -1340,16 +1340,16 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let dir_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); - let mut clip_names = Vec::with_capacity(MAX_RECORDING_ZIP_FILES); + let mut artifact_names = Vec::with_capacity(MAX_RECORDING_ZIP_FILES); for index in 0..MAX_RECORDING_ZIP_FILES { let name = format!("f-{index}.bin"); tokio::fs::write(dir_path.join(&name), b"x").await.expect("write file"); - clip_names.push(name); + artifact_names.push(name); } let plan = RecordingZipPlan { manifest_bytes: b"{}".to_vec(), - clip_names, + artifact_names, }; let error = enforce_recording_zip_limits(&dir_path, &plan) .await From 9f48746a25867b9e08a8c0b650f433a829e57cc3 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 28 Sep 2026 15:52:02 -0400 Subject: [PATCH 04/24] feat(dgw): push session logs via materialType=log Routing .slog pushes into `logs` by file type changed where existing clients' .slog streams land, which breaks backward compatibility. JREC push now takes an optional `materialType` query parameter: `recording` (the default when absent) or `log`. Recording material requires `fileType` and is stored in `files` as before, including `slog`. Log material rejects `fileType`, is opaque to Gateway, and is stored in `logs` as `log-N.slog`, even before any recording exists. Invalid combinations are refused with HTTP 400 before the upgrade. --- devolutions-gateway/src/api/jrec.rs | 70 ++++++- devolutions-gateway/src/recording.intent.md | 15 ++ devolutions-gateway/src/recording.rs | 197 ++++++++++---------- 3 files changed, 178 insertions(+), 104 deletions(-) create mode 100644 devolutions-gateway/src/recording.intent.md diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 842fd56bc..80732b92f 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -30,7 +30,7 @@ use crate::DgwState; use crate::api::heartbeat::recording_storage_health; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{PushOutcome, RecordingMessageSender}; +use crate::recording::{PushMaterial, PushOutcome, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -64,7 +64,31 @@ pub fn make_router(state: DgwState) -> Router { #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { - file_type: RecordingFileType, + file_type: Option, + material_type: Option, +} + +#[derive(Deserialize, Clone, Copy)] +#[serde(rename_all = "lowercase")] +enum MaterialType { + Recording, + Log, +} + +impl JrecPushQueryParam { + fn material(&self) -> Result { + match (self.material_type, self.file_type) { + // A missing material type means recording, as it did before log material existed. + (None | Some(MaterialType::Recording), Some(file_type)) => Ok(PushMaterial::Recording(file_type)), + (None | Some(MaterialType::Recording), None) => { + Err(HttpError::bad_request().msg("fileType is required for recording material")) + } + (Some(MaterialType::Log), None) => Ok(PushMaterial::Log), + (Some(MaterialType::Log), Some(_)) => { + Err(HttpError::bad_request().msg("fileType is not allowed for log material")) + } + } + } } #[derive(Deserialize)] @@ -91,6 +115,8 @@ async fn jrec_push( return Err(HttpError::forbidden().msg("expected push operation")); } + let material = query.material()?; + let conf = conf_handle.get_conf(); // Pre-flight: refuse the upgrade up-front when the recording storage cannot accept @@ -137,7 +163,7 @@ async fn jrec_push( recordings, shutdown_signal, claims, - query.file_type, + material, session_id, source_addr, Duration::from_secs(conf_handle.get_conf().debug.ws_keep_alive_interval), @@ -153,7 +179,7 @@ async fn handle_jrec_push( recordings: RecordingMessageSender, shutdown_signal: ShutdownSignal, claims: JrecTokenClaims, - file_type: RecordingFileType, + material: PushMaterial, session_id: Uuid, source_addr: SocketAddr, keep_alive_interval: Duration, @@ -168,7 +194,7 @@ async fn handle_jrec_push( .client_stream(stream) .recordings(recordings) .claims(claims) - .file_type(file_type) + .material(material) .session_id(session_id) .shutdown_signal(shutdown_signal) .build() @@ -1031,6 +1057,40 @@ mod tests { use super::*; + #[test] + fn push_material_from_query() { + let material = |query: serde_json::Value| { + serde_json::from_value::(query) + .expect("query") + .material() + .map_err(|error| error.code) + }; + + let webm = Ok(PushMaterial::Recording(RecordingFileType::WebM)); + let slog = Ok(PushMaterial::Recording(RecordingFileType::SessionRecordingLog)); + assert_eq!(material(serde_json::json!({ "fileType": "webm" })), webm); + assert_eq!(material(serde_json::json!({ "fileType": "slog" })), slog); + assert_eq!( + material(serde_json::json!({ "fileType": "webm", "materialType": "recording" })), + webm + ); + assert_eq!( + material(serde_json::json!({ "materialType": "log" })), + Ok(PushMaterial::Log) + ); + + let bad_request = Err(StatusCode::BAD_REQUEST); + assert_eq!(material(serde_json::json!({})), bad_request); + assert_eq!( + material(serde_json::json!({ "materialType": "recording" })), + bad_request + ); + assert_eq!( + material(serde_json::json!({ "fileType": "slog", "materialType": "log" })), + bad_request + ); + } + #[test] fn rejects_unsafe_recording_file_names() { assert!(is_safe_recording_file_name("recording-0.webm")); diff --git a/devolutions-gateway/src/recording.intent.md b/devolutions-gateway/src/recording.intent.md new file mode 100644 index 000000000..f3318c2bd --- /dev/null +++ b/devolutions-gateway/src/recording.intent.md @@ -0,0 +1,15 @@ +# Background +Recording in Gateway has always been simple. +The source pushes a stream into Gateway, and Gateway persists it to disk. +Now we would like to add a new feature, AI and machine generated logs to improve searchability. + + +# Logs +Recording manifest should now have a new field called `logs`. +We define material as a file that is pushed to Gateway. +We will have two material types, `recording` and `log`. +To keep everything backward compatible, we will accept `materialType` as a query parameter, and when it is null, we will treat the stream as a recording. +If `materialType` is `recording`, the `fileType` param must be present, we currently have four file types, `webm`, `cast`, `trp` and `slog`. That is right, `slog` can be both a recording file type and the log itself. This is intentional to keep backward compatibility. +`fileType` is mandatory for `recording`, and rejected for `log`. +The content of the log and the recording is transparent to Gateway unless it is streamed, see the `streaming` crates. +The client doesn't own the naming of log and recording files, Gateway does with number-based naming. \ No newline at end of file diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index b5c67731d..975b9680d 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -8,7 +8,7 @@ use std::time::Duration; use anyhow::Context as _; use async_trait::async_trait; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::Utf8PathBuf; use devolutions_gateway_task::{ShutdownSignal, Task}; use futures::future::Either; use parking_lot::Mutex; @@ -54,15 +54,6 @@ struct JrecLog { } impl JrecManifest { - fn has_recording(&self) -> bool { - self.files.iter().any(|file| { - Utf8Path::new(&file.file_name) - .extension() - .and_then(RecordingFileType::from_extension) - .is_some_and(|file_type| file_type != RecordingFileType::SessionRecordingLog) - }) - } - fn read_from_file(path: impl AsRef) -> anyhow::Result { let json = std::fs::read(path)?; let manifest = serde_json::from_slice(&json)?; @@ -89,12 +80,19 @@ pub enum PushOutcome { StorageFull, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PushMaterial { + Recording(RecordingFileType), + /// Opaque to Gateway: stored as-is, whatever its content. + Log, +} + #[derive(TypedBuilder)] pub struct ClientPush { recordings: RecordingMessageSender, claims: JrecTokenClaims, client_stream: S, - file_type: RecordingFileType, + material: PushMaterial, session_id: Uuid, shutdown_signal: ShutdownSignal, } @@ -108,7 +106,7 @@ where recordings, claims, mut client_stream, - file_type, + material, session_id, mut shutdown_signal, } = self; @@ -124,7 +122,7 @@ where } }; - let (recording_file, artifact) = match recordings.connect(session_id, file_type, disconnected_ttl).await { + let (recording_file, artifact) = match recordings.connect(session_id, material, disconnected_ttl).await { Ok(connected) => connected, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); @@ -274,7 +272,7 @@ struct OnGoingRecording { enum RecordingManagerMessage { Connect { id: Uuid, - file_type: RecordingFileType, + material: PushMaterial, disconnected_ttl: Duration, channel: oneshot::Sender<(Utf8PathBuf, CurrentArtifact)>, }, @@ -307,13 +305,13 @@ impl fmt::Debug for RecordingManagerMessage { match self { RecordingManagerMessage::Connect { id, - file_type, + material, disconnected_ttl, channel: _, } => f .debug_struct("Connect") .field("id", id) - .field("file_type", file_type) + .field("material", material) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), @@ -350,14 +348,14 @@ impl RecordingMessageSender { async fn connect( &self, id: Uuid, - file_type: RecordingFileType, + material: PushMaterial, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { id, - file_type, + material, disconnected_ttl, channel: tx, }) @@ -534,7 +532,7 @@ impl RecordingManagerTask { async fn handle_connect( &mut self, id: Uuid, - file_type: RecordingFileType, + material: PushMaterial, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { const LENGTH_WARNING_THRESHOLD: usize = 1000; @@ -548,39 +546,12 @@ impl RecordingManagerTask { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let (manifest, recording_file, artifact) = if recording_path.exists() { - debug!(path = %recording_path, "Recording directory already exists"); - - let mut existing_manifest = - JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")?; - - let (file_name, artifact) = - if file_type == RecordingFileType::SessionRecordingLog && existing_manifest.has_recording() { - let idx = existing_manifest.logs.len(); - let file_name = format!("log-{idx}.{}", file_type.extension()); - existing_manifest.logs.push(JrecLog { - file_name: file_name.clone(), - }); - (file_name, CurrentArtifact::Log(idx)) - } else { - let idx = existing_manifest.files.len(); - let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - let file_name = format!("recording-{idx}.{}", file_type.extension()); - existing_manifest.files.push(JrecFile { - start_time, - duration: 0, - file_name: file_name.clone(), - }); - (file_name, CurrentArtifact::Recording(idx)) - }; + let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - let recording_file = recording_path.join(&file_name); - - existing_manifest - .save_to_file(&manifest_path) - .context("override existing manifest")?; + let mut manifest = if recording_path.exists() { + debug!(path = %recording_path, "Recording directory already exists"); - (existing_manifest, recording_file, artifact) + JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")? } else { debug!(path = %recording_path, "Create recording directory"); @@ -588,31 +559,42 @@ impl RecordingManagerTask { .await .with_context(|| format!("failed to create recording path: {recording_path}"))?; - let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - let file_name = format!("recording-0.{}", file_type.extension()); - let recording_file = recording_path.join(&file_name); - - let first_file = JrecFile { - start_time, - duration: 0, - file_name, - }; - - let initial_manifest = JrecManifest { + JrecManifest { session_id: id, start_time, duration: 0, - files: vec![first_file], + files: Vec::new(), logs: Vec::new(), - }; - - initial_manifest - .save_to_file(&manifest_path) - .context("write initial manifest to disk")?; + } + }; - (initial_manifest, recording_file, CurrentArtifact::Recording(0)) + let (file_name, artifact) = match material { + PushMaterial::Recording(file_type) => { + let idx = manifest.files.len(); + let file_name = format!("recording-{idx}.{}", file_type.extension()); + manifest.files.push(JrecFile { + start_time, + duration: 0, + file_name: file_name.clone(), + }); + (file_name, CurrentArtifact::Recording(idx)) + } + PushMaterial::Log => { + let idx = manifest.logs.len(); + let file_name = format!("log-{idx}.slog"); + manifest.logs.push(JrecLog { + file_name: file_name.clone(), + }); + (file_name, CurrentArtifact::Log(idx)) + } }; + let recording_file = recording_path.join(&file_name); + + manifest + .save_to_file(&manifest_path) + .context("write manifest to disk")?; + let active_recording_count = self.rx.active_recordings.insert(id); // NOTE: the session associated to this recording is not always running through the Devolutions Gateway. @@ -838,8 +820,8 @@ async fn recording_manager_task( debug!(?msg, "Received message"); match msg { - RecordingManagerMessage::Connect { id, file_type, disconnected_ttl, channel } => { - match manager.handle_connect(id, file_type, disconnected_ttl).await { + RecordingManagerMessage::Connect { id, material, disconnected_ttl, channel } => { + match manager.handle_connect(id, material, disconnected_ttl).await { Ok(connected) => { let _ = channel.send(connected); } @@ -1019,6 +1001,9 @@ mod tests { use super::*; + const WEBM: PushMaterial = PushMaterial::Recording(RecordingFileType::WebM); + const SLOG_RECORDING: PushMaterial = PushMaterial::Recording(RecordingFileType::SessionRecordingLog); + const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", "startTime": 1, @@ -1077,16 +1062,16 @@ mod tests { .expect("parse manifest") } - async fn connect(&self, id: Uuid, file_type: RecordingFileType) -> String { + async fn connect(&self, id: Uuid, material: PushMaterial) -> String { let (path, _) = self .sender - .connect(id, file_type, Duration::ZERO) + .connect(id, material, Duration::ZERO) .await .expect("connect"); path.file_name().expect("file name").to_owned() } - async fn client_push_wakes_streamers(&self, id: Uuid, file_type: RecordingFileType) -> bool { + async fn client_push_wakes_streamers(&self, id: Uuid, material: PushMaterial) -> bool { let claims = serde_json::from_value(json!({ "jet_aid": id, "jet_rop": "push", @@ -1101,7 +1086,7 @@ mod tests { .recordings(self.sender.clone()) .claims(claims) .client_stream(server) - .file_type(file_type) + .material(material) .session_id(id) .shutdown_signal(shutdown_signal) .build() @@ -1137,34 +1122,37 @@ mod tests { self.sender.get_count().await.expect("sync with manager"); } - async fn push(&self, id: Uuid, file_type: RecordingFileType) -> String { - let file_name = self.connect(id, file_type).await; + async fn push(&self, id: Uuid, material: PushMaterial) -> String { + let file_name = self.connect(id, material).await; self.disconnect(id).await; file_name } } #[tokio::test] - async fn log_only_session_lists_log_in_files() { + async fn slog_recording_stays_in_files() { let harness = Harness::start(); let id = Uuid::new_v4(); - let file_name = harness.push(id, RecordingFileType::SessionRecordingLog).await; + let first = harness.push(id, SLOG_RECORDING).await; + harness.push(id, WEBM).await; + let third = harness.push(id, SLOG_RECORDING).await; - assert_eq!(file_name, "recording-0.slog"); + assert_eq!(first, "recording-0.slog"); + assert_eq!(third, "recording-2.slog"); let manifest = harness.read_manifest(id); - assert_eq!(manifest["files"][0]["fileName"], "recording-0.slog"); + assert_eq!(manifest["files"][2]["fileName"], "recording-2.slog"); assert!(manifest.get("logs").is_none()); } #[tokio::test] - async fn log_pushed_after_recording_goes_to_logs() { + async fn log_material_goes_to_logs() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, RecordingFileType::WebM).await; - let first_log = harness.push(id, RecordingFileType::SessionRecordingLog).await; - let second_log = harness.push(id, RecordingFileType::SessionRecordingLog).await; + harness.push(id, WEBM).await; + let first_log = harness.push(id, PushMaterial::Log).await; + let second_log = harness.push(id, PushMaterial::Log).await; assert_eq!(first_log, "log-0.slog"); assert_eq!(second_log, "log-1.slog"); @@ -1178,13 +1166,28 @@ mod tests { ); } + #[tokio::test] + async fn log_before_any_recording() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + + let log = harness.push(id, PushMaterial::Log).await; + + assert_eq!(log, "log-0.slog"); + let manifest = harness.read_manifest(id); + assert_eq!(manifest["files"], json!([])); + assert_eq!(manifest["logs"], json!([{ "fileName": "log-0.slog" }])); + + assert_eq!(harness.push(id, WEBM).await, "recording-0.webm"); + } + #[tokio::test] async fn log_push_keeps_recording_durations() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, RecordingFileType::SessionRecordingLog).await; + harness.push(id, PushMaterial::Log).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["duration"], 5); @@ -1195,10 +1198,10 @@ mod tests { async fn recording_reconnect_keeps_logs() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, RecordingFileType::WebM).await; - harness.push(id, RecordingFileType::SessionRecordingLog).await; + harness.push(id, WEBM).await; + harness.push(id, PushMaterial::Log).await; - let file_name = harness.connect(id, RecordingFileType::WebM).await; + let file_name = harness.connect(id, WEBM).await; assert_eq!(file_name, "recording-1.webm"); assert_eq!(harness.read_manifest(id)["logs"], json!([{ "fileName": "log-0.slog" }])); @@ -1212,7 +1215,7 @@ mod tests { async fn shadow_streams_ongoing_recording() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.connect(id, RecordingFileType::WebM).await; + harness.connect(id, WEBM).await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1225,8 +1228,8 @@ mod tests { async fn shadow_refuses_while_only_a_log_is_pushed() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, RecordingFileType::WebM).await; - harness.connect(id, RecordingFileType::SessionRecordingLog).await; + harness.push(id, WEBM).await; + harness.connect(id, PushMaterial::Log).await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1237,22 +1240,18 @@ mod tests { async fn log_chunks_do_not_wake_streamers() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, RecordingFileType::WebM).await; + harness.push(id, WEBM).await; - assert!( - !harness - .client_push_wakes_streamers(id, RecordingFileType::SessionRecordingLog) - .await - ); + assert!(!harness.client_push_wakes_streamers(id, PushMaterial::Log).await); } #[tokio::test] - async fn log_only_session_slog_chunks_wake_streamers() { + async fn slog_recording_chunks_wake_streamers() { let harness = Harness::start(); assert!( harness - .client_push_wakes_streamers(Uuid::new_v4(), RecordingFileType::SessionRecordingLog) + .client_push_wakes_streamers(Uuid::new_v4(), SLOG_RECORDING) .await ); } From aea2d4e891d9aef519a3d8ad7f4f6cb08e2387fa Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 28 Sep 2026 17:08:00 -0400 Subject: [PATCH 05/24] refactor(dgw): push session logs on their own route Logs are pushed to `/jet/jrec/push/{id}/logs` instead of `?materialType=log`, so a log push has no `fileType` to validate. `/jet/jrec/push/{id}?fileType=...` is unchanged, including `slog`. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 99 +++++++++++------------------ 1 file changed, 37 insertions(+), 62 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 80732b92f..25620756e 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -49,7 +49,8 @@ const MAX_RECORDING_ZIP_BYTES: u64 = 2 * 1024 * 1024 * 1024; pub fn make_router(state: DgwState) -> Router { Router::new() - .route("/push/{id}", get(jrec_push)) + .route("/push/{id}", get(jrec_push_recording)) + .route("/push/{id}/logs", get(jrec_push_log)) .route("/delete/{id}", delete(jrec_delete)) .route("/delete", delete(jrec_delete_many)) .route("/list", get(list_recordings)) @@ -64,31 +65,7 @@ pub fn make_router(state: DgwState) -> Router { #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { - file_type: Option, - material_type: Option, -} - -#[derive(Deserialize, Clone, Copy)] -#[serde(rename_all = "lowercase")] -enum MaterialType { - Recording, - Log, -} - -impl JrecPushQueryParam { - fn material(&self) -> Result { - match (self.material_type, self.file_type) { - // A missing material type means recording, as it did before log material existed. - (None | Some(MaterialType::Recording), Some(file_type)) => Ok(PushMaterial::Recording(file_type)), - (None | Some(MaterialType::Recording), None) => { - Err(HttpError::bad_request().msg("fileType is required for recording material")) - } - (Some(MaterialType::Log), None) => Ok(PushMaterial::Log), - (Some(MaterialType::Log), Some(_)) => { - Err(HttpError::bad_request().msg("fileType is not allowed for log material")) - } - } - } + file_type: RecordingFileType, } #[derive(Deserialize)] @@ -98,25 +75,45 @@ pub(crate) struct JrecListQueryParam { active: bool, } +async fn jrec_push_recording( + State(state): State, + JrecToken(claims): JrecToken, + Query(query): Query, + extract::Path(session_id): extract::Path, + ConnectInfo(source_addr): ConnectInfo, + ws: WebSocketUpgrade, +) -> Result { + let material = PushMaterial::Recording(query.file_type); + jrec_push(state, claims, material, session_id, source_addr, ws).await +} + +async fn jrec_push_log( + State(state): State, + JrecToken(claims): JrecToken, + extract::Path(session_id): extract::Path, + ConnectInfo(source_addr): ConnectInfo, + ws: WebSocketUpgrade, +) -> Result { + jrec_push(state, claims, PushMaterial::Log, session_id, source_addr, ws).await +} + async fn jrec_push( - State(DgwState { + DgwState { shutdown_signal, recordings, conf_handle, .. - }): State, - JrecToken(claims): JrecToken, - Query(query): Query, - extract::Path(session_id): extract::Path, - ConnectInfo(source_addr): ConnectInfo, + }: DgwState, + claims: JrecTokenClaims, + material: PushMaterial, + session_id: Uuid, + source_addr: SocketAddr, ws: WebSocketUpgrade, ) -> Result { if claims.jet_rop != RecordingOperation::Push { return Err(HttpError::forbidden().msg("expected push operation")); } - let material = query.material()?; - let conf = conf_handle.get_conf(); // Pre-flight: refuse the upgrade up-front when the recording storage cannot accept @@ -1058,37 +1055,15 @@ mod tests { use super::*; #[test] - fn push_material_from_query() { - let material = |query: serde_json::Value| { - serde_json::from_value::(query) - .expect("query") - .material() - .map_err(|error| error.code) - }; - - let webm = Ok(PushMaterial::Recording(RecordingFileType::WebM)); - let slog = Ok(PushMaterial::Recording(RecordingFileType::SessionRecordingLog)); - assert_eq!(material(serde_json::json!({ "fileType": "webm" })), webm); - assert_eq!(material(serde_json::json!({ "fileType": "slog" })), slog); - assert_eq!( - material(serde_json::json!({ "fileType": "webm", "materialType": "recording" })), - webm - ); - assert_eq!( - material(serde_json::json!({ "materialType": "log" })), - Ok(PushMaterial::Log) - ); + fn recording_push_requires_a_file_type() { + let file_type = + |query: serde_json::Value| serde_json::from_value::(query).map(|query| query.file_type); - let bad_request = Err(StatusCode::BAD_REQUEST); - assert_eq!(material(serde_json::json!({})), bad_request); - assert_eq!( - material(serde_json::json!({ "materialType": "recording" })), - bad_request - ); assert_eq!( - material(serde_json::json!({ "fileType": "slog", "materialType": "log" })), - bad_request + file_type(serde_json::json!({ "fileType": "slog" })).expect("slog"), + RecordingFileType::SessionRecordingLog ); + assert!(file_type(serde_json::json!({})).is_err()); } #[test] From 0b61db5b6b2ab9df5fb824b7924f6074a402bb01 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 10:41:54 -0400 Subject: [PATCH 06/24] refactor(dgw): push session logs with category=log Follows Benoit's route 2: one push route, `?fileType=slog&category=log` stores the stream in `logs`, and a missing `category` still means recording. Keeping `fileType` for logs leaves room for other log formats; only `slog` is accepted today. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 84 ++++++++++++++++------------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 25620756e..1a4efca9e 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -49,8 +49,7 @@ const MAX_RECORDING_ZIP_BYTES: u64 = 2 * 1024 * 1024 * 1024; pub fn make_router(state: DgwState) -> Router { Router::new() - .route("/push/{id}", get(jrec_push_recording)) - .route("/push/{id}/logs", get(jrec_push_log)) + .route("/push/{id}", get(jrec_push)) .route("/delete/{id}", delete(jrec_delete)) .route("/delete", delete(jrec_delete_many)) .route("/list", get(list_recordings)) @@ -66,6 +65,25 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, + category: Option, +} + +#[derive(Deserialize, Clone, Copy)] +#[serde(rename_all = "lowercase")] +enum PushCategory { + Recording, + Log, +} + +impl JrecPushQueryParam { + fn material(&self) -> Result { + match (self.category, self.file_type) { + // A missing category means recording, as it did before logs existed. + (None | Some(PushCategory::Recording), file_type) => Ok(PushMaterial::Recording(file_type)), + (Some(PushCategory::Log), RecordingFileType::SessionRecordingLog) => Ok(PushMaterial::Log), + (Some(PushCategory::Log), _) => Err(HttpError::bad_request().msg("only slog files can be pushed as logs")), + } + } } #[derive(Deserialize)] @@ -75,45 +93,25 @@ pub(crate) struct JrecListQueryParam { active: bool, } -async fn jrec_push_recording( - State(state): State, - JrecToken(claims): JrecToken, - Query(query): Query, - extract::Path(session_id): extract::Path, - ConnectInfo(source_addr): ConnectInfo, - ws: WebSocketUpgrade, -) -> Result { - let material = PushMaterial::Recording(query.file_type); - jrec_push(state, claims, material, session_id, source_addr, ws).await -} - -async fn jrec_push_log( - State(state): State, - JrecToken(claims): JrecToken, - extract::Path(session_id): extract::Path, - ConnectInfo(source_addr): ConnectInfo, - ws: WebSocketUpgrade, -) -> Result { - jrec_push(state, claims, PushMaterial::Log, session_id, source_addr, ws).await -} - async fn jrec_push( - DgwState { + State(DgwState { shutdown_signal, recordings, conf_handle, .. - }: DgwState, - claims: JrecTokenClaims, - material: PushMaterial, - session_id: Uuid, - source_addr: SocketAddr, + }): State, + JrecToken(claims): JrecToken, + Query(query): Query, + extract::Path(session_id): extract::Path, + ConnectInfo(source_addr): ConnectInfo, ws: WebSocketUpgrade, ) -> Result { if claims.jet_rop != RecordingOperation::Push { return Err(HttpError::forbidden().msg("expected push operation")); } + let material = query.material()?; + let conf = conf_handle.get_conf(); // Pre-flight: refuse the upgrade up-front when the recording storage cannot accept @@ -1055,15 +1053,29 @@ mod tests { use super::*; #[test] - fn recording_push_requires_a_file_type() { - let file_type = - |query: serde_json::Value| serde_json::from_value::(query).map(|query| query.file_type); + fn push_material_from_query() { + let material = |query: serde_json::Value| { + serde_json::from_value::(query) + .expect("query") + .material() + .map_err(|error| error.code) + }; + let slog_recording = Ok(PushMaterial::Recording(RecordingFileType::SessionRecordingLog)); + assert_eq!(material(serde_json::json!({ "fileType": "slog" })), slog_recording); + assert_eq!( + material(serde_json::json!({ "fileType": "slog", "category": "recording" })), + slog_recording + ); + assert_eq!( + material(serde_json::json!({ "fileType": "slog", "category": "log" })), + Ok(PushMaterial::Log) + ); assert_eq!( - file_type(serde_json::json!({ "fileType": "slog" })).expect("slog"), - RecordingFileType::SessionRecordingLog + material(serde_json::json!({ "fileType": "webm", "category": "log" })), + Err(StatusCode::BAD_REQUEST) ); - assert!(file_type(serde_json::json!({})).is_err()); + assert!(serde_json::from_value::(serde_json::json!({ "category": "log" })).is_err()); } #[test] From c72b6f7d9fc46fae4bcb85bef112248abb3b16bc Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 10:49:59 -0400 Subject: [PATCH 07/24] refactor(dgw): call the pushed file kind a category and keep the log file type `PushMaterial` becomes `PushCategory`, matching the `category` query parameter. A log push now carries its file type and is named after its extension (`log-N.`), so only the API decides which formats may be pushed as logs; today that is `slog`. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 42 ++++++++--------- devolutions-gateway/src/recording.rs | 67 ++++++++++++++-------------- 2 files changed, 56 insertions(+), 53 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 1a4efca9e..598bbc488 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -30,7 +30,7 @@ use crate::DgwState; use crate::api::heartbeat::recording_storage_health; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{PushMaterial, PushOutcome, RecordingMessageSender}; +use crate::recording::{PushCategory, PushOutcome, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -65,23 +65,25 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, - category: Option, + category: Option, } #[derive(Deserialize, Clone, Copy)] #[serde(rename_all = "lowercase")] -enum PushCategory { +enum CategoryParam { Recording, Log, } impl JrecPushQueryParam { - fn material(&self) -> Result { + fn category(&self) -> Result { match (self.category, self.file_type) { // A missing category means recording, as it did before logs existed. - (None | Some(PushCategory::Recording), file_type) => Ok(PushMaterial::Recording(file_type)), - (Some(PushCategory::Log), RecordingFileType::SessionRecordingLog) => Ok(PushMaterial::Log), - (Some(PushCategory::Log), _) => Err(HttpError::bad_request().msg("only slog files can be pushed as logs")), + (None | Some(CategoryParam::Recording), file_type) => Ok(PushCategory::Recording(file_type)), + (Some(CategoryParam::Log), RecordingFileType::SessionRecordingLog) => { + Ok(PushCategory::Log(RecordingFileType::SessionRecordingLog)) + } + (Some(CategoryParam::Log), _) => Err(HttpError::bad_request().msg("only slog files can be pushed as logs")), } } } @@ -110,7 +112,7 @@ async fn jrec_push( return Err(HttpError::forbidden().msg("expected push operation")); } - let material = query.material()?; + let category = query.category()?; let conf = conf_handle.get_conf(); @@ -158,7 +160,7 @@ async fn jrec_push( recordings, shutdown_signal, claims, - material, + category, session_id, source_addr, Duration::from_secs(conf_handle.get_conf().debug.ws_keep_alive_interval), @@ -174,7 +176,7 @@ async fn handle_jrec_push( recordings: RecordingMessageSender, shutdown_signal: ShutdownSignal, claims: JrecTokenClaims, - material: PushMaterial, + category: PushCategory, session_id: Uuid, source_addr: SocketAddr, keep_alive_interval: Duration, @@ -189,7 +191,7 @@ async fn handle_jrec_push( .client_stream(stream) .recordings(recordings) .claims(claims) - .material(material) + .category(category) .session_id(session_id) .shutdown_signal(shutdown_signal) .build() @@ -1053,26 +1055,26 @@ mod tests { use super::*; #[test] - fn push_material_from_query() { - let material = |query: serde_json::Value| { + fn push_category_from_query() { + let category = |query: serde_json::Value| { serde_json::from_value::(query) .expect("query") - .material() + .category() .map_err(|error| error.code) }; - let slog_recording = Ok(PushMaterial::Recording(RecordingFileType::SessionRecordingLog)); - assert_eq!(material(serde_json::json!({ "fileType": "slog" })), slog_recording); + let slog_recording = Ok(PushCategory::Recording(RecordingFileType::SessionRecordingLog)); + assert_eq!(category(serde_json::json!({ "fileType": "slog" })), slog_recording); assert_eq!( - material(serde_json::json!({ "fileType": "slog", "category": "recording" })), + category(serde_json::json!({ "fileType": "slog", "category": "recording" })), slog_recording ); assert_eq!( - material(serde_json::json!({ "fileType": "slog", "category": "log" })), - Ok(PushMaterial::Log) + category(serde_json::json!({ "fileType": "slog", "category": "log" })), + Ok(PushCategory::Log(RecordingFileType::SessionRecordingLog)) ); assert_eq!( - material(serde_json::json!({ "fileType": "webm", "category": "log" })), + category(serde_json::json!({ "fileType": "webm", "category": "log" })), Err(StatusCode::BAD_REQUEST) ); assert!(serde_json::from_value::(serde_json::json!({ "category": "log" })).is_err()); diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 975b9680d..3309789bf 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -81,10 +81,10 @@ pub enum PushOutcome { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PushMaterial { +pub enum PushCategory { Recording(RecordingFileType), /// Opaque to Gateway: stored as-is, whatever its content. - Log, + Log(RecordingFileType), } #[derive(TypedBuilder)] @@ -92,7 +92,7 @@ pub struct ClientPush { recordings: RecordingMessageSender, claims: JrecTokenClaims, client_stream: S, - material: PushMaterial, + category: PushCategory, session_id: Uuid, shutdown_signal: ShutdownSignal, } @@ -106,7 +106,7 @@ where recordings, claims, mut client_stream, - material, + category, session_id, mut shutdown_signal, } = self; @@ -122,7 +122,7 @@ where } }; - let (recording_file, artifact) = match recordings.connect(session_id, material, disconnected_ttl).await { + let (recording_file, artifact) = match recordings.connect(session_id, category, disconnected_ttl).await { Ok(connected) => connected, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); @@ -272,7 +272,7 @@ struct OnGoingRecording { enum RecordingManagerMessage { Connect { id: Uuid, - material: PushMaterial, + category: PushCategory, disconnected_ttl: Duration, channel: oneshot::Sender<(Utf8PathBuf, CurrentArtifact)>, }, @@ -305,13 +305,13 @@ impl fmt::Debug for RecordingManagerMessage { match self { RecordingManagerMessage::Connect { id, - material, + category, disconnected_ttl, channel: _, } => f .debug_struct("Connect") .field("id", id) - .field("material", material) + .field("category", category) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), @@ -348,14 +348,14 @@ impl RecordingMessageSender { async fn connect( &self, id: Uuid, - material: PushMaterial, + category: PushCategory, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { id, - material, + category, disconnected_ttl, channel: tx, }) @@ -532,7 +532,7 @@ impl RecordingManagerTask { async fn handle_connect( &mut self, id: Uuid, - material: PushMaterial, + category: PushCategory, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { const LENGTH_WARNING_THRESHOLD: usize = 1000; @@ -568,8 +568,8 @@ impl RecordingManagerTask { } }; - let (file_name, artifact) = match material { - PushMaterial::Recording(file_type) => { + let (file_name, artifact) = match category { + PushCategory::Recording(file_type) => { let idx = manifest.files.len(); let file_name = format!("recording-{idx}.{}", file_type.extension()); manifest.files.push(JrecFile { @@ -579,9 +579,9 @@ impl RecordingManagerTask { }); (file_name, CurrentArtifact::Recording(idx)) } - PushMaterial::Log => { + PushCategory::Log(file_type) => { let idx = manifest.logs.len(); - let file_name = format!("log-{idx}.slog"); + let file_name = format!("log-{idx}.{}", file_type.extension()); manifest.logs.push(JrecLog { file_name: file_name.clone(), }); @@ -820,8 +820,8 @@ async fn recording_manager_task( debug!(?msg, "Received message"); match msg { - RecordingManagerMessage::Connect { id, material, disconnected_ttl, channel } => { - match manager.handle_connect(id, material, disconnected_ttl).await { + RecordingManagerMessage::Connect { id, category, disconnected_ttl, channel } => { + match manager.handle_connect(id, category, disconnected_ttl).await { Ok(connected) => { let _ = channel.send(connected); } @@ -1001,8 +1001,9 @@ mod tests { use super::*; - const WEBM: PushMaterial = PushMaterial::Recording(RecordingFileType::WebM); - const SLOG_RECORDING: PushMaterial = PushMaterial::Recording(RecordingFileType::SessionRecordingLog); + const WEBM: PushCategory = PushCategory::Recording(RecordingFileType::WebM); + const SLOG_RECORDING: PushCategory = PushCategory::Recording(RecordingFileType::SessionRecordingLog); + const SLOG_LOG: PushCategory = PushCategory::Log(RecordingFileType::SessionRecordingLog); const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", @@ -1062,16 +1063,16 @@ mod tests { .expect("parse manifest") } - async fn connect(&self, id: Uuid, material: PushMaterial) -> String { + async fn connect(&self, id: Uuid, category: PushCategory) -> String { let (path, _) = self .sender - .connect(id, material, Duration::ZERO) + .connect(id, category, Duration::ZERO) .await .expect("connect"); path.file_name().expect("file name").to_owned() } - async fn client_push_wakes_streamers(&self, id: Uuid, material: PushMaterial) -> bool { + async fn client_push_wakes_streamers(&self, id: Uuid, category: PushCategory) -> bool { let claims = serde_json::from_value(json!({ "jet_aid": id, "jet_rop": "push", @@ -1086,7 +1087,7 @@ mod tests { .recordings(self.sender.clone()) .claims(claims) .client_stream(server) - .material(material) + .category(category) .session_id(id) .shutdown_signal(shutdown_signal) .build() @@ -1122,8 +1123,8 @@ mod tests { self.sender.get_count().await.expect("sync with manager"); } - async fn push(&self, id: Uuid, material: PushMaterial) -> String { - let file_name = self.connect(id, material).await; + async fn push(&self, id: Uuid, category: PushCategory) -> String { + let file_name = self.connect(id, category).await; self.disconnect(id).await; file_name } @@ -1146,13 +1147,13 @@ mod tests { } #[tokio::test] - async fn log_material_goes_to_logs() { + async fn log_category_goes_to_logs() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - let first_log = harness.push(id, PushMaterial::Log).await; - let second_log = harness.push(id, PushMaterial::Log).await; + let first_log = harness.push(id, SLOG_LOG).await; + let second_log = harness.push(id, SLOG_LOG).await; assert_eq!(first_log, "log-0.slog"); assert_eq!(second_log, "log-1.slog"); @@ -1171,7 +1172,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let log = harness.push(id, PushMaterial::Log).await; + let log = harness.push(id, SLOG_LOG).await; assert_eq!(log, "log-0.slog"); let manifest = harness.read_manifest(id); @@ -1187,7 +1188,7 @@ mod tests { let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, PushMaterial::Log).await; + harness.push(id, SLOG_LOG).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["duration"], 5); @@ -1199,7 +1200,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - harness.push(id, PushMaterial::Log).await; + harness.push(id, SLOG_LOG).await; let file_name = harness.connect(id, WEBM).await; assert_eq!(file_name, "recording-1.webm"); @@ -1229,7 +1230,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - harness.connect(id, PushMaterial::Log).await; + harness.connect(id, SLOG_LOG).await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1242,7 +1243,7 @@ mod tests { let id = Uuid::new_v4(); harness.push(id, WEBM).await; - assert!(!harness.client_push_wakes_streamers(id, PushMaterial::Log).await); + assert!(!harness.client_push_wakes_streamers(id, SLOG_LOG).await); } #[tokio::test] From e9e9a402fa5e68a19b70a2e53ffae6c68a2debaa Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 12:01:01 -0400 Subject: [PATCH 08/24] feat(dgw): list non-recording artifacts by role in the manifest Replaces the `logs` list with an `artifacts` object keyed by role, as discussed with Benoit: `?fileType=slog&artifact=ai-analysis` stores the stream as `ai-analysis-N.slog` under `artifacts["ai-analysis"]`. A push without `artifact` is a recording and lands in `files` as before, `slog` included. Roles are a fixed enum for pushes, but the manifest keeps its keys as strings so roles written by a newer Gateway survive a rewrite. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 80 +++++++++++-------- devolutions-gateway/src/recording.rs | 114 +++++++++++++++++++-------- 2 files changed, 124 insertions(+), 70 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 598bbc488..2dcbaa14f 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::fs; use std::io::{self, Seek as _, Write as _}; use std::net::SocketAddr; @@ -30,13 +31,13 @@ use crate::DgwState; use crate::api::heartbeat::recording_storage_health; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{PushCategory, PushOutcome, RecordingMessageSender}; +use crate::recording::{ArtifactRole, PushCategory, PushOutcome, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. const ZIP_CHUNK_SIZE: usize = 64 * 1024; -/// Maximum files in a session ZIP (`recording.json` + every Artifact listed, files and logs). +/// Maximum files in a session ZIP (`recording.json` + every Artifact listed, files and artifacts). /// /// Reconnect windows only mint a small number of clips per session in practice; /// this bound blocks pathological manifests without rejecting normal multi-clip packages. @@ -65,25 +66,20 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, - category: Option, -} - -#[derive(Deserialize, Clone, Copy)] -#[serde(rename_all = "lowercase")] -enum CategoryParam { - Recording, - Log, + artifact: Option, } impl JrecPushQueryParam { fn category(&self) -> Result { - match (self.category, self.file_type) { - // A missing category means recording, as it did before logs existed. - (None | Some(CategoryParam::Recording), file_type) => Ok(PushCategory::Recording(file_type)), - (Some(CategoryParam::Log), RecordingFileType::SessionRecordingLog) => { - Ok(PushCategory::Log(RecordingFileType::SessionRecordingLog)) + match (self.artifact, self.file_type) { + // Without an artifact role the stream is a recording, as it was before artifacts existed. + (None, file_type) => Ok(PushCategory::Recording(file_type)), + (Some(role @ ArtifactRole::AiAnalysis), file_type @ RecordingFileType::SessionRecordingLog) => { + Ok(PushCategory::Artifact(role, file_type)) + } + (Some(ArtifactRole::AiAnalysis), _) => { + Err(HttpError::bad_request().msg("ai-analysis artifacts must be slog files")) } - (Some(CategoryParam::Log), _) => Err(HttpError::bad_request().msg("only slog files can be pushed as logs")), } } } @@ -657,7 +653,7 @@ where struct RecordingZipManifest { files: Vec, #[serde(default)] - logs: Vec, + artifacts: BTreeMap>, } #[derive(Debug, Deserialize)] @@ -697,7 +693,7 @@ impl RecordingZipPlan { } } -/// Snapshots `recording.json` and every Artifact it lists (files and logs) at call time. +/// Snapshots `recording.json` and every Artifact it lists (files and artifacts) at call time. async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result { let manifest_path = recording_dir.join("recording.json"); let manifest_bytes = tokio::fs::read(&manifest_path).await.map_err(|error| { @@ -720,8 +716,13 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result(); + let mut artifact_names = Vec::with_capacity(manifest.files.len() + artifact_count); + for file in manifest + .files + .into_iter() + .chain(manifest.artifacts.into_values().flatten()) + { if !is_safe_recording_file_name(&file.file_name) { warn!( file_name = %file.file_name, @@ -1063,21 +1064,25 @@ mod tests { .map_err(|error| error.code) }; - let slog_recording = Ok(PushCategory::Recording(RecordingFileType::SessionRecordingLog)); - assert_eq!(category(serde_json::json!({ "fileType": "slog" })), slog_recording); assert_eq!( - category(serde_json::json!({ "fileType": "slog", "category": "recording" })), - slog_recording + category(serde_json::json!({ "fileType": "slog" })), + Ok(PushCategory::Recording(RecordingFileType::SessionRecordingLog)) ); assert_eq!( - category(serde_json::json!({ "fileType": "slog", "category": "log" })), - Ok(PushCategory::Log(RecordingFileType::SessionRecordingLog)) + category(serde_json::json!({ "fileType": "slog", "artifact": "ai-analysis" })), + Ok(PushCategory::Artifact( + ArtifactRole::AiAnalysis, + RecordingFileType::SessionRecordingLog + )) ); assert_eq!( - category(serde_json::json!({ "fileType": "webm", "category": "log" })), + category(serde_json::json!({ "fileType": "webm", "artifact": "ai-analysis" })), Err(StatusCode::BAD_REQUEST) ); - assert!(serde_json::from_value::(serde_json::json!({ "category": "log" })).is_err()); + + let parse = |query: serde_json::Value| serde_json::from_value::(query); + assert!(parse(serde_json::json!({ "artifact": "ai-analysis" })).is_err()); + assert!(parse(serde_json::json!({ "fileType": "slog", "artifact": "unknown" })).is_err()); } #[test] @@ -1150,7 +1155,7 @@ mod tests { } #[tokio::test] - async fn snapshots_manifest_logs_for_zip() { + async fn snapshots_manifest_artifacts_for_zip() { let dir = tempfile::tempdir().expect("temp dir"); let dir_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); @@ -1161,16 +1166,18 @@ mod tests { "files": [ { "fileName": "recording-0.webm", "startTime": 1, "duration": 5 } ], - "logs": [ - { "fileName": "log-0.slog" }, - { "fileName": "log-1.slog" } - ] + "artifacts": { + "ai-analysis": [ + { "fileName": "ai-analysis-0.slog" }, + { "fileName": "ai-analysis-1.slog" } + ] + } }); tokio::fs::write(dir_path.join("recording.json"), manifest.to_string()) .await .expect("write manifest"); - for file_name in ["recording-0.webm", "log-0.slog", "log-1.slog"] { + for file_name in ["recording-0.webm", "ai-analysis-0.slog", "ai-analysis-1.slog"] { tokio::fs::write(dir_path.join(file_name), b"content") .await .expect("write artifact"); @@ -1179,7 +1186,10 @@ mod tests { let plan = snapshot_recording_zip_plan(&dir_path) .await .unwrap_or_else(|error| panic!("snapshot plan: {error}")); - assert_eq!(plan.artifact_names, ["recording-0.webm", "log-0.slog", "log-1.slog"]); + assert_eq!( + plan.artifact_names, + ["recording-0.webm", "ai-analysis-0.slog", "ai-analysis-1.slog"] + ); } #[tokio::test] diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 3309789bf..a17da8c57 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -1,6 +1,6 @@ use core::fmt; use std::cmp; -use std::collections::{BinaryHeap, HashMap, HashSet}; +use std::collections::{BTreeMap, BinaryHeap, HashMap, HashSet}; use std::path::Path; use std::pin::pin; use std::sync::Arc; @@ -42,17 +42,34 @@ struct JrecManifest { start_time: i64, duration: i64, files: Vec, - /// Append-only, like `files`: artifact names and `CurrentArtifact` indices are derived from positions. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - logs: Vec, + /// Non-recording artifacts grouped by role. Each list is append-only, like `files`: names and + /// `CurrentArtifact` indices are derived from positions. Keys stay strings so roles written by a + /// newer Gateway survive a rewrite. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + artifacts: BTreeMap>, } #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -struct JrecLog { +struct JrecArtifact { file_name: String, } +/// Role of a non-recording artifact, used as its key in the manifest `artifacts` object. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum ArtifactRole { + AiAnalysis, +} + +impl ArtifactRole { + pub const fn as_str(self) -> &'static str { + match self { + ArtifactRole::AiAnalysis => "ai-analysis", + } + } +} + impl JrecManifest { fn read_from_file(path: impl AsRef) -> anyhow::Result { let json = std::fs::read(path)?; @@ -84,7 +101,7 @@ pub enum PushOutcome { pub enum PushCategory { Recording(RecordingFileType), /// Opaque to Gateway: stored as-is, whatever its content. - Log(RecordingFileType), + Artifact(ArtifactRole, RecordingFileType), } #[derive(TypedBuilder)] @@ -256,7 +273,7 @@ pub enum OnGoingRecordingState { #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum CurrentArtifact { Recording(usize), - Log(usize), + Artifact(ArtifactRole, usize), } #[derive(Debug, Clone)] @@ -564,7 +581,7 @@ impl RecordingManagerTask { start_time, duration: 0, files: Vec::new(), - logs: Vec::new(), + artifacts: BTreeMap::new(), } }; @@ -579,13 +596,14 @@ impl RecordingManagerTask { }); (file_name, CurrentArtifact::Recording(idx)) } - PushCategory::Log(file_type) => { - let idx = manifest.logs.len(); - let file_name = format!("log-{idx}.{}", file_type.extension()); - manifest.logs.push(JrecLog { + PushCategory::Artifact(role, file_type) => { + let artifacts = manifest.artifacts.entry(role.as_str().to_owned()).or_default(); + let idx = artifacts.len(); + let file_name = format!("{}-{idx}.{}", role.as_str(), file_type.extension()); + artifacts.push(JrecArtifact { file_name: file_name.clone(), }); - (file_name, CurrentArtifact::Log(idx)) + (file_name, CurrentArtifact::Artifact(role, idx)) } }; @@ -657,7 +675,7 @@ impl RecordingManagerTask { ¤t_file.file_name } - CurrentArtifact::Log(idx) => &ongoing.manifest.logs[idx].file_name, + CurrentArtifact::Artifact(role, idx) => &ongoing.manifest.artifacts[role.as_str()][idx].file_name, }; let recording_file_path = ongoing @@ -875,7 +893,7 @@ async fn recording_manager_task( }, RecordingManagerMessage::ListFiles { id, channel } => { match manager.ongoing_recordings.get(&id) { - Some(recording) if matches!(recording.artifact, CurrentArtifact::Log(_)) => { + Some(recording) if matches!(recording.artifact, CurrentArtifact::Artifact(..)) => { let _ = channel.send(None); } Some(recording) => { @@ -1003,7 +1021,8 @@ mod tests { const WEBM: PushCategory = PushCategory::Recording(RecordingFileType::WebM); const SLOG_RECORDING: PushCategory = PushCategory::Recording(RecordingFileType::SessionRecordingLog); - const SLOG_LOG: PushCategory = PushCategory::Log(RecordingFileType::SessionRecordingLog); + const AI_ANALYSIS: PushCategory = + PushCategory::Artifact(ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog); const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", @@ -1143,27 +1162,27 @@ mod tests { assert_eq!(third, "recording-2.slog"); let manifest = harness.read_manifest(id); assert_eq!(manifest["files"][2]["fileName"], "recording-2.slog"); - assert!(manifest.get("logs").is_none()); + assert!(manifest.get("artifacts").is_none()); } #[tokio::test] - async fn log_category_goes_to_logs() { + async fn ai_analysis_goes_to_artifacts() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - let first_log = harness.push(id, SLOG_LOG).await; - let second_log = harness.push(id, SLOG_LOG).await; + let first_log = harness.push(id, AI_ANALYSIS).await; + let second_log = harness.push(id, AI_ANALYSIS).await; - assert_eq!(first_log, "log-0.slog"); - assert_eq!(second_log, "log-1.slog"); + assert_eq!(first_log, "ai-analysis-0.slog"); + assert_eq!(second_log, "ai-analysis-1.slog"); let manifest = harness.read_manifest(id); let files = manifest["files"].as_array().expect("files"); assert_eq!(files.len(), 1); assert_eq!(files[0]["fileName"], "recording-0.webm"); assert_eq!( - manifest["logs"], - json!([{ "fileName": "log-0.slog" }, { "fileName": "log-1.slog" }]) + manifest["artifacts"], + json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }, { "fileName": "ai-analysis-1.slog" }] }) ); } @@ -1172,12 +1191,15 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let log = harness.push(id, SLOG_LOG).await; + let log = harness.push(id, AI_ANALYSIS).await; - assert_eq!(log, "log-0.slog"); + assert_eq!(log, "ai-analysis-0.slog"); let manifest = harness.read_manifest(id); assert_eq!(manifest["files"], json!([])); - assert_eq!(manifest["logs"], json!([{ "fileName": "log-0.slog" }])); + assert_eq!( + manifest["artifacts"], + json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) + ); assert_eq!(harness.push(id, WEBM).await, "recording-0.webm"); } @@ -1188,7 +1210,7 @@ mod tests { let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, SLOG_LOG).await; + harness.push(id, AI_ANALYSIS).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["duration"], 5); @@ -1196,20 +1218,26 @@ mod tests { } #[tokio::test] - async fn recording_reconnect_keeps_logs() { + async fn recording_reconnect_keeps_artifacts() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - harness.push(id, SLOG_LOG).await; + harness.push(id, AI_ANALYSIS).await; let file_name = harness.connect(id, WEBM).await; assert_eq!(file_name, "recording-1.webm"); - assert_eq!(harness.read_manifest(id)["logs"], json!([{ "fileName": "log-0.slog" }])); + assert_eq!( + harness.read_manifest(id)["artifacts"], + json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) + ); harness.disconnect(id).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["files"][1]["fileName"], "recording-1.webm"); - assert_eq!(manifest["logs"], json!([{ "fileName": "log-0.slog" }])); + assert_eq!( + manifest["artifacts"], + json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) + ); } #[tokio::test] @@ -1230,7 +1258,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); harness.push(id, WEBM).await; - harness.connect(id, SLOG_LOG).await; + harness.connect(id, AI_ANALYSIS).await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1243,7 +1271,7 @@ mod tests { let id = Uuid::new_v4(); harness.push(id, WEBM).await; - assert!(!harness.client_push_wakes_streamers(id, SLOG_LOG).await); + assert!(!harness.client_push_wakes_streamers(id, AI_ANALYSIS).await); } #[tokio::test] @@ -1257,8 +1285,24 @@ mod tests { ); } + #[tokio::test] + async fn unknown_artifact_roles_survive_a_rewrite() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + let mut manifest: serde_json::Value = serde_json::from_str(MASTER_MANIFEST).expect("manifest"); + manifest["artifacts"] = json!({ "future-role": [{ "fileName": "future-role-0.bin" }] }); + harness.write_manifest(id, &manifest.to_string()); + + harness.push(id, WEBM).await; + + assert_eq!( + harness.read_manifest(id)["artifacts"], + json!({ "future-role": [{ "fileName": "future-role-0.bin" }] }) + ); + } + #[test] - fn manifest_without_logs_round_trips_byte_for_byte() { + fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); let path = dir.path().join("recording.json"); std::fs::write(&path, MASTER_MANIFEST).expect("write manifest"); From 577a807b4b89ad5eee3f5fc9a2b45874e1140545 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 13:07:39 -0400 Subject: [PATCH 09/24] refactor(dgw): name the push destination PushTarget and pin role names `PushCategory` is a leftover of the `category` parameter; the query now uses `artifact`, so the destination is a `PushTarget`. A test checks that each role's manifest key matches its query value. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 32 +++++++-------- devolutions-gateway/src/recording.rs | 61 +++++++++++++++------------- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 2dcbaa14f..1896cc580 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -31,7 +31,7 @@ use crate::DgwState; use crate::api::heartbeat::recording_storage_health; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{ArtifactRole, PushCategory, PushOutcome, RecordingMessageSender}; +use crate::recording::{ArtifactRole, PushOutcome, PushTarget, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -70,12 +70,12 @@ struct JrecPushQueryParam { } impl JrecPushQueryParam { - fn category(&self) -> Result { + fn target(&self) -> Result { match (self.artifact, self.file_type) { // Without an artifact role the stream is a recording, as it was before artifacts existed. - (None, file_type) => Ok(PushCategory::Recording(file_type)), + (None, file_type) => Ok(PushTarget::Recording(file_type)), (Some(role @ ArtifactRole::AiAnalysis), file_type @ RecordingFileType::SessionRecordingLog) => { - Ok(PushCategory::Artifact(role, file_type)) + Ok(PushTarget::Artifact(role, file_type)) } (Some(ArtifactRole::AiAnalysis), _) => { Err(HttpError::bad_request().msg("ai-analysis artifacts must be slog files")) @@ -108,7 +108,7 @@ async fn jrec_push( return Err(HttpError::forbidden().msg("expected push operation")); } - let category = query.category()?; + let target = query.target()?; let conf = conf_handle.get_conf(); @@ -156,7 +156,7 @@ async fn jrec_push( recordings, shutdown_signal, claims, - category, + target, session_id, source_addr, Duration::from_secs(conf_handle.get_conf().debug.ws_keep_alive_interval), @@ -172,7 +172,7 @@ async fn handle_jrec_push( recordings: RecordingMessageSender, shutdown_signal: ShutdownSignal, claims: JrecTokenClaims, - category: PushCategory, + target: PushTarget, session_id: Uuid, source_addr: SocketAddr, keep_alive_interval: Duration, @@ -187,7 +187,7 @@ async fn handle_jrec_push( .client_stream(stream) .recordings(recordings) .claims(claims) - .category(category) + .target(target) .session_id(session_id) .shutdown_signal(shutdown_signal) .build() @@ -1056,27 +1056,27 @@ mod tests { use super::*; #[test] - fn push_category_from_query() { - let category = |query: serde_json::Value| { + fn push_target_from_query() { + let target = |query: serde_json::Value| { serde_json::from_value::(query) .expect("query") - .category() + .target() .map_err(|error| error.code) }; assert_eq!( - category(serde_json::json!({ "fileType": "slog" })), - Ok(PushCategory::Recording(RecordingFileType::SessionRecordingLog)) + target(serde_json::json!({ "fileType": "slog" })), + Ok(PushTarget::Recording(RecordingFileType::SessionRecordingLog)) ); assert_eq!( - category(serde_json::json!({ "fileType": "slog", "artifact": "ai-analysis" })), - Ok(PushCategory::Artifact( + target(serde_json::json!({ "fileType": "slog", "artifact": "ai-analysis" })), + Ok(PushTarget::Artifact( ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog )) ); assert_eq!( - category(serde_json::json!({ "fileType": "webm", "artifact": "ai-analysis" })), + target(serde_json::json!({ "fileType": "webm", "artifact": "ai-analysis" })), Err(StatusCode::BAD_REQUEST) ); diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index a17da8c57..8d3c1b101 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -98,7 +98,7 @@ pub enum PushOutcome { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PushCategory { +pub enum PushTarget { Recording(RecordingFileType), /// Opaque to Gateway: stored as-is, whatever its content. Artifact(ArtifactRole, RecordingFileType), @@ -109,7 +109,7 @@ pub struct ClientPush { recordings: RecordingMessageSender, claims: JrecTokenClaims, client_stream: S, - category: PushCategory, + target: PushTarget, session_id: Uuid, shutdown_signal: ShutdownSignal, } @@ -123,7 +123,7 @@ where recordings, claims, mut client_stream, - category, + target, session_id, mut shutdown_signal, } = self; @@ -139,7 +139,7 @@ where } }; - let (recording_file, artifact) = match recordings.connect(session_id, category, disconnected_ttl).await { + let (recording_file, artifact) = match recordings.connect(session_id, target, disconnected_ttl).await { Ok(connected) => connected, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); @@ -289,7 +289,7 @@ struct OnGoingRecording { enum RecordingManagerMessage { Connect { id: Uuid, - category: PushCategory, + target: PushTarget, disconnected_ttl: Duration, channel: oneshot::Sender<(Utf8PathBuf, CurrentArtifact)>, }, @@ -322,13 +322,13 @@ impl fmt::Debug for RecordingManagerMessage { match self { RecordingManagerMessage::Connect { id, - category, + target, disconnected_ttl, channel: _, } => f .debug_struct("Connect") .field("id", id) - .field("category", category) + .field("target", target) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), @@ -365,14 +365,14 @@ impl RecordingMessageSender { async fn connect( &self, id: Uuid, - category: PushCategory, + target: PushTarget, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { id, - category, + target, disconnected_ttl, channel: tx, }) @@ -549,7 +549,7 @@ impl RecordingManagerTask { async fn handle_connect( &mut self, id: Uuid, - category: PushCategory, + target: PushTarget, disconnected_ttl: Duration, ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { const LENGTH_WARNING_THRESHOLD: usize = 1000; @@ -585,8 +585,8 @@ impl RecordingManagerTask { } }; - let (file_name, artifact) = match category { - PushCategory::Recording(file_type) => { + let (file_name, artifact) = match target { + PushTarget::Recording(file_type) => { let idx = manifest.files.len(); let file_name = format!("recording-{idx}.{}", file_type.extension()); manifest.files.push(JrecFile { @@ -596,7 +596,7 @@ impl RecordingManagerTask { }); (file_name, CurrentArtifact::Recording(idx)) } - PushCategory::Artifact(role, file_type) => { + PushTarget::Artifact(role, file_type) => { let artifacts = manifest.artifacts.entry(role.as_str().to_owned()).or_default(); let idx = artifacts.len(); let file_name = format!("{}-{idx}.{}", role.as_str(), file_type.extension()); @@ -838,8 +838,8 @@ async fn recording_manager_task( debug!(?msg, "Received message"); match msg { - RecordingManagerMessage::Connect { id, category, disconnected_ttl, channel } => { - match manager.handle_connect(id, category, disconnected_ttl).await { + RecordingManagerMessage::Connect { id, target, disconnected_ttl, channel } => { + match manager.handle_connect(id, target, disconnected_ttl).await { Ok(connected) => { let _ = channel.send(connected); } @@ -1019,10 +1019,10 @@ mod tests { use super::*; - const WEBM: PushCategory = PushCategory::Recording(RecordingFileType::WebM); - const SLOG_RECORDING: PushCategory = PushCategory::Recording(RecordingFileType::SessionRecordingLog); - const AI_ANALYSIS: PushCategory = - PushCategory::Artifact(ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog); + const WEBM: PushTarget = PushTarget::Recording(RecordingFileType::WebM); + const SLOG_RECORDING: PushTarget = PushTarget::Recording(RecordingFileType::SessionRecordingLog); + const AI_ANALYSIS: PushTarget = + PushTarget::Artifact(ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog); const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", @@ -1082,16 +1082,12 @@ mod tests { .expect("parse manifest") } - async fn connect(&self, id: Uuid, category: PushCategory) -> String { - let (path, _) = self - .sender - .connect(id, category, Duration::ZERO) - .await - .expect("connect"); + async fn connect(&self, id: Uuid, target: PushTarget) -> String { + let (path, _) = self.sender.connect(id, target, Duration::ZERO).await.expect("connect"); path.file_name().expect("file name").to_owned() } - async fn client_push_wakes_streamers(&self, id: Uuid, category: PushCategory) -> bool { + async fn client_push_wakes_streamers(&self, id: Uuid, target: PushTarget) -> bool { let claims = serde_json::from_value(json!({ "jet_aid": id, "jet_rop": "push", @@ -1106,7 +1102,7 @@ mod tests { .recordings(self.sender.clone()) .claims(claims) .client_stream(server) - .category(category) + .target(target) .session_id(id) .shutdown_signal(shutdown_signal) .build() @@ -1142,8 +1138,8 @@ mod tests { self.sender.get_count().await.expect("sync with manager"); } - async fn push(&self, id: Uuid, category: PushCategory) -> String { - let file_name = self.connect(id, category).await; + async fn push(&self, id: Uuid, target: PushTarget) -> String { + let file_name = self.connect(id, target).await; self.disconnect(id).await; file_name } @@ -1285,6 +1281,13 @@ mod tests { ); } + #[test] + fn artifact_role_names_match_their_serde_names() { + let role = ArtifactRole::AiAnalysis; + let parsed: ArtifactRole = serde_json::from_value(json!(role.as_str())).expect("parse role"); + assert_eq!(parsed, role); + } + #[tokio::test] async fn unknown_artifact_roles_survive_a_rewrite() { let harness = Harness::start(); From 296aeb8ec2ee053ce6aad951a5d4523895e956bc Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 13:29:55 -0400 Subject: [PATCH 10/24] fix(dgw): name the artifact query param kind and keep artifacts out of the recording policy The push query takes `kind=` instead of `artifact=`, as written in recording.intent.md. An artifact push no longer satisfies a session's recording policy: `EnsureRecordingPolicyTask` now asks `is_recording`, which only counts Recording pushes, so pushing an ai-analysis log alone can't keep a session that must be recorded alive. An artifact push also never carries the policy, so its end doesn't kill the session. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 14 ++++----- devolutions-gateway/src/recording.rs | 43 +++++++++++++++++++++++++++- devolutions-gateway/src/session.rs | 6 ++-- 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 1896cc580..1c73562ab 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -66,13 +66,13 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, - artifact: Option, + kind: Option, } impl JrecPushQueryParam { fn target(&self) -> Result { - match (self.artifact, self.file_type) { - // Without an artifact role the stream is a recording, as it was before artifacts existed. + match (self.kind, self.file_type) { + // Without a kind the stream is a recording, as it was before artifacts existed. (None, file_type) => Ok(PushTarget::Recording(file_type)), (Some(role @ ArtifactRole::AiAnalysis), file_type @ RecordingFileType::SessionRecordingLog) => { Ok(PushTarget::Artifact(role, file_type)) @@ -1069,20 +1069,20 @@ mod tests { Ok(PushTarget::Recording(RecordingFileType::SessionRecordingLog)) ); assert_eq!( - target(serde_json::json!({ "fileType": "slog", "artifact": "ai-analysis" })), + target(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })), Ok(PushTarget::Artifact( ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog )) ); assert_eq!( - target(serde_json::json!({ "fileType": "webm", "artifact": "ai-analysis" })), + target(serde_json::json!({ "fileType": "webm", "kind": "ai-analysis" })), Err(StatusCode::BAD_REQUEST) ); let parse = |query: serde_json::Value| serde_json::from_value::(query); - assert!(parse(serde_json::json!({ "artifact": "ai-analysis" })).is_err()); - assert!(parse(serde_json::json!({ "fileType": "slog", "artifact": "unknown" })).is_err()); + assert!(parse(serde_json::json!({ "kind": "ai-analysis" })).is_err()); + assert!(parse(serde_json::json!({ "fileType": "slog", "kind": "unknown" })).is_err()); } #[test] diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 8d3c1b101..5458a9eb3 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -300,6 +300,10 @@ enum RecordingManagerMessage { id: Uuid, channel: oneshot::Sender>, }, + IsRecording { + id: Uuid, + channel: oneshot::Sender, + }, ListFiles { id: Uuid, channel: oneshot::Sender>>, @@ -335,6 +339,9 @@ impl fmt::Debug for RecordingManagerMessage { RecordingManagerMessage::GetState { id, channel: _ } => { f.debug_struct("GetState").field("id", id).finish_non_exhaustive() } + RecordingManagerMessage::IsRecording { id, channel: _ } => { + f.debug_struct("IsRecording").field("id", id).finish_non_exhaustive() + } RecordingManagerMessage::GetCount { channel: _ } => f.debug_struct("GetCount").finish_non_exhaustive(), RecordingManagerMessage::UpdateRecordingPolicy { id, @@ -401,6 +408,17 @@ impl RecordingMessageSender { rx.await.context("couldn't receive recording state") } + /// Returns whether a Recording, not an artifact, is being pushed for this session. + pub async fn is_recording(&self, id: Uuid) -> anyhow::Result { + let (tx, rx) = oneshot::channel(); + self.channel + .send(RecordingManagerMessage::IsRecording { id, channel: tx }) + .await + .ok() + .context("couldn't send IsRecording message")?; + rx.await.context("couldn't receive whether a recording is ongoing") + } + pub async fn get_count(&self) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel @@ -626,7 +644,9 @@ impl RecordingManagerTask { .ok() .flatten() .map(|info| info.recording_policy) - .unwrap_or(false); + .unwrap_or(false) + // An artifact is not the session recording, so its end must not count against the recording policy. + && matches!(artifact, CurrentArtifact::Recording(_)); self.ongoing_recordings.insert( id, @@ -870,6 +890,13 @@ async fn recording_manager_task( let response = manager.ongoing_recordings.get(&id).map(|ongoing| ongoing.state.clone()); let _ = channel.send(response); } + RecordingManagerMessage::IsRecording { id, channel } => { + let is_recording = manager + .ongoing_recordings + .get(&id) + .is_some_and(|ongoing| matches!(ongoing.artifact, CurrentArtifact::Recording(_))); + let _ = channel.send(is_recording); + } RecordingManagerMessage::GetCount { channel } => { let _ = channel.send(manager.ongoing_recordings.len()); } @@ -1288,6 +1315,20 @@ mod tests { assert_eq!(parsed, role); } + #[tokio::test] + async fn only_a_recording_push_counts_as_recording() { + let harness = Harness::start(); + let artifact_only = Uuid::new_v4(); + let recorded = Uuid::new_v4(); + + harness.connect(artifact_only, AI_ANALYSIS).await; + harness.connect(recorded, WEBM).await; + + assert!(!harness.sender.is_recording(artifact_only).await.expect("is recording")); + assert!(harness.sender.is_recording(recorded).await.expect("is recording")); + assert!(!harness.sender.is_recording(Uuid::new_v4()).await.expect("is recording")); + } + #[tokio::test] async fn unknown_artifact_roles_survive_a_rewrite() { let harness = Harness::start(); diff --git a/devolutions-gateway/src/session.rs b/devolutions-gateway/src/session.rs index ef0b1582a..6fd56131b 100644 --- a/devolutions-gateway/src/session.rs +++ b/devolutions-gateway/src/session.rs @@ -610,11 +610,9 @@ impl Task for EnsureRecordingPolicyTask { let is_recording = self .recording_manager_handle - .get_state(self.session_id) + .is_recording(self.session_id) .await - .ok() - .flatten() - .is_some(); + .unwrap_or(false); if is_recording { let _ = self From f1c439ff0aa841a9c7e42aa318a080fb7025dd72 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 14:21:43 -0400 Subject: [PATCH 11/24] refactor(dgw): track pushes per kind so artifacts never touch the recording policy Ongoing pushes are now keyed by (session, kind). Before, an artifact push replaced the session's single entry, which dropped a disconnected must-be-recorded Recording's TTL kill and blocked its reconnect. - Only the Recording kind drives the recording policy, active_recordings, /shadow wake-ups and manifest timing; artifact kinds touch none of them. - A Recording and an artifact may push at the same time; one push per kind. - The manager reads and writes the manifest from disk for each change, so concurrent pushes keep each other's entries. - PushTarget::new owns the "ai-analysis needs slog" rule. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 26 +- devolutions-gateway/src/recording.rs | 716 ++++++++++++++++++--------- devolutions-gateway/src/session.rs | 24 + 3 files changed, 527 insertions(+), 239 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 1c73562ab..831e72644 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -31,7 +31,7 @@ use crate::DgwState; use crate::api::heartbeat::recording_storage_health; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{ArtifactRole, PushOutcome, PushTarget, RecordingMessageSender}; +use crate::recording::{ArtifactKind, PushOutcome, PushTarget, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -66,21 +66,12 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, - kind: Option, + kind: Option, } impl JrecPushQueryParam { fn target(&self) -> Result { - match (self.kind, self.file_type) { - // Without a kind the stream is a recording, as it was before artifacts existed. - (None, file_type) => Ok(PushTarget::Recording(file_type)), - (Some(role @ ArtifactRole::AiAnalysis), file_type @ RecordingFileType::SessionRecordingLog) => { - Ok(PushTarget::Artifact(role, file_type)) - } - (Some(ArtifactRole::AiAnalysis), _) => { - Err(HttpError::bad_request().msg("ai-analysis artifacts must be slog files")) - } - } + PushTarget::new(self.file_type, self.kind).map_err(HttpError::bad_request().err()) } } @@ -1021,7 +1012,7 @@ async fn shadow_recording( let recording_files = match recordings.list_files(id).await { Ok(Some(recording_files)) => recording_files, Ok(None) => { - debug!(%id, "Shadow recording rejected: only a Log is being pushed"); + debug!(%id, "Shadow recording rejected: no recording is being pushed"); return close_with_error(ws, StreamerCloseCode::StreamingEnded); } Err(_) => { @@ -1054,6 +1045,7 @@ mod tests { use zip::ZipArchive; use super::*; + use crate::recording::PushKind; #[test] fn push_target_from_query() { @@ -1061,19 +1053,17 @@ mod tests { serde_json::from_value::(query) .expect("query") .target() + .map(PushTarget::kind) .map_err(|error| error.code) }; assert_eq!( target(serde_json::json!({ "fileType": "slog" })), - Ok(PushTarget::Recording(RecordingFileType::SessionRecordingLog)) + Ok(PushKind::Recording) ); assert_eq!( target(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })), - Ok(PushTarget::Artifact( - ArtifactRole::AiAnalysis, - RecordingFileType::SessionRecordingLog - )) + Ok(PushKind::Artifact(ArtifactKind::AiAnalysis)) ); assert_eq!( target(serde_json::json!({ "fileType": "webm", "kind": "ai-analysis" })), diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 5458a9eb3..a302b6fb6 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -42,9 +42,8 @@ struct JrecManifest { start_time: i64, duration: i64, files: Vec, - /// Non-recording artifacts grouped by role. Each list is append-only, like `files`: names and - /// `CurrentArtifact` indices are derived from positions. Keys stay strings so roles written by a - /// newer Gateway survive a rewrite. + /// Non-recording artifacts grouped by kind. Each list is append-only, like `files`: names are + /// derived from positions. Keys stay strings so kinds written by a newer Gateway survive a rewrite. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] artifacts: BTreeMap>, } @@ -55,17 +54,58 @@ struct JrecArtifact { file_name: String, } -/// Role of a non-recording artifact, used as its key in the manifest `artifacts` object. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +/// Kind of a non-recording artifact, used as its key in the manifest `artifacts` object. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Deserialize)] #[serde(rename_all = "kebab-case")] -pub enum ArtifactRole { +pub enum ArtifactKind { AiAnalysis, } -impl ArtifactRole { +impl ArtifactKind { pub const fn as_str(self) -> &'static str { match self { - ArtifactRole::AiAnalysis => "ai-analysis", + ArtifactKind::AiAnalysis => "ai-analysis", + } + } + + const fn file_type(self) -> RecordingFileType { + match self { + ArtifactKind::AiAnalysis => RecordingFileType::SessionRecordingLog, + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum PushKind { + Recording, + Artifact(ArtifactKind), +} + +/// What a push of a given kind takes part in, decided once from the kind. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct KindPolicy { + /// Counts as the session recording: `is_recording`, `active_recordings` and the kill when the + /// session must be recorded. + recording_policy: bool, + /// Data and end of stream wake `/shadow` streamers. + wakes_streamers: bool, + /// Writes the file and manifest durations. + tracks_duration: bool, +} + +impl PushKind { + const fn policy(self) -> KindPolicy { + match self { + PushKind::Recording => KindPolicy { + recording_policy: true, + wakes_streamers: true, + tracks_duration: true, + }, + PushKind::Artifact(_) => KindPolicy { + recording_policy: false, + wakes_streamers: false, + tracks_duration: false, + }, } } } @@ -97,11 +137,34 @@ pub enum PushOutcome { StorageFull, } +/// Where a push is stored. Artifacts are opaque to Gateway: stored as-is, whatever their content. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PushTarget { - Recording(RecordingFileType), - /// Opaque to Gateway: stored as-is, whatever its content. - Artifact(ArtifactRole, RecordingFileType), +pub struct PushTarget { + kind: PushKind, + file_type: RecordingFileType, +} + +#[derive(Debug, thiserror::Error)] +#[error("{} artifacts must be {} files", kind.as_str(), kind.file_type().extension())] +pub struct UnsupportedArtifactFileType { + kind: ArtifactKind, +} + +impl PushTarget { + /// Without a kind the stream is a recording, as it was before artifacts existed. + pub fn new(file_type: RecordingFileType, kind: Option) -> Result { + let kind = match kind { + None => PushKind::Recording, + Some(kind) if kind.file_type() == file_type => PushKind::Artifact(kind), + Some(kind) => return Err(UnsupportedArtifactFileType { kind }), + }; + + Ok(Self { kind, file_type }) + } + + pub fn kind(self) -> PushKind { + self.kind + } } #[derive(TypedBuilder)] @@ -139,7 +202,10 @@ where } }; - let (recording_file, artifact) = match recordings.connect(session_id, target, disconnected_ttl).await { + let kind = target.kind(); + let policy = kind.policy(); + + let recording_file = match recordings.connect(session_id, target, disconnected_ttl).await { Ok(connected) => connected, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); @@ -177,8 +243,7 @@ where loop { tokio::select! { _ = flush_signal.notified() => { - // Log data is not media, so it must not wake `/shadow` streamers. - if let CurrentArtifact::Recording(_) = artifact { + if policy.wakes_streamers { recordings.new_chunk_appended(session_id)?; } }, @@ -217,7 +282,7 @@ where info!(?res, "Recording finished"); - recordings.disconnect(session_id).await.context("disconnect")?; + recordings.disconnect(session_id, kind).await.context("disconnect")?; res } @@ -270,20 +335,18 @@ pub enum OnGoingRecordingState { LastSeen { timestamp: i64 }, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum CurrentArtifact { - Recording(usize), - Artifact(ArtifactRole, usize), -} - +/// An ongoing push, one per (session, kind). +/// +/// It holds no manifest copy: pushes of other kinds may update the manifest in the meantime. #[derive(Debug, Clone)] -struct OnGoingRecording { +struct OnGoingPush { state: OnGoingRecordingState, - manifest: JrecManifest, manifest_path: Utf8PathBuf, + file_path: Utf8PathBuf, + /// Position of the pushed file in its manifest list. + index: usize, session_must_be_recorded: bool, disconnected_ttl: Duration, - artifact: CurrentArtifact, } enum RecordingManagerMessage { @@ -291,10 +354,11 @@ enum RecordingManagerMessage { id: Uuid, target: PushTarget, disconnected_ttl: Duration, - channel: oneshot::Sender<(Utf8PathBuf, CurrentArtifact)>, + channel: oneshot::Sender, }, Disconnect { id: Uuid, + kind: PushKind, }, GetState { id: Uuid, @@ -335,7 +399,11 @@ impl fmt::Debug for RecordingManagerMessage { .field("target", target) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), - RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), + RecordingManagerMessage::Disconnect { id, kind } => f + .debug_struct("Disconnect") + .field("id", id) + .field("kind", kind) + .finish(), RecordingManagerMessage::GetState { id, channel: _ } => { f.debug_struct("GetState").field("id", id).finish_non_exhaustive() } @@ -369,12 +437,7 @@ pub struct RecordingMessageSender { } impl RecordingMessageSender { - async fn connect( - &self, - id: Uuid, - target: PushTarget, - disconnected_ttl: Duration, - ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { + async fn connect(&self, id: Uuid, target: PushTarget, disconnected_ttl: Duration) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { @@ -390,9 +453,9 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - async fn disconnect(&self, id: Uuid) -> anyhow::Result<()> { + async fn disconnect(&self, id: Uuid, kind: PushKind) -> anyhow::Result<()> { self.channel - .send(RecordingManagerMessage::Disconnect { id }) + .send(RecordingManagerMessage::Disconnect { id, kind }) .await .ok() .context("couldn't send Remove message") @@ -472,7 +535,7 @@ impl RecordingMessageSender { Ok(rx.await?) } - /// Returns `None` when the ongoing push is a Log: there is no Recording to stream. + /// Returns `None` when no Recording is being pushed, even if an artifact is. pub(crate) async fn list_files(&self, recording_id: Uuid) -> anyhow::Result>> { let (tx, rx) = oneshot::channel(); self.channel @@ -512,11 +575,12 @@ pub fn recording_message_channel() -> (RecordingMessageSender, RecordingMessageR struct DisconnectedTtl { deadline: tokio::time::Instant, id: Uuid, + kind: PushKind, } impl PartialEq for DisconnectedTtl { fn eq(&self, other: &Self) -> bool { - self.deadline.eq(&other.deadline) && self.id.eq(&other.id) + self.deadline.eq(&other.deadline) && self.id.eq(&other.id) && self.kind.eq(&other.kind) } } @@ -532,7 +596,7 @@ impl Ord for DisconnectedTtl { fn cmp(&self, other: &Self) -> cmp::Ordering { match self.deadline.cmp(&other.deadline) { cmp::Ordering::Less => cmp::Ordering::Greater, - cmp::Ordering::Equal => self.id.cmp(&other.id), + cmp::Ordering::Equal => (self.id, self.kind).cmp(&(other.id, other.kind)), cmp::Ordering::Greater => cmp::Ordering::Less, } } @@ -540,7 +604,7 @@ impl Ord for DisconnectedTtl { pub struct RecordingManagerTask { rx: RecordingMessageReceiver, - ongoing_recordings: HashMap, + ongoing_recordings: HashMap<(Uuid, PushKind), OnGoingPush>, recording_end_notifier: HashMap>, recordings_path: Utf8PathBuf, session_manager_handle: SessionMessageSender, @@ -569,13 +633,16 @@ impl RecordingManagerTask { id: Uuid, target: PushTarget, disconnected_ttl: Duration, - ) -> anyhow::Result<(Utf8PathBuf, CurrentArtifact)> { + ) -> anyhow::Result { const LENGTH_WARNING_THRESHOLD: usize = 1000; - if let Some(ongoing) = self.ongoing_recordings.get(&id) + let kind = target.kind(); + let policy = kind.policy(); + + if let Some(ongoing) = self.ongoing_recordings.get(&(id, kind)) && matches!(ongoing.state, OnGoingRecordingState::Connected) { - anyhow::bail!("concurrent recording for the same session is not supported"); + anyhow::bail!("concurrent push of the same kind for the same session is not supported"); } let recording_path = self.recordings_path.join(id.to_string()); @@ -594,69 +661,78 @@ impl RecordingManagerTask { .await .with_context(|| format!("failed to create recording path: {recording_path}"))?; + // The session timing belongs to the recordings, so the first Recording push sets it. JrecManifest { session_id: id, - start_time, + start_time: 0, duration: 0, files: Vec::new(), artifacts: BTreeMap::new(), } }; - let (file_name, artifact) = match target { - PushTarget::Recording(file_type) => { - let idx = manifest.files.len(); - let file_name = format!("recording-{idx}.{}", file_type.extension()); + let extension = target.file_type.extension(); + + let (file_name, index) = match kind { + PushKind::Recording => { + if manifest.files.is_empty() { + manifest.start_time = start_time; + } + + let index = manifest.files.len(); + let file_name = format!("recording-{index}.{extension}"); manifest.files.push(JrecFile { start_time, duration: 0, file_name: file_name.clone(), }); - (file_name, CurrentArtifact::Recording(idx)) + (file_name, index) } - PushTarget::Artifact(role, file_type) => { - let artifacts = manifest.artifacts.entry(role.as_str().to_owned()).or_default(); - let idx = artifacts.len(); - let file_name = format!("{}-{idx}.{}", role.as_str(), file_type.extension()); + PushKind::Artifact(artifact_kind) => { + let artifacts = manifest.artifacts.entry(artifact_kind.as_str().to_owned()).or_default(); + let index = artifacts.len(); + let file_name = format!("{}-{index}.{extension}", artifact_kind.as_str()); artifacts.push(JrecArtifact { file_name: file_name.clone(), }); - (file_name, CurrentArtifact::Artifact(role, idx)) + (file_name, index) } }; - let recording_file = recording_path.join(&file_name); + let file_path = recording_path.join(&file_name); manifest .save_to_file(&manifest_path) .context("write manifest to disk")?; - let active_recording_count = self.rx.active_recordings.insert(id); + let active_recording_count = if policy.recording_policy { + self.rx.active_recordings.insert(id) + } else { + 0 + }; // NOTE: the session associated to this recording is not always running through the Devolutions Gateway. // It is a normal situation when the Devolutions is used solely as a recording server. // In such cases, we can only assume there is no recording policy. - let session_must_be_recorded = self - .session_manager_handle - .get_session_info(id) - .await - .inspect_err(|error| error!(%error, session.id = %id, "Failed to retrieve session info")) - .ok() - .flatten() - .map(|info| info.recording_policy) - .unwrap_or(false) - // An artifact is not the session recording, so its end must not count against the recording policy. - && matches!(artifact, CurrentArtifact::Recording(_)); + let session_must_be_recorded = policy.recording_policy + && self + .session_manager_handle + .get_session_info(id) + .await + .inspect_err(|error| error!(%error, session.id = %id, "Failed to retrieve session info")) + .ok() + .flatten() + .is_some_and(|info| info.recording_policy); self.ongoing_recordings.insert( - id, - OnGoingRecording { + (id, kind), + OnGoingPush { state: OnGoingRecordingState::Connected, - manifest, manifest_path, + file_path: file_path.clone(), + index, session_must_be_recorded, disconnected_ttl, - artifact, }, ); let ongoing_recording_count = self.ongoing_recordings.len(); @@ -670,12 +746,14 @@ impl RecordingManagerTask { ); } - Ok((recording_file, artifact)) + Ok(file_path) } - async fn handle_disconnect(&mut self, id: Uuid) -> anyhow::Result<()> { - let Some(ongoing) = self.ongoing_recordings.get_mut(&id) else { - return Err(anyhow::anyhow!("unknown recording for ID {id}")); + async fn handle_disconnect(&mut self, id: Uuid, kind: PushKind) -> anyhow::Result<()> { + let policy = kind.policy(); + + let Some(ongoing) = self.ongoing_recordings.get_mut(&(id, kind)) else { + anyhow::bail!("unknown {kind:?} push for ID {id}"); }; if !matches!(ongoing.state, OnGoingRecordingState::Connected) { @@ -686,33 +764,32 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - let current_file_name = match ongoing.artifact { - CurrentArtifact::Recording(idx) => { - let current_file = &mut ongoing.manifest.files[idx]; - current_file.duration = end_time - current_file.start_time; + if policy.tracks_duration { + debug!(path = %ongoing.manifest_path, "Write updated manifest to disk"); - ongoing.manifest.duration = end_time - ongoing.manifest.start_time; + let mut manifest = JrecManifest::read_from_file(&ongoing.manifest_path) + .with_context(|| format!("read manifest at {}", ongoing.manifest_path))?; - ¤t_file.file_name - } - CurrentArtifact::Artifact(role, idx) => &ongoing.manifest.artifacts[role.as_str()][idx].file_name, - }; + let current_file = manifest.files.get_mut(ongoing.index).with_context(|| { + format!( + "no file at index {} in manifest {}", + ongoing.index, ongoing.manifest_path + ) + })?; + current_file.duration = end_time - current_file.start_time; - let recording_file_path = ongoing - .manifest_path - .parent() - .expect("a parent") - .join(current_file_name); + manifest.duration = end_time - manifest.start_time; - debug!(path = %ongoing.manifest_path, "Write updated manifest to disk"); + manifest + .save_to_file(&ongoing.manifest_path) + .with_context(|| format!("write manifest at {}", ongoing.manifest_path))?; + } - ongoing - .manifest - .save_to_file(&ongoing.manifest_path) - .with_context(|| format!("write manifest at {}", ongoing.manifest_path))?; + let recording_file_path = ongoing.file_path.clone(); - // Notify all the streamers that recording has ended. - if let Some(notify) = self.recording_end_notifier.get(&id) { + if policy.wakes_streamers + && let Some(notify) = self.recording_end_notifier.get(&id) + { notify.notify_waiters(); } @@ -739,8 +816,10 @@ impl RecordingManagerTask { Ok(()) } - fn handle_remove(&mut self, id: Uuid) { - if let Some(ongoing) = self.ongoing_recordings.get(&id) { + fn handle_remove(&mut self, id: Uuid, kind: PushKind) { + let policy = kind.policy(); + + if let Some(ongoing) = self.ongoing_recordings.get(&(id, kind)) { let now = time::OffsetDateTime::now_utc().unix_timestamp(); let disconnected_ttl_secs = i64::try_from(ongoing.disconnected_ttl.as_secs()).expect("TTL can’t be so big"); @@ -748,11 +827,14 @@ impl RecordingManagerTask { // NOTE: Comparing with disconnected_ttl_secs - 1 just in case the sleep returns faster than expected. // (I don’t know if this can actually happen in practice, but it’s better to be safe than sorry.) OnGoingRecordingState::LastSeen { timestamp } if now >= timestamp + disconnected_ttl_secs - 1 => { - debug!(%id, "Mark recording as terminated"); - self.rx.active_recordings.remove(id); + debug!(%id, ?kind, "Mark push as terminated"); + + if policy.recording_policy { + self.rx.active_recordings.remove(id); + } // Check the recording policy of the associated session and kill it if necessary. - if ongoing.session_must_be_recorded { + if policy.recording_policy && ongoing.session_must_be_recorded { tokio::spawn({ let session_manager_handle = self.session_manager_handle.clone(); @@ -785,11 +867,14 @@ impl RecordingManagerTask { }); } - self.ongoing_recordings.remove(&id); - self.recording_end_notifier.remove(&id); + self.ongoing_recordings.remove(&(id, kind)); + + if policy.wakes_streamers { + self.recording_end_notifier.remove(&id); + } } _ => { - trace!(%id, "Recording should not be removed yet"); + trace!(%id, ?kind, "Push should not be removed yet"); } } } @@ -797,7 +882,7 @@ impl RecordingManagerTask { fn subscribe(&mut self, id: Uuid) -> anyhow::Result> { debug!(%id, "Subscribing to ongoing recording"); - if !self.ongoing_recordings.contains_key(&id) { + if !self.ongoing_recordings.contains_key(&(id, PushKind::Recording)) { anyhow::bail!("unknown recording for ID {id}"); } @@ -809,6 +894,28 @@ impl RecordingManagerTask { Ok(notify) } } + + fn list_recording_files(&self, id: Uuid) -> anyhow::Result>> { + let Some(recording) = self.ongoing_recordings.get(&(id, PushKind::Recording)) else { + return Ok(None); + }; + + let recordings_folder = recording + .manifest_path + .parent() + .context("manifest path has no parent")?; + + let manifest = JrecManifest::read_from_file(&recording.manifest_path) + .with_context(|| format!("read manifest at {}", recording.manifest_path))?; + + let files = manifest + .files + .iter() + .map(|file| recordings_folder.join(&file.file_name)) + .collect(); + + Ok(Some(files)) + } } #[async_trait] @@ -842,7 +949,7 @@ async fn recording_manager_task( () = &mut next_remove_sleep, if !disconnected.is_empty() => { let to_remove = disconnected.pop().expect("we check for non-emptiness before entering this block"); - manager.handle_remove(to_remove.id); + manager.handle_remove(to_remove.id, to_remove.kind); // Re-arm the Sleep instance with the next deadline if required if let Some(next) = disconnected.peek() { @@ -866,18 +973,19 @@ async fn recording_manager_task( Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, - RecordingManagerMessage::Disconnect { id } => { - if let Err(e) = manager.handle_disconnect(id).await { + RecordingManagerMessage::Disconnect { id, kind } => { + if let Err(e) = manager.handle_disconnect(id, kind).await { error!(error = format!("{e:#}"), "handle_disconnect"); } - if let Some(ongoing) = manager.ongoing_recordings.get(&id) { + if let Some(ongoing) = manager.ongoing_recordings.get(&(id, kind)) { let now = tokio::time::Instant::now(); let deadline = now + ongoing.disconnected_ttl; disconnected.push(DisconnectedTtl { deadline, id, + kind, }); // Reset the Sleep instance if the new deadline is sooner or it is already elapsed. @@ -887,21 +995,21 @@ async fn recording_manager_task( } } RecordingManagerMessage::GetState { id, channel } => { - let response = manager.ongoing_recordings.get(&id).map(|ongoing| ongoing.state.clone()); + let response = manager + .ongoing_recordings + .get(&(id, PushKind::Recording)) + .map(|ongoing| ongoing.state.clone()); let _ = channel.send(response); } RecordingManagerMessage::IsRecording { id, channel } => { - let is_recording = manager - .ongoing_recordings - .get(&id) - .is_some_and(|ongoing| matches!(ongoing.artifact, CurrentArtifact::Recording(_))); + let is_recording = manager.ongoing_recordings.contains_key(&(id, PushKind::Recording)); let _ = channel.send(is_recording); } RecordingManagerMessage::GetCount { channel } => { let _ = channel.send(manager.ongoing_recordings.len()); } RecordingManagerMessage::UpdateRecordingPolicy { id, session_must_be_recorded } => { - if let Some(ongoing) = manager.ongoing_recordings.get_mut(&id) { + if let Some(ongoing) = manager.ongoing_recordings.get_mut(&(id, PushKind::Recording)) { ongoing.session_must_be_recorded = session_must_be_recorded; trace!( session.id = %id, @@ -919,25 +1027,11 @@ async fn recording_manager_task( } }, RecordingManagerMessage::ListFiles { id, channel } => { - match manager.ongoing_recordings.get(&id) { - Some(recording) if matches!(recording.artifact, CurrentArtifact::Artifact(..)) => { - let _ = channel.send(None); - } - Some(recording) => { - let recordings_folder = recording.manifest_path.parent().expect("a parent"); - - let files = recording - .manifest - .files - .iter() - .map(|file| recordings_folder.join(&file.file_name)) - .collect(); - - let _ = channel.send(Some(files)); - } - None => { - warn!(%id, "No recording found for provided ID"); + match manager.list_recording_files(id) { + Ok(files) => { + let _ = channel.send(files); } + Err(e) => error!(error = format!("{e:#}"), session.id = %id, "list recording files"), } } } @@ -968,11 +1062,11 @@ async fn recording_manager_task( }; debug!(?msg, "Received message"); - if let RecordingManagerMessage::Disconnect { id } = msg { - if let Err(e) = manager.handle_disconnect(id).await { + if let RecordingManagerMessage::Disconnect { id, kind } = msg { + if let Err(e) = manager.handle_disconnect(id, kind).await { error!(error = format!("{e:#}"), "handle_disconnect"); } - manager.ongoing_recordings.remove(&id); + manager.ongoing_recordings.remove(&(id, kind)); } } @@ -1046,10 +1140,7 @@ mod tests { use super::*; - const WEBM: PushTarget = PushTarget::Recording(RecordingFileType::WebM); - const SLOG_RECORDING: PushTarget = PushTarget::Recording(RecordingFileType::SessionRecordingLog); - const AI_ANALYSIS: PushTarget = - PushTarget::Artifact(ArtifactRole::AiAnalysis, RecordingFileType::SessionRecordingLog); + const AI_ANALYSIS_KIND: PushKind = PushKind::Artifact(ArtifactKind::AiAnalysis); const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", @@ -1064,10 +1155,23 @@ mod tests { ] }"#; + fn webm() -> PushTarget { + PushTarget::new(RecordingFileType::WebM, None).expect("webm recording") + } + + fn slog_recording() -> PushTarget { + PushTarget::new(RecordingFileType::SessionRecordingLog, None).expect("slog recording") + } + + fn ai_analysis() -> PushTarget { + PushTarget::new(RecordingFileType::SessionRecordingLog, Some(ArtifactKind::AiAnalysis)).expect("ai-analysis") + } + struct Harness { _dir: tempfile::TempDir, recordings_path: Utf8PathBuf, sender: RecordingMessageSender, + kills: mpsc::UnboundedReceiver, _shutdown_handle: ShutdownHandle, } @@ -1076,7 +1180,7 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let recordings_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); let (sender, receiver) = recording_message_channel(); - let (session_manager_handle, _) = crate::session::session_manager_channel(); + let (session_manager_handle, kills) = crate::session::spawn_fake_session_manager(); let (job_queue_handle, _) = JobQueueHandle::new(); let task = RecordingManagerTask::new( receiver, @@ -1091,6 +1195,7 @@ mod tests { _dir: dir, recordings_path, sender, + kills, _shutdown_handle: shutdown_handle, } } @@ -1109,12 +1214,68 @@ mod tests { .expect("parse manifest") } - async fn connect(&self, id: Uuid, target: PushTarget) -> String { - let (path, _) = self.sender.connect(id, target, Duration::ZERO).await.expect("connect"); + async fn try_connect(&self, id: Uuid, target: PushTarget) -> anyhow::Result { + self.sender.connect(id, target, Duration::ZERO).await + } + + async fn connect_with_ttl(&self, id: Uuid, target: PushTarget, disconnected_ttl: Duration) -> String { + let path = self + .sender + .connect(id, target, disconnected_ttl) + .await + .expect("connect"); path.file_name().expect("file name").to_owned() } - async fn client_push_wakes_streamers(&self, id: Uuid, target: PushTarget) -> bool { + async fn connect(&self, id: Uuid, target: PushTarget) -> String { + self.connect_with_ttl(id, target, Duration::ZERO).await + } + + async fn disconnect(&self, id: Uuid, kind: PushKind) { + self.sender.disconnect(id, kind).await.expect("disconnect"); + // Messages are processed in order, so this waits for the disconnect to be handled. + self.sender.get_count().await.expect("sync with manager"); + } + + async fn push(&self, id: Uuid, target: PushTarget) -> String { + let file_name = self.connect(id, target).await; + self.disconnect(id, target.kind()).await; + file_name + } + + async fn must_be_recorded(&self, id: Uuid) { + self.sender + .update_recording_policy(id, true) + .await + .expect("update recording policy"); + } + + async fn wait_until_no_push(&self) { + tokio::time::timeout(Duration::from_secs(5), async { + while self.sender.get_count().await.expect("count") != 0 { + tokio::task::yield_now().await; + } + }) + .await + .expect("expired pushes are removed"); + } + + async fn next_kill(&mut self) -> Uuid { + tokio::time::timeout(Duration::from_secs(5), self.kills.recv()) + .await + .expect("a kill request") + .expect("session manager alive") + } + + fn start_client_push( + &self, + id: Uuid, + target: PushTarget, + ) -> ( + io::DuplexStream, + tokio::task::JoinHandle>, + ShutdownHandle, + ) { let claims = serde_json::from_value(json!({ "jet_aid": id, "jet_rop": "push", @@ -1122,7 +1283,7 @@ mod tests { "jti": Uuid::new_v4(), })) .expect("claims"); - let (mut client, server) = io::duplex(1024); + let (client, server) = io::duplex(1024); let (shutdown_handle, shutdown_signal) = ShutdownHandle::new(); let push = tokio::spawn( ClientPush::builder() @@ -1135,40 +1296,7 @@ mod tests { .build() .run(), ); - - let (tx, mut woken) = oneshot::channel(); - self.sender.add_new_chunk_listener(id, tx); - - // The push flushes, and so signals, whenever it runs out of input. - let woke = tokio::time::timeout(Duration::from_secs(2), async { - loop { - client.write_all(b"chunk").await.expect("write chunk"); - tokio::select! { - _ = &mut woken => break, - () = tokio::time::sleep(Duration::from_millis(20)) => {} - } - } - }) - .await - .is_ok(); - - drop(client); - push.await.expect("join push").expect("push"); - self.sender.get_count().await.expect("sync with manager"); - drop(shutdown_handle); - woke - } - - async fn disconnect(&self, id: Uuid) { - self.sender.disconnect(id).await.expect("disconnect"); - // Messages are processed in order, so this waits for the disconnect to be handled. - self.sender.get_count().await.expect("sync with manager"); - } - - async fn push(&self, id: Uuid, target: PushTarget) -> String { - let file_name = self.connect(id, target).await; - self.disconnect(id).await; - file_name + (client, push, shutdown_handle) } } @@ -1177,9 +1305,9 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let first = harness.push(id, SLOG_RECORDING).await; - harness.push(id, WEBM).await; - let third = harness.push(id, SLOG_RECORDING).await; + let first = harness.push(id, slog_recording()).await; + harness.push(id, webm()).await; + let third = harness.push(id, slog_recording()).await; assert_eq!(first, "recording-0.slog"); assert_eq!(third, "recording-2.slog"); @@ -1193,12 +1321,12 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, WEBM).await; - let first_log = harness.push(id, AI_ANALYSIS).await; - let second_log = harness.push(id, AI_ANALYSIS).await; + harness.push(id, webm()).await; + let first_artifact = harness.push(id, ai_analysis()).await; + let second_artifact = harness.push(id, ai_analysis()).await; - assert_eq!(first_log, "ai-analysis-0.slog"); - assert_eq!(second_log, "ai-analysis-1.slog"); + assert_eq!(first_artifact, "ai-analysis-0.slog"); + assert_eq!(second_artifact, "ai-analysis-1.slog"); let manifest = harness.read_manifest(id); let files = manifest["files"].as_array().expect("files"); assert_eq!(files.len(), 1); @@ -1210,51 +1338,60 @@ mod tests { } #[tokio::test] - async fn log_before_any_recording() { + async fn artifact_first_leaves_session_timing_to_the_recording() { let harness = Harness::start(); let id = Uuid::new_v4(); - let log = harness.push(id, AI_ANALYSIS).await; + let artifact = harness.push(id, ai_analysis()).await; - assert_eq!(log, "ai-analysis-0.slog"); + assert_eq!(artifact, "ai-analysis-0.slog"); let manifest = harness.read_manifest(id); assert_eq!(manifest["files"], json!([])); + assert_eq!(manifest["startTime"], 0); + assert_eq!(manifest["duration"], 0); assert_eq!( manifest["artifacts"], json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) ); - assert_eq!(harness.push(id, WEBM).await, "recording-0.webm"); + assert_eq!(harness.connect(id, webm()).await, "recording-0.webm"); + let manifest = harness.read_manifest(id); + assert_ne!(manifest["startTime"], 0); + assert_eq!(manifest["startTime"], manifest["files"][0]["startTime"]); } #[tokio::test] - async fn log_push_keeps_recording_durations() { + async fn artifact_push_keeps_recording_timing() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, AI_ANALYSIS).await; + harness.push(id, ai_analysis()).await; let manifest = harness.read_manifest(id); + assert_eq!(manifest["startTime"], 1); assert_eq!(manifest["duration"], 5); assert_eq!(manifest["files"][0]["duration"], 5); + + harness.connect(id, webm()).await; + assert_eq!(harness.read_manifest(id)["startTime"], 1); } #[tokio::test] async fn recording_reconnect_keeps_artifacts() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, WEBM).await; - harness.push(id, AI_ANALYSIS).await; + harness.push(id, webm()).await; + harness.push(id, ai_analysis()).await; - let file_name = harness.connect(id, WEBM).await; + let file_name = harness.connect(id, webm()).await; assert_eq!(file_name, "recording-1.webm"); assert_eq!( harness.read_manifest(id)["artifacts"], json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) ); - harness.disconnect(id).await; + harness.disconnect(id, PushKind::Recording).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["files"][1]["fileName"], "recording-1.webm"); assert_eq!( @@ -1267,7 +1404,7 @@ mod tests { async fn shadow_streams_ongoing_recording() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.connect(id, WEBM).await; + harness.connect(id, webm()).await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1277,42 +1414,91 @@ mod tests { } #[tokio::test] - async fn shadow_refuses_while_only_a_log_is_pushed() { + async fn shadow_refuses_while_only_an_artifact_is_pushed() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, WEBM).await; - harness.connect(id, AI_ANALYSIS).await; + harness.connect(id, ai_analysis()).await; - let files = harness.sender.list_files(id).await.expect("list files"); - - assert!(files.is_none()); + assert!(harness.sender.list_files(id).await.expect("list files").is_none()); + assert!(harness.sender.get_state(id).await.expect("get state").is_none()); + assert!(!harness.sender.active_recordings.contains(id)); } #[tokio::test] - async fn log_chunks_do_not_wake_streamers() { + async fn artifact_chunks_do_not_wake_streamers() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, WEBM).await; - - assert!(!harness.client_push_wakes_streamers(id, AI_ANALYSIS).await); + let (tx, mut woken) = oneshot::channel(); + harness.sender.add_new_chunk_listener(id, tx); + + let (mut client, push, _shutdown_handle) = harness.start_client_push(id, ai_analysis()); + client.write_all(b"chunk").await.expect("write chunk"); + drop(client); + push.await.expect("join push").expect("push"); + harness.sender.get_count().await.expect("sync with manager"); + + let artifact_path = harness.recordings_path.join(id.to_string()).join("ai-analysis-0.slog"); + assert_eq!(std::fs::read(artifact_path).expect("read artifact"), b"chunk"); + assert_eq!(woken.try_recv(), Err(oneshot::error::TryRecvError::Empty)); } #[tokio::test] async fn slog_recording_chunks_wake_streamers() { let harness = Harness::start(); + let id = Uuid::new_v4(); + let (tx, mut woken) = oneshot::channel(); + harness.sender.add_new_chunk_listener(id, tx); + + let (mut client, push, _shutdown_handle) = harness.start_client_push(id, slog_recording()); + + // The push flushes, and so signals, whenever it runs out of input. + tokio::time::timeout(Duration::from_secs(5), async { + loop { + client.write_all(b"chunk").await.expect("write chunk"); + tokio::select! { + _ = &mut woken => break, + () = tokio::time::sleep(Duration::from_millis(20)) => {} + } + } + }) + .await + .expect("streamers woken"); - assert!( - harness - .client_push_wakes_streamers(Uuid::new_v4(), SLOG_RECORDING) - .await + drop(client); + push.await.expect("join push").expect("push"); + } + + #[test] + fn only_the_recording_kind_takes_part_in_the_recording_policy() { + assert_eq!( + PushKind::Recording.policy(), + KindPolicy { + recording_policy: true, + wakes_streamers: true, + tracks_duration: true, + } + ); + assert_eq!( + AI_ANALYSIS_KIND.policy(), + KindPolicy { + recording_policy: false, + wakes_streamers: false, + tracks_duration: false, + } ); } #[test] - fn artifact_role_names_match_their_serde_names() { - let role = ArtifactRole::AiAnalysis; - let parsed: ArtifactRole = serde_json::from_value(json!(role.as_str())).expect("parse role"); - assert_eq!(parsed, role); + fn ai_analysis_must_be_slog() { + let error = PushTarget::new(RecordingFileType::WebM, Some(ArtifactKind::AiAnalysis)).expect_err("webm"); + assert_eq!(error.to_string(), "ai-analysis artifacts must be slog files"); + } + + #[test] + fn artifact_kind_names_match_their_serde_names() { + let kind = ArtifactKind::AiAnalysis; + let parsed: ArtifactKind = serde_json::from_value(json!(kind.as_str())).expect("parse kind"); + assert_eq!(parsed, kind); } #[tokio::test] @@ -1321,27 +1507,115 @@ mod tests { let artifact_only = Uuid::new_v4(); let recorded = Uuid::new_v4(); - harness.connect(artifact_only, AI_ANALYSIS).await; - harness.connect(recorded, WEBM).await; + harness.connect(artifact_only, ai_analysis()).await; + harness.connect(recorded, webm()).await; + harness.connect(recorded, ai_analysis()).await; + harness.disconnect(recorded, AI_ANALYSIS_KIND).await; assert!(!harness.sender.is_recording(artifact_only).await.expect("is recording")); assert!(harness.sender.is_recording(recorded).await.expect("is recording")); assert!(!harness.sender.is_recording(Uuid::new_v4()).await.expect("is recording")); + assert!(!harness.sender.active_recordings.contains(artifact_only)); + assert!(harness.sender.active_recordings.contains(recorded)); + } + + #[tokio::test] + async fn artifact_push_does_not_disarm_the_recording_ttl_kill() { + let mut harness = Harness::start(); + let id = Uuid::new_v4(); + harness.connect_with_ttl(id, webm(), Duration::from_secs(1)).await; + harness.must_be_recorded(id).await; + harness.disconnect(id, PushKind::Recording).await; + + harness.push(id, ai_analysis()).await; + + assert!(matches!( + harness.sender.get_state(id).await.expect("get state"), + Some(OnGoingRecordingState::LastSeen { .. }) + )); + assert!(harness.sender.is_recording(id).await.expect("is recording")); + assert_eq!(harness.next_kill().await, id); + } + + #[tokio::test] + async fn artifact_ttl_expiry_never_kills_the_session() { + let mut harness = Harness::start(); + let artifact_only = Uuid::new_v4(); + let recorded = Uuid::new_v4(); + + harness.connect(artifact_only, ai_analysis()).await; + harness.must_be_recorded(artifact_only).await; + harness.disconnect(artifact_only, AI_ANALYSIS_KIND).await; + harness.wait_until_no_push().await; + + harness.connect(recorded, webm()).await; + harness.must_be_recorded(recorded).await; + harness.disconnect(recorded, PushKind::Recording).await; + + // Kill requests reach the session manager in the order they were issued. + assert_eq!(harness.next_kill().await, recorded); + } + + #[tokio::test] + async fn recording_and_artifact_push_at_the_same_time() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + + harness.connect(id, webm()).await; + assert_eq!(harness.connect(id, ai_analysis()).await, "ai-analysis-0.slog"); + assert!(harness.try_connect(id, webm()).await.is_err()); + assert!(harness.try_connect(id, ai_analysis()).await.is_err()); + + harness.disconnect(id, AI_ANALYSIS_KIND).await; + assert!(matches!( + harness.sender.get_state(id).await.expect("get state"), + Some(OnGoingRecordingState::Connected) + )); + + harness.disconnect(id, PushKind::Recording).await; + harness.connect(id, ai_analysis()).await; + assert_eq!(harness.connect(id, webm()).await, "recording-1.webm"); + } + + #[tokio::test] + async fn concurrent_pushes_keep_each_others_manifest_entries() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + let expected_artifacts = json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }); + + harness.connect(id, webm()).await; + harness.connect(id, ai_analysis()).await; + harness.disconnect(id, PushKind::Recording).await; + assert_eq!(harness.read_manifest(id)["artifacts"], expected_artifacts); + + harness.connect(id, webm()).await; + harness.disconnect(id, AI_ANALYSIS_KIND).await; + harness.disconnect(id, PushKind::Recording).await; + + let manifest = harness.read_manifest(id); + let file_names: Vec<_> = manifest["files"] + .as_array() + .expect("files") + .iter() + .map(|file| file["fileName"].clone()) + .collect(); + assert_eq!(file_names, [json!("recording-0.webm"), json!("recording-1.webm")]); + assert_eq!(manifest["artifacts"], expected_artifacts); } #[tokio::test] - async fn unknown_artifact_roles_survive_a_rewrite() { + async fn unknown_artifact_kinds_survive_a_rewrite() { let harness = Harness::start(); let id = Uuid::new_v4(); let mut manifest: serde_json::Value = serde_json::from_str(MASTER_MANIFEST).expect("manifest"); - manifest["artifacts"] = json!({ "future-role": [{ "fileName": "future-role-0.bin" }] }); + manifest["artifacts"] = json!({ "future-kind": [{ "fileName": "future-kind-0.bin" }] }); harness.write_manifest(id, &manifest.to_string()); - harness.push(id, WEBM).await; + harness.push(id, webm()).await; assert_eq!( harness.read_manifest(id)["artifacts"], - json!({ "future-role": [{ "fileName": "future-role-0.bin" }] }) + json!({ "future-kind": [{ "fileName": "future-kind-0.bin" }] }) ); } diff --git a/devolutions-gateway/src/session.rs b/devolutions-gateway/src/session.rs index 6fd56131b..0c2e93d58 100644 --- a/devolutions-gateway/src/session.rs +++ b/devolutions-gateway/src/session.rs @@ -280,6 +280,30 @@ pub fn session_manager_channel() -> (SessionMessageSender, SessionMessageReceive mpsc::channel(64).pipe(|(tx, rx)| (SessionMessageSender(tx), SessionMessageReceiver(rx))) } +/// A session manager that knows no session and reports every kill request, in order. +#[cfg(test)] +pub(crate) fn spawn_fake_session_manager() -> (SessionMessageSender, mpsc::UnboundedReceiver) { + let (handle, SessionMessageReceiver(mut rx)) = session_manager_channel(); + let (kills_tx, kills_rx) = mpsc::unbounded_channel(); + + tokio::spawn(async move { + while let Some(msg) = rx.recv().await { + match msg { + SessionManagerMessage::GetInfo { channel, .. } => { + let _ = channel.send(None); + } + SessionManagerMessage::Kill { id, channel } => { + let _ = kills_tx.send(id); + let _ = channel.send(KillResult::NotFound); + } + _ => {} + } + } + }); + + (handle, kills_rx) +} + struct WithTtlInfo { deadline: tokio::time::Instant, session_id: Uuid, From ca0a6b175819de09bc72aa236caf29f29abdb252 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 14:34:20 -0400 Subject: [PATCH 12/24] refactor(dgw): hard-type the manifest artifacts per kind `artifacts` is a struct with one list per `ArtifactKind` instead of a string-keyed map, reached through an exhaustive match, so adding a kind forces every reader and writer to handle it. Kinds unknown to this Gateway are no longer kept when it rewrites a manifest. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 27 ++++++++++----- devolutions-gateway/src/recording.rs | 50 +++++++++++++++------------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 831e72644..9a8d993ff 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeMap; use std::fs; use std::io::{self, Seek as _, Write as _}; use std::net::SocketAddr; @@ -644,7 +643,21 @@ where struct RecordingZipManifest { files: Vec, #[serde(default)] - artifacts: BTreeMap>, + artifacts: RecordingZipManifestArtifacts, +} + +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "kebab-case")] +struct RecordingZipManifestArtifacts { + #[serde(default)] + ai_analysis: Vec, +} + +impl RecordingZipManifestArtifacts { + fn into_files(self) -> Vec { + let Self { ai_analysis } = self; + ai_analysis + } } #[derive(Debug, Deserialize)] @@ -707,13 +720,9 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result(); - let mut artifact_names = Vec::with_capacity(manifest.files.len() + artifact_count); - for file in manifest - .files - .into_iter() - .chain(manifest.artifacts.into_values().flatten()) - { + let artifacts = manifest.artifacts.into_files(); + let mut artifact_names = Vec::with_capacity(manifest.files.len() + artifacts.len()); + for file in manifest.files.into_iter().chain(artifacts) { if !is_safe_recording_file_name(&file.file_name) { warn!( file_name = %file.file_name, diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index a302b6fb6..24e814e09 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -1,6 +1,6 @@ use core::fmt; use std::cmp; -use std::collections::{BTreeMap, BinaryHeap, HashMap, HashSet}; +use std::collections::{BinaryHeap, HashMap, HashSet}; use std::path::Path; use std::pin::pin; use std::sync::Arc; @@ -42,10 +42,30 @@ struct JrecManifest { start_time: i64, duration: i64, files: Vec, - /// Non-recording artifacts grouped by kind. Each list is append-only, like `files`: names are - /// derived from positions. Keys stay strings so kinds written by a newer Gateway survive a rewrite. - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - artifacts: BTreeMap>, + #[serde(default, skip_serializing_if = "JrecArtifacts::is_empty")] + artifacts: JrecArtifacts, +} + +/// Non-recording artifacts, one list per [`ArtifactKind`]. Each list is append-only, like `files`: names +/// are derived from positions. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +struct JrecArtifacts { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + ai_analysis: Vec, +} + +impl JrecArtifacts { + fn is_empty(&self) -> bool { + let Self { ai_analysis } = self; + ai_analysis.is_empty() + } + + fn of_kind_mut(&mut self, kind: ArtifactKind) -> &mut Vec { + match kind { + ArtifactKind::AiAnalysis => &mut self.ai_analysis, + } + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -667,7 +687,7 @@ impl RecordingManagerTask { start_time: 0, duration: 0, files: Vec::new(), - artifacts: BTreeMap::new(), + artifacts: JrecArtifacts::default(), } }; @@ -689,7 +709,7 @@ impl RecordingManagerTask { (file_name, index) } PushKind::Artifact(artifact_kind) => { - let artifacts = manifest.artifacts.entry(artifact_kind.as_str().to_owned()).or_default(); + let artifacts = manifest.artifacts.of_kind_mut(artifact_kind); let index = artifacts.len(); let file_name = format!("{}-{index}.{extension}", artifact_kind.as_str()); artifacts.push(JrecArtifact { @@ -1603,22 +1623,6 @@ mod tests { assert_eq!(manifest["artifacts"], expected_artifacts); } - #[tokio::test] - async fn unknown_artifact_kinds_survive_a_rewrite() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - let mut manifest: serde_json::Value = serde_json::from_str(MASTER_MANIFEST).expect("manifest"); - manifest["artifacts"] = json!({ "future-kind": [{ "fileName": "future-kind-0.bin" }] }); - harness.write_manifest(id, &manifest.to_string()); - - harness.push(id, webm()).await; - - assert_eq!( - harness.read_manifest(id)["artifacts"], - json!({ "future-kind": [{ "fileName": "future-kind-0.bin" }] }) - ); - } - #[test] fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); From 2dc82c96f974e85300252b434cead3fd4e2e5b03 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 14:43:25 -0400 Subject: [PATCH 13/24] refactor(dgw): move the artifact types into artifacts.rs `ArtifactKind`, the manifest `artifacts` struct and its entries, and the file-type error now live in their own module. The session ZIP reuses the same typed struct instead of a second copy of it. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 33 ++++-------- devolutions-gateway/src/artifacts.rs | 77 ++++++++++++++++++++++++++++ devolutions-gateway/src/lib.rs | 1 + devolutions-gateway/src/recording.rs | 63 +---------------------- 4 files changed, 89 insertions(+), 85 deletions(-) create mode 100644 devolutions-gateway/src/artifacts.rs diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 9a8d993ff..266cab265 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -28,9 +28,10 @@ use zip::write::SimpleFileOptions; use crate::DgwState; use crate::api::heartbeat::recording_storage_health; +use crate::artifacts::{ArtifactKind, JrecArtifacts}; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{ArtifactKind, PushOutcome, PushTarget, RecordingMessageSender}; +use crate::recording::{PushOutcome, PushTarget, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -643,21 +644,7 @@ where struct RecordingZipManifest { files: Vec, #[serde(default)] - artifacts: RecordingZipManifestArtifacts, -} - -#[derive(Debug, Default, Deserialize)] -#[serde(rename_all = "kebab-case")] -struct RecordingZipManifestArtifacts { - #[serde(default)] - ai_analysis: Vec, -} - -impl RecordingZipManifestArtifacts { - fn into_files(self) -> Vec { - let Self { ai_analysis } = self; - ai_analysis - } + artifacts: JrecArtifacts, } #[derive(Debug, Deserialize)] @@ -720,23 +707,23 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result, +} + +impl JrecArtifacts { + pub(crate) fn is_empty(&self) -> bool { + let Self { ai_analysis } = self; + ai_analysis.is_empty() + } + + pub(crate) fn into_file_names(self) -> Vec { + let Self { ai_analysis } = self; + ai_analysis.into_iter().map(|artifact| artifact.file_name).collect() + } + + pub(crate) fn of_kind_mut(&mut self, kind: ArtifactKind) -> &mut Vec { + match kind { + ArtifactKind::AiAnalysis => &mut self.ai_analysis, + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct JrecArtifact { + pub(crate) file_name: String, +} + +/// Kind of a non-recording artifact, used as its key in the manifest `artifacts` object. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum ArtifactKind { + AiAnalysis, +} + +impl ArtifactKind { + pub const fn as_str(self) -> &'static str { + match self { + ArtifactKind::AiAnalysis => "ai-analysis", + } + } + + pub(crate) const fn file_type(self) -> RecordingFileType { + match self { + ArtifactKind::AiAnalysis => RecordingFileType::SessionRecordingLog, + } + } +} + +#[derive(Debug, thiserror::Error)] +#[error("{} artifacts must be {} files", kind.as_str(), kind.file_type().extension())] +pub struct UnsupportedArtifactFileType { + pub(crate) kind: ArtifactKind, +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn artifact_kind_names_match_their_serde_names() { + let kind = ArtifactKind::AiAnalysis; + let parsed: ArtifactKind = serde_json::from_value(json!(kind.as_str())).expect("parse kind"); + assert_eq!(parsed, kind); + } +} diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 1fb08ff85..cd53900b2 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -13,6 +13,7 @@ extern crate tracing; pub mod openapi; pub mod api; +pub mod artifacts; pub mod cli; pub mod config; pub mod credential; diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 24e814e09..980eb85ef 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -20,6 +20,7 @@ use typed_builder::TypedBuilder; use uuid::Uuid; use video_streamer::SignalWriter; +use crate::artifacts::{ArtifactKind, JrecArtifact, JrecArtifacts, UnsupportedArtifactFileType}; use crate::job_queue::JobQueueHandle; use crate::session::SessionMessageSender; use crate::token::{JrecTokenClaims, RecordingFileType}; @@ -46,55 +47,6 @@ struct JrecManifest { artifacts: JrecArtifacts, } -/// Non-recording artifacts, one list per [`ArtifactKind`]. Each list is append-only, like `files`: names -/// are derived from positions. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case")] -struct JrecArtifacts { - #[serde(default, skip_serializing_if = "Vec::is_empty")] - ai_analysis: Vec, -} - -impl JrecArtifacts { - fn is_empty(&self) -> bool { - let Self { ai_analysis } = self; - ai_analysis.is_empty() - } - - fn of_kind_mut(&mut self, kind: ArtifactKind) -> &mut Vec { - match kind { - ArtifactKind::AiAnalysis => &mut self.ai_analysis, - } - } -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -struct JrecArtifact { - file_name: String, -} - -/// Kind of a non-recording artifact, used as its key in the manifest `artifacts` object. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Deserialize)] -#[serde(rename_all = "kebab-case")] -pub enum ArtifactKind { - AiAnalysis, -} - -impl ArtifactKind { - pub const fn as_str(self) -> &'static str { - match self { - ArtifactKind::AiAnalysis => "ai-analysis", - } - } - - const fn file_type(self) -> RecordingFileType { - match self { - ArtifactKind::AiAnalysis => RecordingFileType::SessionRecordingLog, - } - } -} - #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum PushKind { Recording, @@ -164,12 +116,6 @@ pub struct PushTarget { file_type: RecordingFileType, } -#[derive(Debug, thiserror::Error)] -#[error("{} artifacts must be {} files", kind.as_str(), kind.file_type().extension())] -pub struct UnsupportedArtifactFileType { - kind: ArtifactKind, -} - impl PushTarget { /// Without a kind the stream is a recording, as it was before artifacts existed. pub fn new(file_type: RecordingFileType, kind: Option) -> Result { @@ -1514,13 +1460,6 @@ mod tests { assert_eq!(error.to_string(), "ai-analysis artifacts must be slog files"); } - #[test] - fn artifact_kind_names_match_their_serde_names() { - let kind = ArtifactKind::AiAnalysis; - let parsed: ArtifactKind = serde_json::from_value(json!(kind.as_str())).expect("parse kind"); - assert_eq!(parsed, kind); - } - #[tokio::test] async fn only_a_recording_push_counts_as_recording() { let harness = Harness::start(); From b62c8d9283dfe0cf4eef5c21dd679aff842319c7 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 14:53:30 -0400 Subject: [PATCH 14/24] refactor(dgw): keep artifact pushes out of the recording lifecycle Artifact pushes no longer take any part in the recording lifecycle: the recording manager only allocates the next `-N.` name and adds it to the manifest `artifacts` list, then the push streams bytes to the file. No ongoing entry, disconnect, TTL, recording policy, active recordings, shadow wake-up or timing writes. Recording pushes are back to master's shape (one per session, keyed by session ID). Their disconnect re-reads the manifest from disk and updates its own `files` entry, so artifact entries added meanwhile are kept. Removes `KindPolicy`, `PushKind`, `is_recording` and the test-only fake session manager. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 19 +- devolutions-gateway/src/recording.rs | 764 +++++++++++---------------- devolutions-gateway/src/session.rs | 30 +- 3 files changed, 308 insertions(+), 505 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 266cab265..d7fff5842 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -1005,16 +1005,9 @@ async fn shadow_recording( return close_with_error(ws, StreamerCloseCode::InternalError); }; - let recording_files = match recordings.list_files(id).await { - Ok(Some(recording_files)) => recording_files, - Ok(None) => { - debug!(%id, "Shadow recording rejected: no recording is being pushed"); - return close_with_error(ws, StreamerCloseCode::StreamingEnded); - } - Err(_) => { - warn!(%id, "Shadow recording rejected: failed to list recording files"); - return close_with_error(ws, StreamerCloseCode::InternalError); - } + let Ok(recording_files) = recordings.list_files(id).await else { + warn!(%id, "Shadow recording rejected: failed to list recording files"); + return close_with_error(ws, StreamerCloseCode::InternalError); }; let Some(recording_path) = recording_files.last() else { @@ -1041,7 +1034,6 @@ mod tests { use zip::ZipArchive; use super::*; - use crate::recording::PushKind; #[test] fn push_target_from_query() { @@ -1049,17 +1041,16 @@ mod tests { serde_json::from_value::(query) .expect("query") .target() - .map(PushTarget::kind) .map_err(|error| error.code) }; assert_eq!( target(serde_json::json!({ "fileType": "slog" })), - Ok(PushKind::Recording) + Ok(PushTarget::Recording(RecordingFileType::SessionRecordingLog)) ); assert_eq!( target(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })), - Ok(PushKind::Artifact(ArtifactKind::AiAnalysis)) + Ok(PushTarget::Artifact(ArtifactKind::AiAnalysis)) ); assert_eq!( target(serde_json::json!({ "fileType": "webm", "kind": "ai-analysis" })), diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 980eb85ef..972786338 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -47,41 +47,6 @@ struct JrecManifest { artifacts: JrecArtifacts, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] -pub enum PushKind { - Recording, - Artifact(ArtifactKind), -} - -/// What a push of a given kind takes part in, decided once from the kind. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -struct KindPolicy { - /// Counts as the session recording: `is_recording`, `active_recordings` and the kill when the - /// session must be recorded. - recording_policy: bool, - /// Data and end of stream wake `/shadow` streamers. - wakes_streamers: bool, - /// Writes the file and manifest durations. - tracks_duration: bool, -} - -impl PushKind { - const fn policy(self) -> KindPolicy { - match self { - PushKind::Recording => KindPolicy { - recording_policy: true, - wakes_streamers: true, - tracks_duration: true, - }, - PushKind::Artifact(_) => KindPolicy { - recording_policy: false, - wakes_streamers: false, - tracks_duration: false, - }, - } - } -} - impl JrecManifest { fn read_from_file(path: impl AsRef) -> anyhow::Result { let json = std::fs::read(path)?; @@ -111,25 +76,19 @@ pub enum PushOutcome { /// Where a push is stored. Artifacts are opaque to Gateway: stored as-is, whatever their content. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct PushTarget { - kind: PushKind, - file_type: RecordingFileType, +pub enum PushTarget { + Recording(RecordingFileType), + Artifact(ArtifactKind), } impl PushTarget { /// Without a kind the stream is a recording, as it was before artifacts existed. pub fn new(file_type: RecordingFileType, kind: Option) -> Result { - let kind = match kind { - None => PushKind::Recording, - Some(kind) if kind.file_type() == file_type => PushKind::Artifact(kind), - Some(kind) => return Err(UnsupportedArtifactFileType { kind }), - }; - - Ok(Self { kind, file_type }) - } - - pub fn kind(self) -> PushKind { - self.kind + match kind { + None => Ok(Self::Recording(file_type)), + Some(kind) if kind.file_type() == file_type => Ok(Self::Artifact(kind)), + Some(kind) => Err(UnsupportedArtifactFileType { kind }), + } } } @@ -168,11 +127,15 @@ where } }; - let kind = target.kind(); - let policy = kind.policy(); + let is_recording = matches!(target, PushTarget::Recording(_)); + + let recording_file = match target { + PushTarget::Recording(file_type) => recordings.connect(session_id, file_type, disconnected_ttl).await, + PushTarget::Artifact(kind) => recordings.add_artifact(session_id, kind).await, + }; - let recording_file = match recordings.connect(session_id, target, disconnected_ttl).await { - Ok(connected) => connected, + let recording_file = match recording_file { + Ok(recording_file) => recording_file, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); client_stream.shutdown().await.context("shutdown")?; @@ -209,7 +172,7 @@ where loop { tokio::select! { _ = flush_signal.notified() => { - if policy.wakes_streamers { + if is_recording { recordings.new_chunk_appended(session_id)?; } }, @@ -248,7 +211,9 @@ where info!(?res, "Recording finished"); - recordings.disconnect(session_id, kind).await.context("disconnect")?; + if is_recording { + recordings.disconnect(session_id).await.context("disconnect")?; + } res } @@ -301,16 +266,11 @@ pub enum OnGoingRecordingState { LastSeen { timestamp: i64 }, } -/// An ongoing push, one per (session, kind). -/// -/// It holds no manifest copy: pushes of other kinds may update the manifest in the meantime. #[derive(Debug, Clone)] -struct OnGoingPush { +struct OnGoingRecording { state: OnGoingRecordingState, manifest_path: Utf8PathBuf, - file_path: Utf8PathBuf, - /// Position of the pushed file in its manifest list. - index: usize, + file_index: usize, session_must_be_recorded: bool, disconnected_ttl: Duration, } @@ -318,25 +278,25 @@ struct OnGoingPush { enum RecordingManagerMessage { Connect { id: Uuid, - target: PushTarget, + file_type: RecordingFileType, disconnected_ttl: Duration, channel: oneshot::Sender, }, + AddArtifact { + id: Uuid, + kind: ArtifactKind, + channel: oneshot::Sender, + }, Disconnect { id: Uuid, - kind: PushKind, }, GetState { id: Uuid, channel: oneshot::Sender>, }, - IsRecording { - id: Uuid, - channel: oneshot::Sender, - }, ListFiles { id: Uuid, - channel: oneshot::Sender>>, + channel: oneshot::Sender>, }, GetCount { channel: oneshot::Sender, @@ -356,26 +316,24 @@ impl fmt::Debug for RecordingManagerMessage { match self { RecordingManagerMessage::Connect { id, - target, + file_type, disconnected_ttl, channel: _, } => f .debug_struct("Connect") .field("id", id) - .field("target", target) + .field("file_type", file_type) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), - RecordingManagerMessage::Disconnect { id, kind } => f - .debug_struct("Disconnect") + RecordingManagerMessage::AddArtifact { id, kind, channel: _ } => f + .debug_struct("AddArtifact") .field("id", id) .field("kind", kind) - .finish(), + .finish_non_exhaustive(), + RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), RecordingManagerMessage::GetState { id, channel: _ } => { f.debug_struct("GetState").field("id", id).finish_non_exhaustive() } - RecordingManagerMessage::IsRecording { id, channel: _ } => { - f.debug_struct("IsRecording").field("id", id).finish_non_exhaustive() - } RecordingManagerMessage::GetCount { channel: _ } => f.debug_struct("GetCount").finish_non_exhaustive(), RecordingManagerMessage::UpdateRecordingPolicy { id, @@ -403,12 +361,17 @@ pub struct RecordingMessageSender { } impl RecordingMessageSender { - async fn connect(&self, id: Uuid, target: PushTarget, disconnected_ttl: Duration) -> anyhow::Result { + async fn connect( + &self, + id: Uuid, + file_type: RecordingFileType, + disconnected_ttl: Duration, + ) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::Connect { id, - target, + file_type, disconnected_ttl, channel: tx, }) @@ -419,33 +382,32 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - async fn disconnect(&self, id: Uuid, kind: PushKind) -> anyhow::Result<()> { + async fn add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + let (tx, rx) = oneshot::channel(); self.channel - .send(RecordingManagerMessage::Disconnect { id, kind }) + .send(RecordingManagerMessage::AddArtifact { id, kind, channel: tx }) .await .ok() - .context("couldn't send Remove message") + .context("couldn't send AddArtifact message")?; + rx.await.context("couldn't receive artifact file path") } - pub async fn get_state(&self, id: Uuid) -> anyhow::Result> { - let (tx, rx) = oneshot::channel(); + async fn disconnect(&self, id: Uuid) -> anyhow::Result<()> { self.channel - .send(RecordingManagerMessage::GetState { id, channel: tx }) + .send(RecordingManagerMessage::Disconnect { id }) .await .ok() - .context("couldn't send GetState message")?; - rx.await.context("couldn't receive recording state") + .context("couldn't send Remove message") } - /// Returns whether a Recording, not an artifact, is being pushed for this session. - pub async fn is_recording(&self, id: Uuid) -> anyhow::Result { + pub async fn get_state(&self, id: Uuid) -> anyhow::Result> { let (tx, rx) = oneshot::channel(); self.channel - .send(RecordingManagerMessage::IsRecording { id, channel: tx }) + .send(RecordingManagerMessage::GetState { id, channel: tx }) .await .ok() - .context("couldn't send IsRecording message")?; - rx.await.context("couldn't receive whether a recording is ongoing") + .context("couldn't send GetState message")?; + rx.await.context("couldn't receive recording state") } pub async fn get_count(&self) -> anyhow::Result { @@ -501,8 +463,7 @@ impl RecordingMessageSender { Ok(rx.await?) } - /// Returns `None` when no Recording is being pushed, even if an artifact is. - pub(crate) async fn list_files(&self, recording_id: Uuid) -> anyhow::Result>> { + pub(crate) async fn list_files(&self, recording_id: Uuid) -> anyhow::Result> { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::ListFiles { @@ -541,12 +502,11 @@ pub fn recording_message_channel() -> (RecordingMessageSender, RecordingMessageR struct DisconnectedTtl { deadline: tokio::time::Instant, id: Uuid, - kind: PushKind, } impl PartialEq for DisconnectedTtl { fn eq(&self, other: &Self) -> bool { - self.deadline.eq(&other.deadline) && self.id.eq(&other.id) && self.kind.eq(&other.kind) + self.deadline.eq(&other.deadline) && self.id.eq(&other.id) } } @@ -562,7 +522,7 @@ impl Ord for DisconnectedTtl { fn cmp(&self, other: &Self) -> cmp::Ordering { match self.deadline.cmp(&other.deadline) { cmp::Ordering::Less => cmp::Ordering::Greater, - cmp::Ordering::Equal => (self.id, self.kind).cmp(&(other.id, other.kind)), + cmp::Ordering::Equal => self.id.cmp(&other.id), cmp::Ordering::Greater => cmp::Ordering::Less, } } @@ -570,7 +530,7 @@ impl Ord for DisconnectedTtl { pub struct RecordingManagerTask { rx: RecordingMessageReceiver, - ongoing_recordings: HashMap<(Uuid, PushKind), OnGoingPush>, + ongoing_recordings: HashMap, recording_end_notifier: HashMap>, recordings_path: Utf8PathBuf, session_manager_handle: SessionMessageSender, @@ -597,29 +557,48 @@ impl RecordingManagerTask { async fn handle_connect( &mut self, id: Uuid, - target: PushTarget, + file_type: RecordingFileType, disconnected_ttl: Duration, ) -> anyhow::Result { const LENGTH_WARNING_THRESHOLD: usize = 1000; - let kind = target.kind(); - let policy = kind.policy(); - - if let Some(ongoing) = self.ongoing_recordings.get(&(id, kind)) + if let Some(ongoing) = self.ongoing_recordings.get(&id) && matches!(ongoing.state, OnGoingRecordingState::Connected) { - anyhow::bail!("concurrent push of the same kind for the same session is not supported"); + anyhow::bail!("concurrent recording for the same session is not supported"); } let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - - let mut manifest = if recording_path.exists() { + let (file_index, recording_file) = if recording_path.exists() { debug!(path = %recording_path, "Recording directory already exists"); - JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")? + let mut existing_manifest = + JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")?; + let next_file_idx = existing_manifest.files.len(); + + let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); + + // An artifact push may have created the manifest before any recording. + if existing_manifest.files.is_empty() { + existing_manifest.start_time = start_time; + } + + let file_name = format!("recording-{next_file_idx}.{}", file_type.extension()); + let recording_file = recording_path.join(&file_name); + + existing_manifest.files.push(JrecFile { + start_time, + duration: 0, + file_name, + }); + + existing_manifest + .save_to_file(&manifest_path) + .context("override existing manifest")?; + + (next_file_idx, recording_file) } else { debug!(path = %recording_path, "Create recording directory"); @@ -627,76 +606,52 @@ impl RecordingManagerTask { .await .with_context(|| format!("failed to create recording path: {recording_path}"))?; - // The session timing belongs to the recordings, so the first Recording push sets it. - JrecManifest { + let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); + let file_name = format!("recording-0.{}", file_type.extension()); + let recording_file = recording_path.join(&file_name); + + let first_file = JrecFile { + start_time, + duration: 0, + file_name, + }; + + let initial_manifest = JrecManifest { session_id: id, - start_time: 0, + start_time, duration: 0, - files: Vec::new(), + files: vec![first_file], artifacts: JrecArtifacts::default(), - } - }; - - let extension = target.file_type.extension(); + }; - let (file_name, index) = match kind { - PushKind::Recording => { - if manifest.files.is_empty() { - manifest.start_time = start_time; - } + initial_manifest + .save_to_file(&manifest_path) + .context("write initial manifest to disk")?; - let index = manifest.files.len(); - let file_name = format!("recording-{index}.{extension}"); - manifest.files.push(JrecFile { - start_time, - duration: 0, - file_name: file_name.clone(), - }); - (file_name, index) - } - PushKind::Artifact(artifact_kind) => { - let artifacts = manifest.artifacts.of_kind_mut(artifact_kind); - let index = artifacts.len(); - let file_name = format!("{}-{index}.{extension}", artifact_kind.as_str()); - artifacts.push(JrecArtifact { - file_name: file_name.clone(), - }); - (file_name, index) - } + (0, recording_file) }; - let file_path = recording_path.join(&file_name); - - manifest - .save_to_file(&manifest_path) - .context("write manifest to disk")?; - - let active_recording_count = if policy.recording_policy { - self.rx.active_recordings.insert(id) - } else { - 0 - }; + let active_recording_count = self.rx.active_recordings.insert(id); // NOTE: the session associated to this recording is not always running through the Devolutions Gateway. // It is a normal situation when the Devolutions is used solely as a recording server. // In such cases, we can only assume there is no recording policy. - let session_must_be_recorded = policy.recording_policy - && self - .session_manager_handle - .get_session_info(id) - .await - .inspect_err(|error| error!(%error, session.id = %id, "Failed to retrieve session info")) - .ok() - .flatten() - .is_some_and(|info| info.recording_policy); + let session_must_be_recorded = self + .session_manager_handle + .get_session_info(id) + .await + .inspect_err(|error| error!(%error, session.id = %id, "Failed to retrieve session info")) + .ok() + .flatten() + .map(|info| info.recording_policy) + .unwrap_or(false); self.ongoing_recordings.insert( - (id, kind), - OnGoingPush { + id, + OnGoingRecording { state: OnGoingRecordingState::Connected, manifest_path, - file_path: file_path.clone(), - index, + file_index, session_must_be_recorded, disconnected_ttl, }, @@ -712,14 +667,12 @@ impl RecordingManagerTask { ); } - Ok(file_path) + Ok(recording_file) } - async fn handle_disconnect(&mut self, id: Uuid, kind: PushKind) -> anyhow::Result<()> { - let policy = kind.policy(); - - let Some(ongoing) = self.ongoing_recordings.get_mut(&(id, kind)) else { - anyhow::bail!("unknown {kind:?} push for ID {id}"); + async fn handle_disconnect(&mut self, id: Uuid) -> anyhow::Result<()> { + let Some(ongoing) = self.ongoing_recordings.get_mut(&id) else { + return Err(anyhow::anyhow!("unknown recording for ID {id}")); }; if !matches!(ongoing.state, OnGoingRecordingState::Connected) { @@ -730,32 +683,32 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - if policy.tracks_duration { - debug!(path = %ongoing.manifest_path, "Write updated manifest to disk"); + // Artifact pushes may have added entries since the recording started; a cached copy would drop them. + let mut manifest = JrecManifest::read_from_file(&ongoing.manifest_path) + .with_context(|| format!("read manifest at {}", ongoing.manifest_path))?; - let mut manifest = JrecManifest::read_from_file(&ongoing.manifest_path) - .with_context(|| format!("read manifest at {}", ongoing.manifest_path))?; + let current_file = manifest + .files + .get_mut(ongoing.file_index) + .with_context(|| format!("no recording file at index {} (this is a bug)", ongoing.file_index))?; + current_file.duration = end_time - current_file.start_time; - let current_file = manifest.files.get_mut(ongoing.index).with_context(|| { - format!( - "no file at index {} in manifest {}", - ongoing.index, ongoing.manifest_path - ) - })?; - current_file.duration = end_time - current_file.start_time; + manifest.duration = end_time - manifest.start_time; - manifest.duration = end_time - manifest.start_time; + let recording_file_path = ongoing + .manifest_path + .parent() + .expect("a parent") + .join(¤t_file.file_name); - manifest - .save_to_file(&ongoing.manifest_path) - .with_context(|| format!("write manifest at {}", ongoing.manifest_path))?; - } + debug!(path = %ongoing.manifest_path, "Write updated manifest to disk"); - let recording_file_path = ongoing.file_path.clone(); + manifest + .save_to_file(&ongoing.manifest_path) + .with_context(|| format!("write manifest at {}", ongoing.manifest_path))?; - if policy.wakes_streamers - && let Some(notify) = self.recording_end_notifier.get(&id) - { + // Notify all the streamers that recording has ended. + if let Some(notify) = self.recording_end_notifier.get(&id) { notify.notify_waiters(); } @@ -782,10 +735,42 @@ impl RecordingManagerTask { Ok(()) } - fn handle_remove(&mut self, id: Uuid, kind: PushKind) { - let policy = kind.policy(); + async fn handle_add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + let recording_path = self.recordings_path.join(id.to_string()); + let manifest_path = recording_path.join("recording.json"); + + let mut manifest = if recording_path.exists() { + JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")? + } else { + fs::create_dir_all(&recording_path) + .await + .with_context(|| format!("failed to create recording path: {recording_path}"))?; + + // The session timing belongs to the recordings, so the first recording push sets it. + JrecManifest { + session_id: id, + start_time: 0, + duration: 0, + files: Vec::new(), + artifacts: JrecArtifacts::default(), + } + }; + + let artifacts = manifest.artifacts.of_kind_mut(kind); + let file_name = format!("{}-{}.{}", kind.as_str(), artifacts.len(), kind.file_type().extension()); + artifacts.push(JrecArtifact { + file_name: file_name.clone(), + }); + + manifest + .save_to_file(&manifest_path) + .context("write manifest to disk")?; + + Ok(recording_path.join(file_name)) + } - if let Some(ongoing) = self.ongoing_recordings.get(&(id, kind)) { + fn handle_remove(&mut self, id: Uuid) { + if let Some(ongoing) = self.ongoing_recordings.get(&id) { let now = time::OffsetDateTime::now_utc().unix_timestamp(); let disconnected_ttl_secs = i64::try_from(ongoing.disconnected_ttl.as_secs()).expect("TTL can’t be so big"); @@ -793,14 +778,11 @@ impl RecordingManagerTask { // NOTE: Comparing with disconnected_ttl_secs - 1 just in case the sleep returns faster than expected. // (I don’t know if this can actually happen in practice, but it’s better to be safe than sorry.) OnGoingRecordingState::LastSeen { timestamp } if now >= timestamp + disconnected_ttl_secs - 1 => { - debug!(%id, ?kind, "Mark push as terminated"); - - if policy.recording_policy { - self.rx.active_recordings.remove(id); - } + debug!(%id, "Mark recording as terminated"); + self.rx.active_recordings.remove(id); // Check the recording policy of the associated session and kill it if necessary. - if policy.recording_policy && ongoing.session_must_be_recorded { + if ongoing.session_must_be_recorded { tokio::spawn({ let session_manager_handle = self.session_manager_handle.clone(); @@ -833,14 +815,11 @@ impl RecordingManagerTask { }); } - self.ongoing_recordings.remove(&(id, kind)); - - if policy.wakes_streamers { - self.recording_end_notifier.remove(&id); - } + self.ongoing_recordings.remove(&id); + self.recording_end_notifier.remove(&id); } _ => { - trace!(%id, ?kind, "Push should not be removed yet"); + trace!(%id, "Recording should not be removed yet"); } } } @@ -848,7 +827,7 @@ impl RecordingManagerTask { fn subscribe(&mut self, id: Uuid) -> anyhow::Result> { debug!(%id, "Subscribing to ongoing recording"); - if !self.ongoing_recordings.contains_key(&(id, PushKind::Recording)) { + if !self.ongoing_recordings.contains_key(&id) { anyhow::bail!("unknown recording for ID {id}"); } @@ -860,28 +839,6 @@ impl RecordingManagerTask { Ok(notify) } } - - fn list_recording_files(&self, id: Uuid) -> anyhow::Result>> { - let Some(recording) = self.ongoing_recordings.get(&(id, PushKind::Recording)) else { - return Ok(None); - }; - - let recordings_folder = recording - .manifest_path - .parent() - .context("manifest path has no parent")?; - - let manifest = JrecManifest::read_from_file(&recording.manifest_path) - .with_context(|| format!("read manifest at {}", recording.manifest_path))?; - - let files = manifest - .files - .iter() - .map(|file| recordings_folder.join(&file.file_name)) - .collect(); - - Ok(Some(files)) - } } #[async_trait] @@ -915,7 +872,7 @@ async fn recording_manager_task( () = &mut next_remove_sleep, if !disconnected.is_empty() => { let to_remove = disconnected.pop().expect("we check for non-emptiness before entering this block"); - manager.handle_remove(to_remove.id, to_remove.kind); + manager.handle_remove(to_remove.id); // Re-arm the Sleep instance with the next deadline if required if let Some(next) = disconnected.peek() { @@ -931,27 +888,34 @@ async fn recording_manager_task( debug!(?msg, "Received message"); match msg { - RecordingManagerMessage::Connect { id, target, disconnected_ttl, channel } => { - match manager.handle_connect(id, target, disconnected_ttl).await { - Ok(connected) => { - let _ = channel.send(connected); + RecordingManagerMessage::Connect { id, file_type, disconnected_ttl, channel } => { + match manager.handle_connect(id, file_type, disconnected_ttl).await { + Ok(recording_file) => { + let _ = channel.send(recording_file); } Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, - RecordingManagerMessage::Disconnect { id, kind } => { - if let Err(e) = manager.handle_disconnect(id, kind).await { + RecordingManagerMessage::AddArtifact { id, kind, channel } => { + match manager.handle_add_artifact(id, kind).await { + Ok(artifact_file) => { + let _ = channel.send(artifact_file); + } + Err(e) => error!(error = format!("{e:#}"), "handle_add_artifact"), + } + }, + RecordingManagerMessage::Disconnect { id } => { + if let Err(e) = manager.handle_disconnect(id).await { error!(error = format!("{e:#}"), "handle_disconnect"); } - if let Some(ongoing) = manager.ongoing_recordings.get(&(id, kind)) { + if let Some(ongoing) = manager.ongoing_recordings.get(&id) { let now = tokio::time::Instant::now(); let deadline = now + ongoing.disconnected_ttl; disconnected.push(DisconnectedTtl { deadline, id, - kind, }); // Reset the Sleep instance if the new deadline is sooner or it is already elapsed. @@ -961,21 +925,14 @@ async fn recording_manager_task( } } RecordingManagerMessage::GetState { id, channel } => { - let response = manager - .ongoing_recordings - .get(&(id, PushKind::Recording)) - .map(|ongoing| ongoing.state.clone()); + let response = manager.ongoing_recordings.get(&id).map(|ongoing| ongoing.state.clone()); let _ = channel.send(response); } - RecordingManagerMessage::IsRecording { id, channel } => { - let is_recording = manager.ongoing_recordings.contains_key(&(id, PushKind::Recording)); - let _ = channel.send(is_recording); - } RecordingManagerMessage::GetCount { channel } => { let _ = channel.send(manager.ongoing_recordings.len()); } RecordingManagerMessage::UpdateRecordingPolicy { id, session_must_be_recorded } => { - if let Some(ongoing) = manager.ongoing_recordings.get_mut(&(id, PushKind::Recording)) { + if let Some(ongoing) = manager.ongoing_recordings.get_mut(&id) { ongoing.session_must_be_recorded = session_must_be_recorded; trace!( session.id = %id, @@ -993,11 +950,26 @@ async fn recording_manager_task( } }, RecordingManagerMessage::ListFiles { id, channel } => { - match manager.list_recording_files(id) { - Ok(files) => { - let _ = channel.send(files); + match manager.ongoing_recordings.get(&id) { + Some(recording) => { + let recordings_folder = recording.manifest_path.parent().expect("a parent"); + + match JrecManifest::read_from_file(&recording.manifest_path) { + Ok(manifest) => { + let files = manifest + .files + .iter() + .map(|file| recordings_folder.join(&file.file_name)) + .collect(); + + let _ = channel.send(files); + } + Err(e) => error!(error = format!("{e:#}"), %id, "Failed to read recording manifest"), + } + } + None => { + warn!(%id, "No recording found for provided ID"); } - Err(e) => error!(error = format!("{e:#}"), session.id = %id, "list recording files"), } } } @@ -1028,11 +1000,11 @@ async fn recording_manager_task( }; debug!(?msg, "Received message"); - if let RecordingManagerMessage::Disconnect { id, kind } = msg { - if let Err(e) = manager.handle_disconnect(id, kind).await { + if let RecordingManagerMessage::Disconnect { id } = msg { + if let Err(e) = manager.handle_disconnect(id).await { error!(error = format!("{e:#}"), "handle_disconnect"); } - manager.ongoing_recordings.remove(&(id, kind)); + manager.ongoing_recordings.remove(&id); } } @@ -1106,8 +1078,6 @@ mod tests { use super::*; - const AI_ANALYSIS_KIND: PushKind = PushKind::Artifact(ArtifactKind::AiAnalysis); - const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", "startTime": 1, @@ -1121,23 +1091,14 @@ mod tests { ] }"#; - fn webm() -> PushTarget { - PushTarget::new(RecordingFileType::WebM, None).expect("webm recording") - } - - fn slog_recording() -> PushTarget { - PushTarget::new(RecordingFileType::SessionRecordingLog, None).expect("slog recording") - } - - fn ai_analysis() -> PushTarget { - PushTarget::new(RecordingFileType::SessionRecordingLog, Some(ArtifactKind::AiAnalysis)).expect("ai-analysis") - } + const WEBM: PushTarget = PushTarget::Recording(RecordingFileType::WebM); + const SLOG_RECORDING: PushTarget = PushTarget::Recording(RecordingFileType::SessionRecordingLog); + const AI_ANALYSIS: PushTarget = PushTarget::Artifact(ArtifactKind::AiAnalysis); struct Harness { _dir: tempfile::TempDir, recordings_path: Utf8PathBuf, sender: RecordingMessageSender, - kills: mpsc::UnboundedReceiver, _shutdown_handle: ShutdownHandle, } @@ -1146,7 +1107,8 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let recordings_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); let (sender, receiver) = recording_message_channel(); - let (session_manager_handle, kills) = crate::session::spawn_fake_session_manager(); + // No session runs through this Gateway, as when it is used solely as a recording server. + let (session_manager_handle, _) = crate::session::session_manager_channel(); let (job_queue_handle, _) = JobQueueHandle::new(); let task = RecordingManagerTask::new( receiver, @@ -1161,7 +1123,6 @@ mod tests { _dir: dir, recordings_path, sender, - kills, _shutdown_handle: shutdown_handle, } } @@ -1181,58 +1142,31 @@ mod tests { } async fn try_connect(&self, id: Uuid, target: PushTarget) -> anyhow::Result { - self.sender.connect(id, target, Duration::ZERO).await - } - - async fn connect_with_ttl(&self, id: Uuid, target: PushTarget, disconnected_ttl: Duration) -> String { - let path = self - .sender - .connect(id, target, disconnected_ttl) - .await - .expect("connect"); - path.file_name().expect("file name").to_owned() + match target { + PushTarget::Recording(file_type) => self.sender.connect(id, file_type, Duration::from_secs(60)).await, + PushTarget::Artifact(kind) => self.sender.add_artifact(id, kind).await, + } } async fn connect(&self, id: Uuid, target: PushTarget) -> String { - self.connect_with_ttl(id, target, Duration::ZERO).await + let path = self.try_connect(id, target).await.expect("connect"); + path.file_name().expect("file name").to_owned() } - async fn disconnect(&self, id: Uuid, kind: PushKind) { - self.sender.disconnect(id, kind).await.expect("disconnect"); + async fn disconnect(&self, id: Uuid) { + self.sender.disconnect(id).await.expect("disconnect"); // Messages are processed in order, so this waits for the disconnect to be handled. self.sender.get_count().await.expect("sync with manager"); } async fn push(&self, id: Uuid, target: PushTarget) -> String { let file_name = self.connect(id, target).await; - self.disconnect(id, target.kind()).await; + if matches!(target, PushTarget::Recording(_)) { + self.disconnect(id).await; + } file_name } - async fn must_be_recorded(&self, id: Uuid) { - self.sender - .update_recording_policy(id, true) - .await - .expect("update recording policy"); - } - - async fn wait_until_no_push(&self) { - tokio::time::timeout(Duration::from_secs(5), async { - while self.sender.get_count().await.expect("count") != 0 { - tokio::task::yield_now().await; - } - }) - .await - .expect("expired pushes are removed"); - } - - async fn next_kill(&mut self) -> Uuid { - tokio::time::timeout(Duration::from_secs(5), self.kills.recv()) - .await - .expect("a kill request") - .expect("session manager alive") - } - fn start_client_push( &self, id: Uuid, @@ -1271,9 +1205,9 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let first = harness.push(id, slog_recording()).await; - harness.push(id, webm()).await; - let third = harness.push(id, slog_recording()).await; + let first = harness.push(id, SLOG_RECORDING).await; + harness.push(id, WEBM).await; + let third = harness.push(id, SLOG_RECORDING).await; assert_eq!(first, "recording-0.slog"); assert_eq!(third, "recording-2.slog"); @@ -1287,9 +1221,9 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, webm()).await; - let first_artifact = harness.push(id, ai_analysis()).await; - let second_artifact = harness.push(id, ai_analysis()).await; + harness.push(id, WEBM).await; + let first_artifact = harness.push(id, AI_ANALYSIS).await; + let second_artifact = harness.push(id, AI_ANALYSIS).await; assert_eq!(first_artifact, "ai-analysis-0.slog"); assert_eq!(second_artifact, "ai-analysis-1.slog"); @@ -1308,7 +1242,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let artifact = harness.push(id, ai_analysis()).await; + let artifact = harness.push(id, AI_ANALYSIS).await; assert_eq!(artifact, "ai-analysis-0.slog"); let manifest = harness.read_manifest(id); @@ -1320,7 +1254,7 @@ mod tests { json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) ); - assert_eq!(harness.connect(id, webm()).await, "recording-0.webm"); + assert_eq!(harness.connect(id, WEBM).await, "recording-0.webm"); let manifest = harness.read_manifest(id); assert_ne!(manifest["startTime"], 0); assert_eq!(manifest["startTime"], manifest["files"][0]["startTime"]); @@ -1332,62 +1266,85 @@ mod tests { let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, ai_analysis()).await; + harness.push(id, AI_ANALYSIS).await; let manifest = harness.read_manifest(id); assert_eq!(manifest["startTime"], 1); assert_eq!(manifest["duration"], 5); assert_eq!(manifest["files"][0]["duration"], 5); - harness.connect(id, webm()).await; + harness.connect(id, WEBM).await; assert_eq!(harness.read_manifest(id)["startTime"], 1); } #[tokio::test] - async fn recording_reconnect_keeps_artifacts() { + async fn recording_and_artifacts_at_the_same_time_keep_each_others_entries() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.push(id, webm()).await; - harness.push(id, ai_analysis()).await; + let expected_artifacts = + json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }, { "fileName": "ai-analysis-1.slog" }] }); - let file_name = harness.connect(id, webm()).await; - assert_eq!(file_name, "recording-1.webm"); - assert_eq!( - harness.read_manifest(id)["artifacts"], - json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) - ); + harness.connect(id, WEBM).await; + harness.connect(id, AI_ANALYSIS).await; + harness.connect(id, AI_ANALYSIS).await; + assert!(harness.try_connect(id, WEBM).await.is_err()); + + harness.disconnect(id).await; + assert_eq!(harness.read_manifest(id)["artifacts"], expected_artifacts); + + assert_eq!(harness.connect(id, WEBM).await, "recording-1.webm"); + harness.disconnect(id).await; - harness.disconnect(id, PushKind::Recording).await; let manifest = harness.read_manifest(id); - assert_eq!(manifest["files"][1]["fileName"], "recording-1.webm"); - assert_eq!( - manifest["artifacts"], - json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) - ); + let file_names: Vec<_> = manifest["files"] + .as_array() + .expect("files") + .iter() + .map(|file| file["fileName"].clone()) + .collect(); + assert_eq!(file_names, [json!("recording-0.webm"), json!("recording-1.webm")]); + assert_eq!(manifest["artifacts"], expected_artifacts); } #[tokio::test] - async fn shadow_streams_ongoing_recording() { + async fn shadow_lists_the_ongoing_recording_files() { let harness = Harness::start(); let id = Uuid::new_v4(); - harness.connect(id, webm()).await; + harness.connect(id, WEBM).await; + harness.connect(id, AI_ANALYSIS).await; let files = harness.sender.list_files(id).await.expect("list files"); - let files = files.expect("a Recording is being pushed"); let file_names: Vec<_> = files.iter().filter_map(|path| path.file_name()).collect(); assert_eq!(file_names, ["recording-0.webm"]); } #[tokio::test] - async fn shadow_refuses_while_only_an_artifact_is_pushed() { + async fn artifact_push_stays_out_of_the_recording_lifecycle() { let harness = Harness::start(); - let id = Uuid::new_v4(); - harness.connect(id, ai_analysis()).await; + let artifact_only = Uuid::new_v4(); + let recorded = Uuid::new_v4(); + + harness.connect(artifact_only, AI_ANALYSIS).await; + assert!( + harness + .sender + .get_state(artifact_only) + .await + .expect("get state") + .is_none() + ); + assert!(!harness.sender.active_recordings.contains(artifact_only)); + assert_eq!(harness.sender.get_count().await.expect("count"), 0); - assert!(harness.sender.list_files(id).await.expect("list files").is_none()); - assert!(harness.sender.get_state(id).await.expect("get state").is_none()); - assert!(!harness.sender.active_recordings.contains(id)); + harness.push(recorded, WEBM).await; + harness.push(recorded, AI_ANALYSIS).await; + assert!(matches!( + harness.sender.get_state(recorded).await.expect("get state"), + Some(OnGoingRecordingState::LastSeen { .. }) + )); + assert!(harness.sender.active_recordings.contains(recorded)); + assert_eq!(harness.sender.get_count().await.expect("count"), 1); } #[tokio::test] @@ -1397,11 +1354,10 @@ mod tests { let (tx, mut woken) = oneshot::channel(); harness.sender.add_new_chunk_listener(id, tx); - let (mut client, push, _shutdown_handle) = harness.start_client_push(id, ai_analysis()); + let (mut client, push, _shutdown_handle) = harness.start_client_push(id, AI_ANALYSIS); client.write_all(b"chunk").await.expect("write chunk"); drop(client); push.await.expect("join push").expect("push"); - harness.sender.get_count().await.expect("sync with manager"); let artifact_path = harness.recordings_path.join(id.to_string()).join("ai-analysis-0.slog"); assert_eq!(std::fs::read(artifact_path).expect("read artifact"), b"chunk"); @@ -1415,7 +1371,7 @@ mod tests { let (tx, mut woken) = oneshot::channel(); harness.sender.add_new_chunk_listener(id, tx); - let (mut client, push, _shutdown_handle) = harness.start_client_push(id, slog_recording()); + let (mut client, push, _shutdown_handle) = harness.start_client_push(id, SLOG_RECORDING); // The push flushes, and so signals, whenever it runs out of input. tokio::time::timeout(Duration::from_secs(5), async { @@ -1434,134 +1390,12 @@ mod tests { push.await.expect("join push").expect("push"); } - #[test] - fn only_the_recording_kind_takes_part_in_the_recording_policy() { - assert_eq!( - PushKind::Recording.policy(), - KindPolicy { - recording_policy: true, - wakes_streamers: true, - tracks_duration: true, - } - ); - assert_eq!( - AI_ANALYSIS_KIND.policy(), - KindPolicy { - recording_policy: false, - wakes_streamers: false, - tracks_duration: false, - } - ); - } - #[test] fn ai_analysis_must_be_slog() { let error = PushTarget::new(RecordingFileType::WebM, Some(ArtifactKind::AiAnalysis)).expect_err("webm"); assert_eq!(error.to_string(), "ai-analysis artifacts must be slog files"); } - #[tokio::test] - async fn only_a_recording_push_counts_as_recording() { - let harness = Harness::start(); - let artifact_only = Uuid::new_v4(); - let recorded = Uuid::new_v4(); - - harness.connect(artifact_only, ai_analysis()).await; - harness.connect(recorded, webm()).await; - harness.connect(recorded, ai_analysis()).await; - harness.disconnect(recorded, AI_ANALYSIS_KIND).await; - - assert!(!harness.sender.is_recording(artifact_only).await.expect("is recording")); - assert!(harness.sender.is_recording(recorded).await.expect("is recording")); - assert!(!harness.sender.is_recording(Uuid::new_v4()).await.expect("is recording")); - assert!(!harness.sender.active_recordings.contains(artifact_only)); - assert!(harness.sender.active_recordings.contains(recorded)); - } - - #[tokio::test] - async fn artifact_push_does_not_disarm_the_recording_ttl_kill() { - let mut harness = Harness::start(); - let id = Uuid::new_v4(); - harness.connect_with_ttl(id, webm(), Duration::from_secs(1)).await; - harness.must_be_recorded(id).await; - harness.disconnect(id, PushKind::Recording).await; - - harness.push(id, ai_analysis()).await; - - assert!(matches!( - harness.sender.get_state(id).await.expect("get state"), - Some(OnGoingRecordingState::LastSeen { .. }) - )); - assert!(harness.sender.is_recording(id).await.expect("is recording")); - assert_eq!(harness.next_kill().await, id); - } - - #[tokio::test] - async fn artifact_ttl_expiry_never_kills_the_session() { - let mut harness = Harness::start(); - let artifact_only = Uuid::new_v4(); - let recorded = Uuid::new_v4(); - - harness.connect(artifact_only, ai_analysis()).await; - harness.must_be_recorded(artifact_only).await; - harness.disconnect(artifact_only, AI_ANALYSIS_KIND).await; - harness.wait_until_no_push().await; - - harness.connect(recorded, webm()).await; - harness.must_be_recorded(recorded).await; - harness.disconnect(recorded, PushKind::Recording).await; - - // Kill requests reach the session manager in the order they were issued. - assert_eq!(harness.next_kill().await, recorded); - } - - #[tokio::test] - async fn recording_and_artifact_push_at_the_same_time() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - - harness.connect(id, webm()).await; - assert_eq!(harness.connect(id, ai_analysis()).await, "ai-analysis-0.slog"); - assert!(harness.try_connect(id, webm()).await.is_err()); - assert!(harness.try_connect(id, ai_analysis()).await.is_err()); - - harness.disconnect(id, AI_ANALYSIS_KIND).await; - assert!(matches!( - harness.sender.get_state(id).await.expect("get state"), - Some(OnGoingRecordingState::Connected) - )); - - harness.disconnect(id, PushKind::Recording).await; - harness.connect(id, ai_analysis()).await; - assert_eq!(harness.connect(id, webm()).await, "recording-1.webm"); - } - - #[tokio::test] - async fn concurrent_pushes_keep_each_others_manifest_entries() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - let expected_artifacts = json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }); - - harness.connect(id, webm()).await; - harness.connect(id, ai_analysis()).await; - harness.disconnect(id, PushKind::Recording).await; - assert_eq!(harness.read_manifest(id)["artifacts"], expected_artifacts); - - harness.connect(id, webm()).await; - harness.disconnect(id, AI_ANALYSIS_KIND).await; - harness.disconnect(id, PushKind::Recording).await; - - let manifest = harness.read_manifest(id); - let file_names: Vec<_> = manifest["files"] - .as_array() - .expect("files") - .iter() - .map(|file| file["fileName"].clone()) - .collect(); - assert_eq!(file_names, [json!("recording-0.webm"), json!("recording-1.webm")]); - assert_eq!(manifest["artifacts"], expected_artifacts); - } - #[test] fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/devolutions-gateway/src/session.rs b/devolutions-gateway/src/session.rs index 0c2e93d58..ef0b1582a 100644 --- a/devolutions-gateway/src/session.rs +++ b/devolutions-gateway/src/session.rs @@ -280,30 +280,6 @@ pub fn session_manager_channel() -> (SessionMessageSender, SessionMessageReceive mpsc::channel(64).pipe(|(tx, rx)| (SessionMessageSender(tx), SessionMessageReceiver(rx))) } -/// A session manager that knows no session and reports every kill request, in order. -#[cfg(test)] -pub(crate) fn spawn_fake_session_manager() -> (SessionMessageSender, mpsc::UnboundedReceiver) { - let (handle, SessionMessageReceiver(mut rx)) = session_manager_channel(); - let (kills_tx, kills_rx) = mpsc::unbounded_channel(); - - tokio::spawn(async move { - while let Some(msg) = rx.recv().await { - match msg { - SessionManagerMessage::GetInfo { channel, .. } => { - let _ = channel.send(None); - } - SessionManagerMessage::Kill { id, channel } => { - let _ = kills_tx.send(id); - let _ = channel.send(KillResult::NotFound); - } - _ => {} - } - } - }); - - (handle, kills_rx) -} - struct WithTtlInfo { deadline: tokio::time::Instant, session_id: Uuid, @@ -634,9 +610,11 @@ impl Task for EnsureRecordingPolicyTask { let is_recording = self .recording_manager_handle - .is_recording(self.session_id) + .get_state(self.session_id) .await - .unwrap_or(false); + .ok() + .flatten() + .is_some(); if is_recording { let _ = self From 0ca58cd11ad6c279c1e290072d881cb4a42d9302 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 14:57:13 -0400 Subject: [PATCH 15/24] docs(dgw): rewrite the recording intent for artifacts Describes recording pushes, artifact pushes with `kind`, and that artifacts take no part in the recording policy or session shadowing. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/recording.intent.md | 24 ++++++++++----------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/devolutions-gateway/src/recording.intent.md b/devolutions-gateway/src/recording.intent.md index f3318c2bd..db24c636c 100644 --- a/devolutions-gateway/src/recording.intent.md +++ b/devolutions-gateway/src/recording.intent.md @@ -1,15 +1,13 @@ -# Background -Recording in Gateway has always been simple. -The source pushes a stream into Gateway, and Gateway persists it to disk. -Now we would like to add a new feature, AI and machine generated logs to improve searchability. +# Recording +Gateway recording is a continuous stream of bytes pushed from the client who pocesses the valid recording token. +The url is `/jet/jrec/push/{sessionId}?fileType={fileType}`. +We expect the `fileType` to be one of the following file types: `webm`, `cast`, `trp` and `slog`, which must be specified. +When connection is established with request of recordings for the session, if recording is not enabled within a short period of time, the connection will be closed with indication of violation of the recording policy. -# Logs -Recording manifest should now have a new field called `logs`. -We define material as a file that is pushed to Gateway. -We will have two material types, `recording` and `log`. -To keep everything backward compatible, we will accept `materialType` as a query parameter, and when it is null, we will treat the stream as a recording. -If `materialType` is `recording`, the `fileType` param must be present, we currently have four file types, `webm`, `cast`, `trp` and `slog`. That is right, `slog` can be both a recording file type and the log itself. This is intentional to keep backward compatibility. -`fileType` is mandatory for `recording`, and rejected for `log`. -The content of the log and the recording is transparent to Gateway unless it is streamed, see the `streaming` crates. -The client doesn't own the naming of log and recording files, Gateway does with number-based naming. \ No newline at end of file +# Artifacts +Artifacts are files that are not recordings, currently only have `ai-analysis` with combination to `slog` file type. +We reuse the same url but with one extra query parameter, `/jet/jrec/push/{sessionId}?fileType={fileType}&kind={kind}`, where the `kind`, if not specified, we treat it as recordings. +Artifacts can be pushed without any recordings started. +Pushing artifacts should not trigger any recording policy as recordings. +Session shadowing does not support artifacts. \ No newline at end of file From 808f4b4bd68604f17ea395b8dd2ad2f6eb232bc6 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 15:23:58 -0400 Subject: [PATCH 16/24] refactor(dgw): update the last recording entry like master did A session has one recording push at a time and artifacts never go in `files`, so the pushing recording is always the last entry. Drop the stored index; only re-read the manifest from disk before updating it. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/recording.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 972786338..a86d26585 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -270,7 +270,6 @@ pub enum OnGoingRecordingState { struct OnGoingRecording { state: OnGoingRecordingState, manifest_path: Utf8PathBuf, - file_index: usize, session_must_be_recorded: bool, disconnected_ttl: Duration, } @@ -571,7 +570,7 @@ impl RecordingManagerTask { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let (file_index, recording_file) = if recording_path.exists() { + let recording_file = if recording_path.exists() { debug!(path = %recording_path, "Recording directory already exists"); let mut existing_manifest = @@ -598,7 +597,7 @@ impl RecordingManagerTask { .save_to_file(&manifest_path) .context("override existing manifest")?; - (next_file_idx, recording_file) + recording_file } else { debug!(path = %recording_path, "Create recording directory"); @@ -628,7 +627,7 @@ impl RecordingManagerTask { .save_to_file(&manifest_path) .context("write initial manifest to disk")?; - (0, recording_file) + recording_file }; let active_recording_count = self.rx.active_recordings.insert(id); @@ -651,7 +650,6 @@ impl RecordingManagerTask { OnGoingRecording { state: OnGoingRecordingState::Connected, manifest_path, - file_index, session_must_be_recorded, disconnected_ttl, }, @@ -683,14 +681,11 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - // Artifact pushes may have added entries since the recording started; a cached copy would drop them. + // Re-read from disk: an artifact push may have added entries since this recording connected. let mut manifest = JrecManifest::read_from_file(&ongoing.manifest_path) .with_context(|| format!("read manifest at {}", ongoing.manifest_path))?; - let current_file = manifest - .files - .get_mut(ongoing.file_index) - .with_context(|| format!("no recording file at index {} (this is a bug)", ongoing.file_index))?; + let current_file = manifest.files.last_mut().context("no recording file (this is a bug)")?; current_file.duration = end_time - current_file.start_time; manifest.duration = end_time - manifest.start_time; From ccd91161c3eb76ae962c0a8b42f115a609bf483a Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 15:28:50 -0400 Subject: [PATCH 17/24] refactor(dgw): trim the JREC push and ZIP changes back toward master Keep master's `clip_names` naming and docs for the session ZIP, build the `PushTarget` directly in the push handler instead of through a wrapper, and leave the "ai-analysis must be slog" check to the recording tests. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 68 ++++++++++------------------- 1 file changed, 24 insertions(+), 44 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index d7fff5842..50ac95cd1 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -37,7 +37,7 @@ use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. const ZIP_CHUNK_SIZE: usize = 64 * 1024; -/// Maximum files in a session ZIP (`recording.json` + every Artifact listed, files and artifacts). +/// Maximum files in a session ZIP (`recording.json` + clips and artifacts). /// /// Reconnect windows only mint a small number of clips per session in practice; /// this bound blocks pathological manifests without rejecting normal multi-clip packages. @@ -69,12 +69,6 @@ struct JrecPushQueryParam { kind: Option, } -impl JrecPushQueryParam { - fn target(&self) -> Result { - PushTarget::new(self.file_type, self.kind).map_err(HttpError::bad_request().err()) - } -} - #[derive(Deserialize)] #[serde(rename_all = "camelCase")] pub(crate) struct JrecListQueryParam { @@ -99,7 +93,7 @@ async fn jrec_push( return Err(HttpError::forbidden().msg("expected push operation")); } - let target = query.target()?; + let target = PushTarget::new(query.file_type, query.kind).map_err(HttpError::bad_request().err())?; let conf = conf_handle.get_conf(); @@ -670,21 +664,21 @@ fn recording_file_content_type(path: &Utf8Path) -> &'static str { /// Immutable package membership for one download attempt. /// -/// `manifest_bytes` are the exact `recording.json` contents used to derive `artifact_names`, -/// so the archived manifest cannot drift from the Artifacts included in the ZIP. +/// `manifest_bytes` are the exact `recording.json` contents used to derive `clip_names`, +/// so the archived manifest cannot drift from the clips included in the ZIP. #[derive(Debug, Clone)] struct RecordingZipPlan { manifest_bytes: Vec, - artifact_names: Vec, + clip_names: Vec, } impl RecordingZipPlan { fn entry_count(&self) -> usize { - 1 /* recording.json */ + self.artifact_names.len() + 1 /* recording.json */ + self.clip_names.len() } } -/// Snapshots `recording.json` and every Artifact it lists (files and artifacts) at call time. +/// Snapshots `recording.json` and the clip and artifact files it references at call time. async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result { let manifest_path = recording_dir.join("recording.json"); let manifest_bytes = tokio::fs::read(&manifest_path).await.map_err(|error| { @@ -708,7 +702,7 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result Result Result Result<(), HttpError> { let mut total_bytes = u64::try_from(plan.manifest_bytes.len()).unwrap_or(u64::MAX); - for file_name in &plan.artifact_names { + for file_name in &plan.clip_names { let path = recording_dir.join(file_name); let metadata = tokio::fs::metadata(&path).await.map_err(|error| { if error.kind() == io::ErrorKind::NotFound { @@ -878,7 +872,7 @@ fn build_recording_zip_archive( zip.write_all(&plan.manifest_bytes) .context("write recording.json ZIP entry")?; - for file_name in &plan.artifact_names { + for file_name in &plan.clip_names { if cancel.load(Ordering::Relaxed) { return Err(anyhow::Error::new(RecordingZipCancelled)); } @@ -1036,28 +1030,14 @@ mod tests { use super::*; #[test] - fn push_target_from_query() { - let target = |query: serde_json::Value| { - serde_json::from_value::(query) - .expect("query") - .target() - .map_err(|error| error.code) - }; + fn push_query_requires_a_file_type_and_a_known_kind() { + let parse = |query: serde_json::Value| serde_json::from_value::(query); - assert_eq!( - target(serde_json::json!({ "fileType": "slog" })), - Ok(PushTarget::Recording(RecordingFileType::SessionRecordingLog)) - ); - assert_eq!( - target(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })), - Ok(PushTarget::Artifact(ArtifactKind::AiAnalysis)) - ); - assert_eq!( - target(serde_json::json!({ "fileType": "webm", "kind": "ai-analysis" })), - Err(StatusCode::BAD_REQUEST) - ); + let recording = parse(serde_json::json!({ "fileType": "slog" })).expect("recording"); + assert_eq!(recording.kind, None); + let artifact = parse(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })).expect("artifact"); + assert_eq!(artifact.kind, Some(ArtifactKind::AiAnalysis)); - let parse = |query: serde_json::Value| serde_json::from_value::(query); assert!(parse(serde_json::json!({ "kind": "ai-analysis" })).is_err()); assert!(parse(serde_json::json!({ "fileType": "slog", "kind": "unknown" })).is_err()); } @@ -1126,7 +1106,7 @@ mod tests { .unwrap_or_else(|error| panic!("snapshot plan: {error}")); assert_eq!(plan.manifest_bytes, manifest_bytes); assert_eq!( - plan.artifact_names, + plan.clip_names, vec!["recording-0.webm".to_owned(), "recording-1.webm".to_owned()] ); } @@ -1164,7 +1144,7 @@ mod tests { .await .unwrap_or_else(|error| panic!("snapshot plan: {error}")); assert_eq!( - plan.artifact_names, + plan.clip_names, ["recording-0.webm", "ai-analysis-0.slog", "ai-analysis-1.slog"] ); } @@ -1340,7 +1320,7 @@ mod tests { let plan = RecordingZipPlan { manifest_bytes: b"{}".to_vec(), - artifact_names: vec!["missing-clip.webm".to_owned()], + clip_names: vec!["missing-clip.webm".to_owned()], }; let (_shutdown_handle, shutdown_signal) = devolutions_gateway_task::ShutdownHandle::new(); let error = recording_zip_body(dir_path, plan, Uuid::nil(), shutdown_signal) @@ -1376,16 +1356,16 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let dir_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); - let mut artifact_names = Vec::with_capacity(MAX_RECORDING_ZIP_FILES); + let mut clip_names = Vec::with_capacity(MAX_RECORDING_ZIP_FILES); for index in 0..MAX_RECORDING_ZIP_FILES { let name = format!("f-{index}.bin"); tokio::fs::write(dir_path.join(&name), b"x").await.expect("write file"); - artifact_names.push(name); + clip_names.push(name); } let plan = RecordingZipPlan { manifest_bytes: b"{}".to_vec(), - artifact_names, + clip_names, }; let error = enforce_recording_zip_limits(&dir_path, &plan) .await From 9d3a40a33b15e6d91c33a650ee32ca2bfcb2f9c7 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 15:44:22 -0400 Subject: [PATCH 18/24] refactor(dgw): drop artifact pushes; add artifacts from Gateway only The JREC push endpoint and ClientPush go back to master. Artifacts are added through RecordingMessageSender::add_artifact, which moves a file into the session folder and lists it in the manifest. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 26 +-- devolutions-gateway/src/artifacts.rs | 23 ++- devolutions-gateway/src/recording.rs | 240 +++++++++------------------ 3 files changed, 93 insertions(+), 196 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index 50ac95cd1..ed4a67337 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -28,10 +28,10 @@ use zip::write::SimpleFileOptions; use crate::DgwState; use crate::api::heartbeat::recording_storage_health; -use crate::artifacts::{ArtifactKind, JrecArtifacts}; +use crate::artifacts::JrecArtifacts; use crate::extract::{JrecToken, RecordingDeleteScope, RecordingsReadScope}; use crate::http::{HttpError, HttpErrorBuilder}; -use crate::recording::{PushOutcome, PushTarget, RecordingMessageSender}; +use crate::recording::{PushOutcome, RecordingMessageSender}; use crate::token::{JrecTokenClaims, RecordingFileType, RecordingOperation}; /// Read chunk size when streaming a finished session ZIP from the temp file. @@ -66,7 +66,6 @@ pub fn make_router(state: DgwState) -> Router { #[serde(rename_all = "camelCase")] struct JrecPushQueryParam { file_type: RecordingFileType, - kind: Option, } #[derive(Deserialize)] @@ -93,8 +92,6 @@ async fn jrec_push( return Err(HttpError::forbidden().msg("expected push operation")); } - let target = PushTarget::new(query.file_type, query.kind).map_err(HttpError::bad_request().err())?; - let conf = conf_handle.get_conf(); // Pre-flight: refuse the upgrade up-front when the recording storage cannot accept @@ -141,7 +138,7 @@ async fn jrec_push( recordings, shutdown_signal, claims, - target, + query.file_type, session_id, source_addr, Duration::from_secs(conf_handle.get_conf().debug.ws_keep_alive_interval), @@ -157,7 +154,7 @@ async fn handle_jrec_push( recordings: RecordingMessageSender, shutdown_signal: ShutdownSignal, claims: JrecTokenClaims, - target: PushTarget, + file_type: RecordingFileType, session_id: Uuid, source_addr: SocketAddr, keep_alive_interval: Duration, @@ -172,7 +169,7 @@ async fn handle_jrec_push( .client_stream(stream) .recordings(recordings) .claims(claims) - .target(target) + .file_type(file_type) .session_id(session_id) .shutdown_signal(shutdown_signal) .build() @@ -1029,19 +1026,6 @@ mod tests { use super::*; - #[test] - fn push_query_requires_a_file_type_and_a_known_kind() { - let parse = |query: serde_json::Value| serde_json::from_value::(query); - - let recording = parse(serde_json::json!({ "fileType": "slog" })).expect("recording"); - assert_eq!(recording.kind, None); - let artifact = parse(serde_json::json!({ "fileType": "slog", "kind": "ai-analysis" })).expect("artifact"); - assert_eq!(artifact.kind, Some(ArtifactKind::AiAnalysis)); - - assert!(parse(serde_json::json!({ "kind": "ai-analysis" })).is_err()); - assert!(parse(serde_json::json!({ "fileType": "slog", "kind": "unknown" })).is_err()); - } - #[test] fn rejects_unsafe_recording_file_names() { assert!(is_safe_recording_file_name("recording-0.webm")); diff --git a/devolutions-gateway/src/artifacts.rs b/devolutions-gateway/src/artifacts.rs index e336fcb44..b63259213 100644 --- a/devolutions-gateway/src/artifacts.rs +++ b/devolutions-gateway/src/artifacts.rs @@ -36,8 +36,7 @@ pub(crate) struct JrecArtifact { } /// Kind of a non-recording artifact, used as its key in the manifest `artifacts` object. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Deserialize)] -#[serde(rename_all = "kebab-case")] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum ArtifactKind { AiAnalysis, } @@ -56,22 +55,20 @@ impl ArtifactKind { } } -#[derive(Debug, thiserror::Error)] -#[error("{} artifacts must be {} files", kind.as_str(), kind.file_type().extension())] -pub struct UnsupportedArtifactFileType { - pub(crate) kind: ArtifactKind, -} - #[cfg(test)] mod tests { - use serde_json::json; - use super::*; #[test] - fn artifact_kind_names_match_their_serde_names() { + fn manifest_key_is_the_kind_name() { let kind = ArtifactKind::AiAnalysis; - let parsed: ArtifactKind = serde_json::from_value(json!(kind.as_str())).expect("parse kind"); - assert_eq!(parsed, kind); + let mut artifacts = JrecArtifacts::default(); + artifacts.of_kind_mut(kind).push(JrecArtifact { + file_name: "file".to_owned(), + }); + + let json = serde_json::to_value(&artifacts).expect("serialize artifacts"); + let keys: Vec<_> = json.as_object().expect("object").keys().collect(); + assert_eq!(keys, [kind.as_str()]); } } diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index a86d26585..679e11c41 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -20,7 +20,7 @@ use typed_builder::TypedBuilder; use uuid::Uuid; use video_streamer::SignalWriter; -use crate::artifacts::{ArtifactKind, JrecArtifact, JrecArtifacts, UnsupportedArtifactFileType}; +use crate::artifacts::{ArtifactKind, JrecArtifact, JrecArtifacts}; use crate::job_queue::JobQueueHandle; use crate::session::SessionMessageSender; use crate::token::{JrecTokenClaims, RecordingFileType}; @@ -74,30 +74,12 @@ pub enum PushOutcome { StorageFull, } -/// Where a push is stored. Artifacts are opaque to Gateway: stored as-is, whatever their content. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PushTarget { - Recording(RecordingFileType), - Artifact(ArtifactKind), -} - -impl PushTarget { - /// Without a kind the stream is a recording, as it was before artifacts existed. - pub fn new(file_type: RecordingFileType, kind: Option) -> Result { - match kind { - None => Ok(Self::Recording(file_type)), - Some(kind) if kind.file_type() == file_type => Ok(Self::Artifact(kind)), - Some(kind) => Err(UnsupportedArtifactFileType { kind }), - } - } -} - #[derive(TypedBuilder)] pub struct ClientPush { recordings: RecordingMessageSender, claims: JrecTokenClaims, client_stream: S, - target: PushTarget, + file_type: RecordingFileType, session_id: Uuid, shutdown_signal: ShutdownSignal, } @@ -111,7 +93,7 @@ where recordings, claims, mut client_stream, - target, + file_type, session_id, mut shutdown_signal, } = self; @@ -127,14 +109,7 @@ where } }; - let is_recording = matches!(target, PushTarget::Recording(_)); - - let recording_file = match target { - PushTarget::Recording(file_type) => recordings.connect(session_id, file_type, disconnected_ttl).await, - PushTarget::Artifact(kind) => recordings.add_artifact(session_id, kind).await, - }; - - let recording_file = match recording_file { + let recording_file = match recordings.connect(session_id, file_type, disconnected_ttl).await { Ok(recording_file) => recording_file, Err(e) => { warn!(error = format!("{e:#}"), "Unable to start recording"); @@ -172,9 +147,7 @@ where loop { tokio::select! { _ = flush_signal.notified() => { - if is_recording { - recordings.new_chunk_appended(session_id)?; - } + recordings.new_chunk_appended(session_id)?; }, _ = shutdown_signal_clone.wait() => { break; @@ -211,9 +184,7 @@ where info!(?res, "Recording finished"); - if is_recording { - recordings.disconnect(session_id).await.context("disconnect")?; - } + recordings.disconnect(session_id).await.context("disconnect")?; res } @@ -284,7 +255,8 @@ enum RecordingManagerMessage { AddArtifact { id: Uuid, kind: ArtifactKind, - channel: oneshot::Sender, + source: Utf8PathBuf, + channel: oneshot::Sender>, }, Disconnect { id: Uuid, @@ -324,10 +296,16 @@ impl fmt::Debug for RecordingManagerMessage { .field("file_type", file_type) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), - RecordingManagerMessage::AddArtifact { id, kind, channel: _ } => f + RecordingManagerMessage::AddArtifact { + id, + kind, + source, + channel: _, + } => f .debug_struct("AddArtifact") .field("id", id) .field("kind", kind) + .field("source", source) .finish_non_exhaustive(), RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), RecordingManagerMessage::GetState { id, channel: _ } => { @@ -381,14 +359,20 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - async fn add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + /// Moves `source` into the session folder as the next artifact of `kind`, and returns its file name. + pub async fn add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel - .send(RecordingManagerMessage::AddArtifact { id, kind, channel: tx }) + .send(RecordingManagerMessage::AddArtifact { + id, + kind, + source, + channel: tx, + }) .await .ok() .context("couldn't send AddArtifact message")?; - rx.await.context("couldn't receive artifact file path") + rx.await.context("couldn't receive AddArtifact result")? } async fn disconnect(&self, id: Uuid) -> anyhow::Result<()> { @@ -579,7 +563,7 @@ impl RecordingManagerTask { let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - // An artifact push may have created the manifest before any recording. + // An artifact may have created the manifest before any recording. if existing_manifest.files.is_empty() { existing_manifest.start_time = start_time; } @@ -681,7 +665,7 @@ impl RecordingManagerTask { ongoing.state = OnGoingRecordingState::LastSeen { timestamp: end_time }; - // Re-read from disk: an artifact push may have added entries since this recording connected. + // Re-read from disk: an artifact may have been added since this recording connected. let mut manifest = JrecManifest::read_from_file(&ongoing.manifest_path) .with_context(|| format!("read manifest at {}", ongoing.manifest_path))?; @@ -730,7 +714,7 @@ impl RecordingManagerTask { Ok(()) } - async fn handle_add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + async fn handle_add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); @@ -753,6 +737,13 @@ impl RecordingManagerTask { let artifacts = manifest.artifacts.of_kind_mut(kind); let file_name = format!("{}-{}.{}", kind.as_str(), artifacts.len(), kind.file_type().extension()); + + // The file is in place before the manifest lists it, so readers never see a dangling entry. + let artifact_path = recording_path.join(&file_name); + fs::rename(&source, &artifact_path) + .await + .with_context(|| format!("move {source} to {artifact_path}"))?; + artifacts.push(JrecArtifact { file_name: file_name.clone(), }); @@ -761,7 +752,7 @@ impl RecordingManagerTask { .save_to_file(&manifest_path) .context("write manifest to disk")?; - Ok(recording_path.join(file_name)) + Ok(file_name) } fn handle_remove(&mut self, id: Uuid) { @@ -891,13 +882,8 @@ async fn recording_manager_task( Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, - RecordingManagerMessage::AddArtifact { id, kind, channel } => { - match manager.handle_add_artifact(id, kind).await { - Ok(artifact_file) => { - let _ = channel.send(artifact_file); - } - Err(e) => error!(error = format!("{e:#}"), "handle_add_artifact"), - } + RecordingManagerMessage::AddArtifact { id, kind, source, channel } => { + let _ = channel.send(manager.handle_add_artifact(id, kind, source).await); }, RecordingManagerMessage::Disconnect { id } => { if let Err(e) = manager.handle_disconnect(id).await { @@ -1086,12 +1072,12 @@ mod tests { ] }"#; - const WEBM: PushTarget = PushTarget::Recording(RecordingFileType::WebM); - const SLOG_RECORDING: PushTarget = PushTarget::Recording(RecordingFileType::SessionRecordingLog); - const AI_ANALYSIS: PushTarget = PushTarget::Artifact(ArtifactKind::AiAnalysis); + const WEBM: RecordingFileType = RecordingFileType::WebM; + const SLOG: RecordingFileType = RecordingFileType::SessionRecordingLog; + const AI_ANALYSIS: ArtifactKind = ArtifactKind::AiAnalysis; struct Harness { - _dir: tempfile::TempDir, + dir: tempfile::TempDir, recordings_path: Utf8PathBuf, sender: RecordingMessageSender, _shutdown_handle: ShutdownHandle, @@ -1100,7 +1086,7 @@ mod tests { impl Harness { fn start() -> Self { let dir = tempfile::tempdir().expect("temp dir"); - let recordings_path = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).expect("utf8 path"); + let recordings_path = Utf8PathBuf::from_path_buf(dir.path().join("recordings")).expect("utf8 path"); let (sender, receiver) = recording_message_channel(); // No session runs through this Gateway, as when it is used solely as a recording server. let (session_manager_handle, _) = crate::session::session_manager_channel(); @@ -1115,7 +1101,7 @@ mod tests { tokio::spawn(recording_manager_task(task, shutdown_signal)); Self { - _dir: dir, + dir, recordings_path, sender, _shutdown_handle: shutdown_handle, @@ -1136,15 +1122,12 @@ mod tests { .expect("parse manifest") } - async fn try_connect(&self, id: Uuid, target: PushTarget) -> anyhow::Result { - match target { - PushTarget::Recording(file_type) => self.sender.connect(id, file_type, Duration::from_secs(60)).await, - PushTarget::Artifact(kind) => self.sender.add_artifact(id, kind).await, - } + async fn try_connect(&self, id: Uuid, file_type: RecordingFileType) -> anyhow::Result { + self.sender.connect(id, file_type, Duration::from_secs(60)).await } - async fn connect(&self, id: Uuid, target: PushTarget) -> String { - let path = self.try_connect(id, target).await.expect("connect"); + async fn connect(&self, id: Uuid, file_type: RecordingFileType) -> String { + let path = self.try_connect(id, file_type).await.expect("connect"); path.file_name().expect("file name").to_owned() } @@ -1154,44 +1137,24 @@ mod tests { self.sender.get_count().await.expect("sync with manager"); } - async fn push(&self, id: Uuid, target: PushTarget) -> String { - let file_name = self.connect(id, target).await; - if matches!(target, PushTarget::Recording(_)) { - self.disconnect(id).await; - } + async fn push(&self, id: Uuid, file_type: RecordingFileType) -> String { + let file_name = self.connect(id, file_type).await; + self.disconnect(id).await; file_name } - fn start_client_push( - &self, - id: Uuid, - target: PushTarget, - ) -> ( - io::DuplexStream, - tokio::task::JoinHandle>, - ShutdownHandle, - ) { - let claims = serde_json::from_value(json!({ - "jet_aid": id, - "jet_rop": "push", - "exp": i64::MAX, - "jti": Uuid::new_v4(), - })) - .expect("claims"); - let (client, server) = io::duplex(1024); - let (shutdown_handle, shutdown_signal) = ShutdownHandle::new(); - let push = tokio::spawn( - ClientPush::builder() - .recordings(self.sender.clone()) - .claims(claims) - .client_stream(server) - .target(target) - .session_id(id) - .shutdown_signal(shutdown_signal) - .build() - .run(), - ); - (client, push, shutdown_handle) + async fn add_artifact(&self, id: Uuid, content: &str) -> String { + let source = + Utf8PathBuf::from_path_buf(self.dir.path().join(Uuid::new_v4().to_string())).expect("utf8 path"); + std::fs::write(&source, content).expect("write artifact source"); + self.sender + .add_artifact(id, AI_ANALYSIS, source) + .await + .expect("add artifact") + } + + fn read_file(&self, id: Uuid, file_name: &str) -> String { + std::fs::read_to_string(self.recordings_path.join(id.to_string()).join(file_name)).expect("read file") } } @@ -1200,9 +1163,9 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let first = harness.push(id, SLOG_RECORDING).await; + let first = harness.push(id, SLOG).await; harness.push(id, WEBM).await; - let third = harness.push(id, SLOG_RECORDING).await; + let third = harness.push(id, SLOG).await; assert_eq!(first, "recording-0.slog"); assert_eq!(third, "recording-2.slog"); @@ -1217,11 +1180,13 @@ mod tests { let id = Uuid::new_v4(); harness.push(id, WEBM).await; - let first_artifact = harness.push(id, AI_ANALYSIS).await; - let second_artifact = harness.push(id, AI_ANALYSIS).await; + let first_artifact = harness.add_artifact(id, "first").await; + let second_artifact = harness.add_artifact(id, "second").await; assert_eq!(first_artifact, "ai-analysis-0.slog"); assert_eq!(second_artifact, "ai-analysis-1.slog"); + assert_eq!(harness.read_file(id, &first_artifact), "first"); + assert_eq!(harness.read_file(id, &second_artifact), "second"); let manifest = harness.read_manifest(id); let files = manifest["files"].as_array().expect("files"); assert_eq!(files.len(), 1); @@ -1237,7 +1202,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); - let artifact = harness.push(id, AI_ANALYSIS).await; + let artifact = harness.add_artifact(id, "analysis").await; assert_eq!(artifact, "ai-analysis-0.slog"); let manifest = harness.read_manifest(id); @@ -1256,12 +1221,12 @@ mod tests { } #[tokio::test] - async fn artifact_push_keeps_recording_timing() { + async fn artifact_keeps_recording_timing() { let harness = Harness::start(); let id = Uuid::new_v4(); harness.write_manifest(id, MASTER_MANIFEST); - harness.push(id, AI_ANALYSIS).await; + harness.add_artifact(id, "analysis").await; let manifest = harness.read_manifest(id); assert_eq!(manifest["startTime"], 1); @@ -1273,15 +1238,15 @@ mod tests { } #[tokio::test] - async fn recording_and_artifacts_at_the_same_time_keep_each_others_entries() { + async fn artifact_added_during_a_recording_survives_its_disconnect() { let harness = Harness::start(); let id = Uuid::new_v4(); let expected_artifacts = json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }, { "fileName": "ai-analysis-1.slog" }] }); harness.connect(id, WEBM).await; - harness.connect(id, AI_ANALYSIS).await; - harness.connect(id, AI_ANALYSIS).await; + harness.add_artifact(id, "first").await; + harness.add_artifact(id, "second").await; assert!(harness.try_connect(id, WEBM).await.is_err()); harness.disconnect(id).await; @@ -1306,7 +1271,7 @@ mod tests { let harness = Harness::start(); let id = Uuid::new_v4(); harness.connect(id, WEBM).await; - harness.connect(id, AI_ANALYSIS).await; + harness.add_artifact(id, "analysis").await; let files = harness.sender.list_files(id).await.expect("list files"); @@ -1315,12 +1280,12 @@ mod tests { } #[tokio::test] - async fn artifact_push_stays_out_of_the_recording_lifecycle() { + async fn artifact_stays_out_of_the_recording_lifecycle() { let harness = Harness::start(); let artifact_only = Uuid::new_v4(); let recorded = Uuid::new_v4(); - harness.connect(artifact_only, AI_ANALYSIS).await; + harness.add_artifact(artifact_only, "analysis").await; assert!( harness .sender @@ -1333,7 +1298,7 @@ mod tests { assert_eq!(harness.sender.get_count().await.expect("count"), 0); harness.push(recorded, WEBM).await; - harness.push(recorded, AI_ANALYSIS).await; + harness.add_artifact(recorded, "analysis").await; assert!(matches!( harness.sender.get_state(recorded).await.expect("get state"), Some(OnGoingRecordingState::LastSeen { .. }) @@ -1342,55 +1307,6 @@ mod tests { assert_eq!(harness.sender.get_count().await.expect("count"), 1); } - #[tokio::test] - async fn artifact_chunks_do_not_wake_streamers() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - let (tx, mut woken) = oneshot::channel(); - harness.sender.add_new_chunk_listener(id, tx); - - let (mut client, push, _shutdown_handle) = harness.start_client_push(id, AI_ANALYSIS); - client.write_all(b"chunk").await.expect("write chunk"); - drop(client); - push.await.expect("join push").expect("push"); - - let artifact_path = harness.recordings_path.join(id.to_string()).join("ai-analysis-0.slog"); - assert_eq!(std::fs::read(artifact_path).expect("read artifact"), b"chunk"); - assert_eq!(woken.try_recv(), Err(oneshot::error::TryRecvError::Empty)); - } - - #[tokio::test] - async fn slog_recording_chunks_wake_streamers() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - let (tx, mut woken) = oneshot::channel(); - harness.sender.add_new_chunk_listener(id, tx); - - let (mut client, push, _shutdown_handle) = harness.start_client_push(id, SLOG_RECORDING); - - // The push flushes, and so signals, whenever it runs out of input. - tokio::time::timeout(Duration::from_secs(5), async { - loop { - client.write_all(b"chunk").await.expect("write chunk"); - tokio::select! { - _ = &mut woken => break, - () = tokio::time::sleep(Duration::from_millis(20)) => {} - } - } - }) - .await - .expect("streamers woken"); - - drop(client); - push.await.expect("join push").expect("push"); - } - - #[test] - fn ai_analysis_must_be_slog() { - let error = PushTarget::new(RecordingFileType::WebM, Some(ArtifactKind::AiAnalysis)).expect_err("webm"); - assert_eq!(error.to_string(), "ai-analysis artifacts must be slog files"); - } - #[test] fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); From da4f172872fad1200691b146e2a0c3418977f5b7 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 15:57:08 -0400 Subject: [PATCH 19/24] docs(dgw): artifacts have no push endpoint yet Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/recording.intent.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/devolutions-gateway/src/recording.intent.md b/devolutions-gateway/src/recording.intent.md index db24c636c..14bb0371e 100644 --- a/devolutions-gateway/src/recording.intent.md +++ b/devolutions-gateway/src/recording.intent.md @@ -7,7 +7,4 @@ When connection is established with request of recordings for the session, if re # Artifacts Artifacts are files that are not recordings, currently only have `ai-analysis` with combination to `slog` file type. -We reuse the same url but with one extra query parameter, `/jet/jrec/push/{sessionId}?fileType={fileType}&kind={kind}`, where the `kind`, if not specified, we treat it as recordings. -Artifacts can be pushed without any recordings started. -Pushing artifacts should not trigger any recording policy as recordings. -Session shadowing does not support artifacts. \ No newline at end of file +Currently, we do not have an api endpoint to push artifacts, but we will have one in the future. \ No newline at end of file From 26126aac168ce6036c9da6cdcbeb70e4b2888072 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 15:59:39 -0400 Subject: [PATCH 20/24] refactor(dgw): drop unused ArtifactKind derives Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/artifacts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devolutions-gateway/src/artifacts.rs b/devolutions-gateway/src/artifacts.rs index b63259213..2441958a3 100644 --- a/devolutions-gateway/src/artifacts.rs +++ b/devolutions-gateway/src/artifacts.rs @@ -36,7 +36,7 @@ pub(crate) struct JrecArtifact { } /// Kind of a non-recording artifact, used as its key in the manifest `artifacts` object. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ArtifactKind { AiAnalysis, } From 5b0b8aba94c0f2802af41fe3b6da5db2ef5ea281 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Wed, 30 Sep 2026 12:05:55 -0400 Subject: [PATCH 21/24] refactor(dgw): stream artifacts through a writer instead of moving a local file `create_artifact` returns an `ArtifactWriter` (`AsyncWrite`) backed by a temporary file in the recordings folder; `finish` moves it into the session folder as the next `-N.` and lists it in the manifest. Producers no longer need a finished file on the same volume, and a writer dropped without `finish` leaves nothing behind. Also returns an iterator from `JrecArtifacts::into_file_names`. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/api/jrec.rs | 10 +- devolutions-gateway/src/artifacts.rs | 4 +- devolutions-gateway/src/recording.rs | 136 ++++++++++++++++++++++++--- 3 files changed, 134 insertions(+), 16 deletions(-) diff --git a/devolutions-gateway/src/api/jrec.rs b/devolutions-gateway/src/api/jrec.rs index ed4a67337..3690204cc 100644 --- a/devolutions-gateway/src/api/jrec.rs +++ b/devolutions-gateway/src/api/jrec.rs @@ -698,9 +698,13 @@ async fn snapshot_recording_zip_plan(recording_dir: &Utf8Path) -> Result Vec { + pub(crate) fn into_file_names(self) -> impl IntoIterator { let Self { ai_analysis } = self; - ai_analysis.into_iter().map(|artifact| artifact.file_name).collect() + ai_analysis.into_iter().map(|artifact| artifact.file_name) } pub(crate) fn of_kind_mut(&mut self, kind: ArtifactKind) -> &mut Vec { diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 679e11c41..214b16c4f 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -2,8 +2,9 @@ use core::fmt; use std::cmp; use std::collections::{BinaryHeap, HashMap, HashSet}; use std::path::Path; -use std::pin::pin; +use std::pin::{Pin, pin}; use std::sync::Arc; +use std::task::{Context, Poll}; use std::time::Duration; use anyhow::Context as _; @@ -252,6 +253,9 @@ enum RecordingManagerMessage { disconnected_ttl: Duration, channel: oneshot::Sender, }, + NewArtifactPath { + channel: oneshot::Sender, + }, AddArtifact { id: Uuid, kind: ArtifactKind, @@ -296,6 +300,9 @@ impl fmt::Debug for RecordingManagerMessage { .field("file_type", file_type) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), + RecordingManagerMessage::NewArtifactPath { channel: _ } => { + f.debug_struct("NewArtifactPath").finish_non_exhaustive() + } RecordingManagerMessage::AddArtifact { id, kind, @@ -330,6 +337,57 @@ impl fmt::Debug for RecordingManagerMessage { } } +/// Streams an artifact into the recordings folder, returned by [`RecordingMessageSender::create_artifact`]. +/// +/// Dropping it without [`ArtifactWriter::finish`] discards what was written. +#[derive(Debug)] +pub struct ArtifactWriter { + file: Option, + temp_path: Utf8PathBuf, + id: Uuid, + kind: ArtifactKind, + sender: RecordingMessageSender, +} + +impl ArtifactWriter { + /// Adds the written artifact to the session manifest and returns its file name. + pub async fn finish(mut self) -> anyhow::Result { + let mut file = self.file.take().expect("only taken here"); + file.flush().await.context("flush the artifact")?; + // Closed first: Windows may refuse to move a file that is still open. + drop(file.into_std().await); + + self.sender + .add_artifact(self.id, self.kind, self.temp_path.clone()) + .await + } + + fn file(&mut self) -> Pin<&mut fs::File> { + Pin::new(self.file.as_mut().expect("only taken by finish")) + } +} + +impl AsyncWrite for ArtifactWriter { + fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { + self.get_mut().file().poll_write(cx, buf) + } + + fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + self.get_mut().file().poll_flush(cx) + } + + fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + self.get_mut().file().poll_shutdown(cx) + } +} + +impl Drop for ArtifactWriter { + fn drop(&mut self) { + // After a successful finish the file was moved away, so there is nothing left to remove. + let _ = std::fs::remove_file(&self.temp_path); + } +} + #[derive(Clone, Debug)] pub struct RecordingMessageSender { channel: mpsc::Sender, @@ -359,8 +417,36 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - /// Moves `source` into the session folder as the next artifact of `kind`, and returns its file name. - pub async fn add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { + /// Starts a new artifact of `kind` for the session; it is listed in the manifest only once + /// [`ArtifactWriter::finish`] succeeds. + pub async fn create_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + let (tx, rx) = oneshot::channel(); + self.channel + .send(RecordingManagerMessage::NewArtifactPath { channel: tx }) + .await + .ok() + .context("couldn't send NewArtifactPath message")?; + let temp_path = rx.await.context("couldn't receive the artifact path")?; + + let recordings_path = temp_path.parent().expect("a parent"); + fs::create_dir_all(recordings_path) + .await + .with_context(|| format!("create {recordings_path}"))?; + + let file = fs::File::create(&temp_path) + .await + .with_context(|| format!("create {temp_path}"))?; + + Ok(ArtifactWriter { + file: Some(file), + temp_path, + id, + kind, + sender: self.clone(), + }) + } + + async fn add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::AddArtifact { @@ -882,6 +968,12 @@ async fn recording_manager_task( Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, + RecordingManagerMessage::NewArtifactPath { channel } => { + // Outside every session folder (not a session ID, so never listed), on the same volume so + // `finish` can move it in atomically. + let path = manager.recordings_path.join(format!(".artifact-{}.part", Uuid::new_v4())); + let _ = channel.send(path); + }, RecordingManagerMessage::AddArtifact { id, kind, source, channel } => { let _ = channel.send(manager.handle_add_artifact(id, kind, source).await); }, @@ -1077,7 +1169,7 @@ mod tests { const AI_ANALYSIS: ArtifactKind = ArtifactKind::AiAnalysis; struct Harness { - dir: tempfile::TempDir, + _dir: tempfile::TempDir, recordings_path: Utf8PathBuf, sender: RecordingMessageSender, _shutdown_handle: ShutdownHandle, @@ -1101,7 +1193,7 @@ mod tests { tokio::spawn(recording_manager_task(task, shutdown_signal)); Self { - dir, + _dir: dir, recordings_path, sender, _shutdown_handle: shutdown_handle, @@ -1144,13 +1236,13 @@ mod tests { } async fn add_artifact(&self, id: Uuid, content: &str) -> String { - let source = - Utf8PathBuf::from_path_buf(self.dir.path().join(Uuid::new_v4().to_string())).expect("utf8 path"); - std::fs::write(&source, content).expect("write artifact source"); - self.sender - .add_artifact(id, AI_ANALYSIS, source) + let mut writer = self + .sender + .create_artifact(id, AI_ANALYSIS) .await - .expect("add artifact") + .expect("create artifact"); + writer.write_all(content.as_bytes()).await.expect("write artifact"); + writer.finish().await.expect("finish artifact") } fn read_file(&self, id: Uuid, file_name: &str) -> String { @@ -1307,6 +1399,28 @@ mod tests { assert_eq!(harness.sender.get_count().await.expect("count"), 1); } + #[tokio::test] + async fn unfinished_artifact_leaves_nothing_behind() { + let harness = Harness::start(); + let id = Uuid::new_v4(); + harness.push(id, WEBM).await; + + let mut writer = harness + .sender + .create_artifact(id, AI_ANALYSIS) + .await + .expect("create artifact"); + writer.write_all(b"partial").await.expect("write artifact"); + drop(writer); + + assert!(harness.read_manifest(id).get("artifacts").is_none()); + let leftovers: Vec<_> = std::fs::read_dir(&harness.recordings_path) + .expect("read recordings") + .map(|entry| entry.expect("entry").file_name()) + .collect(); + assert_eq!(leftovers, [std::ffi::OsString::from(id.to_string())]); + } + #[test] fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir"); From 67b93c74a3b2fa72d81f21dc87d6fc75fbfdaec8 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Wed, 30 Sep 2026 12:21:01 -0400 Subject: [PATCH 22/24] refactor(dgw): move the artifact writer into artifacts.rs `ArtifactWriter` (now `ArtifactWriter::create`), the temp file and artifact file naming live next to the artifact types. `recording.rs` keeps only what the recording manager must own: handing out the recordings folder and listing a finished artifact in the manifest. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/artifacts.rs | 94 ++++++++++++++++++++++ devolutions-gateway/src/recording.rs | 114 ++++++--------------------- 2 files changed, 116 insertions(+), 92 deletions(-) diff --git a/devolutions-gateway/src/artifacts.rs b/devolutions-gateway/src/artifacts.rs index 073a005d0..b95bf6e75 100644 --- a/devolutions-gateway/src/artifacts.rs +++ b/devolutions-gateway/src/artifacts.rs @@ -1,5 +1,14 @@ +use std::pin::Pin; +use std::task::{Context, Poll}; + +use anyhow::Context as _; +use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; +use tokio::fs; +use tokio::io::{self, AsyncWrite, AsyncWriteExt as _}; +use uuid::Uuid; +use crate::recording::RecordingMessageSender; use crate::token::RecordingFileType; /// Non-recording artifacts, one list per [`ArtifactKind`]. Each list is append-only, like `files`: names @@ -53,6 +62,91 @@ impl ArtifactKind { ArtifactKind::AiAnalysis => RecordingFileType::SessionRecordingLog, } } + + /// Name of the artifact at `index` in its kind's list, such as `ai-analysis-0.slog`. + pub(crate) fn file_name(self, index: usize) -> String { + format!("{}-{index}.{}", self.as_str(), self.file_type().extension()) + } +} + +/// Streams a new artifact into a session. +/// +/// Dropping it without [`ArtifactWriter::finish`] discards what was written. +#[derive(Debug)] +pub struct ArtifactWriter { + file: Option, + temp_path: Utf8PathBuf, + id: Uuid, + kind: ArtifactKind, + recordings: RecordingMessageSender, +} + +impl ArtifactWriter { + /// Starts a new artifact of `kind` for the session; it is listed in the manifest only once + /// [`ArtifactWriter::finish`] succeeds. + pub async fn create(recordings: &RecordingMessageSender, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + let recordings_path = recordings.get_recordings_path().await?; + let temp_path = temp_path(&recordings_path); + + fs::create_dir_all(&recordings_path) + .await + .with_context(|| format!("create {recordings_path}"))?; + + let file = fs::File::create(&temp_path) + .await + .with_context(|| format!("create {temp_path}"))?; + + Ok(Self { + file: Some(file), + temp_path, + id, + kind, + recordings: recordings.clone(), + }) + } + + /// Adds the written artifact to the session manifest and returns its file name. + pub async fn finish(mut self) -> anyhow::Result { + let mut file = self.file.take().expect("only taken here"); + file.flush().await.context("flush the artifact")?; + // Closed first: Windows may refuse to move a file that is still open. + drop(file.into_std().await); + + self.recordings + .add_artifact(self.id, self.kind, self.temp_path.clone()) + .await + } + + fn file(&mut self) -> Pin<&mut fs::File> { + Pin::new(self.file.as_mut().expect("only taken by finish")) + } +} + +impl AsyncWrite for ArtifactWriter { + fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { + self.get_mut().file().poll_write(cx, buf) + } + + fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + self.get_mut().file().poll_flush(cx) + } + + fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + self.get_mut().file().poll_shutdown(cx) + } +} + +impl Drop for ArtifactWriter { + fn drop(&mut self) { + // After a successful finish the file was moved away, so there is nothing left to remove. + let _ = std::fs::remove_file(&self.temp_path); + } +} + +/// Outside every session folder (not a session ID, so never listed as a recording), on the same volume so +/// [`ArtifactWriter::finish`] can move it in atomically. +fn temp_path(recordings_path: &Utf8Path) -> Utf8PathBuf { + recordings_path.join(format!(".artifact-{}.part", Uuid::new_v4())) } #[cfg(test)] diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 214b16c4f..31a6b1406 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -2,9 +2,8 @@ use core::fmt; use std::cmp; use std::collections::{BinaryHeap, HashMap, HashSet}; use std::path::Path; -use std::pin::{Pin, pin}; +use std::pin::pin; use std::sync::Arc; -use std::task::{Context, Poll}; use std::time::Duration; use anyhow::Context as _; @@ -253,7 +252,7 @@ enum RecordingManagerMessage { disconnected_ttl: Duration, channel: oneshot::Sender, }, - NewArtifactPath { + GetRecordingsPath { channel: oneshot::Sender, }, AddArtifact { @@ -300,8 +299,8 @@ impl fmt::Debug for RecordingManagerMessage { .field("file_type", file_type) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), - RecordingManagerMessage::NewArtifactPath { channel: _ } => { - f.debug_struct("NewArtifactPath").finish_non_exhaustive() + RecordingManagerMessage::GetRecordingsPath { channel: _ } => { + f.debug_struct("GetRecordingsPath").finish_non_exhaustive() } RecordingManagerMessage::AddArtifact { id, @@ -337,57 +336,6 @@ impl fmt::Debug for RecordingManagerMessage { } } -/// Streams an artifact into the recordings folder, returned by [`RecordingMessageSender::create_artifact`]. -/// -/// Dropping it without [`ArtifactWriter::finish`] discards what was written. -#[derive(Debug)] -pub struct ArtifactWriter { - file: Option, - temp_path: Utf8PathBuf, - id: Uuid, - kind: ArtifactKind, - sender: RecordingMessageSender, -} - -impl ArtifactWriter { - /// Adds the written artifact to the session manifest and returns its file name. - pub async fn finish(mut self) -> anyhow::Result { - let mut file = self.file.take().expect("only taken here"); - file.flush().await.context("flush the artifact")?; - // Closed first: Windows may refuse to move a file that is still open. - drop(file.into_std().await); - - self.sender - .add_artifact(self.id, self.kind, self.temp_path.clone()) - .await - } - - fn file(&mut self) -> Pin<&mut fs::File> { - Pin::new(self.file.as_mut().expect("only taken by finish")) - } -} - -impl AsyncWrite for ArtifactWriter { - fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { - self.get_mut().file().poll_write(cx, buf) - } - - fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.get_mut().file().poll_flush(cx) - } - - fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.get_mut().file().poll_shutdown(cx) - } -} - -impl Drop for ArtifactWriter { - fn drop(&mut self) { - // After a successful finish the file was moved away, so there is nothing left to remove. - let _ = std::fs::remove_file(&self.temp_path); - } -} - #[derive(Clone, Debug)] pub struct RecordingMessageSender { channel: mpsc::Sender, @@ -417,36 +365,24 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - /// Starts a new artifact of `kind` for the session; it is listed in the manifest only once - /// [`ArtifactWriter::finish`] succeeds. - pub async fn create_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { + pub(crate) async fn get_recordings_path(&self) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel - .send(RecordingManagerMessage::NewArtifactPath { channel: tx }) + .send(RecordingManagerMessage::GetRecordingsPath { channel: tx }) .await .ok() - .context("couldn't send NewArtifactPath message")?; - let temp_path = rx.await.context("couldn't receive the artifact path")?; - - let recordings_path = temp_path.parent().expect("a parent"); - fs::create_dir_all(recordings_path) - .await - .with_context(|| format!("create {recordings_path}"))?; - - let file = fs::File::create(&temp_path) - .await - .with_context(|| format!("create {temp_path}"))?; - - Ok(ArtifactWriter { - file: Some(file), - temp_path, - id, - kind, - sender: self.clone(), - }) + .context("couldn't send GetRecordingsPath message")?; + rx.await.context("couldn't receive the recordings path") } - async fn add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { + /// Moves `source`, a file in the recordings folder, into the session folder as the next artifact of `kind`, + /// and lists it in the manifest. + pub(crate) async fn add_artifact( + &self, + id: Uuid, + kind: ArtifactKind, + source: Utf8PathBuf, + ) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel .send(RecordingManagerMessage::AddArtifact { @@ -822,7 +758,7 @@ impl RecordingManagerTask { }; let artifacts = manifest.artifacts.of_kind_mut(kind); - let file_name = format!("{}-{}.{}", kind.as_str(), artifacts.len(), kind.file_type().extension()); + let file_name = kind.file_name(artifacts.len()); // The file is in place before the manifest lists it, so readers never see a dangling entry. let artifact_path = recording_path.join(&file_name); @@ -968,11 +904,8 @@ async fn recording_manager_task( Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, - RecordingManagerMessage::NewArtifactPath { channel } => { - // Outside every session folder (not a session ID, so never listed), on the same volume so - // `finish` can move it in atomically. - let path = manager.recordings_path.join(format!(".artifact-{}.part", Uuid::new_v4())); - let _ = channel.send(path); + RecordingManagerMessage::GetRecordingsPath { channel } => { + let _ = channel.send(manager.recordings_path.clone()); }, RecordingManagerMessage::AddArtifact { id, kind, source, channel } => { let _ = channel.send(manager.handle_add_artifact(id, kind, source).await); @@ -1150,6 +1083,7 @@ mod tests { use serde_json::json; use super::*; + use crate::artifacts::ArtifactWriter; const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", @@ -1236,9 +1170,7 @@ mod tests { } async fn add_artifact(&self, id: Uuid, content: &str) -> String { - let mut writer = self - .sender - .create_artifact(id, AI_ANALYSIS) + let mut writer = ArtifactWriter::create(&self.sender, id, AI_ANALYSIS) .await .expect("create artifact"); writer.write_all(content.as_bytes()).await.expect("write artifact"); @@ -1405,9 +1337,7 @@ mod tests { let id = Uuid::new_v4(); harness.push(id, WEBM).await; - let mut writer = harness - .sender - .create_artifact(id, AI_ANALYSIS) + let mut writer = ArtifactWriter::create(&harness.sender, id, AI_ANALYSIS) .await .expect("create artifact"); writer.write_all(b"partial").await.expect("write artifact"); From b560157e4a2913930677e67fb9ecd5cbb712dd9a Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Wed, 30 Sep 2026 13:36:52 -0400 Subject: [PATCH 23/24] refactor(dgw): add artifacts only to recorded sessions `ArtifactWriter::finish` now fails when the session has no recording, so a recording deleted while an artifact is being written stays deleted; the artifact-first manifest path goes away with it. The temp file cleanup moves to a small guard so the writer holds its file directly, and a FIXME notes that a Gateway stopped mid-write leaves the temp file behind. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/artifacts.rs | 51 ++++++++++++--------- devolutions-gateway/src/recording.rs | 67 +++++++++------------------- 2 files changed, 49 insertions(+), 69 deletions(-) diff --git a/devolutions-gateway/src/artifacts.rs b/devolutions-gateway/src/artifacts.rs index b95bf6e75..e6f2f5c27 100644 --- a/devolutions-gateway/src/artifacts.rs +++ b/devolutions-gateway/src/artifacts.rs @@ -74,8 +74,8 @@ impl ArtifactKind { /// Dropping it without [`ArtifactWriter::finish`] discards what was written. #[derive(Debug)] pub struct ArtifactWriter { - file: Option, - temp_path: Utf8PathBuf, + file: fs::File, + temp: TempFile, id: Uuid, kind: ArtifactKind, recordings: RecordingMessageSender, @@ -86,19 +86,19 @@ impl ArtifactWriter { /// [`ArtifactWriter::finish`] succeeds. pub async fn create(recordings: &RecordingMessageSender, id: Uuid, kind: ArtifactKind) -> anyhow::Result { let recordings_path = recordings.get_recordings_path().await?; - let temp_path = temp_path(&recordings_path); fs::create_dir_all(&recordings_path) .await .with_context(|| format!("create {recordings_path}"))?; - let file = fs::File::create(&temp_path) + let temp = TempFile(temp_path(&recordings_path)); + let file = fs::File::create(&temp.0) .await - .with_context(|| format!("create {temp_path}"))?; + .with_context(|| format!("create {}", temp.0))?; Ok(Self { - file: Some(file), - temp_path, + file, + temp, id, kind, recordings: recordings.clone(), @@ -106,45 +106,52 @@ impl ArtifactWriter { } /// Adds the written artifact to the session manifest and returns its file name. - pub async fn finish(mut self) -> anyhow::Result { - let mut file = self.file.take().expect("only taken here"); + /// + /// Fails when the session has no recording, such as one deleted while the artifact was being written. + pub async fn finish(self) -> anyhow::Result { + let Self { + mut file, + temp, + id, + kind, + recordings, + } = self; + file.flush().await.context("flush the artifact")?; // Closed first: Windows may refuse to move a file that is still open. drop(file.into_std().await); - self.recordings - .add_artifact(self.id, self.kind, self.temp_path.clone()) - .await - } - - fn file(&mut self) -> Pin<&mut fs::File> { - Pin::new(self.file.as_mut().expect("only taken by finish")) + recordings.add_artifact(id, kind, temp.0.clone()).await } } impl AsyncWrite for ArtifactWriter { fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { - self.get_mut().file().poll_write(cx, buf) + Pin::new(&mut self.get_mut().file).poll_write(cx, buf) } fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.get_mut().file().poll_flush(cx) + Pin::new(&mut self.get_mut().file).poll_flush(cx) } fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.get_mut().file().poll_shutdown(cx) + Pin::new(&mut self.get_mut().file).poll_shutdown(cx) } } -impl Drop for ArtifactWriter { +/// Removes the temporary file when dropped; once `finish` moved it into the session, there is nothing to remove. +#[derive(Debug)] +struct TempFile(Utf8PathBuf); + +impl Drop for TempFile { fn drop(&mut self) { - // After a successful finish the file was moved away, so there is nothing left to remove. - let _ = std::fs::remove_file(&self.temp_path); + let _ = std::fs::remove_file(&self.0); } } /// Outside every session folder (not a session ID, so never listed as a recording), on the same volume so /// [`ArtifactWriter::finish`] can move it in atomically. +// FIXME: a Gateway stopped while a writer is open leaves this file behind; nothing sweeps them yet. fn temp_path(recordings_path: &Utf8Path) -> Utf8PathBuf { recordings_path.join(format!(".artifact-{}.part", Uuid::new_v4())) } diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index 31a6b1406..c03da73b2 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -585,11 +585,6 @@ impl RecordingManagerTask { let start_time = time::OffsetDateTime::now_utc().unix_timestamp(); - // An artifact may have created the manifest before any recording. - if existing_manifest.files.is_empty() { - existing_manifest.start_time = start_time; - } - let file_name = format!("recording-{next_file_idx}.{}", file_type.extension()); let recording_file = recording_path.join(&file_name); @@ -740,22 +735,12 @@ impl RecordingManagerTask { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - let mut manifest = if recording_path.exists() { - JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")? - } else { - fs::create_dir_all(&recording_path) - .await - .with_context(|| format!("failed to create recording path: {recording_path}"))?; + // A recording deleted while its artifact was being written must stay deleted. + if !manifest_path.exists() { + anyhow::bail!("session {id} has no recording"); + } - // The session timing belongs to the recordings, so the first recording push sets it. - JrecManifest { - session_id: id, - start_time: 0, - duration: 0, - files: Vec::new(), - artifacts: JrecArtifacts::default(), - } - }; + let mut manifest = JrecManifest::read_from_file(&manifest_path).context("read manifest from disk")?; let artifacts = manifest.artifacts.of_kind_mut(kind); let file_name = kind.file_name(artifacts.len()); @@ -1222,26 +1207,20 @@ mod tests { } #[tokio::test] - async fn artifact_first_leaves_session_timing_to_the_recording() { + async fn artifact_needs_a_recording() { let harness = Harness::start(); let id = Uuid::new_v4(); - let artifact = harness.add_artifact(id, "analysis").await; - - assert_eq!(artifact, "ai-analysis-0.slog"); - let manifest = harness.read_manifest(id); - assert_eq!(manifest["files"], json!([])); - assert_eq!(manifest["startTime"], 0); - assert_eq!(manifest["duration"], 0); - assert_eq!( - manifest["artifacts"], - json!({ "ai-analysis": [{ "fileName": "ai-analysis-0.slog" }] }) - ); + let mut writer = ArtifactWriter::create(&harness.sender, id, AI_ANALYSIS) + .await + .expect("create artifact"); + writer.write_all(b"analysis").await.expect("write artifact"); - assert_eq!(harness.connect(id, WEBM).await, "recording-0.webm"); - let manifest = harness.read_manifest(id); - assert_ne!(manifest["startTime"], 0); - assert_eq!(manifest["startTime"], manifest["files"][0]["startTime"]); + assert!(writer.finish().await.is_err()); + let leftovers = std::fs::read_dir(&harness.recordings_path) + .expect("read recordings") + .count(); + assert_eq!(leftovers, 0); } #[tokio::test] @@ -1306,19 +1285,13 @@ mod tests { #[tokio::test] async fn artifact_stays_out_of_the_recording_lifecycle() { let harness = Harness::start(); - let artifact_only = Uuid::new_v4(); + let finished = Uuid::new_v4(); let recorded = Uuid::new_v4(); + harness.write_manifest(finished, MASTER_MANIFEST); - harness.add_artifact(artifact_only, "analysis").await; - assert!( - harness - .sender - .get_state(artifact_only) - .await - .expect("get state") - .is_none() - ); - assert!(!harness.sender.active_recordings.contains(artifact_only)); + harness.add_artifact(finished, "analysis").await; + assert!(harness.sender.get_state(finished).await.expect("get state").is_none()); + assert!(!harness.sender.active_recordings.contains(finished)); assert_eq!(harness.sender.get_count().await.expect("count"), 0); harness.push(recorded, WEBM).await; From e86ed29d7be4c8f5b16fc464226c19e0835ceb9b Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Wed, 30 Sep 2026 13:54:45 -0400 Subject: [PATCH 24/24] refactor(dgw): hand out a listed artifact file instead of a streaming writer Like `Connect` for recordings, `add_artifact(id, kind)` has the recording manager allocate the next `-N.` in the session folder, create it empty, list it in the manifest and return its path; the caller writes into it. This drops the writer, its temp file and the recordings-path message. It still fails when the session has no recording. Co-Authored-By: Claude Opus 5.5 (1M context) --- devolutions-gateway/src/artifacts.rs | 96 ----------------------- devolutions-gateway/src/recording.rs | 113 +++++---------------------- 2 files changed, 21 insertions(+), 188 deletions(-) diff --git a/devolutions-gateway/src/artifacts.rs b/devolutions-gateway/src/artifacts.rs index e6f2f5c27..a2ecd5fa8 100644 --- a/devolutions-gateway/src/artifacts.rs +++ b/devolutions-gateway/src/artifacts.rs @@ -1,14 +1,5 @@ -use std::pin::Pin; -use std::task::{Context, Poll}; - -use anyhow::Context as _; -use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; -use tokio::fs; -use tokio::io::{self, AsyncWrite, AsyncWriteExt as _}; -use uuid::Uuid; -use crate::recording::RecordingMessageSender; use crate::token::RecordingFileType; /// Non-recording artifacts, one list per [`ArtifactKind`]. Each list is append-only, like `files`: names @@ -69,93 +60,6 @@ impl ArtifactKind { } } -/// Streams a new artifact into a session. -/// -/// Dropping it without [`ArtifactWriter::finish`] discards what was written. -#[derive(Debug)] -pub struct ArtifactWriter { - file: fs::File, - temp: TempFile, - id: Uuid, - kind: ArtifactKind, - recordings: RecordingMessageSender, -} - -impl ArtifactWriter { - /// Starts a new artifact of `kind` for the session; it is listed in the manifest only once - /// [`ArtifactWriter::finish`] succeeds. - pub async fn create(recordings: &RecordingMessageSender, id: Uuid, kind: ArtifactKind) -> anyhow::Result { - let recordings_path = recordings.get_recordings_path().await?; - - fs::create_dir_all(&recordings_path) - .await - .with_context(|| format!("create {recordings_path}"))?; - - let temp = TempFile(temp_path(&recordings_path)); - let file = fs::File::create(&temp.0) - .await - .with_context(|| format!("create {}", temp.0))?; - - Ok(Self { - file, - temp, - id, - kind, - recordings: recordings.clone(), - }) - } - - /// Adds the written artifact to the session manifest and returns its file name. - /// - /// Fails when the session has no recording, such as one deleted while the artifact was being written. - pub async fn finish(self) -> anyhow::Result { - let Self { - mut file, - temp, - id, - kind, - recordings, - } = self; - - file.flush().await.context("flush the artifact")?; - // Closed first: Windows may refuse to move a file that is still open. - drop(file.into_std().await); - - recordings.add_artifact(id, kind, temp.0.clone()).await - } -} - -impl AsyncWrite for ArtifactWriter { - fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { - Pin::new(&mut self.get_mut().file).poll_write(cx, buf) - } - - fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - Pin::new(&mut self.get_mut().file).poll_flush(cx) - } - - fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - Pin::new(&mut self.get_mut().file).poll_shutdown(cx) - } -} - -/// Removes the temporary file when dropped; once `finish` moved it into the session, there is nothing to remove. -#[derive(Debug)] -struct TempFile(Utf8PathBuf); - -impl Drop for TempFile { - fn drop(&mut self) { - let _ = std::fs::remove_file(&self.0); - } -} - -/// Outside every session folder (not a session ID, so never listed as a recording), on the same volume so -/// [`ArtifactWriter::finish`] can move it in atomically. -// FIXME: a Gateway stopped while a writer is open leaves this file behind; nothing sweeps them yet. -fn temp_path(recordings_path: &Utf8Path) -> Utf8PathBuf { - recordings_path.join(format!(".artifact-{}.part", Uuid::new_v4())) -} - #[cfg(test)] mod tests { use super::*; diff --git a/devolutions-gateway/src/recording.rs b/devolutions-gateway/src/recording.rs index c03da73b2..59905df5b 100644 --- a/devolutions-gateway/src/recording.rs +++ b/devolutions-gateway/src/recording.rs @@ -252,14 +252,10 @@ enum RecordingManagerMessage { disconnected_ttl: Duration, channel: oneshot::Sender, }, - GetRecordingsPath { - channel: oneshot::Sender, - }, AddArtifact { id: Uuid, kind: ArtifactKind, - source: Utf8PathBuf, - channel: oneshot::Sender>, + channel: oneshot::Sender>, }, Disconnect { id: Uuid, @@ -299,19 +295,10 @@ impl fmt::Debug for RecordingManagerMessage { .field("file_type", file_type) .field("disconnected_ttl", disconnected_ttl) .finish_non_exhaustive(), - RecordingManagerMessage::GetRecordingsPath { channel: _ } => { - f.debug_struct("GetRecordingsPath").finish_non_exhaustive() - } - RecordingManagerMessage::AddArtifact { - id, - kind, - source, - channel: _, - } => f + RecordingManagerMessage::AddArtifact { id, kind, channel: _ } => f .debug_struct("AddArtifact") .field("id", id) .field("kind", kind) - .field("source", source) .finish_non_exhaustive(), RecordingManagerMessage::Disconnect { id } => f.debug_struct("Disconnect").field("id", id).finish(), RecordingManagerMessage::GetState { id, channel: _ } => { @@ -365,32 +352,13 @@ impl RecordingMessageSender { .context("couldn't receive recording file path for this recording") } - pub(crate) async fn get_recordings_path(&self) -> anyhow::Result { - let (tx, rx) = oneshot::channel(); - self.channel - .send(RecordingManagerMessage::GetRecordingsPath { channel: tx }) - .await - .ok() - .context("couldn't send GetRecordingsPath message")?; - rx.await.context("couldn't receive the recordings path") - } - - /// Moves `source`, a file in the recordings folder, into the session folder as the next artifact of `kind`, - /// and lists it in the manifest. - pub(crate) async fn add_artifact( - &self, - id: Uuid, - kind: ArtifactKind, - source: Utf8PathBuf, - ) -> anyhow::Result { + /// Adds an empty artifact of `kind` to the session and returns its path, for the caller to write into. + /// + /// Fails when the session has no recording. + pub async fn add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { let (tx, rx) = oneshot::channel(); self.channel - .send(RecordingManagerMessage::AddArtifact { - id, - kind, - source, - channel: tx, - }) + .send(RecordingManagerMessage::AddArtifact { id, kind, channel: tx }) .await .ok() .context("couldn't send AddArtifact message")?; @@ -731,11 +699,11 @@ impl RecordingManagerTask { Ok(()) } - async fn handle_add_artifact(&self, id: Uuid, kind: ArtifactKind, source: Utf8PathBuf) -> anyhow::Result { + async fn handle_add_artifact(&self, id: Uuid, kind: ArtifactKind) -> anyhow::Result { let recording_path = self.recordings_path.join(id.to_string()); let manifest_path = recording_path.join("recording.json"); - // A recording deleted while its artifact was being written must stay deleted. + // A recording deleted while its artifact was being generated must stay deleted. if !manifest_path.exists() { anyhow::bail!("session {id} has no recording"); } @@ -744,22 +712,19 @@ impl RecordingManagerTask { let artifacts = manifest.artifacts.of_kind_mut(kind); let file_name = kind.file_name(artifacts.len()); - - // The file is in place before the manifest lists it, so readers never see a dangling entry. let artifact_path = recording_path.join(&file_name); - fs::rename(&source, &artifact_path) + + fs::File::create(&artifact_path) .await - .with_context(|| format!("move {source} to {artifact_path}"))?; + .with_context(|| format!("create {artifact_path}"))?; - artifacts.push(JrecArtifact { - file_name: file_name.clone(), - }); + artifacts.push(JrecArtifact { file_name }); manifest .save_to_file(&manifest_path) .context("write manifest to disk")?; - Ok(file_name) + Ok(artifact_path) } fn handle_remove(&mut self, id: Uuid) { @@ -889,11 +854,8 @@ async fn recording_manager_task( Err(e) => error!(error = format!("{e:#}"), "handle_connect"), } }, - RecordingManagerMessage::GetRecordingsPath { channel } => { - let _ = channel.send(manager.recordings_path.clone()); - }, - RecordingManagerMessage::AddArtifact { id, kind, source, channel } => { - let _ = channel.send(manager.handle_add_artifact(id, kind, source).await); + RecordingManagerMessage::AddArtifact { id, kind, channel } => { + let _ = channel.send(manager.handle_add_artifact(id, kind).await); }, RecordingManagerMessage::Disconnect { id } => { if let Err(e) = manager.handle_disconnect(id).await { @@ -1068,8 +1030,6 @@ mod tests { use serde_json::json; use super::*; - use crate::artifacts::ArtifactWriter; - const MASTER_MANIFEST: &str = r#"{ "sessionId": "22fcd533-5e72-4db7-aa0f-29952dbbca9f", "startTime": 1, @@ -1155,11 +1115,9 @@ mod tests { } async fn add_artifact(&self, id: Uuid, content: &str) -> String { - let mut writer = ArtifactWriter::create(&self.sender, id, AI_ANALYSIS) - .await - .expect("create artifact"); - writer.write_all(content.as_bytes()).await.expect("write artifact"); - writer.finish().await.expect("finish artifact") + let path = self.sender.add_artifact(id, AI_ANALYSIS).await.expect("add artifact"); + std::fs::write(&path, content).expect("write artifact"); + path.file_name().expect("file name").to_owned() } fn read_file(&self, id: Uuid, file_name: &str) -> String { @@ -1209,18 +1167,9 @@ mod tests { #[tokio::test] async fn artifact_needs_a_recording() { let harness = Harness::start(); - let id = Uuid::new_v4(); - let mut writer = ArtifactWriter::create(&harness.sender, id, AI_ANALYSIS) - .await - .expect("create artifact"); - writer.write_all(b"analysis").await.expect("write artifact"); - - assert!(writer.finish().await.is_err()); - let leftovers = std::fs::read_dir(&harness.recordings_path) - .expect("read recordings") - .count(); - assert_eq!(leftovers, 0); + assert!(harness.sender.add_artifact(Uuid::new_v4(), AI_ANALYSIS).await.is_err()); + assert!(!harness.recordings_path.exists()); } #[tokio::test] @@ -1304,26 +1253,6 @@ mod tests { assert_eq!(harness.sender.get_count().await.expect("count"), 1); } - #[tokio::test] - async fn unfinished_artifact_leaves_nothing_behind() { - let harness = Harness::start(); - let id = Uuid::new_v4(); - harness.push(id, WEBM).await; - - let mut writer = ArtifactWriter::create(&harness.sender, id, AI_ANALYSIS) - .await - .expect("create artifact"); - writer.write_all(b"partial").await.expect("write artifact"); - drop(writer); - - assert!(harness.read_manifest(id).get("artifacts").is_none()); - let leftovers: Vec<_> = std::fs::read_dir(&harness.recordings_path) - .expect("read recordings") - .map(|entry| entry.expect("entry").file_name()) - .collect(); - assert_eq!(leftovers, [std::ffi::OsString::from(id.to_string())]); - } - #[test] fn manifest_without_artifacts_round_trips_byte_for_byte() { let dir = tempfile::tempdir().expect("temp dir");