Repository navigation
Conversation
Closes #38. CheckpointManager.create_checkpoint() derived snapshot_id to whole-second precision, so two checkpoints created within the same second shared one ID: the second write replaced the first snapshot file while CHECKPOINT_REGISTRY.json kept two entries, and loading the first name returned the second state (reproducible data loss). The fix allocates IDs as SNAP_YYYYMMDD_HHMMSS_NNN (the layout already documented in the module docstring, mirroring rege/protocols/recovery.py), deriving the sequence from IDs claimed in the checkpoint registry and snapshot files on disk so allocation stays correct across manager instances and process restarts, even when the clock does not advance between calls. Save-then-register ordering is unchanged: a failed snapshot write raises PersistenceError before any registry entry is created (fail closed). Tests: rege/tests/test_checkpoint_collision.py (5 regression tests: same-second distinct IDs/files, per-entry state resolution, cross-instance sequence continuation, fail-closed write failure, legacy unsuffixed ID coexistence). Verified: new file 5 passed; rege/tests/test_persistence.py + test_persistence_coverage.py 47 passed with 4 pre-existing environment failures (root-user permission tests, identical on pristine base).
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThe PR fixes same-second checkpoint collisions by allocating timestamp-based IDs with zero-padded sequence suffixes derived from both registry entries and on-disk snapshots, while retaining save-before-register semantics. New regression tests cover collision prevention, state integrity, restart continuity, write failures, and legacy snapshot compatibility. Sequence diagram for collision-free checkpoint creationsequenceDiagram
participant Manager as CheckpointManager
participant Registry as CheckpointRegistry
participant Disk as SnapshotDirectory
Manager->>Registry: load_checkpoint_registry()
Registry-->>Manager: claimed snapshot IDs
Manager->>Disk: glob(SNAP_*.json)
Disk-->>Manager: existing snapshot files
Manager->>Manager: _allocate_snapshot_id(when)
Manager->>Disk: _save_snapshot(snapshot)
alt snapshot write succeeds
Manager->>Registry: register checkpoint
else snapshot write fails
Manager-->>Manager: raise PersistenceError
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Green progress receipt (exact-head 8bde311): Python CI workflow completed with success on this draft PR. Core tests for the collision-free snapshot ID allocation are green. Awaiting remaining checks before any further progress. |
Closes #38.
Defect (reproduced pre-fix at exact base)
CheckpointManager.create_checkpoint()derivedsnapshot_idto whole-second precision (SNAP_{now.strftime('%Y%m%d_%H%M%S')}). Withdatetime.now()fixed to one instant for two calls:SNAP_20261023_231500)CHECKPOINT_REGISTRY.jsonheld two entries with that IDget_checkpoint_by_name("first")loaded the SECOND checkpoint's state (data loss)Fix
rege/persistence/checkpoint.py: new_allocate_snapshot_id()— keeps theSNAP_YYYYMMDD_HHMMSSbase with a zero-padded_NNNsequence suffix (the layout the module docstring already documents; mirrorsrege/protocols/recovery.py). The sequence is derived from IDs claimed in the checkpoint registry plus snapshot files on disk, so same-second creates get distinct IDs even when the clock never advances, across manager instances and restarts.PersistenceErrorbefore any registry entry is created (fail closed).rege/tests/test_checkpoint_collision.py(new): 5 regression tests — same-second distinct IDs/files, per-entry state resolution, cross-instance sequence continuation, fail-closed write failure, legacy unsuffixed-ID coexistence.Scope honored: only
checkpoint.py+ the new regression test; no workflow, packaging, or archive changes.models.py'sStateSnapshotfallback keeps its existing behavior (out of scope, noted for follow-up).Verification (local, base 685e682 worktree)
_001/_002, two files, each name loads its own state — PASSpython -m pytest -q rege/tests/test_checkpoint_collision.py→ 5 passedpython -m pytest -q rege/tests/test_persistence.py rege/tests/test_persistence_coverage.py→ 47 passed, 4 failed — the 4 failures are pre-existing environment failures (permission-denied tests running as root); verified identical on pristine base, untouched assertions.Base / head
main@685e682ce7fdbca3d2dcf0eb2ba4a1946d0e241a(exact head pinned in Prevent same-second checkpoint ID collisions from overwriting snapshots #38)8bde311ca29ef78b10ed575d9828cea2c2a936b2Draft per the issue's delivery boundary (no merge/release authority granted). CI must be honestly green at the exact head before this counts as done.
Summary by Sourcery
Allocate collision-free checkpoint snapshot IDs to prevent same-second checkpoints from overwriting one another.
Bug Fixes:
Enhancements:
Tests: