fix(metadata): stop masking local metadata write errors with UnboundLocalError

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

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"]