Repository navigation
Python: fix(core): reject checkpoint IDs that collide by letter case on save - #9466
Yufeng He (he-yufeng) wants to merge 3 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. |
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.