fix(sessions): make crystallize retry idempotent for summary pages#256
fix(sessions): make crystallize retry idempotent for summary pages#256Yurii214 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Changescrystallize() retry upsert fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 `@src/vouch/sessions.py`:
- Around line 110-115: The current check-then-act pattern in the store.get_page,
store.put_page, and store.update_page sequence creates a race condition where
concurrent calls can both pass the get_page check and then fail when put_page
tries to create an already-existing page. Reverse the logic to attempt put_page
first and catch the "already exists" error (likely a specific exception), then
fall back to store.update_page in the exception handler instead of the
try-except-else structure. This makes the upsert operation race-safe by handling
the concurrent creation case gracefully.
In `@src/vouch/storage.py`:
- Around line 353-358: The update_page method is missing claim existence
validation that is enforced in put_page(), allowing pages to be persisted with
dangling claims references. Add the same claim validation logic used in
put_page() to the update_page method before the page is written to storage
(before the _page_path write_text call). This ensures that all claims referenced
in the page being updated are verified to exist, maintaining page-claim
integrity.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0af788b-afb8-4c55-ab8e-4f8457a86a0b
📒 Files selected for processing (4)
CHANGELOG.mdsrc/vouch/sessions.pysrc/vouch/storage.pytests/test_sessions.py
| try: | ||
| store.get_page(page.id) | ||
| except ArtifactNotFoundError: | ||
| store.put_page(page) | ||
| else: | ||
| store.update_page(page) |
There was a problem hiding this comment.
Make summary-page upsert race-safe.
Lines 110-115 use a check-then-create flow. Concurrent crystallize() calls can both miss on get_page(), then one put_page() fails with “already exists”, breaking retry safety.
Suggested patch
- try:
- store.get_page(page.id)
- except ArtifactNotFoundError:
- store.put_page(page)
- else:
- store.update_page(page)
+ try:
+ store.update_page(page)
+ except ArtifactNotFoundError:
+ try:
+ store.put_page(page)
+ except ValueError:
+ # Lost create race; page now exists.
+ store.update_page(page)🤖 Prompt for 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.
In `@src/vouch/sessions.py` around lines 110 - 115, The current check-then-act
pattern in the store.get_page, store.put_page, and store.update_page sequence
creates a race condition where concurrent calls can both pass the get_page check
and then fail when put_page tries to create an already-existing page. Reverse
the logic to attempt put_page first and catch the "already exists" error (likely
a specific exception), then fall back to store.update_page in the exception
handler instead of the try-except-else structure. This makes the upsert
operation race-safe by handling the concurrent creation case gracefully.
| def update_page(self, page: Page) -> Page: | ||
| if not self._page_path(page.id).exists(): | ||
| raise ArtifactNotFoundError(f"page {page.id}") | ||
| self._page_path(page.id).write_text(_serialize_page(page)) | ||
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | ||
| return page |
There was a problem hiding this comment.
Preserve page-claim integrity checks in update_page.
Line 353 adds an update path that skips the claim existence validation enforced by put_page(). That allows persisting pages with dangling claims references.
Suggested patch
def update_page(self, page: Page) -> Page:
+ for cid in page.claims:
+ if not self._claim_path(cid).exists():
+ raise ValueError(f"page {page.id} references unknown claim {cid}")
if not self._page_path(page.id).exists():
raise ArtifactNotFoundError(f"page {page.id}")
self._page_path(page.id).write_text(_serialize_page(page))
self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}")
return page📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def update_page(self, page: Page) -> Page: | |
| if not self._page_path(page.id).exists(): | |
| raise ArtifactNotFoundError(f"page {page.id}") | |
| self._page_path(page.id).write_text(_serialize_page(page)) | |
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | |
| return page | |
| def update_page(self, page: Page) -> Page: | |
| for cid in page.claims: | |
| if not self._claim_path(cid).exists(): | |
| raise ValueError(f"page {page.id} references unknown claim {cid}") | |
| if not self._page_path(page.id).exists(): | |
| raise ArtifactNotFoundError(f"page {page.id}") | |
| self._page_path(page.id).write_text(_serialize_page(page)) | |
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | |
| return page |
🤖 Prompt for 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.
In `@src/vouch/storage.py` around lines 353 - 358, The update_page method is
missing claim existence validation that is enforced in put_page(), allowing
pages to be persisted with dangling claims references. Add the same claim
validation logic used in put_page() to the update_page method before the page is
written to storage (before the _page_path write_text call). This ensures that
all claims referenced in the page being updated are verified to exist,
maintaining page-claim integrity.
|
Thanks for your contribution! Fix the conflict |
|
@plind-junior Thanks for the earlier note on the conflict — rebased onto latest Working through the CodeRabbit race/validation notes next; happy to push an update if you’d rather review the core retry fix first. Appreciate the review 🙏 |
Partial crystallize runs that already wrote session-{id} summary pages
failed on retry with "page already exists". Upsert the summary page
and derive its artifact list from all approved session proposals.
Fixes vouchdev#139
Co-authored-by: Cursor <cursoragent@cursor.com>
Yurii214
force-pushed
the
fix/139-crystallize-retry-idempotency
branch
from
8440927 to
0f9687e
Compare
github-actions
Bot
added
docs
3 participants
Summary
crystallize()runs that already wrote asession-{id}summary page no longer fail on retry withpage session-... already exists.KBStore.update_page()and upserts the summary page (put_pageon first write,update_pageon retry).Why
Operators retry
crystallizeafter transient approval failures. The first partial run could approve one proposal and create the summary page; a second run then calledput_page()again and crashed before completing.What might break
pages/session-{id}.mdpath and shape.crystallizeon a session that already has approved proposals but no pending ones will now create or refresh the summary page whenwrite_summary_page=True.put_page()remains exclusive-create everywhere else.Tests
PYTHONPATH=src uv run pytest tests/test_sessions.pyPYTHONPATH=src uv run pytest --ignore=tests/embeddings(full suite minus optional embedding/MCP extras)PYTHONPATH=src uv run ruff check src/vouch/sessions.py src/vouch/storage.py tests/test_sessions.pytest_crystallize_retry_updates_existing_summary_page(partial approve → retry → same summary page lists all claims)Fixes #139
Summary by CodeRabbit
Bug Fixes
Tests