Repository navigation
Python: fix(core): reject checkpoint IDs that collide by letter case on save #9466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
78ddc7d
183ae7b
cddc816
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| import os | ||
| import threading | ||
| import time | ||
| import unicodedata | ||
| import uuid | ||
| from collections.abc import Callable, Mapping | ||
| from concurrent.futures import Future | ||
|
|
@@ -571,6 +572,61 @@ def _validate_file_path(self, checkpoint_id: CheckpointID) -> Path: | |
| raise WorkflowCheckpointException(f"Invalid checkpoint ID: {checkpoint_id}") | ||
| return file_path | ||
|
|
||
| @staticmethod | ||
| def _assert_no_variant_collision(file_path: Path, checkpoint_id: CheckpointID) -> None: | ||
| """Reject an ID that the filesystem cannot store as an independent file. | ||
|
|
||
| On case-insensitive filesystems (the default on Windows and macOS), | ||
| ``Round-A.json`` and ``round-a.json`` name the same file, so a save under | ||
| the variant spelling would silently replace the other checkpoint and its | ||
| reads would then return the wrong workflow's state (#9444). APFS and HFS+ | ||
| additionally fold Unicode normalization forms, so ``café-1.json`` spelled | ||
| NFC and NFD is also one file there (#9466 review). On filesystems without | ||
| those folds the variant path simply does not exist, so the directory scan | ||
| below is skipped entirely there. | ||
| """ | ||
| if not file_path.exists(): | ||
| return | ||
| # Derive the expected spelling from the requested ID, not the resolved | ||
| # path: on Windows Path.resolve() canonicalizes an existing file to its | ||
| # on-disk casing, so file_path.name would already be the colliding | ||
| # name and the check below would accept the overwrite (#9444). | ||
| expected_name = f"{checkpoint_id}.json" | ||
| try: | ||
| siblings = [entry.name for entry in file_path.parent.iterdir()] | ||
| except OSError as exc: | ||
| # Permissions can allow lookup/creation/replacement while denying | ||
| # enumeration, so skipping the scan here would let a variant | ||
| # save proceed unchecked. Fail closed instead. | ||
| raise WorkflowCheckpointException( | ||
| f"Cannot verify checkpoint ID {checkpoint_id!r} against existing checkpoints: " | ||
| "the storage directory could not be listed." | ||
| ) from exc | ||
| # The filesystem key is the NFC-normalized, case-folded spelling: both | ||
| # folds are lossy on the volumes that perform them, so two IDs sharing | ||
| # a key cannot coexist there. str.casefold() alone does not normalize. | ||
|
|
||
| def _fs_key(name: str) -> str: | ||
| return unicodedata.normalize("NFC", name).casefold() | ||
|
|
||
| expected_key = _fs_key(expected_name) | ||
| variant = next( | ||
| (name for name in siblings if name != expected_name and _fs_key(name) == expected_key), | ||
| None, | ||
| ) | ||
| if variant is None or expected_name in siblings: | ||
| # No filesystem-folded variant exists, or the exact spelling is | ||
| # already stored (a same-ID update on a volume where distinct | ||
| # spellings legitimately coexist). | ||
| return | ||
| raise WorkflowCheckpointException( | ||
| f"Checkpoint ID {checkpoint_id!r} collides with the existing checkpoint " | ||
| f"{variant.removesuffix('.json')!r}: on this filesystem both names " | ||
| "resolve to one file, and saving would replace that checkpoint's data. " | ||
| "Choose a checkpoint ID that differs by more than letter case or " | ||
| "Unicode normalization form." | ||
| ) | ||
|
|
||
| @classmethod | ||
| def _delete_lock(cls, file_path: Path) -> threading.Lock: | ||
| """Return the process-local deletion lock for a checkpoint file.""" | ||
|
|
@@ -592,6 +648,7 @@ async def save(self, checkpoint: WorkflowCheckpoint) -> CheckpointID: | |
| 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check occurs before |
||
| checkpoint_dict = checkpoint.to_dict() | ||
| # Fail at save time if encoding or restore validation fails (#8181). | ||
| try: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please enforce the same exact checkpoint-ID identity on
load()anddelete(). On a case/normalization-folding filesystem, after savingRound-A,_validate_file_path("round-a")resolves to the same existing file:load("round-a")returns the other checkpoint anddelete("round-a")deletesRound-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.