From 143249e29542498fd36a1e20ecebf7d337bce399 Mon Sep 17 00:00:00 2001
From: kevin9327 <5299031+kevin9327@users.noreply.github.com>
Date: Thu, 10 Sep 2026 22:59:58 +0900
Subject: [PATCH] fix(metadata): stop masking local metadata write errors with
 UnboundLocalError

_dump_json_to_file opens its temp file inside the try block, so when the
NamedTemporaryFile constructor is what fails, `f` is never bound and the
finally clause raises

    UnboundLocalError: cannot access local variable 'f' where it is not
    associated with a value

over the top of the real OSError. A user whose .metaflow directory is
read-only or owned by another account sees that instead of the
PermissionError naming the path.

Bind f = None before the try so the cleanup runs only when there is
something to clean up and the original error propagates. This matches the
sibling temp-file-then-rename helper, LocalStorage._atomic_write, which
creates its temp file outside the try for the same reason.
---
 metaflow/plugins/metadata_providers/local.py |  1 +
 test/unit/test_local_metadata_provider.py    | 43 ++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/metaflow/plugins/metadata_providers/local.py b/metaflow/plugins/metadata_providers/local.py
index e723f9f1927..48f2da72967 100644
--- a/metaflow/plugins/metadata_providers/local.py
+++ b/metaflow/plugins/metadata_providers/local.py
@@ -607,6 +607,7 @@ def _get_metadir(cls, flow_name=None, run_id=None, step_name=None, task_id=None)
     def _dump_json_to_file(cls, filepath, data, allow_overwrite=False):
         if os.path.isfile(filepath) and not allow_overwrite:
             return
+        f = None
         try:
             with tempfile.NamedTemporaryFile(
                 mode="w", dir=os.path.dirname(filepath), delete=False
diff --git a/test/unit/test_local_metadata_provider.py b/test/unit/test_local_metadata_provider.py
index 3c7c49b9909..896076d47c5 100644
--- a/test/unit/test_local_metadata_provider.py
+++ b/test/unit/test_local_metadata_provider.py
@@ -1,3 +1,8 @@
+import json
+import os
+
+import pytest
+
 from metaflow.plugins.metadata_providers.local import LocalMetadataProvider
 
 
@@ -67,3 +72,41 @@ def filter_for(pattern):
     assert filter_for("middle:1,.*") == ["Flow/run/middle/4"]
     # and the match-all pattern keeps returning everything
     assert len(filter_for(".*")) == len(paths)
+
+
+def test_dump_json_to_file_reports_the_real_error_when_temp_file_fails(tmp_path):
+    # _dump_json_to_file opens its temp file *inside* the try, so when the
+    # NamedTemporaryFile constructor is the thing that fails, `f` is never
+    # bound and the finally clause raises UnboundLocalError over the top of
+    # the real OSError. The caller then has no idea what actually went wrong.
+    target = tmp_path / "missing_dir" / "_self.json"
+    with pytest.raises(OSError) as excinfo:
+        LocalMetadataProvider._dump_json_to_file(str(target), {"a": 1})
+    assert "missing_dir" in str(excinfo.value)
+
+
+def test_dump_json_to_file_reports_permission_error(tmp_path, mocker):
+    # The same masking happens for an unwritable metadata directory, which is
+    # the case a user is most likely to hit (read-only mount, .metaflow owned
+    # by another account).
+    mocker.patch(
+        "metaflow.plugins.metadata_providers.local.tempfile.NamedTemporaryFile",
+        side_effect=PermissionError(13, "Permission denied"),
+    )
+    with pytest.raises(PermissionError):
+        LocalMetadataProvider._dump_json_to_file(str(tmp_path / "_self.json"), {"a": 1})
+
+
+def test_dump_json_to_file_writes_and_leaves_no_temp_file(tmp_path):
+    target = tmp_path / "_self.json"
+    LocalMetadataProvider._dump_json_to_file(str(target), {"a": 1})
+    assert json.loads(target.read_text()) == {"a": 1}
+    assert os.listdir(str(tmp_path)) == ["_self.json"]
+
+
+def test_dump_json_to_file_does_not_overwrite_by_default(tmp_path):
+    target = tmp_path / "_self.json"
+    LocalMetadataProvider._dump_json_to_file(str(target), {"a": 1})
+    LocalMetadataProvider._dump_json_to_file(str(target), {"a": 2})
+    assert json.loads(target.read_text()) == {"a": 1}
+    assert os.listdir(str(tmp_path)) == ["_self.json"]