Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -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." },
Expand Down
6 changes: 6 additions & 0 deletions lore-capi/lore.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions lore-client/src/cli/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,10 @@ pub enum LoreCliError {
pub fn lore_globals_from_args(cli: &LoreCli) -> LoreGlobalArgs {
let mut args = LoreGlobalArgs {
repository_path: get_repository_path(cli.repository.clone()),
working_directory: std::env::current_dir()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rather than having the CLI set this, maybe it should be set in lore/src/remote/call.rs::service_call_impl or lore/src/call_delegation.rs::dispatch_call so that any C API callers using the service pick this up as well.
It can also only overwrite the LoreGlobalArgs::working_directory if its "", so that an installed tool running from C:\Program Files or whatever can set it and have relative paths work as expected as well.

.map(|path| path.display().to_string())
.unwrap_or_default()
.into(),

force: cli.force.into(),
dry_run: cli.dry_run.into(),
Expand Down
27 changes: 27 additions & 0 deletions lore-client/src/cli/commands/service/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,40 @@ 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<tokio::sync::oneshot::Sender<()>>,
) -> Result<(), ServiceMainError> {
if !uds_supported() {
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 {
Expand Down
8 changes: 4 additions & 4 deletions lore-revision/src/file/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down Expand Up @@ -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
};
Expand Down
10 changes: 10 additions & 0 deletions lore-revision/src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down
38 changes: 29 additions & 9 deletions lore-revision/src/util/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<str>) -> Result<PathBuf, PathError> {
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<str>,
base: Option<&Path>,
) -> Result<PathBuf, PathError> {
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)),
}
}

Expand Down Expand Up @@ -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();

Expand Down
17 changes: 10 additions & 7 deletions lore/src/call.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -221,13 +222,15 @@ async fn prepare_repository_call(
mut globals: LoreGlobalArgs,
callback: LoreEventCallback,
) -> Result<(PathBuf, Arc<ExecutionContext>), 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);

Expand Down
25 changes: 25 additions & 0 deletions scripts/test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,31 @@ 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):
"""Starts the service in a chosen directory, for tests that need the
service's own working directory to differ from the caller's."""
processes = []

def start(directory):
service_process = subprocess.Popen(
[lore_executable_path, "service", "run"], cwd=str(directory)
)
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"]
Expand Down
71 changes: 71 additions & 0 deletions scripts/test/test_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import os
import platform
import subprocess

import pytest

Expand All @@ -19,6 +20,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()
Expand All @@ -29,6 +31,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()
)
Expand All @@ -48,3 +51,71 @@ 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_executable_path, 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.
"""
repo: Lore = new_lore_repo()
with repo.open_file("seed.txt", "w+") as seed_file:
seed_file.write("seed\n")
repo.stage(scan=True, offline=True)
repo.commit("Seed", offline=True)
repo.push()

service_directory = tmp_path / "service_elsewhere"
caller_directory = tmp_path / "caller"
service_directory.mkdir()
caller_directory.mkdir()
lore_service_in_directory(service_directory)

environment = os.environ.copy()
environment.update(LORE_SERVICE_ENVIRONMENT)

def run_in(directory, args):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is supported by Lore's .run method already. If you pass a cwd kwarg to any of its functions they'll get passed to .run and used to set the run_cwd variable.

You'd also need to add back in the environment_vars=LORE_SERVICE_ENVIRONMENT.copy() to the new_lore_repo() command.

result = subprocess.run(
[lore_executable_path, *args],
capture_output=True,
text=True,
cwd=str(directory),
env=environment,
)
assert result.returncode == 0, (
f"{args} failed in {directory}: {result.stdout}{result.stderr}"
)
return result.stdout + result.stderr

clone_name = "relative_clone"
run_in(caller_directory, ["clone", repo.remote_path, clone_name])

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())}"
)

file_name = "added.uasset"
(clone_path / file_name).write_bytes(os.urandom(30))

run_in(clone_path, ["stage", file_name])

status_output = run_in(clone_path, ["status"])
assert "A " + file_name in map(
lambda line: line.strip(" "), status_output.splitlines()
), f"Staged file should show as added: {status_output}"