fix(storage): receipt automatic embeddings retention - #3837
Conversation
Promotion now records active, retained, eligible, and reclaimed generation states before automatic collection. It rejects an ownerless predecessor before the active pointer changes, preserving a rollback-capable previous generation through the one-generation boundary. Co-Authored-By: Codex <noreply@openai.com>
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesGeneration retention lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IndexGenerationStore
participant ActivePointer
participant GenerationDirectories
participant RetentionReceiptStorage
IndexGenerationStore->>ActivePointer: validate predecessor ownership
IndexGenerationStore->>ActivePointer: update active generation
IndexGenerationStore->>GenerationDirectories: classify rollback and eligible generations
IndexGenerationStore->>RetentionReceiptStorage: persist pre-reclamation receipt
IndexGenerationStore->>GenerationDirectories: remove eligible directories
IndexGenerationStore->>RetentionReceiptStorage: persist completed receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/storage/index_generation.py`:
- Around line 835-840: Update IndexGeneration.create and the retention ordering
in polylogue/storage/index_generation.py:835-840 to use a monotonic creation
sequence or nanosecond timestamp for recency, retaining generation_id only as
the final deterministic tiebreaker, and revise the nearby comment accordingly.
Update tests/unit/storage/test_index_generation.py:472-491 to assert ordering by
the corrected key; if that key is not yet implemented, force distinct creation
times so the expected newer generation is unambiguous.
- Around line 909-936: Bound retention-receipts growth in the
generation-retention method alongside pruning of generation directories and
retired markers. Add receipt-history pruning using a retention boundary such as
_RETENTION_RECEIPT_HISTORY, ensuring it is larger than
SUPERSEDED_GENERATION_RETENTION so the receipt written by the current run is not
removed; retain the newest receipts and remove older orphaned JSON files before
or after _write_retention_receipt.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa9d9e6b-7861-497c-a65e-095c819ff890
📒 Files selected for processing (2)
polylogue/storage/index_generation.pytests/unit/storage/test_index_generation.py
![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: ab1bc83088
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| generation = IndexGeneration(**json.loads(metadata_path.read_text(encoding="utf-8"))) | ||
| except (OSError, ValueError, TypeError): | ||
| continue |
There was a problem hiding this comment.
When a prior active generation has malformed or incomplete metadata—for example, owner_id is missing—construction raises TypeError, but this preflight silently skips the record and proceeds with the pointer swap. The collector skips the same record, so the supposedly ownerless-predecessor refusal is bypassed and the generation becomes unaccounted, contrary to the fail-closed ownership invariant; unreadable prior promoted metadata should abort promotion.
Useful? React with 👍 / 👎.
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| temporary = path.with_suffix(".json.tmp") | ||
| temporary.write_text(json.dumps(asdict(receipt), indent=2, sort_keys=True), encoding="utf-8") | ||
| os.replace(temporary, path) | ||
| _fsync_directory(path.parent) |
There was a problem hiding this comment.
On the first receipt, this creates retention-receipts and fsyncs only that new directory after writing the file; it does not fsync generations_root to durably persist the new directory entry until after generation deletion begins. A power loss during reclamation can therefore leave a reclaimed generation without the promised durable eligibility receipt even though the receipt write returned successfully; fsync the parent immediately after creating the receipt directory, before any deletion.
Useful? React with 👍 / 👎.
| try: | ||
| self.prune_superseded_generations() | ||
| self._collect_superseded_generations(promoted) | ||
| except OSError: | ||
| logger.warning("index generation pruning failed after promotion", exc_info=True) | ||
| logger.warning("index generation retention collection failed after promotion", exc_info=True) |
There was a problem hiding this comment.
If the process exits after the generation metadata is written as active but before this call finishes—or if an OSError occurs before the initial receipt is written—the promotion remains committed while collection is abandoned. No startup path retries active generations lacking receipts, and the existing recovery methods only act on promoting metadata, so this promotion permanently lacks its lifecycle receipt and superseded generations remain unreclaimed unless another promotion happens; retain a recoverable pending state or reconcile missing receipts after restart.
Useful? React with 👍 / 👎.
| retention_owner_id: str | None = None | ||
| retention_state: str | None = None |
There was a problem hiding this comment.
A fast-forward receipt prepared or activated before this upgrade contains the prior generation metadata shape, without these two fields, while IndexGenerationStore.load() now supplies them as None through the dataclass defaults. devtools/index_fast_forward.py compares asdict(generation) with the receipt payload in _require_generation_binding() and _require_active_receipt_binding(), so activation of an already prepared candidate—and even an idempotent retry of an already activated receipt—now fails with a metadata-changed error despite no on-disk mutation; normalize missing legacy fields before those comparisons.
Useful? React with 👍 / 👎.
Problem: millisecond timestamps plus UUID tie-breaking could retain a non-predecessor generation, retention receipts accumulated indefinitely, and recovered promotions bypassed automatic collection. What changed: persist monotonic creation and promotion chronology, pin the observed predecessor during normal promotion, route recovery through the same receipt-backed collector, and bound receipts to the active and preceding proofs. Compatibility: metadata and receipt readers default newly-added timestamps for existing archives. Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cea388fef2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| temporary.write_text(json.dumps(asdict(receipt), indent=2, sort_keys=True), encoding="utf-8") | ||
| os.replace(temporary, path) | ||
| _fsync_directory(path.parent) |
There was a problem hiding this comment.
If the host loses power after the initial receipt rename and the subsequent rmtree, write_text() has only closed the temporary file; fsyncing the directory persists the name but does not guarantee that the file's contents reached stable storage. The reclaimed generation can therefore disappear while its eligibility receipt survives as empty or corrupt JSON. Fsync the temporary receipt file before replacing it, prior to beginning reclamation.
Useful? React with 👍 / 👎.
| candidates.sort(key=lambda item: (item[0], item[1], item[2]), reverse=True) | ||
| predecessor = promoted.predecessor_generation_id | ||
| if predecessor is not None: | ||
| candidates.sort(key=lambda item: item[4].generation_id != predecessor) |
There was a problem hiding this comment.
When completing a pointer-swapped promotion written by the pre-upgrade code, predecessor_generation_id and every legacy promoted_at_ns are absent, so this fallback orders prior generations by creation time. If an older-created inactive candidate was promoted after a newer-created generation, recovery retains the newer-created generation and reclaims the actual predecessor, destroying the intended rollback target. Legacy recovery without recorded predecessor chronology should retain ambiguous candidates rather than collect them.
Useful? React with 👍 / 👎.
1 participant
Summary
Make index-generation retention part of the blue-green promotion lifecycle. Each promotion now records the active, retained, eligible, and reclaimed generation states, keeps one rollback target, and reclaims older history automatically.
Problem
The old promotion path invoked unreceipted pruning after moving the active pointer. It retained one predecessor in practice, but did not record retention ownership or a durable lifecycle receipt. An ownerless predecessor could also be superseded without blocking the pointer swap.
Solution
IndexGenerationStore.promote()now preflights the owner of every prior promoted generation before moving the pointer. The production collector assigns retention ownership, writes an eligibility receipt before deletion, then records reclaimed state after automatic collection. The former manual pruning entry point is removed. Focused SQLite-backed tests exercise three real promotions and the ownerless-predecessor refusal.Verification
direnv exec . devtools test tests/unit/storage/test_index_generation.py -k 'promotion_records_automatic_retention_and_reclamation or promotion_refuses_ownerless_predecessor_before_pointer_swap'->2 passed, 32 deselected.direnv exec . ruff format --check polylogue/storage/index_generation.py->1 file already formatted.direnv exec . ruff check polylogue/storage/index_generation.py tests/unit/storage/test_index_generation.py->All checks passed!.direnv exec . mypy polylogue/storage/index_generation.py->Success: no issues found in 1 source file.direnv exec . git push -u origin feature/fix/embeddings-generation-retentioncompleted the repository pre-push quick verification baseline.Acceptance criteria
test_promotion_records_automatic_retention_and_reclamationasserts the persisted promotion receipt and real generation paths.retention_boundary=1, retention ownership, and the retained predecessor remains present.Review finding dispositions
(created_at_ms, generation_id)can retain the wrong rollback target within one millisecond.test_promotion_retains_actual_predecessor_when_generation_ids_reversedrives three same-millisecond promotions with reverse UUID ordering.RETENTION_RECEIPT_HISTORY = 2), then deletes older parseable receipts. Malformed receipts remain for investigation.test_promotion_bounds_retention_receipt_historydrives four promotions and asserts the bounded on-disk set.Ref polylogue-embeddings-retention.
Summary by CodeRabbit
New Features
Bug Fixes