Skip to content

Python: FileCheckpointStorage save/load symmetry (#8181) - #8214

Open
lsmlhi_25 (FOWEPJF255) wants to merge 7 commits into
microsoft:mainfrom
FOWEPJF255:fix/file-checkpoint-save-load-symmetry-8181
Open

Python: FileCheckpointStorage save/load symmetry (#8181)#8214
lsmlhi_25 (FOWEPJF255) wants to merge 7 commits into
microsoft:mainfrom
FOWEPJF255:fix/file-checkpoint-save-load-symmetry-8181

Conversation

@FOWEPJF255

Copy link
Copy Markdown
Contributor

Motivation & Context

FileCheckpointStorage could save checkpoints that the same storage could not restore, surface raw JSONDecodeError/UnicodeDecodeError on load, and disagree between list_checkpoints and list_checkpoint_ids when decoding failed. This breaks durable pause/resume and makes failures harder to handle.

Fixes #8181.

Description & Review Guide

  • What are the major changes?
    • Validate encoded checkpoints against this storage's allowed_checkpoint_types before writing (including encoding failures), so save refuses payloads that load would reject.
    • Wrap invalid JSON and invalid UTF-8 on load as WorkflowCheckpointException.
    • Make list_checkpoint_ids delegate to list_checkpoints so both share one decode path.
  • What is the impact of these changes?
    • Save/load/list stay consistent; callers get a single exception type for corrupt files and unrestorable payloads.
  • What do you want reviewers to focus on?
    • Save-time encode+restore validation, UTF-8/JSONDecodeError wrapping on load, and list_checkpoint_ids delegation.

Related Issue

Fixes #8181

This replaces closed PR #8193 (closed for missing PR template). Same issue; no other open PR for #8181.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

save() still writes checkpoint JSON without an explicit UTF-8 encoding while load() now forces UTF-8, which can reintroduce save/load asymmetry on non-UTF-8 default locales.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves durability and debuggability of the Python FileCheckpointStorage by enforcing save/load symmetry, standardizing exception behavior for corrupt checkpoint files, and aligning listing APIs so they don’t disagree when decoding fails.

Changes:

  • Validate at save-time that an encoded checkpoint can be decoded under the same storage’s allowed_checkpoint_types, preventing “save succeeds, load fails” scenarios.
  • Wrap invalid JSON / invalid UTF-8 during load() into WorkflowCheckpointException (per the CheckpointStorage contract).
  • Make list_checkpoint_ids() delegate to list_checkpoints() so both use the same decode/filter path.
File summaries
File Description
python/packages/core/agent_framework/_workflows/_checkpoint.py Adds save-time restore validation, wraps JSON/UTF-8 read failures on load, and aligns ID listing with decoded listing.
python/packages/core/tests/workflow/test_checkpoint.py Adds regression tests for invalid JSON/UTF-8 on load and for list ID alignment with decode filtering.
python/packages/core/tests/workflow/test_checkpoint_unrestricted_pickle.py Adds test ensuring file storage refuses to save checkpoints it cannot restore under its own allowed-types policy.
Review details

Suppressed comments (2)

python/packages/core/agent_framework/_workflows/_checkpoint.py:472

  • Since list_checkpoint_ids() now delegates to list_checkpoints(), listing IDs inherits list_checkpoints() file-reading behavior. list_checkpoints() currently opens checkpoint files without encoding="utf-8", while load() explicitly uses UTF-8; this can make listing locale-dependent and cause list_checkpoint_ids()/get_latest() to miss checkpoints that load() could otherwise read (or vice versa) when non-ASCII is present.
        checkpoints = await self.list_checkpoints(workflow_name=workflow_name)
        return [checkpoint.checkpoint_id for checkpoint in checkpoints]

python/packages/core/agent_framework/_workflows/_checkpoint.py:382

  • This error message is also used for UnicodeDecodeError (invalid UTF-8), but it only mentions JSON. That can be misleading when the file is valid JSON in another encoding or simply not UTF-8.
            raise WorkflowCheckpointException(
                f"Checkpoint file for {checkpoint_id} is not valid JSON and cannot be loaded."
            ) from ex
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/_workflows/_checkpoint.py
@FOWEPJF255

Copy link
Copy Markdown
Contributor Author

Follow-up: save() and list now open checkpoint JSON with explicit encoding=utf-8 to match load().

lhl-lhi (lhl-lhi) and others added 3 commits September 10, 2026 21:44
Include encoding in save-time validation, wrap invalid UTF-8 on load,
and delegate list_checkpoint_ids to list_checkpoints.
Match load()'s explicit UTF-8 encoding so save/load stay symmetric on non-UTF-8 default locales.
@FOWEPJF255
lsmlhi_25 (FOWEPJF255) force-pushed the fix/file-checkpoint-save-load-symmetry-8181 branch from f2ba45a to b443bbf Compare September 10, 2026 13:44
…icrosoft#8181)

Copilot review: UnicodeDecodeError should not claim invalid JSON.
@FOWEPJF255

Copy link
Copy Markdown
Contributor Author

Follow-up: save()/list/load all use explicit UTF-8; load distinguishes UnicodeDecodeError vs JSONDecodeError. Please re-review when convenient.

@FOWEPJF255

Copy link
Copy Markdown
Contributor Author

Thanks for the approval. Branch is up to date / CI running; ready to merge when checks are green.

…8181)

Package Checks ruff rule blocking-path-method-in-async-function failed
on Path(tmpdir).glob in an async test. Use asyncio.to_thread like other
core tests.
@ktz03

Copy link
Copy Markdown

lsmlhi_25 (@FOWEPJF255) Already has Eduard van Valkenburg (@eavanvalkenburg) approval and the UTF-8 save/load follow-up. Flagging here in case this just needs a final maintainer merge once required workflows are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

5 participants