fix(volume): preserve existing speed test directories - #3082

core/src/volume/speed.rs (2)

124-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use structured fields in the cleanup warnings.

Both warn! calls embed the error in the message string instead of using named fields. Structured fields let log tooling filter on the failing path or error kind without parsing free text.

♻️ Proposed fix for structured logging

-				warn!("Failed to remove test file: {}", e);
+				warn!(error = %e, path = %self.test_file.display(), "Failed to remove test file");

-				warn!("Failed to remove test directory: {}", e);
+				warn!(error = %e, path = %dir.display(), "Failed to remove test directory");

As per coding guidelines, "Include relevant context fields in structured logging, e.g., debug!(job_id = %self.id, "message")."

Also applies to: 135-135

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/src/volume/speed.rs` at line 124, Update both cleanup warning calls
around the test-file removal and cleanup paths to use structured logging fields
for the error instead of interpolating it into the message; preserve the
existing warning context and include the error as a named field such as error =
%e.

Source: Coding guidelines

---

69-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Preserve the primary test result if cleanup ever becomes fallible.

cleanup() always returns Ok(()): every failure branch (Line 124, Line 135) only calls warn! and continues. Given that, test_location.cleanup().await?; at Line 72 can never actually fail today, so the ? is currently a no-op.

This is fragile. cleanup()'s signature promises VolumeResult<()>, so a future change that makes it genuinely propagate a remove_file/remove_dir error is a natural evolution. If that happens, the current ordering discards the true perform_speed_test outcome (for example a VolumeError::Timeout) in favor of a cleanup error, because test_location.cleanup().await?; runs before let result = result?;.

Restructure so a cleanup failure never shadows the primary result.

♻️ Proposed fix to prioritize the primary result

 	let mut test_location = TestLocation::new(&volume.mount_point, &volume.mount_type).await?;
 	let result = perform_speed_test(&mut test_location, &config).await;
-
-	test_location.cleanup().await?;
-	let result = result?;
+	if let Err(cleanup_err) = test_location.cleanup().await {
+		warn!("Failed to clean up speed test resources: {}", cleanup_err);
+	}
+	let result = result?;

Also applies to: 110-141

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/src/volume/speed.rs` around lines 69 - 73, Update the test flow around
perform_speed_test and TestLocation::cleanup so the primary test result is
resolved before propagating any cleanup error: preserve and return the
perform_speed_test error while still always attempting cleanup, and only
propagate cleanup failure when the test itself succeeded. Apply the same
ordering to the cleanup-related flow in the referenced TestLocation
implementation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

core/src/volume/speed.rs (1)

86-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a shared remove-and-log helper.

cleanup() (lines 111-125) and the permission-probe removal (lines 275-287) implement the same pattern: remove a file, treat Ok and NotFound as success, and warn! on any other error. Extract one helper to avoid divergence between the two call sites as this code evolves.

♻️ Proposed refactor

+async fn remove_file_best_effort(path: &std::path::Path, context: &str) -> bool {
+	match tokio::fs::remove_file(path).await {
+		Ok(()) => true,
+		Err(e) if e.kind() == std::io::ErrorKind::NotFound => true,
+		Err(e) => {
+			warn!(error = %e, path = %path.display(), "Failed to remove {}", context);
+			false
+		}
+	}
+}
+
 	/// Clean up the test file
 	async fn cleanup(&mut self) {
 		// Never remove a file unless this speed test successfully created it.
 		if self.test_file_created {
-			match tokio::fs::remove_file(&self.test_file).await {
-				Ok(()) => {
-					self.test_file_created = false;
-				}
-				Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
-					self.test_file_created = false;
-				}
-				Err(e) => {
-					warn!(
-						error = %e,
-						path = %self.test_file.display(),
-						"Failed to remove speed test file"
-					);
-				}
-			}
+			if remove_file_best_effort(&self.test_file, "speed test file").await {
+				self.test_file_created = false;
+			}
 		}
 	}

 					Ok(mut file) => {
-						let write_result = file.write_all(b"test").await;
+						let write_ok = file.write_all(b"test").await.is_ok();
 						drop(file);
-						let cleanup_succeeded = match tokio::fs::remove_file(&permission_file).await
-						{
-							Ok(()) => true,
-							Err(error) if error.kind() == std::io::ErrorKind::NotFound => true,
-							Err(error) => {
-								warn!(
-									error = %error,
-									path = %permission_file.display(),
-									"Failed to remove speed test permission probe"
-								);
-								false
-							}
-						};
-						write_result.is_ok() && cleanup_succeeded
+						write_ok
+							&& remove_file_best_effort(&permission_file, "speed test permission probe")
+								.await
 					}

Also applies to: 262-296

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/src/volume/speed.rs` around lines 86 - 128, Extract the shared
remove-and-log behavior from TestLocation::cleanup and the permission-probe
removal around the permission test flow into one helper. Have the helper remove
the provided path, treat successful removal and NotFound as success, and warn
with the error and path for other failures; update both call sites to use it
while preserving test_file_created state handling in cleanup.