Fix localfs cache tmp-file leak when key contains a path separator (#69741)

changelog/69741.fixed.md

@@ -0,0 +1 @@
+Fixed `localfs` cache leaking temporary files and raising `FileNotFoundError` when the cache key contained a path separator (e.g. a `pillarenv` with `/` in it). `localfs.store()` now creates the parent directory of the target file and always removes its `tempfile.mkstemp` scratch file on failure.

salt/cache/localfs.py

@@ -56,6 +56,21 @@ def store(bank, key, data, cachedir):
             )
 
     outfile = salt.utils.path.join(base, f"{key}.p")
+    # A ``key`` may legitimately contain path separators (e.g. the pillar
+    # cache uses ``<minion_id>:<pillarenv>`` as its key, and ``pillarenv``
+    # may itself contain ``/`` when pillar_roots use hierarchical names).
+    # In that case ``outfile`` lands in a subdirectory that may not exist
+    # yet -- create it so the atomic rename below can succeed. See
+    # issue #69741.
+    outdir = os.path.dirname(outfile)
+    if outdir and outdir != base:
+        try:
+            os.makedirs(outdir, exist_ok=True)
+        except OSError as exc:
+            raise SaltCacheError(
+                f"The cache directory, {outdir}, could not be created: {exc}"
+            )
+
     tmpfh, tmpfname = tempfile.mkstemp(dir=base)
     os.close(tmpfh)
     try:
@@ -67,6 +82,19 @@ def store(bank, key, data, cachedir):
         raise SaltCacheError(
             f"There was an error writing the cache file, {base}: {exc}"
         )
+    finally:
+        # ``atomic_rename`` moves ``tmpfname`` to ``outfile`` on success, so
+        # the tmp file is only left behind when the write or rename failed.
+        # Not cleaning this up caused the pillar cache to accumulate
+        # millions of leaked ``tmp*`` files (issue #69741).
+        if os.path.exists(tmpfname):
+            try:
+                os.remove(tmpfname)
+            except OSError:
+                log.debug(
+                    "Could not remove leftover localfs cache tmp file %s",
+                    tmpfname,
+                )
 
 
 def fetch(bank, key, cachedir):

tests/pytests/functional/cache/test_localfs.py

@@ -85,6 +85,85 @@ def test_contains_is_constrained_to_cachedir(cache, tmp_path, key):
     assert not cache.contains(str(tmp_path), key)
 
 
+def test_store_key_with_path_separator_does_not_leak_tmp_files_69741(cache):
+    """
+    Regression test for issue #69741.
+
+    Since 3008.0 the pillar cache uses ``<minion_id>:<pillarenv>`` as its
+    cache key. When ``pillarenv`` contains ``/`` (e.g. a
+    hierarchical ``pillar_roots`` name like ``someenv/beta``) the key
+    contains a path separator, so ``outfile`` in ``localfs.store()``
+    lands in a subdirectory that did not exist yet. The atomic rename
+    then failed with ``FileNotFoundError`` and the tmp file created by
+    ``tempfile.mkstemp`` was left behind. Reporters saw millions of
+    leaked ``tmp*`` files under ``/var/cache/salt/master/pillar/``.
+
+    Storing a key that contains ``/`` must:
+      * succeed without raising,
+      * write the value to the expected nested path,
+      * be readable back via ``fetch``,
+      * and leave no ``tmp*`` files behind in the bank directory.
+    """
+    bank = "pillar"
+    key = "minion.example:someenv/beta"
+
+    cache.store(bank, key, {"hello": "world"})
+
+    assert cache.fetch(bank, key) == {"hello": "world"}
+
+    bank_dir = Path(cache.cachedir) / bank
+    leftover = [
+        entry.name
+        for entry in bank_dir.iterdir()
+        if entry.name.startswith("tmp") and entry.is_file()
+    ]
+    assert not leftover, (
+        f"localfs.store() leaked tmp files into {bank_dir}: {leftover} "
+        "(issue #69741)"
+    )
+
+
+def test_store_tmp_file_cleaned_up_on_write_failure_69741(cache, monkeypatch):
+    """
+    Regression test for issue #69741 (defensive).
+
+    Even when the atomic rename fails for reasons unrelated to the key
+    path (e.g. a lower-level ``OSError``), ``localfs.store()`` must not
+    leave the ``tempfile.mkstemp`` tmp file behind. Prior to the fix,
+    every failed store leaked one ``tmp*`` file into the bank
+    directory; over time this produced millions of orphan files.
+    """
+    import salt.utils.atomicfile
+
+    def _boom(src, dst):
+        raise OSError(2, "boom", src)
+
+    monkeypatch.setattr(salt.utils.atomicfile, "atomic_rename", _boom)
+    # localfs.py binds ``salt.utils.atomicfile`` at import time via
+    # ``salt.utils.atomicfile.atomic_rename``; patching the attribute on
+    # the module object is sufficient because the lookup happens at call
+    # time.
+
+    bank = "pillar"
+    key = "some-minion"
+
+    from salt.exceptions import SaltCacheError
+
+    with pytest.raises(SaltCacheError):
+        cache.store(bank, key, {"hello": "world"})
+
+    bank_dir = Path(cache.cachedir) / bank
+    leftover = [
+        entry.name
+        for entry in bank_dir.iterdir()
+        if entry.name.startswith("tmp") and entry.is_file()
+    ]
+    assert not leftover, (
+        f"localfs.store() leaked tmp files into {bank_dir}: {leftover} "
+        "(issue #69741)"
+    )
+
+
 def test_clean_expired_does_not_drop_unexpired_entries_69307(cache):
     """
     Regression test for issue #69307.