[fix] Make checkpoint replacement atomic via tmp/old rename sequence - #155
[fix] Make checkpoint replacement atomic via tmp/old rename sequence#155dodatboii wants to merge 3 commits into
Conversation
Previously, save_checkpoint deleted the existing checkpoint directory before renaming the new one into place. A failure between those two steps (cross-device rename, disk full) destroyed both the old and new checkpoints. Now the old directory is renamed aside to `.old` first and only removed once the new checkpoint is confirmed in place; on failure it is restored automatically. Directory layout and the public API are unchanged. Signed-off-by: dodatboii <zhangyixiang9@huawei.com>
Signed-off-by: dodatboii <zhangyixiang9@huawei.com>
CLA Signature Passdodatboii, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce4d39fb85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| checkpoint_dir.rename(old_dir) | ||
|
|
||
| tmp_dir.rename(checkpoint_dir) |
There was a problem hiding this comment.
When replacing an existing checkpoint, checkpoint_dir.rename(old_dir) removes the supported load path before the following rename installs the new version. A concurrent load_checkpoint(checkpoint_dir) can therefore observe a missing directory, and process termination between these statements leaves a restart unable to load the checkpoint without manually discovering .old. This two-rename sequence does not provide the documented atomic replacement or automatic rollback; use an atomic indirection/exchange mechanism or add startup/load recovery for .old.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
Signed-off-by: dodatboii <zhangyixiang9@huawei.com>
CLA Signature Passdodatboii, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
2 participants
Previously, save_checkpoint deleted the existing checkpoint directory before renaming the new one into place. A failure between those two steps (cross-device rename, disk full) destroyed both the old and new checkpoints.
Now the old directory is renamed aside to
.oldfirst and only removed once the new checkpoint is confirmed in place; on failure it is restored automatically. Directory layout and the public API are unchanged.