fix: dedup uniqueKey entries in Node enqueueMany - #541
Conversation
Java carried its own keyed/keyless split; move it to taskito-core so the Node binding can reuse it, and run keyed rows through enqueue_unique_batch in one transaction instead of one per job.
The batch path went straight to enqueue_batch, so a key colliding with an active job hit the partial unique index and failed the whole batch with a raw storage error.
|
Warning Review limit reached
Next review available in: 38 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughBatch enqueue now deduplicates jobs with ChangesBatch deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NodeQueue
participant enqueue_batch_dedup
participant StorageBackend
NodeQueue->>enqueue_batch_dedup: enqueueMany batch
enqueue_batch_dedup->>StorageBackend: enqueue_unique_batch keyed entries
enqueue_batch_dedup->>StorageBackend: enqueue_batch keyless entries
StorageBackend-->>enqueue_batch_dedup: return Job results
enqueue_batch_dedup-->>NodeQueue: return IDs in input order
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/taskito-core/src/storage/mod.rs`:
- Around line 141-146: Update the documentation comment above
Storage::enqueue_unique_batch to limit the SDK routing claim to the Java and
Node SDKs. Keep the descriptions of keyed and unkeyed entries and the partial
unique index behavior unchanged, while explicitly excluding Python from the
claim.
- Around line 163-172: Make enqueueMany’s keyed and plain partition handling
atomic: replace the separate enqueue_unique_batch and enqueue_batch commits in
the mixed-batch path with one backend transaction or transactional mixed-batch
primitive that rolls back both partitions if either insert fails. Preserve
scatter’s created-job mapping, and add a regression test proving a plain-route
failure leaves keyed jobs unpersisted.
In `@docs/content/docs/java/api-reference/queue/index.mdx`:
- Line 53: Update the enqueueMany API reference entry to describe mixed batches
as batched enqueue without promising a single storage call. Retain the
guarantees about returned IDs preserving input order and uniqueKey entries being
deduplicated.
In `@sdks/node/src/queue.ts`:
- Around line 553-555: Update the documentation for the bulk enqueue method near
the typed args/options description to say it returns job IDs in input order,
removing the inaccurate “new” qualifier while preserving the existing uniqueKey
deduplication behavior.
🪄 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: d2bf1fac-90ed-449e-8fb6-fa84dce91b4f
📒 Files selected for processing (12)
crates/taskito-core/src/storage/mod.rscrates/taskito-core/tests/rust/storage_tests.rscrates/taskito-java/src/backend.rscrates/taskito-node/src/queue/mod.rsdocs/content/docs/java/api-reference/queue/index.mdxdocs/content/docs/java/more/examples/bulk-emails.mdxdocs/content/docs/node/api-reference/queue/index.mdxdocs/content/docs/node/guides/core/enqueue-options.mdxdocs/content/docs/node/more/examples/bulk-emails.mdxdocs/content/docs/shared/guides/reliability/idempotency.mdxsdks/node/src/queue.tssdks/node/test/core/batch.test.ts
Splitting keyed and keyless rows across two storage calls left keyed jobs persisted when the second call failed. enqueue_unique_batch already passes keyless rows through, so one keyed entry routes the whole batch there.
Redis loops enqueue_unique_batch per row, so a mixed batch is only all-or-nothing on SQLite/Postgres. The shared suite now asserts just the dependency validation both share.
|
CI was red on My fault, and it exposed a real overclaim in the atomicity fix. Redis's Fixed in 8ce120a:
Verified against a local |
Fixes #520.
Problem
enqueueManysent every job straight toStorage::enqueue_batch, a rawmulti-row INSERT with no dedup check. The partial unique index
idx_jobs_unique_key(unique_key IS NOT NULL AND status IN (pending, running))then rejected the insert, so a colliding
uniqueKeydid not silently duplicate —it failed the whole batch with a raw storage error:
Single
enqueuehas always deduped, and Java's batch path did too, so this wasalso a cross-SDK behavioural gap.
Fix
New shared helper
taskito_core::storage::enqueue_batch_dedup. It splits abatch by keyed-ness, preserving input order: entries with a
unique_keygothrough
enqueue_unique_batch(one transaction, returning the existing activejob on a collision), entries without one keep the plain
enqueue_batchfastpath — so a batch that doesn't dedupe costs exactly what it did before.
Java carried its own copy of this split, doing one
enqueue_uniquetransactionper keyed job; it now delegates to the core helper and gets the single-transaction
keyed path for free.
Tests
test_enqueue_batch_dedupin the backend-agnostic storage suite: mixed batchcovering a collision with an already-active job, a key repeated inside the
batch, and keyless rows.
test/core/batch.test.ts: dedup with input-order ids, andjob.enqueuedemitted once per entry (matching singleenqueue).Docs
Five pages claimed
enqueueManyapplies nouniqueKeydedup. The two Javapages carried the same claim and were already wrong before this change.
Note for a follow-up
Python has the same defect.
Queue.enqueue_manyresolves a per-row unique keyand then hands the batch to plain
enqueue_batch, so a duplicate raisesRuntimeError: storage error: UNIQUE constraint failed: jobs.unique_key.Issue #520 lists Python as already correct; it is not. The Python docs describe
the current behaviour accurately, so this is a deliberate-looking gap rather
than a silent one — left out of this PR, but the new core helper reduces the fix
to routing
PyQueue::enqueue_batchthrough it.Summary by CodeRabbit
New Features
enqueueManynow deduplicates entries with matchinguniqueKeyvalues, including duplicates already pending or running.uniqueKeycontinue using efficient bulk insertion.Documentation