Skip to content

[AEP][high] AEP-20260924-r2-REPO-B01: replace the checkpoint index atomically - #333

Merged
mattmre merged 2 commits into
mainfrom
aep/high/AEP-20260924-r2-REPO-B01/checkpoint-index
Sep 24, 2026
Merged

mattmre merged 2 commits into
mainfrom
aep/high/AEP-20260924-r2-REPO-B01/checkpoint-index

Conversation

@mattmre

@mattmre mattmre commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Description

checkpoint_metadata.json is replaced through a temp file, fsync, and os.replace. A failed save does not truncate the live index and does not return an id. A 0-byte index raises. Delete replaces the index before removing the directory.

Follow-up 9d9869f refuses a checkpoint id that already exists on disk or in the index before mkdir or copy2, so a failed save cannot delete a checkpoint this call did not create.

Type of Change

  • Bug fix

Testing

Spark Python unittest test_checkpoint_manager.py: 42 passed on 9d9869f. Ruff passed. Review grok-tierb-333b-20260924 probed the same-second collision and a failed dump of a new id.

Brutal Honesty

  • restore_checkpoint is unchanged.
  • The 0-byte index raises JSONDecodeError from json.load.
  • The first review of 8790f95 scored 90 because of the same-second collision. This score is for 9d9869f.

EVIDENCE: 42 checkpoint tests passed on 9d9869f; review grok-tierb-333b-20260924
SMOKE: targeted unittest only; bash scripts/smoke.sh was not run on this branch
BHS_SELF_DRAFT: 100
BHS_SELF_DRAFT_AGENT: grok-aep-fix-20260924
BHS_TIER_B: 100
BHS_TIER_B_AGENT: grok-tierb-333b-20260924
BHS_TIER_B_SEVERITY: none
BHS_OFFICIAL: 100
CARRY_FORWARD: none
DEFERRED_SCOPE: corpus restore stays on PR 335
LOOP_ITERATIONS: 2
OPERATOR_OVERRIDE: none

@mattmre

mattmre commented Sep 24, 2026

Copy link
Copy Markdown
Owner Author

Review of 8790f95. The atomic index replace holds for a new id: temp file, fsync, os.replace, no truncate of the live index, 0-byte index raises, delete replaces the index before rmtree. One new cleanup path destroys a checkpoint this call did not create.

create_checkpoint builds checkpoint_id from the name plus a one-second timestamp (checkpoint_manager.py:123), then mkdir(..., exist_ok=True) (:127) and shutil.copy2 onto adapter_weights.pt (:134) before _save_metadata. If that save raises, the handler shutil.rmtrees checkpoint_path (:156-162). A second create_checkpoint with the same name in the same second reuses the committed directory, overwrites its adapter file, and deletes the directory when the save fails. restore_checkpoint then has no file.

Required fix: if checkpoint_path already exists, or that id is already in self.metadata["checkpoints"], raise before mkdir and before copy2. Do not delete that directory. rmtree only a directory this call created. Add a test that freezes the timestamp, commits one create_checkpoint("same", ...), fails the next json.dump, and asserts the directory and the original adapter bytes remain, the index bytes are unchanged, and no id is returned.

Tier B on this SHA: 90, severity important. This commit does not merge.

@mattmre

mattmre commented Sep 24, 2026

Copy link
Copy Markdown
Owner Author

Fix pushed as 9d9869f. create_checkpoint raises FileExistsError before mkdir or copy2 when that id is already on disk or in the index, and a failed save removes a directory only when this call created it.

Parent re-ran python -m unittest test_checkpoint_manager.py: 42 passed. Ruff passed. Not merged. A fresh Tier B review of this SHA is still required before the official score can move.

@mattmre
mattmre merged commit 6f24159 into main Sep 24, 2026
12 of 16 checks passed
@mattmre
mattmre deleted the aep/high/AEP-20260924-r2-REPO-B01/checkpoint-index branch September 24, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant