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

[kevin9327]

PR Type

• Bug fix
• New feature
• Core Runtime change (higher bar -- see CONTRIBUTING.md)
• Docs / tooling
• Refactoring

Summary

When writing local metadata fails, the user gets
UnboundLocalError: cannot access local variable 'f' from inside Metaflow
instead of the PermissionError / FileNotFoundError that names the path they
cannot write to.

Issue

No existing issue -- happy to open one first if you'd rather have it linked.

Reproduction

Runtime: local (--metadata local, the default)

LocalMetadataProvider._dump_json_to_file is on the hot path of every local
run: _save_meta calls it for register_run_id, register_task_id,
register_metadata and register_data_artifacts. Any failure to create the
temp file it writes through -- unwritable .metaflow directory, read-only
mount, directory removed underneath us -- takes this path.

Commands to run:

pip install -e .
python repro.py

repro.py:

import os, tempfile
from metaflow.plugins.metadata_providers.local import LocalMetadataProvider

base = tempfile.mkdtemp()
target = os.path.join(base, "not_created_yet", "_self.json")
try:
    LocalMetadataProvider._dump_json_to_file(target, {"a": 1})
except Exception as e:
    print("RAISED: %s -> %s" % (type(e).__name__, e))

Where evidence shows up: parent console / the traceback the user sees.

Before (on master)

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

After

RAISED: FileNotFoundError -> [Errno 2] No such file or directory: '.../not_created_yet/tmpec4e4v07'

Root Cause

metaflow/plugins/metadata_providers/local.py:607:

try:
    with tempfile.NamedTemporaryFile(
        mode="w", dir=os.path.dirname(filepath), delete=False
    ) as f:
        json.dump(data, f)
    os.rename(f.name, filepath)
finally:
    # clean up in case anything goes wrong
    if f and os.path.isfile(f.name):
        os.remove(f.name)

The name f is bound by the with statement inside the try. If the
NamedTemporaryFile(...) call is itself what raises, f is never assigned, so
the finally clause -- which always runs -- evaluates if f against an unbound
local and raises UnboundLocalError. That exception is raised while the
original OSError is propagating, so it replaces it as the visible error and
the real cause is only reachable through __context__.

