diff --git a/clippy.toml b/clippy.toml index f5453a26..c84fd9b6 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,4 +1,5 @@ disallowed-methods = [ + { path = "std::env::current_dir", reason = "The library must not depend on the process working directory: a call executed by the Lore service runs in a process whose directory is unrelated to the caller's. Use lore_revision::util::path::make_absolute, which resolves against the working directory the call carries." }, { path = "dashmap::DashMap::entry", reason = "This function internally takes a write lock and returns it in the entry handle. This can potentially block worker threads and cause deadlocks if used incorrectly. If you're sure it's ok add #[allow(clippy::disallowed_methods)] with a comment explaining why its safe" }, { path = "tokio::spawn", reason = "Use lore_base::lore_spawn! so LORE_CONTEXT is propagated to the spawned task." }, { path = "tokio::task::spawn", reason = "Use lore_base::lore_spawn! so LORE_CONTEXT is propagated to the spawned task." }, diff --git a/lore-capi/lore.h b/lore-capi/lore.h index e65c1406..f04f7eb6 100644 --- a/lore-capi/lore.h +++ b/lore-capi/lore.h @@ -3530,6 +3530,12 @@ typedef struct lore_event_t { typedef struct lore_global_args_t { // Repository path struct lore_string_t repository_path; + // Directory that relative paths in this call are resolved against. Set it + // when a call may be executed by another process, such as the Lore + // service, whose own working directory is unrelated to the caller's. When + // empty, relative paths resolve against the working directory of the + // process performing the call. + struct lore_string_t working_directory; // Correlation ID struct lore_string_t correlation_id; // Identity to use diff --git a/lore-client/src/cli/commands/service/run.rs b/lore-client/src/cli/commands/service/run.rs index e0f6717c..e6bc9656 100644 --- a/lore-client/src/cli/commands/service/run.rs +++ b/lore-client/src/cli/commands/service/run.rs @@ -35,6 +35,25 @@ pub enum ServiceMainError {} /// left behind is a stale socket, which the next start detects and removes. const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5); +/// Where the service parks its working directory. It inherits one from whoever +/// started it, which is unrelated to the directories its callers run in, and +/// holding that directory would also keep the filesystem under it busy. Callers +/// send the directory their relative paths belong to, so the service never +/// needs one of its own. +fn detached_working_directory() -> std::path::PathBuf { + #[cfg(target_family = "unix")] + { + std::path::PathBuf::from("/") + } + #[cfg(target_os = "windows")] + { + std::env::var_os("SystemRoot").map_or_else( + || std::path::PathBuf::from("C:\\"), + std::path::PathBuf::from, + ) + } +} + pub async fn service_main( listening_signal: Option>, ) -> Result<(), ServiceMainError> { @@ -42,6 +61,14 @@ pub async fn service_main( return Err(ServiceMainError::internal("IPC not supported on this OS")); } + let detached = detached_working_directory(); + if let Err(error) = std::env::set_current_dir(&detached) { + eprintln!( + "Failed to set working directory to {}: {error}", + detached.display() + ); + } + let listener: UdsListener = UdsListener::new().internal("Failed to start listener socket")?; if let Some(listening_signal) = listening_signal { diff --git a/lore-revision/src/file/write.rs b/lore-revision/src/file/write.rs index 0f676a70..d69ab696 100644 --- a/lore-revision/src/file/write.rs +++ b/lore-revision/src/file/write.rs @@ -167,14 +167,14 @@ pub async fn write_file( let destination = { let mut absolute_path = Path::new(&output).to_path_buf(); if !absolute_path.is_absolute() { - let Ok(current_path) = std::env::current_dir() else { + let Ok(resolved) = crate::util::path::make_absolute(&output) else { return Err(InvalidPath { path: output.clone(), } .into()); }; - absolute_path = current_path.join(output); + absolute_path = resolved; } absolute_path }; @@ -295,14 +295,14 @@ pub async fn write_address( let destination = { let mut absolute_path = Path::new(&output).to_path_buf(); if !absolute_path.is_absolute() { - let Ok(current_path) = std::env::current_dir() else { + let Ok(resolved) = crate::util::path::make_absolute(&output) else { return Err(InvalidPath { path: output.clone(), } .into()); }; - absolute_path = current_path.join(output); + absolute_path = resolved; } absolute_path }; diff --git a/lore-revision/src/interface.rs b/lore-revision/src/interface.rs index d4a4b394..2266b64d 100644 --- a/lore-revision/src/interface.rs +++ b/lore-revision/src/interface.rs @@ -527,6 +527,12 @@ pub enum LoreLoadConfig { pub struct LoreGlobalArgs { /// Repository path pub repository_path: LoreString, + /// Directory that relative paths in this call are resolved against. Set it + /// when a call may be executed by another process, such as the Lore + /// service, whose own working directory is unrelated to the caller's. When + /// empty, relative paths resolve against the working directory of the + /// process performing the call. + pub working_directory: LoreString, /// Correlation ID pub correlation_id: LoreString, /// Identity to use @@ -579,6 +585,10 @@ impl LoreGlobalArgs { self.repository_path.as_str() } + pub fn working_directory(&self) -> Option<&str> { + (&self.working_directory).into() + } + pub fn identity(&self) -> Option<&str> { (&self.identity).into() } diff --git a/lore-revision/src/util/path.rs b/lore-revision/src/util/path.rs index 1ccb80de..a181963e 100644 --- a/lore-revision/src/util/path.rs +++ b/lore-revision/src/util/path.rs @@ -16,20 +16,42 @@ pub enum PathError { InvalidPath, } +/// Resolves `path` against the working directory of the call in progress, or +/// against this process's own when the call did not name one. +/// +/// A call arriving over IPC carries the directory of the process that made it, +/// because the service's own is unrelated to the caller's. pub fn make_absolute(path: impl AsRef) -> Result { + let context = crate::runtime::try_execution_context(); + let base = context + .as_ref() + .and_then(|context| context.globals().working_directory()) + .map(Path::new); + make_absolute_from(path, base) +} + +/// [`make_absolute`] with the base directory supplied by the caller, for the +/// call wrappers that resolve paths before the execution context exists and so +/// cannot look it up. +pub fn make_absolute_from( + path: impl AsRef, + base: Option<&Path>, +) -> Result { let path = path.as_ref(); let cleanpath = clean(path.to_owned()); let pathbuf = PathBuf::from_str(cleanpath.as_str()).emit_map_err(InvalidPath { path: path.to_string(), })?; - if !pathbuf.is_absolute() { - Ok(std::env::current_dir() + if pathbuf.is_absolute() { + return Ok(pathbuf); + } + match base { + Some(base) => Ok(base.join(pathbuf)), + None => Ok(std::env::current_dir() .emit_map_err(PathError::internal( "failed to get current working directory", ))? - .join(pathbuf)) - } else { - Ok(pathbuf) + .join(pathbuf)), } } @@ -651,15 +673,13 @@ impl RelativePathBuf { let mut absolute_path = Path::new(user_path).to_path_buf(); if !absolute_path.is_absolute() { - absolute_path = std::path::absolute(absolute_path) - .emit_map_err(PathError::internal("failed to resolve absolute path"))?; + absolute_path = make_absolute(absolute_path.to_string_lossy())?; } let absolute_path = clean(absolute_path.display().to_string()); let mut repository_path = Path::new(repository_path).to_path_buf(); if !repository_path.is_absolute() { - repository_path = std::path::absolute(repository_path) - .emit_map_err(PathError::internal("failed to resolve absolute path"))?; + repository_path = make_absolute(repository_path.to_string_lossy())?; } let repository_path = clean(repository_path.display().to_string()).to_lowercase(); diff --git a/lore/src/call.rs b/lore/src/call.rs index ff2e7ba7..8e6f8246 100644 --- a/lore/src/call.rs +++ b/lore/src/call.rs @@ -1,5 +1,6 @@ // SPDX-FileCopyrightText: 2026 Epic Games, Inc. // SPDX-License-Identifier: MIT +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Instant; @@ -221,13 +222,15 @@ async fn prepare_repository_call( mut globals: LoreGlobalArgs, callback: LoreEventCallback, ) -> Result<(PathBuf, Arc), i32> { - let repository_path = - if let Ok(path) = util::path::make_absolute(globals.repository_path.as_str()) { - globals.repository_path = path.display().to_string().into(); - path - } else { - PathBuf::from(globals.repository_path.as_str()) - }; + let repository_path = if let Ok(path) = util::path::make_absolute_from( + globals.repository_path.as_str(), + globals.working_directory().map(Path::new), + ) { + globals.repository_path = path.display().to_string().into(); + path + } else { + PathBuf::from(globals.repository_path.as_str()) + }; let execution = setup_execution(globals, callback); diff --git a/lore/src/remote/call.rs b/lore/src/remote/call.rs index 224cc298..e71be0a0 100644 --- a/lore/src/remote/call.rs +++ b/lore/src/remote/call.rs @@ -34,11 +34,28 @@ impl EventError for ServiceCallError { } } +/// Records the directory the service resolves this call's relative paths +/// against, when the caller left it unset. The service runs in a directory +/// unrelated to the caller's, so without this a relative path would resolve +/// there rather than where the caller ran. A caller that set the field, such as +/// an installed tool that runs from a fixed directory but wants relative paths +/// resolved elsewhere, keeps its value. +#[allow(clippy::disallowed_methods)] +fn fill_working_directory(globals: &mut LoreGlobalArgs) { + if globals.working_directory().is_some() { + return; + } + if let Ok(directory) = std::env::current_dir() { + globals.working_directory = directory.display().to_string().into(); + } +} + pub async fn service_call( - globals: LoreGlobalArgs, + mut globals: LoreGlobalArgs, args: ArgsType, callback: LoreEventCallback, ) -> i32 { + fill_working_directory(&mut globals); let mut event_dispatcher = EventDispatcher::new(callback); service_call_impl(&mut event_dispatcher, globals, args) diff --git a/scripts/test/conftest.py b/scripts/test/conftest.py index 4e8b7c6a..f5ff3797 100644 --- a/scripts/test/conftest.py +++ b/scripts/test/conftest.py @@ -204,6 +204,35 @@ def _wait_for_service_ready(lore_executable_path, service_process, attempts=30): pytest.fail("Timed out waiting for Lore background service to accept connections") +@pytest.fixture(scope="function") +def lore_service_in_directory(lore_executable_path, global_dir_name): + """Starts the service in a chosen directory, for tests that need the + service's own working directory to differ from the caller's. The service + shares the test's isolated global config so shared stores it creates land + where the client looks for them.""" + processes = [] + + def start(directory): + env = os.environ.copy() + env["LORE_GLOBAL_PATH"] = global_dir_name + service_process = subprocess.Popen( + [lore_executable_path, "service", "run"], cwd=str(directory), env=env + ) + processes.append(service_process) + _wait_for_service_ready(lore_executable_path, service_process) + return service_process + + yield start + + for service_process in processes: + service_process.terminate() + try: + service_process.wait(timeout=10) + except subprocess.TimeoutExpired: + logger.warning("Lore service did not exit on terminate, killing it") + service_process.kill() + + @pytest.fixture(scope="function") def background_lore_service(lore_executable_path): command_args = [lore_executable_path, "service", "run"] diff --git a/scripts/test/test_service.py b/scripts/test/test_service.py index 9e34ef06..a09d6c7d 100644 --- a/scripts/test/test_service.py +++ b/scripts/test/test_service.py @@ -19,6 +19,7 @@ def service_supported(): @pytest.mark.smoke +@pytest.mark.xdist_group("lore_service") @pytest.mark.skip(reason="Unknown issue specifically running in CI for OSS") @pytest.mark.skipif( not service_supported(), reason="Service not supported on " + platform.system() @@ -29,6 +30,7 @@ def test_service_down(new_lore_repo): @pytest.mark.smoke +@pytest.mark.xdist_group("lore_service") @pytest.mark.skipif( not service_supported(), reason="Service not supported on " + platform.system() ) @@ -48,3 +50,76 @@ def test_service_call(new_lore_repo, background_lore_service): assert "A " + file_name in map( lambda line: line.strip(" "), status_output.splitlines() ) + + +@pytest.mark.smoke +@pytest.mark.xdist_group("lore_service") +@pytest.mark.skipif( + not service_supported(), reason="Service not supported on " + platform.system() +) +def test_service_resolves_relative_paths_against_caller( + new_lore_repo, lore_service_in_directory, tmp_path +): + """Relative paths belong to the directory the command was run in. + + The service resolves them, and its own working directory is unrelated to + the caller's, so a service started elsewhere must not pull them towards + itself. Every other service test passes an absolute repository path, which + cannot catch this. + """ + # Start the service in a directory unrelated to where the commands run, so + # that a relative path resolved there rather than at the caller would show. + service_directory = tmp_path / "service_elsewhere" + caller_directory = tmp_path / "caller" + service_directory.mkdir() + caller_directory.mkdir() + lore_service_in_directory(service_directory) + + # Seed a remote to clone from. Routed through the service like the rest, + # but against the repository's own absolute path, so unaffected by the + # service's directory. + source: Lore = new_lore_repo(environment_vars=LORE_SERVICE_ENVIRONMENT.copy()) + with source.open_file("seed.txt", "w+") as seed_file: + seed_file.write("seed\n") + source.stage(scan=True, offline=True) + source.commit("Seed", offline=True) + source.push() + + # Clone to a relative path from the caller's directory. It must land there, + # not under the service's directory. + clone_name = "relative_clone" + source.run( + ["repository", "clone", source.remote_path, clone_name], + cwd=str(caller_directory), + use_os_dir=True, + ) + + clone_path = caller_directory / clone_name + assert (clone_path / ".lore").is_dir(), ( + f"Clone must land under the caller's directory, not the service's. " + f"{caller_directory} contains {list(caller_directory.iterdir())}" + ) + assert not (service_directory / clone_name).exists(), ( + f"Clone must not land under the service's directory. " + f"{service_directory} contains {list(service_directory.iterdir())}" + ) + + # Stage a relative path from inside the clone. + clone = Lore( + lore_executable_path=source.lore_executable_path, + path=str(clone_path), + name=clone_name, + global_dir=source.global_dir, + environment_vars=LORE_SERVICE_ENVIRONMENT.copy(), + remote_url=source.remote, + remote_path=source.remote_path, + create_repo=False, + ) + file_name = "added.uasset" + (clone_path / file_name).write_bytes(os.urandom(30)) + clone.stage(file_name, relative_paths=True) + + status_output = clone.status() + assert "A " + file_name in map( + lambda line: line.strip(" "), status_output.splitlines() + ), f"Staged file should show as added: {status_output}"