From 98500176fc91ea38ba7c018d8a5463349c271b5e Mon Sep 17 00:00:00 2001 From: Sandy Chapman Date: Tue, 11 Aug 2026 11:21:35 -0300 Subject: [PATCH] fix(files): keep local file blobs in the platform data directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NMP_DATA_DIR` relocated the entity-store database but not the Files service: both bundled local configurations pinned the literal `~/.local/share/nemo/files` for blob storage. A local run with the variable set therefore put the database in the chosen directory and the blobs in the default one. A half-isolated instance is worse than an un-isolated one. It looks isolated, so wiping the chosen directory silently leaves the blobs behind — and `SETUP.md` tells users that directory holds "the entity-store database, encryption key, files, job history, secrets", which was not true. `LocalServicesConfig.data_dir` documents the same promise, and the e2e harness already renders the files path as `/files`; only the bundled configs disagreed. An empty `path` on `LocalStorageConfig` now means "the platform user-data directory", resolved in the existing field validator, and both local configurations use that instead of a literal path. Resolved in the validator rather than as a field default on purpose: the config-reference documentation is generated from field defaults and committed, so a default derived from the environment would bake the generating machine's home directory into the repository. This mirrors the SQLite path, which is computed in `get_database_url` for the same reason. Behaviour is unchanged for anyone who has not set `NMP_DATA_DIR` — an empty path resolves to exactly the location the configs used to name — and an explicit path still wins, which is what the container image, the Helm chart and the agentic runners all rely on. Signed-off-by: Sandy Chapman --- .../files/storage_config.py | 10 ++++ .../tests/files/test_storage_config_path.py | 53 +++++++++++++++++++ packages/nmp_platform/config/local.yaml | 4 +- .../src/nmp/platform_runner/config/local.yaml | 4 +- 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 packages/nemo_platform_plugin/tests/files/test_storage_config_path.py diff --git a/packages/nemo_platform_plugin/src/nemo_platform_plugin/files/storage_config.py b/packages/nemo_platform_plugin/src/nemo_platform_plugin/files/storage_config.py index 51aa44afe1..8a9601a3b3 100644 --- a/packages/nemo_platform_plugin/src/nemo_platform_plugin/files/storage_config.py +++ b/packages/nemo_platform_plugin/src/nemo_platform_plugin/files/storage_config.py @@ -17,6 +17,7 @@ Self, ) +from nemo_platform_plugin.config import nmp_user_data_dir from nemo_platform_plugin.schema import SecretRef from pydantic import BaseModel, Field, field_validator, model_validator @@ -91,7 +92,16 @@ def make_path_relative_to_program(cls, v: str) -> str: This allows the config to pass in absolute paths, ``~``-prefixed paths (expanded against the running user's home dir), or relative paths like ``./files_storage`` (joined against cwd). + + An **empty** path means the platform's user-data directory, so blobs follow + ``NMP_DATA_DIR`` alongside the entity-store database. + + Resolved here rather than as a field default because a machine-dependent default is + rendered into the committed config reference — the same reason the SQLite path is + computed in ``get_database_url``. """ + if not v: + return str(nmp_user_data_dir() / "files") return str(Path.cwd() / Path(v).expanduser()) @property diff --git a/packages/nemo_platform_plugin/tests/files/test_storage_config_path.py b/packages/nemo_platform_plugin/tests/files/test_storage_config_path.py new file mode 100644 index 0000000000..439cdb89bd --- /dev/null +++ b/packages/nemo_platform_plugin/tests/files/test_storage_config_path.py @@ -0,0 +1,53 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""How a local storage path is resolved, and which state it keeps together. + +Regression: ``NMP_DATA_DIR`` relocates the entity-store database, but the bundled local +configurations pinned the literal blob location, so a run with it set produced a *half* +isolated instance — database in the chosen directory, blobs in the default one. That is worse +than an un-isolated instance: it looks isolated, and wiping the chosen directory silently +leaves the blobs behind. +""" + +import pytest +from nemo_platform_plugin.files.storage_config import LocalStorageConfig + + +def test_an_empty_path_follows_the_platform_data_dir(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: + """The fix: blobs land under the chosen data directory, beside the entity-store database.""" + monkeypatch.setenv("NMP_DATA_DIR", str(tmp_path)) + assert LocalStorageConfig(path="").path == str(tmp_path / "files") + + +def test_an_empty_path_without_a_data_dir_keeps_the_previous_location( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The value the bundled configs used to hard-code. Anyone who has not opted into a data + directory must see no change at all.""" + monkeypatch.delenv("NMP_DATA_DIR", raising=False) + monkeypatch.setenv("HOME", "/home/someone") + monkeypatch.delenv("XDG_DATA_HOME", raising=False) + assert LocalStorageConfig(path="").path == "/home/someone/.local/share/nemo/files" + + +def test_an_explicit_path_is_never_overridden(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: + """The container image, the Helm chart and the agentic runners all set this explicitly.""" + monkeypatch.setenv("NMP_DATA_DIR", str(tmp_path)) + assert LocalStorageConfig(path="/data/files_storage").path == "/data/files_storage" + + +def test_home_relative_paths_still_expand(monkeypatch: pytest.MonkeyPatch) -> None: + """Only the empty path gains meaning; every other form resolves exactly as before.""" + monkeypatch.setenv("HOME", "/home/someone") + assert LocalStorageConfig(path="~/blobs").path == "/home/someone/blobs" + + +def test_the_data_dir_is_read_per_construction(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: + """Resolution happens in the validator, not as a field default, so it tracks the + environment at construction — and, just as importantly, keeps a machine-dependent path out + of the field default that generates the committed config reference.""" + monkeypatch.setenv("NMP_DATA_DIR", str(tmp_path / "one")) + assert LocalStorageConfig(path="").path == str(tmp_path / "one" / "files") + monkeypatch.setenv("NMP_DATA_DIR", str(tmp_path / "two")) + assert LocalStorageConfig(path="").path == str(tmp_path / "two" / "files") diff --git a/packages/nmp_platform/config/local.yaml b/packages/nmp_platform/config/local.yaml index 364abe4103..8caa44d0fe 100644 --- a/packages/nmp_platform/config/local.yaml +++ b/packages/nmp_platform/config/local.yaml @@ -178,7 +178,9 @@ secrets: files: default_storage_config: type: local - path: ~/.local/share/nemo/files + # Empty means the platform user-data directory, so blobs follow NMP_DATA_DIR + # alongside the entity-store database. + path: "" studio: static_files_path: web/packages/studio/dist diff --git a/packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yaml b/packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yaml index 1e22469627..92929d60a6 100644 --- a/packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yaml +++ b/packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yaml @@ -90,4 +90,6 @@ secrets: files: default_storage_config: type: local - path: ~/.local/share/nemo/files + # Empty means the platform user-data directory, so blobs follow NMP_DATA_DIR + # alongside the entity-store database. + path: ""