Python: fix: make FileCheckpointStorage tmp filename unique per save

[Zhengzhuo Wang (wangzhengzhuo05)]

Motivation & Context

FileCheckpointStorage.save writes atomically via a FIXED temp filename (file_path.with_suffix(".json.tmp")) per checkpoint id. Two concurrent saves of the same checkpoint id both write the same .tmp path and both call os.replace(tmp, final); whichever replace runs first consumes the tmp file and the other save dies with a raw FileNotFoundError — neither WorkflowCheckpointException nor a documented failure.

Fixes #8182

Description & Review Guide

• What are the major changes? _write_atomic now uses a unique per-save temp filename ({file_path.name}.{uuid.uuid4().hex}.tmp) and best-effort cleanup in a finally block, so concurrent saves of the same checkpoint id never share a tmp file. os.replace remains the atomic commit point.
• What is the impact of these changes? Concurrent saves of the same id no longer race; a crashed writer leaves at most a uniquely-named orphan .tmp file (removed best-effort), never a torn checkpoint.
• What do you want reviewers to focus on? The tmp naming scheme and the best-effort cleanup semantics (deliberately swallows OSError so cleanup can never mask a real write failure).

Related Issue

Fixes #8182 — verified no open PR exists for it.

AI-Generated Code Disclosure

This change was implemented with AI assistance (OpenCode + muse-spark) and reviewed for correctness, security, and license compatibility.

Verification

• New test: test_file_checkpoint_storage_concurrent_saves_same_id — 8 concurrent saves of the same id via asyncio.gather; before the fix this raises FileNotFoundError (confirmed by mutation check: reverting to the fixed name reproduces the crash).
• Local: 115 passed (test_checkpoint, test_checkpoint_storage_conformance, test_checkpoint_encode, test_checkpoint_decode, test_checkpoint_validation).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Zhengzhuo Wang (wangzhengzhuo05)]

@microsoft-github-policy-service agree

[Manjunath Janardhan (manjunathshiva)]

Heads up on some history you probably could not have seen — your diagnosis is correct, and this is not a criticism of the fix.

The same race is already open as #7748 (Evan Mattson (@ermattson), 19 Aug), and #8182 that this PR fixes is a duplicate of it filed today. There is a PR in review for #7748 — #7757, mine — and it started as almost exactly this change: unique per-save temp name plus best-effort cleanup.

The reason I am flagging it rather than leaving you to find out at review: Evan Mattson (@moonbox3)'s review on #7757 established that the shared temp name was one of three coupled defects, not the whole thing. The other two only appear once the temp-name collision is gone:

• Serializing the writes. With unique temp names both saves still reach os.replace on the same destination concurrently, so last-writer-wins stops matching the order callers observed — and on Windows the replaces can trip a transient PermissionError on each other.
• Cancellation. A save cancelled mid-write leaves its os.replace in flight, so a later save can complete and then be overwritten by the cancelled one's stale data.

So a unique temp name closes the FileNotFoundError you identified but leaves the durability side of it. #7757 now carries the temp name, per-destination serialization keyed by the canonical path, a cancellation contract, and reference-counted registry cleanup, after three rounds with Evan Mattson (@moonbox3) who picked the shape.

One concrete note that applies whichever PR lands: {file_path.name}.{uuid4().hex}.tmp embeds the destination filename, so the temp name runs about 37 characters longer than the destination it replaces. A checkpoint id that _validate_file_path accepts can therefore produce a temp name over the 255-byte filename limit, and the save fails for a destination that is otherwise perfectly legal. #7757 uses a short id-independent .maf-ckpt-{uuid}.tmp for exactly that reason — worth adopting either way.

Which PR to take is entirely up to the maintainers; I am not arguing for mine. I would rather you knew now than after spending review cycles on it. If they prefer this one, the two follow-on defects above are worth folding in, and I am happy to hand over what the review turned up.

[Zhengzhuo Wang (wangzhengzhuo05)]

Thanks for the heads-up — very helpful context, and I appreciate you flagging #7748/#7757 rather than letting me burn review cycles discovering it.

I've updated the fix to use a short, id-independent temp name (.maf-ckpt-{uuid}.tmp) so a checkpoint id accepted by _validate_file_path can never push the temp name over the filename-length limit — good catch on that one.

Agreed the serialize-writes and cancellation defects in #7757 are real follow-ons; I've kept this PR scoped to the temp-name collision you diagnosed, and I'm happy to defer entirely if the maintainers prefer #7757's more complete fix.