Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/workers/continuum-core/src/modules/sentinel/checkpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,17 @@ use std::path::PathBuf;

use super::types::{PipelineCheckpoint, PipelineStatus};

/// Base directory for checkpoint storage
/// Base directory for checkpoint storage.
///
/// Default: `~/.continuum/sentinel/checkpoints`.
/// Overridable via `CONTINUUM_CHECKPOINT_DIR` env var — used by tests to
/// isolate checkpoint state from the user's real `~/.continuum` (and to
/// survive the case where root-owned directories from a previous docker
/// container run leave the default path unwritable for the dev user).
fn checkpoints_dir() -> PathBuf {
if let Ok(override_dir) = std::env::var("CONTINUUM_CHECKPOINT_DIR") {
return PathBuf::from(override_dir);
Comment on lines +18 to +19
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkpoints_dir() uses std::env::var, which requires UTF-8 and will also accept an empty string (mapping to PathBuf::from(""), i.e., the current working directory). Since this env var is intended to be a filesystem path override, consider using std::env::var_os and ignoring empty values so an accidentally-set empty/invalid override doesn’t redirect checkpoint writes into the process CWD.

Suggested change
if let Ok(override_dir) = std::env::var("CONTINUUM_CHECKPOINT_DIR") {
return PathBuf::from(override_dir);
if let Some(override_dir) = std::env::var_os("CONTINUUM_CHECKPOINT_DIR") {
if !override_dir.is_empty() {
return PathBuf::from(override_dir);
}

Copilot uses AI. Check for mistakes.
}
let home = dirs::home_dir().expect("Failed to resolve home directory");
home.join(".continuum").join("sentinel").join("checkpoints")
}
Expand Down Expand Up @@ -137,6 +146,35 @@ mod tests {
use super::*;
use crate::modules::sentinel::types::*;
use std::collections::HashMap;
use std::sync::OnceLock;
use tempfile::TempDir;

/// Process-global tempdir for checkpoint tests, lazily initialized on
/// first access. All tests in this module share it — they use unique
/// UUID-derived handles, so file collisions don't happen. This isolates
/// the test runs from the user's real `~/.continuum/sentinel/checkpoints`
/// (which may be root-owned-and-unwritable on a dev box that previously
/// ran a docker container that mounted $HOME and chmod'd the dir under
/// root).
///
/// Stored in a static so the TempDir is dropped (and cleaned up) only
/// when the test process exits — a per-test TempDir would race with
/// cargo's default parallel test execution since `set_var` is process-
/// global.
fn ensure_test_checkpoint_dir() {
static TMPDIR: OnceLock<TempDir> = OnceLock::new();
let dir = TMPDIR.get_or_init(|| {
tempfile::tempdir().expect("Failed to create test checkpoint tempdir")
});
// SAFETY: set_var is unsafe in newer Rust (process-global, racy with
// other threads reading env). Tests in this module only ever write
// the SAME path, so concurrent setters write the same value — race-
// free in practice.
std::env::set_var(
"CONTINUUM_CHECKPOINT_DIR",
dir.path(),
);
Comment on lines +166 to +176
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_test_checkpoint_dir() calls std::env::set_var on every test invocation. With cargo’s parallel test execution, multiple tests can call set_var concurrently, which is exactly the racy/unsound pattern the comment warns about (even if all writers set the same value). To avoid this, set the env var only once (e.g., inside the OnceLock init path or via an additional Once guard) so only a single thread ever performs the set_var call.

Suggested change
let dir = TMPDIR.get_or_init(|| {
tempfile::tempdir().expect("Failed to create test checkpoint tempdir")
});
// SAFETY: set_var is unsafe in newer Rust (process-global, racy with
// other threads reading env). Tests in this module only ever write
// the SAME path, so concurrent setters write the same value — race-
// free in practice.
std::env::set_var(
"CONTINUUM_CHECKPOINT_DIR",
dir.path(),
);
TMPDIR.get_or_init(|| {
let dir =
tempfile::tempdir().expect("Failed to create test checkpoint tempdir");
// SAFETY: set_var is process-global and must not race with other
// writers. Performing it inside the OnceLock init closure ensures
// only a single thread sets the test override for the lifetime of
// this test process.
std::env::set_var(
"CONTINUUM_CHECKPOINT_DIR",
dir.path(),
);
dir
});

Copilot uses AI. Check for mistakes.
}

fn make_test_checkpoint(handle: &str) -> PipelineCheckpoint {
PipelineCheckpoint {
Expand Down Expand Up @@ -189,6 +227,7 @@ mod tests {

#[test]
fn test_save_load_checkpoint() {
ensure_test_checkpoint_dir();
let handle = format!(
"test-ckpt-{}",
uuid::Uuid::new_v4().to_string()[..8].to_string()
Expand All @@ -208,6 +247,7 @@ mod tests {

#[test]
fn test_list_checkpoints() {
ensure_test_checkpoint_dir();
let handle = format!(
"test-list-{}",
uuid::Uuid::new_v4().to_string()[..8].to_string()
Expand All @@ -223,6 +263,7 @@ mod tests {

#[test]
fn test_recover_interrupted() {
ensure_test_checkpoint_dir();
let handle = format!(
"test-recover-{}",
uuid::Uuid::new_v4().to_string()[..8].to_string()
Expand Down
Loading