refactor: extract dead_letter, archival, worker ops into diesel_common#207
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtracts duplicate Diesel database logic into three macros in ChangesStorage Macro Consolidation
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 `@crates/taskito-core/src/storage/diesel_common/dead_letter.rs`:
- Around line 82-87: The code in retry_dead currently maps every DB error from
dead_letter::table.find(...).first(&mut conn) to QueueError::JobNotFound and
later doesn't verify the delete actually removed a row, which causes races and
masks real DB errors; change the first() call to match the diesel error: if
Err(diesel::result::Error::NotFound) return
QueueError::JobNotFound(dead_id.to_string()), otherwise propagate or wrap other
diesel errors as a DB error (e.g., QueueError::Db or return Err(err.into())).
For the delete step (the diesel::delete(...).execute(&mut conn) call around
where DeadLetterRow is removed), check the returned affected rows count and if
it is 0 return QueueError::JobNotFound(dead_id.to_string()), and if execute
returns any other DB error propagate/wrap it instead of treating it as
not-found; apply the same pattern to the other similar block around lines
129-136.
In `@crates/taskito-core/src/storage/diesel_common/workers.rs`:
- Around line 53-60: The current logic selects dead_ids from workers::table and
then deletes rows by worker_id alone, which can remove workers that updated
their heartbeat after the select; change the delete to also require
last_heartbeat < cutoff so the removal only affects rows still stale (e.g., use
diesel::delete(workers::table.filter(workers::worker_id.eq_any(&dead_ids).and(workers::last_heartbeat.lt(cutoff))))
or equivalently include the last_heartbeat value with each selected id and
delete by matching both worker_id and that timestamp), keeping the symbols
workers::table, workers::worker_id, workers::last_heartbeat and the cutoff
variable referenced to ensure recovered workers are not deleted.
🪄 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
Run ID: f27fe902-0f1f-4b66-8efd-b084664b8d84
📒 Files selected for processing (10)
crates/taskito-core/src/storage/diesel_common/archival.rscrates/taskito-core/src/storage/diesel_common/dead_letter.rscrates/taskito-core/src/storage/diesel_common/mod.rscrates/taskito-core/src/storage/diesel_common/workers.rscrates/taskito-core/src/storage/postgres/archival.rscrates/taskito-core/src/storage/postgres/dead_letter.rscrates/taskito-core/src/storage/postgres/workers.rscrates/taskito-core/src/storage/sqlite/archival.rscrates/taskito-core/src/storage/sqlite/dead_letter.rscrates/taskito-core/src/storage/sqlite/workers.rs
retry_dead previously masked every Diesel error as JobNotFound and did not verify the dead-letter delete affected a row. Two concurrent retries could both commit a fresh job for the same dead id. Match on diesel::result::Error::NotFound explicitly, propagate other errors as Storage, and assert the in-transaction delete removed exactly one row — otherwise abort the transaction so the freshly inserted job rolls back.
reap_dead_workers scanned for stale workers, then deleted by worker_id alone. A worker that heartbeated between the scan and the delete was still erased, deregistering a healthy fleet member. Apply the last_heartbeat < cutoff predicate to the DELETE as well so only workers that are still stale at delete time are removed.
Summary
impl_diesel_dead_letter_ops!macro (100% identical between SQLite/Postgres)list_archivedintoimpl_diesel_archival_ops!macro (archive_old_jobsstays backend-specific due to INSERT OR IGNORE vs ON CONFLICT DO NOTHING)impl_diesel_worker_ops!macro (register_workerstays backend-specific due to replace_into vs on_conflict.do_update)Eliminates ~250 LOC of duplication across 3 file pairs, extending the existing
diesel_common/macro pattern (jobs, locks, logs, metrics).Test plan
cargo test --workspacepassescargo check --workspace --features postgrescompilescargo clippy --workspacecleanSummary by CodeRabbit