fix(queue): requeue failed jobs per row so a dedupe collision cannot abort the batch - #139
Conversation
…abort the batch `requeue_failed_where` flipped every matching `failed` row to `ready` in one UPDATE. `idx_mem_tree_jobs_dedupe_active` is a partial unique index over `dedupe_key` covering only `ready`/`running`, so failed rows sit outside it and the same key can legitimately accumulate several failed rows over time. The moment that UPDATE moved two siblings into the index at once they collided, SQLite aborted the whole statement, and nothing was requeued. Observed in production as `UNIQUE constraint failed: mem_tree_jobs.dedupe_key` from the periodic self-heal on every boot and every 3h tick, with the queue permanently parked. The manual retry path shares the same helper and aborted identically, so there was no way out from either side. Select the candidates first, then apply per row: - skip a key already held by a live `ready`/`running` row (that work is already in flight, and requeueing onto it is the collision); - among matched failed rows sharing a key, requeue only the newest and settle its older siblings as `cancelled` — they are duplicates of the same unit of work, and leaving them `failed` would keep them counted as failures the user must act on; - rows with a NULL `dedupe_key` are outside the index and always requeue. Selection and mutation share one transaction so a concurrent claim cannot insert an active row between the read and the write. The return value still counts only rows actually flipped to `ready`.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change makes failed-job requeue transactional and duplicate-aware. It also validates UTF-8, front matter, and body preservation before persisting markdown tag updates. Regression tests cover duplicate jobs, active keys, malformed files, and body changes. ChangesFailed-job requeue handling
Markdown tag rewrite validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant requeue_failed_where
participant QueueDatabase
participant JobState
requeue_failed_where->>QueueDatabase: Select failed jobs newest-first
QueueDatabase-->>requeue_failed_where: Return failed candidates
requeue_failed_where->>JobState: Check active duplicate keys
requeue_failed_where->>QueueDatabase: Cancel superseded duplicates
requeue_failed_where->>QueueDatabase: Reset and requeue eligible jobs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![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 (3)
src/memory/queue/store_settle.rs (3)
346-374: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: prepare the two UPDATE statements once outside the loops.
Each iteration compiles the same SQL again. Use
tx.preparebefore the loop and callstmt.executeper row.Also note that a cancelled row keeps its old
failure_reasonandfailure_class. The requeue branch clears both. If any status panel reads those columns, a cancelled duplicate still reports a failure classification. Clear them on cancel if that is not intended.♻️ Prepared statement reuse
- for id in &to_cancel { - tx.execute( - "UPDATE mem_tree_jobs + if !to_cancel.is_empty() { + let mut stmt = tx.prepare( + "UPDATE mem_tree_jobs SET status = 'cancelled', locked_until_ms = NULL, completed_at_ms = ?2, last_error = 'superseded by a newer job with the same dedupe_key' - WHERE id = ?1", - params![id, now_ms], - )?; + WHERE id = ?1", + )?; + for id in &to_cancel { + stmt.execute(params![id, now_ms])?; + } }🤖 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/memory/queue/store_settle.rs` around lines 346 - 374, Prepare each repeated UPDATE statement once before its corresponding loop and reuse the prepared statements for every ID. In the cancellation UPDATE, also clear failure_reason and failure_class so cancelled superseded jobs do not retain failure metadata; leave the existing requeue cleanup behavior unchanged.
258-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc block is accurate; align the "skipped" wording with the cancel behavior.
Lines 278-279 state that a key already held by an active row is skipped entirely. The code matches that:
skipped_activeonly counts the row and leaves it infailed. The test doc insrc/memory/queue/store_settle_tests.rs(Lines 332-335) claims the same row "settles as a superseded duplicate", which the implementation does not do. Keep one description of this behavior in both files.🤖 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/memory/queue/store_settle.rs` around lines 258 - 289, Align the test documentation for the active-row case with the implementation: update the relevant description in the requeue tests around the active dedupe key to state that the failed row is skipped and remains failed, not cancelled as a superseded duplicate. Preserve the existing cancellation wording only for matched failed siblings when no active row already exists, keeping the documentation in requeue_failed/requeue_transient_failed consistent.
290-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
unchecked_transaction()withTransaction::new_unchecked()andTransactionBehavior::Immediateto eliminate the read-write race condition.The current code uses
unchecked_transaction(), which opens a deferred transaction. The firstSELECTacquires only a read lock. If another connection commits a claim before the laterUPDATEexecutes, theUPDATEfails withSQLITE_BUSY_SNAPSHOT(WAL mode) orSQLITE_BUSY, causing the entire requeue operation to return an error. The doc comment at line 285 promises that selection and mutation run atomically in a single transaction; a deferred transaction does not guarantee this.Use
Transaction::new_unchecked(&conn, TransactionBehavior::Immediate)instead. An immediate transaction acquires the write lock up front, eliminating this window.Add imports:
use rusqlite::{Transaction, TransactionBehavior};Change line 293 from:
let tx = conn.unchecked_transaction()?;to:
let tx = Transaction::new_unchecked(conn, TransactionBehavior::Immediate)?;🤖 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/memory/queue/store_settle.rs` around lines 290 - 293, Update requeue_failed_where to import and use rusqlite::Transaction and TransactionBehavior, replacing conn.unchecked_transaction() with Transaction::new_unchecked(conn, TransactionBehavior::Immediate) so the transaction acquires the write lock before selection and mutation.
🤖 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/memory/queue/store_settle_tests.rs`:
- Around line 332-369: Update the doc comment for
requeue_failed_skips_keys_already_held_by_an_active_row to state that the
occupied-key failed row remains failed and is counted as skipped_active, rather
than settling as a superseded duplicate. Replace the broad assert_ne! on
failed_id with an exact JobStatus::Failed assertion while preserving the
existing assertions for the requeued and live rows.
In `@src/memory/queue/store_settle.rs`:
- Around line 295-307: The candidate ordering in the settlement query must use a
monotonic enqueue-order value instead of UUID `id DESC` when completion
timestamps tie. Update the relevant job schema, enqueue logic in `enqueue_conn`,
and the query around `select` to persist and order by that sequence after
`COALESCE(completed_at_ms, created_at_ms) DESC`; add coverage for completions
occurring in the same millisecond and verify the newest enqueued row is
retained.
---
Nitpick comments:
In `@src/memory/queue/store_settle.rs`:
- Around line 346-374: Prepare each repeated UPDATE statement once before its
corresponding loop and reuse the prepared statements for every ID. In the
cancellation UPDATE, also clear failure_reason and failure_class so cancelled
superseded jobs do not retain failure metadata; leave the existing requeue
cleanup behavior unchanged.
- Around line 258-289: Align the test documentation for the active-row case with
the implementation: update the relevant description in the requeue tests around
the active dedupe key to state that the failed row is skipped and remains
failed, not cancelled as a superseded duplicate. Preserve the existing
cancellation wording only for matched failed siblings when no active row already
exists, keeping the documentation in requeue_failed/requeue_transient_failed
consistent.
- Around line 290-293: Update requeue_failed_where to import and use
rusqlite::Transaction and TransactionBehavior, replacing
conn.unchecked_transaction() with Transaction::new_unchecked(conn,
TransactionBehavior::Immediate) so the transaction acquires the write lock
before selection and mutation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ece258e-37e3-4065-85ac-1f84d71cc8ec
📒 Files selected for processing (4)
src/memory/queue/store_settle.rssrc/memory/queue/store_settle_tests.rssrc/memory/store/content/tags.rssrc/memory/store/content/tags_tests.rs
…s (PR tinyhumansai#139 review) - ORDER BY breaks exact completed_at_ms ties by created_at_ms then id, so the "keep newest per dedupe_key" pick is reproducible instead of hinging on a random UUID. Rows sharing a dedupe_key are the same unit of work, so the pick is immaterial to correctness -- no monotonic-sequence migration needed. - Correct the skip-active-key test doc (the occupied-key row stays failed, skipped, not settled) and assert JobStatus::Failed exactly. - Add a test for equal-millisecond completions asserting the one-Ready / one-Cancelled invariant holds regardless of tiebreak. - Refresh the JobStatus::Cancelled doc: requeue supersession is its first real producer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Addressed the review in
Local: |
|
Heads-up on CI: the one red check — Features (sync) — failed at the setup step with a GitHub Actions ISE ( |
The 03e6de7 CI run failed only on GitHub's action-resolution outage ("Service Unavailable" resolving actions/checkout, rust-toolchain, rust-cache); no job compiled. GitHub Actions is Operational again but dropped events can't replay, so this empty commit re-triggers the pipeline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
3 participants
Summary
Requeueing failed Memory Tree jobs aborted whenever two failed rows shared a
dedupe_key. Both the periodic self-heal and the manual "retry failed" path issued one blanketUPDATE ... SET status = 'ready'over every failed row. Two siblings sharing a key then entered the partial unique indexidx_mem_tree_jobs_dedupe_active(which covers onlystatus IN ('ready','running')) at the same instant, SQLite aborted the whole statement withUNIQUE constraint failed: mem_tree_jobs.dedupe_key, and nothing was requeued — on every boot and every ~3h self-heal tick, leaving the queue permanently parked.Requeue is now applied per row inside a single transaction:
dedupe_keyalready held by a live (ready/running) row is skipped — the live row already covers that work, and requeueing onto it is the collision;cancelled(superseded duplicates of the same unit of work) so they stop being counted as failures the user must act on;dedupe_key IS NULLare outside the index and always requeue.Selection and mutation run in one transaction under the shared-connection mutex, so a concurrent claim cannot slip an active row in between the read and the write.
API Or Behavior Changes
requeue_failed/requeue_transient_failed(andretry_all_failed, which delegates) no longer abort on duplicatededupe_keys. The return value is unchanged — the count of rows flipped toready; superseded duplicates are settled ascancelledand are not counted.Tests
3 new tests in
src/memory/queue/store_settle_tests.rs: duplicate-key survival on the manual retry path, the same on the self-heal path, and the active-key-held skip case (one unrelated row must still requeue in the same batch).cargo test— 99/99 queue tests pass (3 new)cargo fmt --checkcargo clippy --all-targets -- -D warningscargo build --all-targetsDocumentation
Behavior is documented in the
requeue_failed_wheredoc comment (why the requeue is filtered rather than one blanketUPDATE, with the partial-index reasoning). No external docs needed.Summary by CodeRabbit
Bug Fixes
Tests