fix(lists): get_entry cursor scope + reorder rollback on failure (tsk-u23vjy) - #2361
Conversation
…k-u23vjy) Re-scoped fix-forward of closed duplicate PR #2183. Of the card's four defects, two are already resolved or impossible on dev: position-0 collision was fixed by #2265's atomic in-INSERT allocation (existing concurrency test guards it), and NULL list_id rows cannot exist (schema NOT NULL). The two real ones land here: - get_entry read cur.description outside the cursor context; moved inside. - reorder_entries left already-issued UPDATEs pending when one raised, so the next unrelated commit() flushed a half-applied reorder; now rolls back and re-raises, with a test proving the pending write neither survives immediately nor resurfaces via a later unrelated commit (proven red against the unguarded store).
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe project list store now builds entry metadata while the cursor remains open and rolls back partial reorder updates when updates, cancellation, or commit operations fail. Regression tests and a changelog entry document these fixes. ChangesProject list store fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🧹 Nitpick comments (1)
tinyagentos/projects/lists_store.py (1)
179-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet
strict=Trueonzip.Ruff reports B905 on Line 179. Use
zip(keys, row, strict=True)so an unexpected column-value mismatch fails immediately.🤖 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 `@tinyagentos/projects/lists_store.py` at line 179, Update the dict construction in the row-mapping method around dict(zip(keys, row)) to call zip with strict=True, ensuring mismatched key and value lengths fail immediately.Source: Linters/SAST tools
🤖 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 `@tests/projects/test_lists_store.py`:
- Around line 351-397: Extend the reorder rollback tests around
test_reorder_entries_rolls_back_on_exception with separate cases for
asyncio.CancelledError during the second position UPDATE and an exception from
commit(). In each case, invoke reorder_entries with the swapped positions,
restore any monkeypatches, perform an unrelated add_entry(), and assert both
original entries still have their initial positions.
In `@tinyagentos/projects/lists_store.py`:
- Around line 263-279: The reorder transaction spanning
tinyagentos/projects/lists_store.py:263-279 must include both the UPDATE loop
and commit within rollback-protected handling, explicitly covering
asyncio.CancelledError as well as ordinary exceptions before re-raising. Add
tests in tests/projects/test_lists_store.py:351-397 for cancellation during a
later UPDATE and for commit failure, asserting rollback occurs and partial
changes are not persisted.
---
Nitpick comments:
In `@tinyagentos/projects/lists_store.py`:
- Line 179: Update the dict construction in the row-mapping method around
dict(zip(keys, row)) to call zip with strict=True, ensuring mismatched key and
value lengths fail immediately.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 6b055fcc-2787-4007-896d-a075965071f4
📒 Files selected for processing (3)
changelog.d/2361-lists-store-cursor-rollback.mdtests/projects/test_lists_store.pytinyagentos/projects/lists_store.py
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 2c5c8a3)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2c5c8a3)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 66.1K · Output: 33.8K · Cached: 617.3K |
|
nemotron-super review VERDICT: No blocking issues found Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
…it() CodeRabbit's Major on #2361, folded: asyncio.CancelledError does not inherit Exception, so task cancellation mid-reorder left the issued UPDATEs pending exactly like the original hazard, and commit() sat outside the guard. The commit moves inside the try and the handler catches BaseException, rolling back before re-raising. Regression tests for both paths; the cancellation test proven red against the except-Exception guard.
|
Both CodeRabbit findings folded: the Major was real — |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| await self._db.rollback() | ||
| return False | ||
| await self._db.commit() | ||
| except BaseException: |
There was a problem hiding this comment.
[WARNING]: except BaseException catches SystemExit and KeyboardInterrupt
except BaseException is broader than needed. It captures SystemExit and KeyboardInterrupt, which could suppress process-exit signals if rollback() fails during shutdown or interrupt handling. Consider narrowing to except (Exception, asyncio.CancelledError): to catch only the intended exceptions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Lead completion of board card tsk-u23vjy (bounced PR #2359 — analysis there). Re-scoped against what is actually true on dev:
Card defects 1 and 2 need no code. Position-0 collision is already fixed on dev by #2265's atomic in-INSERT allocation, guarded by
test_concurrent_add_entry_distinct_positions(which #2359 had rewritten into a sequential test — restored untouched here). NULLlist_idrows are structurally impossible: the schema sayslist_id TEXT NOT NULL, so the IS-NULL branch the card asks for is dead code on arrival (proven: insertinglist_id=NoneraisesIntegrityError)._get_next_positionitself has zero callers on dev. The card's acceptance criteria were written against the closed duplicate PR #2183's branch, where both defects were real.Defects 3 and 4 are real and land here.
Red-first evidence (lead-run at merge ref)
The rollback test executed against dev's unguarded
reorder_entries:The failure mode is the card's exact hazard: the first UPDATE stays pending on the shared connection after the exception, and the assertion catches the half-applied position. With the fix, 19/19 pass. The
get_entrycursor fix is latent-hazard hardening (aiosqlite's closed cursor happens to retaindescriptiontoday), so no red is possible for it; stated honestly rather than manufactured.Timing/atomicity statement
reorder_entries' single-commit atomicity is untouched (the card forbids restructuring it); the change only adds rollback + re-raise on the failure path.Summary by CodeRabbit
Bug Fixes
Tests
Documentation