The invariant the finally block assumes ("by the time cleanup runs, f
names a temp file") does not hold on the one path where cleanup has nothing to
do.

Why This Fix Is Correct

One line, f = None, ahead of the try. The cleanup condition if f and ...
was already written to tolerate a falsy f; it just never had a value to read
when the constructor failed. With f pre-bound:

• constructor raises -> finally sees None, skips cleanup, the original
  OSError propagates untouched
• json.dump or os.rename raises -> f is bound, the temp file is removed
  exactly as before

This is also what the sibling helper in this codebase already does:
LocalStorage._atomic_write (metaflow/plugins/datastores/local_storage.py:113)
creates its temp file before the try, so a failing mkstemp propagates
cleanly. This change brings _dump_json_to_file in line with it without
restructuring it.

Failure Modes Considered

1. Cleanup silently skipped when it was needed. The only way f is still
   None in finally is if NamedTemporaryFile raised, and in that case no
   temp file was created -- delete=False only matters once construction
   succeeds. Nothing leaks. test_dump_json_to_file_writes_and_leaves_no_temp_file
   pins that the success path still leaves the directory with just the target.
2. Changing which exception callers see. This makes the original error
   visible where previously an UnboundLocalError shadowed it. Callers of
   _save_meta do not catch UnboundLocalError anywhere (I grepped), so no
   handler is relying on the current behaviour; anything catching OSError
   starts working as intended.
3. Concurrent writers. Unchanged -- this touches only name binding, not the
   write-then-rename sequence or the allow_overwrite short-circuit, both of
   which are pinned by the new tests.

Tests

• Unit tests added/updated
• Reproduction script provided
• CI passes
• If tests are impractical: explain why below and provide manual evidence above

Added to test/unit/test_local_metadata_provider.py:

• test_dump_json_to_file_reports_the_real_error_when_temp_file_fails -- real
  missing directory, asserts an OSError naming the path
• test_dump_json_to_file_reports_permission_error -- the case a user actually
  hits, via mocker.patch on the constructor
• test_dump_json_to_file_writes_and_leaves_no_temp_file (pin)
• test_dump_json_to_file_does_not_overwrite_by_default (pin)

# before, on unmodified master, metaflow/ untouched
$ python -m pytest test_local_metadata_provider.py -q
E   UnboundLocalError: cannot access local variable 'f' where it is not associated with a value
FAILED test_local_metadata_provider.py::test_dump_json_to_file_reports_the_real_error_when_temp_file_fails
FAILED test_local_metadata_provider.py::test_dump_json_to_file_reports_permission_error
2 failed, 4 passed

# after
$ python -m pytest test_local_metadata_provider.py -q
6 passed

Whole of test/unit, same invocation, before and after:

before: 21 failed, 499 passed, 20 skipped, 103 errors
after:  21 failed, 503 passed, 20 skipped, 103 errors

The delta is exactly the four new tests.

What I could not run: I develop on Windows, which Metaflow does not support
(setup.py classifiers list macOS and Linux only) -- several core modules
import fcntl at module scope, so every test that spawns a real run fails here
regardless of this patch. That is the 21/103 above; the counts are identical
before and after. Concretely for this file, I could not exercise the
allow_overwrite=True branch locally, because os.rename onto an existing path
raises FileExistsError on Windows -- unrelated to this change, and the reason
that pin only covers the default allow_overwrite=False behaviour. I am relying
on CI for the Linux/macOS matrix.

black 25.12.0 with the target list from .pre-commit-config.yaml reports both
files unchanged.

Non-Goals

I did not restructure the helper to match _atomic_write more closely, did not
touch os.rename vs os.replace, and did not change what _save_meta does
with the error.

AI Tool Usage

• No AI tools were used in this contribution
• AI tools were used (describe below)

Claude Code, used to search for callers and to draft this description. The
reproduction and the before/after runs above are commands I actually ran; I
understand why f is unbound only on the constructor-failure path and why
pre-binding it is sufficient.

[greptile-apps]

Greptile Summary

This PR initializes the local metadata helper's temporary-file variable before construction so filesystem errors propagate without being replaced by UnboundLocalError.

• Preserves cleanup behavior after serialization or rename failures.
• Adds regression coverage for missing directories and permission errors.
• Adds coverage for successful writes, temporary-file cleanup, and default no-overwrite behavior.

Confidence Score: 4/5

The functional fix appears safe to merge; only non-blocking misleading test commentary should be corrected.

The initialization correctly preserves the original filesystem exception without changing successful writes or cleanup, and the added tests cover the relevant paths; the sole finding concerns stale explanatory wording.

Files Needing Attention: test/unit/test_local_metadata_provider.py

Important Files Changed

Filename	Overview
metaflow/plugins/metadata_providers/local.py	Safely initializes the cleanup variable so the original temporary-file creation error propagates.
test/unit/test_local_metadata_provider.py	Adds appropriate regression and behavior tests, but two explanatory comments inaccurately describe the fixed behavior as current.

_{Reviews (1): Last reviewed commit: "fix(metadata): stop masking local metada..." | Re-trigger Greptile}

[greptile-apps]

Comments Describe Fixed Bug

These comments describe the old masking behavior in the present tense, even though f is now initialized before the try. Rephrase them as regression history so future maintainers are not led to believe the current implementation still raises UnboundLocalError. The permission-error test below has the same stale wording.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[Shriprasad-P]

The implementation itself is correct: I reproduced master masking both FileNotFoundError and PermissionError as UnboundLocalError, and verified the PR preserves the original exceptions and cleanup behavior across the requested failure and success paths.

There is, however, a repository-process blocker separate from source correctness. This change is under metaflow/plugins/metadata_providers/, which CONTRIBUTING.md classifies as Core Runtime. The PR currently marks only Bug fix, says “No existing issue,” and does not link an open, maintainer-acknowledged issue. The Core Runtime policy and external-contributor guidance require that issue and approval before acceptance. Please link the approved issue and update the Core Runtime classification/checklist.

The existing Greptile thread on test/unit/test_local_metadata_provider.py:81 already covers the stale present-tense regression comments, so I am not duplicating that inline comment.

[Shriprasad-P]

Review

Looks like a solid fix for the UnboundLocalError masking real I/O failures in _dump_json_to_file.

What works well

• Initializing f = None before the try is the minimal correct fix for the finally path.
• Regression tests cover constructor failure, mocked PermissionError, happy-path write, and no-overwrite behavior.

LGTM from a correctness standpoint.

[Shriprasad-P]

Review

Looks like a solid fix for the UnboundLocalError masking real I/O failures in _dump_json_to_file.

What works well

• Initializing f = None before the try is the minimal correct fix for the finally path.
• Regression tests cover constructor failure, mocked PermissionError, happy-path write, and no-overwrite behavior.

LGTM from a correctness standpoint.