Repository navigation
Python: fix(core): reject checkpoint IDs that collide by letter case on save - #9466
Yufeng He (he-yufeng) wants to merge 5 commits into
Conversation
…on save On case-insensitive filesystems (default Windows/macOS), Round-A.json and round-a.json are the same file. FileCheckpointStorage.save accepted both spellings, so the second save silently replaced the first checkpoint and reads of the first ID then returned the other workflow's state, with the first workflow's listing left empty. save now fails closed: when the destination exists under a differently-cased spelling, the save raises WorkflowCheckpointException naming the collision instead of replacing a different checkpoint's data. Case-sensitive filesystems skip the scan entirely (the variant path does not exist there), and re-saving the exact same ID stays a normal update everywhere. Tests: a conformance regression pins the rejection, the first checkpoint's survival, and the same-ID update path; red without the guard on a case-insensitive volume.
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
🟡 Changes recommended
The collision guard still permits data loss in supported scenarios and can reject valid updates, while the test probe introduces a collection race.
4 open issues
What changed in this PR
This PR addresses #9444 by attempting to prevent case-variant checkpoint IDs from overwriting stored snapshots.
Changes:
- Adds a filename-collision guard before checkpoint encoding and saving.
- Adds regression coverage for collision rejection, snapshot preservation, and same-ID updates.
| File | Description |
|---|---|
| python/packages/core/tests/workflow/test_checkpoint_storage_conformance.py | Adds filesystem detection and collision regression coverage. |
| python/packages/core/agent_framework/_workflows/_checkpoint.py | Checks for case-variant filenames before saving. |
Review effort: Balanced
| """ | ||
| if not file_path.exists(): | ||
| return | ||
| expected_name = file_path.name |
There was a problem hiding this comment.
Correct, fixed in 183ae7b. resolve() on Windows would hand back the on-disk casing, so the guard now derives the expected filename from the requested checkpoint ID (f"{checkpoint_id}.json") instead of file_path.name; the resolved path is still used only for the containment check and the directory scan.
| except OSError: | ||
| # A transient listing/read failure must not turn a routine save into a | ||
| # hard failure; the write below still fails loudly if the path is bad. | ||
| return |
There was a problem hiding this comment.
Agreed, fail-closed is the right call for a guard whose entire purpose is preventing silent data loss. In 183ae7b an OSError from iterdir() now raises WorkflowCheckpointException ("the storage directory could not be listed") instead of skipping the scan.
| return | ||
| expected_name = file_path.name | ||
| try: | ||
| for sibling in file_path.parent.iterdir(): |
There was a problem hiding this comment.
Right, enumeration order made same-ID updates depend on which spelling appeared first. The guard in 183ae7b now collects the directory names once and allows the save when the exact spelling already exists; the case-folded raise only fires when no exact match is present, so coexistence on case-sensitive volumes works regardless of iterdir() order.
| @pytest.mark.skipif( | ||
| not _filesystem_is_case_insensitive(Path(__file__).parent), | ||
| reason="case-variant collision only occurs on case-insensitive filesystems", | ||
| ) | ||
| async def test_file_save_rejects_case_variant_checkpoint_id(tmp_path: Path) -> None: | ||
| """A case-only variant ID must not silently replace the other checkpoint (#9444).""" |
There was a problem hiding this comment.
Fixed in 183ae7b: the probe now runs inside the test against tmp_path (unique per xdist worker, and the volume that actually holds the checkpoints) and calls pytest.skip when the volume is case-sensitive. The module-level skipif decorator is gone.
…ization Address Copilot review on the microsoft#9444 fix: derive the expected filename from the requested checkpoint ID instead of the resolved path (Windows resolve() canonicalizes to on-disk casing, which would accept the colliding save), allow same-ID updates when the exact spelling already exists on case-sensitive volumes, and fail closed when the directory cannot be listed. Move the case-sensitivity probe into the test body on tmp_path so parallel xdist workers cannot race a shared collection-time probe.
GitHub Actions
Active
Somay kousis (Somay-kousis)
left a comment
Somay kousis (Somay-kousis)
left a comment
There was a problem hiding this comment.
Ran the PR's conformance file on macOS 26.6 APFS (case-insensitive, default volume):
- main 44fe223 + PR test file:
1 failed, 18 passed(test_file_save_rejects_case_variant_checkpoint_idfails) - PR head 183ae7b:
19 passed
So the case-only collision from #9444 is fixed here.
One input still reaches the same overwrite: IDs that differ by Unicode normalization. APFS treats NFC and NFD spellings as one file ("café-1" vs "café-1"), and str.casefold() does not normalize, so _assert_no_case_variant_collision finds no variant and the second save replaces the first.
Script (public API only, same shape as the issue's repro): two saves, then load(first_id).
== main 44fe223d
case: load('Round-A') -> id='round-a' wf=second-workflow state={'marker': 'second'}; first-workflow ids=[]
nfc/nfd: load('café-1') -> id='café-1' wf=second-workflow state={'marker': 'second'}; first-workflow ids=[]
case+nfd: load('Café-1') -> id='café-1' wf=second-workflow state={'marker': 'second'}; first-workflow ids=[]
== PR head 183ae7b7
case: second save rejected: WorkflowCheckpointException
nfc/nfd: load('café-1') -> id='café-1' wf=second-workflow state={'marker': 'second'}; first-workflow ids=[]
case+nfd: load('Café-1') -> id='café-1' wf=second-workflow state={'marker': 'second'}; first-workflow ids=[]
(the printed IDs look identical; the second one is NFD, 7 code points vs 6)
Comparing normalized keys in the variant scan closes it:
def _key(name: str) -> str:
return unicodedata.normalize("NFC", name).casefold()
variant = next(
(name for name in siblings if name != expected_name and _key(name) == _key(expected_name)),
None,
)With that change, all three rows above raise WorkflowCheckpointException, and test_checkpoint_storage_conformance.py plus test_checkpoint.py still pass. Could the new regression be parametrized over ("Round-A", "round-a"), (NFC "café-1", NFD "café-1") and (NFC "Café-1", NFD "café-1")?
Not checked: Windows/NTFS and HFS+.
…lision guard APFS and HFS+ treat NFC and NFD spellings of a name as one file, and str.casefold() does not normalize, so a checkpoint ID differing only by normalization form still reached the silent overwrite from microsoft#9444. Compare siblings under NFC + casefold and reject those variants too. Thanks to Somay-kousis for the on-device report and the fix sketch.
GitHub Actions
Active
|
Somay kousis (@Somay-kousis) Thanks for the on-device verification and for the normalization catch. Landed in cddc816. The guard now compares siblings under On your "not checked" list: NTFS is normalization-sensitive, so the NFD save finds no existing file, the guard returns early, and both checkpoints coexist without an overwrite, which is safe (if visually confusing); the reject only engages where the filesystem actually folds. Reproduced the hole before the fix on this machine (APFS): both new parametrized cases fail against the previous guard and pass now, and the full checkpoint family (153 tests across the five checkpoint test files) plus pyright stay green. |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
The sequential save guard is much improved, but the file backend still permits variant-ID data loss and identity aliasing in supported operations.
| return file_path | ||
|
|
||
| @staticmethod | ||
| def _assert_no_variant_collision(file_path: Path, checkpoint_id: CheckpointID) -> None: |
There was a problem hiding this comment.
Please enforce the same exact checkpoint-ID identity on load() and delete(). On a case/normalization-folding filesystem, after saving Round-A, _validate_file_path("round-a") resolves to the same existing file: load("round-a") returns the other checkpoint and delete("round-a") deletes Round-A. Under the fail-closed policy, a variant ID should not read or remove a checkpoint whose stored ID/spelling differs; add regressions for both operations.
There was a problem hiding this comment.
Both operations now go through _assert_stored_spelling, which compares the directory's stored spelling against the requested one and refuses when only a case/normalization fold brings them together: load("round-a") raises rather than returning Round-A's state, and delete("round-a") raises rather than unlinking it. Same-ID access and genuinely distinct IDs on a case-sensitive volume are unchanged. Regressions are test_file_load_rejects_case_variant_checkpoint_id and test_file_delete_rejects_case_variant_checkpoint_id in the conformance file.
| from ._checkpoint_encoding import decode_checkpoint_value, encode_checkpoint_value | ||
|
|
||
| file_path = self._validate_file_path(checkpoint.checkpoint_id) | ||
| self._assert_no_variant_collision(file_path, checkpoint.checkpoint_id) |
There was a problem hiding this comment.
This check occurs before _enqueue_write, so two first-time variant saves can both observe no destination and pass before either write lands; the second atomic replace can still overwrite the first. Please serialize by the filesystem-folded collision key and repeat the guard after the predecessor completes/immediately before replacement, with a concurrent-save regression. Cross-process guarantees can remain a separately documented limitation, but same-process saves should not retain the corruption race.
There was a problem hiding this comment.
Two changes close it. The destination queue is now keyed by the filesystem-folded name (NFC + casefold) instead of the raw path, so variant spellings of one checkpoint share one queue and the second save cannot begin until the first has actually landed. And _write_atomic repeats the collision guard under that ownership, immediately before the replace, so a variant sibling created by the predecessor is seen and the save fails closed. test_concurrent_variant_saves_do_not_overwrite gathers both first-time variant saves and asserts exactly one is rejected with the survivor's data intact. Cross-process saves stay what the module comment says: atomic replace, undefined winner.
GitHub Actions
Active
|
Status note for the reviewer: the red Public API Compatibility check is not from this diff. griffe crashes while resolving the base snapshot's |
GitHub Actions
Active
Fixes #9444.
FileCheckpointStorage.save()resolved{id}.jsonunder the storage directory and atomically replaced whatever sat there. On case-insensitive filesystems (the default on Windows and macOS), two explicit checkpoint IDs that differ only by case (Round-A/round-a) resolve to the same file, so the second save silently replaced the first checkpoint; reads of the first ID then returned the other workflow's name and state, and the first workflow's listing came back empty. Reproduced locally on macOS APFS with the issue's exact snippet against main (44fe223): two "successful" save IDs, thenload("Round-A")returnsround-a/second-workflow/{"marker": "second"}.On the policy question the issue raised: this PR takes the fail-closed option — an ID this backend cannot store independently is rejected before any existing checkpoint is replaced, rather than normalizing spellings (which would silently merge two IDs the caller deliberately distinguished).
save()now checks the destination before encoding: when the path exists and the on-disk spelling differs only by letter case, it raisesWorkflowCheckpointExceptionnaming the collision. On case-sensitive filesystems the variant path does not exist, so the check is a no-op there, and re-saving the exact same ID stays a normal update everywhere.Tests: a new conformance regression (
test_file_save_rejects_case_variant_checkpoint_id, skipped on case-sensitive volumes) pins the rejection, the survival of the first checkpoint's data and listing, and the same-ID update path; it is red without the guard on a case-insensitive volume and green with it. Full checkpoint sweep on the worktree: 271 passed, 2 pre-existing failures intest_checkpoint_unrestricted_pickle.py(they fail identically on the unmodified tree in this environment — the local env lacks the OpenAI SDK surface those two need), 2 skipped. pyright on the changed module: 0 errors.Scope note: the guard runs at save entry, so two differently-cased saves racing in the same process are not mutually serialized (the write-ticket queue keys on the spelled path) and a cross-process same-instant pair can still interleave; sequential saves, the reported corruption shape, are covered. Load-side case strictness was left out deliberately — happy to add it if maintainers want the read path hardened too.