From 6edd0cc655bc0fa2de9201ca33b8bd1cee074bf4 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 18:55:09 +0530 Subject: [PATCH 01/16] perf(storage): narrow projection for job and DLQ listings List/archive/DLQ paths selected full payload/result blobs per row. Select a blob-free narrow projection instead (Redis strips post-load); blobs load only via get_job. Uniform blob-free listing contract across backends. --- crates/taskito-core/src/job.rs | 35 ++++++++++- .../src/storage/diesel_common/archival.rs | 8 ++- .../src/storage/diesel_common/dead_letter.rs | 22 ++++--- .../src/storage/diesel_common/jobs.rs | 20 ++++-- crates/taskito-core/src/storage/mod.rs | 26 ++++++++ crates/taskito-core/src/storage/models.rs | 54 ++++++++++++++++ .../src/storage/postgres/archival.rs | 2 +- .../src/storage/redis_backend/archival.rs | 5 +- .../src/storage/redis_backend/dead_letter.rs | 14 +++-- .../src/storage/redis_backend/jobs/query.rs | 8 ++- .../src/storage/redis_backend/mod.rs | 15 +++++ .../src/storage/sqlite/archival.rs | 2 +- crates/taskito-core/src/storage/traits.rs | 5 ++ .../taskito-core/tests/rust/storage_tests.rs | 62 +++++++++++++++++-- 14 files changed, 242 insertions(+), 36 deletions(-) diff --git a/crates/taskito-core/src/job.rs b/crates/taskito-core/src/job.rs index cc94fb06..cbeacce9 100644 --- a/crates/taskito-core/src/job.rs +++ b/crates/taskito-core/src/job.rs @@ -2,7 +2,7 @@ use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use uuid::Uuid; -use crate::storage::models::{ArchivedJobRow, JobRow, NarrowJobRow}; +use crate::storage::models::{ArchivedJobRow, JobRow, NarrowArchivedJobRow, NarrowJobRow}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[repr(i32)] @@ -190,6 +190,39 @@ impl Job { has_deps: narrow.has_deps, } } + + /// Assemble a terminal [`Job`] from a blob-free [`NarrowArchivedJobRow`]. + /// Listing paths use this so paging the archive never loads `payload`/ + /// `result`; both come back empty (fetch the full job via `get_job`). + /// Archived jobs are terminal and never re-dequeued, so `has_deps` is false. + pub fn from_narrow_archived(narrow: NarrowArchivedJobRow) -> Self { + Self { + id: narrow.id, + queue: narrow.queue, + task_name: narrow.task_name, + payload: Vec::new(), + status: JobStatus::from_i32(narrow.status).unwrap_or(JobStatus::Pending), + priority: narrow.priority, + created_at: narrow.created_at, + scheduled_at: narrow.scheduled_at, + started_at: narrow.started_at, + completed_at: narrow.completed_at, + retry_count: narrow.retry_count, + max_retries: narrow.max_retries, + result: None, + error: narrow.error, + timeout_ms: narrow.timeout_ms, + unique_key: narrow.unique_key, + progress: narrow.progress, + metadata: narrow.metadata, + notes: narrow.notes, + cancel_requested: narrow.cancel_requested != 0, + expires_at: narrow.expires_at, + result_ttl_ms: narrow.result_ttl_ms, + namespace: narrow.namespace, + has_deps: false, + } + } } /// A successful job outcome to persist. Batches the three writes the success diff --git a/crates/taskito-core/src/storage/diesel_common/archival.rs b/crates/taskito-core/src/storage/diesel_common/archival.rs index 58707e40..baedead8 100644 --- a/crates/taskito-core/src/storage/diesel_common/archival.rs +++ b/crates/taskito-core/src/storage/diesel_common/archival.rs @@ -9,14 +9,16 @@ macro_rules! impl_diesel_archival_ops { pub fn list_archived(&self, limit: i64, offset: i64) -> Result> { let mut conn = self.conn()?; - let rows: Vec = archived_jobs::table + // Narrow projection: archive listings never render the + // arg/result blobs, so leave them on TOAST/overflow pages. + let rows: Vec = archived_jobs::table .order(archived_jobs::completed_at.desc()) .limit(limit) .offset(offset) - .select(ArchivedJobRow::as_select()) + .select(NarrowArchivedJobRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(Job::from).collect()) + Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) } } }; diff --git a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs index 876639f9..30dfe712 100644 --- a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs +++ b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs @@ -90,14 +90,15 @@ macro_rules! impl_diesel_dead_letter_ops { pub fn list_dead(&self, limit: i64, offset: i64) -> Result> { let mut conn = self.conn()?; - let rows: Vec = dead_letter::table + // Narrow projection: DLQ listings never render the arg blob. + let rows: Vec = dead_letter::table .order(dead_letter::failed_at.desc()) .limit(limit) .offset(offset) - .select(DeadLetterRow::as_select()) + .select(NarrowDeadLetterRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(DeadJob::from).collect()) + Ok(rows.into_iter().map(DeadJob::from_narrow).collect()) } /// List dead letter entries for a single task, newest first. @@ -117,15 +118,15 @@ macro_rules! impl_diesel_dead_letter_ops { let mut conn = self.conn()?; - let rows: Vec = dead_letter::table + let rows: Vec = dead_letter::table .filter(dead_letter::task_name.eq(task_name)) .order(dead_letter::failed_at.desc()) .limit(limit) .offset(offset) - .select(DeadLetterRow::as_select()) + .select(NarrowDeadLetterRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(DeadJob::from).collect()) + Ok(rows.into_iter().map(DeadJob::from_narrow).collect()) } /// Delete every dead letter entry for a task. Returns the count removed. @@ -321,13 +322,16 @@ macro_rules! impl_diesel_dead_letter_ops { None => query = query.filter(dead_letter::namespace.is_null()), } - let rows: Vec = query + // Narrow projection: the auto-retry loop reads only `id`/ + // `dlq_retry_count`; `retry_dead(id)` re-reads the full row to + // rebuild the payload, so listing never needs the arg blob. + let rows: Vec = query .order(dead_letter::failed_at.asc()) .limit(limit) - .select(DeadLetterRow::as_select()) + .select(NarrowDeadLetterRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(DeadJob::from).collect()) + Ok(rows.into_iter().map(DeadJob::from_narrow).collect()) } } }; diff --git a/crates/taskito-core/src/storage/diesel_common/jobs.rs b/crates/taskito-core/src/storage/diesel_common/jobs.rs index 2f82b517..2498991c 100644 --- a/crates/taskito-core/src/storage/diesel_common/jobs.rs +++ b/crates/taskito-core/src/storage/diesel_common/jobs.rs @@ -1348,13 +1348,19 @@ macro_rules! impl_diesel_job_ops { query = query.filter(jobs::namespace.eq(ns)); } - let rows: Vec = query + // Listings never render the arg/result blobs, so select the + // narrow projection: the blobs stay on SQLite overflow pages / + // Postgres TOAST and are read only by a `get_job` detail lookup. + let rows: Vec = query .limit(limit) .offset(offset) - .select(JobRow::as_select()) + .select(NarrowJobRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(Job::from).collect()) + Ok(rows + .into_iter() + .map(|r| Job::from_narrow(r, Vec::new(), None)) + .collect()) } /// Query the `archived_jobs` table with the shared filter set. @@ -1403,13 +1409,15 @@ macro_rules! impl_diesel_job_ops { query = query.filter(archived_jobs::namespace.eq(ns)); } - let rows: Vec = query + // Narrow projection: terminal listings never render blobs, so + // leave `payload`/`result` on TOAST/overflow pages. + let rows: Vec = query .limit(limit) .offset(offset) - .select(ArchivedJobRow::as_select()) + .select(NarrowArchivedJobRow::as_select()) .load(&mut conn)?; - Ok(rows.into_iter().map(Job::from).collect()) + Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) } /// Purge completed jobs older than the given timestamp. Terminal diff --git a/crates/taskito-core/src/storage/mod.rs b/crates/taskito-core/src/storage/mod.rs index aecc38bb..bc0acca9 100644 --- a/crates/taskito-core/src/storage/mod.rs +++ b/crates/taskito-core/src/storage/mod.rs @@ -172,6 +172,32 @@ impl From for DeadJob { } } +impl DeadJob { + /// Build a [`DeadJob`] from a blob-free [`NarrowDeadLetterRow`]. Listing + /// paths use this so paging the DLQ never loads the `payload` blob; it + /// comes back empty and is only read when a single entry is requeued by id. + pub fn from_narrow(row: models::NarrowDeadLetterRow) -> Self { + Self { + id: row.id, + original_job_id: row.original_job_id, + queue: row.queue, + task_name: row.task_name, + payload: Vec::new(), + error: row.error, + retry_count: row.retry_count, + failed_at: row.failed_at, + metadata: row.metadata, + notes: row.notes, + priority: row.priority, + max_retries: row.max_retries, + timeout_ms: row.timeout_ms, + result_ttl_ms: row.result_ttl_ms, + namespace: row.namespace, + dlq_retry_count: row.dlq_retry_count, + } + } +} + // ── impl_storage! macro ─────────────────────────────────────────────── // // Every concrete backend (Sqlite, Postgres, Redis) has inherent methods diff --git a/crates/taskito-core/src/storage/models.rs b/crates/taskito-core/src/storage/models.rs index b5368b39..d14f359d 100644 --- a/crates/taskito-core/src/storage/models.rs +++ b/crates/taskito-core/src/storage/models.rs @@ -121,6 +121,30 @@ pub struct DeadLetterRow { pub dlq_retry_count: i32, } +/// A `dead_letter` row without the `payload` blob. Listing paths select this so +/// paging the DLQ never drags each entry's arg blob off overflow pages/TOAST; +/// the blob is loaded (via the full [`DeadLetterRow`]) only when a single entry +/// is requeued by id. +#[derive(Queryable, Selectable, Debug, Clone)] +#[diesel(table_name = dead_letter)] +pub struct NarrowDeadLetterRow { + pub id: String, + pub original_job_id: String, + pub queue: String, + pub task_name: String, + pub error: Option, + pub retry_count: i32, + pub failed_at: i64, + pub metadata: Option, + pub notes: Option, + pub priority: i32, + pub max_retries: i32, + pub timeout_ms: i64, + pub result_ttl_ms: Option, + pub namespace: Option, + pub dlq_retry_count: i32, +} + /// Insertable struct for dead letter entries. #[derive(Insertable, Debug)] #[diesel(table_name = dead_letter)] @@ -519,6 +543,36 @@ pub struct ArchivedJobRow { pub namespace: Option, } +/// An `archived_jobs` row without the `payload`/`result` blobs. Terminal-status +/// listings select this so paging the archive never reads the arg/result blobs; +/// they are loaded (via the full [`ArchivedJobRow`]) only by a `get_job` detail +/// lookup. Mirrors [`NarrowJobRow`] for the archive table. +#[derive(Queryable, Selectable, Debug, Clone)] +#[diesel(table_name = archived_jobs)] +pub struct NarrowArchivedJobRow { + pub id: String, + pub queue: String, + pub task_name: String, + pub status: i32, + pub priority: i32, + pub created_at: i64, + pub scheduled_at: i64, + pub started_at: Option, + pub completed_at: Option, + pub retry_count: i32, + pub max_retries: i32, + pub error: Option, + pub timeout_ms: i64, + pub unique_key: Option, + pub progress: Option, + pub metadata: Option, + pub notes: Option, + pub cancel_requested: i32, + pub expires_at: Option, + pub result_ttl_ms: Option, + pub namespace: Option, +} + /// Insertable struct for archived job entries. /// /// Mirrors [`ArchivedJobRow`] with borrowed fields. The `archived_jobs` table diff --git a/crates/taskito-core/src/storage/postgres/archival.rs b/crates/taskito-core/src/storage/postgres/archival.rs index a1bf686d..8116d964 100644 --- a/crates/taskito-core/src/storage/postgres/archival.rs +++ b/crates/taskito-core/src/storage/postgres/archival.rs @@ -1,6 +1,6 @@ use diesel::prelude::*; -use super::super::models::ArchivedJobRow; +use super::super::models::NarrowArchivedJobRow; use super::super::schema::archived_jobs; use super::PostgresStorage; use crate::error::Result; diff --git a/crates/taskito-core/src/storage/redis_backend/archival.rs b/crates/taskito-core/src/storage/redis_backend/archival.rs index 6f85b37a..41c7eb8b 100644 --- a/crates/taskito-core/src/storage/redis_backend/archival.rs +++ b/crates/taskito-core/src/storage/redis_backend/archival.rs @@ -1,6 +1,6 @@ use redis::Commands; -use super::{map_err, RedisStorage}; +use super::{map_err, strip_list_blobs, RedisStorage}; use crate::error::{QueueError, Result}; use crate::job::{Job, JobStatus}; @@ -54,8 +54,9 @@ impl RedisStorage { let archived_key = self.key(&["archived", &id]); let data: Option = conn.get(&archived_key).map_err(map_err)?; if let Some(d) = data { - let job: Job = + let mut job: Job = serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; + strip_list_blobs(&mut job); jobs.push(job); } } diff --git a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs index 0c634cfc..e0aa51de 100644 --- a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs +++ b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs @@ -1,7 +1,7 @@ use redis::Commands; use serde::{Deserialize, Serialize}; -use super::{map_err, RedisStorage}; +use super::{map_err, strip_dead_blob, RedisStorage}; use crate::error::{QueueError, Result}; use crate::job::{now_millis, Job, JobStatus, NewJob}; use crate::storage::DeadJob; @@ -150,7 +150,9 @@ impl RedisStorage { if let Some(d) = data { let entry: DeadJobEntry = serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; - results.push(DeadJob::from(entry)); + let mut dead = DeadJob::from(entry); + strip_dead_blob(&mut dead); + results.push(dead); } } @@ -186,7 +188,9 @@ impl RedisStorage { let entry: DeadJobEntry = serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; if entry.task_name == task_name { - matches.push(DeadJob::from(entry)); + let mut dead = DeadJob::from(entry); + strip_dead_blob(&mut dead); + matches.push(dead); // Stop once we have enough matches to satisfy this page. // saturating: offset/limit are public i64 inputs. if matches.len() >= offset.saturating_add(limit) { @@ -436,7 +440,9 @@ impl RedisStorage { && entry.namespace.as_deref() == namespace && queues.iter().any(|q| q == &entry.queue) { - results.push(DeadJob::from(entry)); + let mut dead = DeadJob::from(entry); + strip_dead_blob(&mut dead); + results.push(dead); } } } diff --git a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs index 81796564..953a9f8d 100644 --- a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs +++ b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs @@ -4,7 +4,7 @@ use redis::Commands; use crate::error::Result; use crate::job::{Job, JobStatus}; -use crate::storage::redis_backend::{map_err, RedisStorage}; +use crate::storage::redis_backend::{map_err, strip_list_blobs, RedisStorage}; use crate::storage::QueueStats; impl RedisStorage { @@ -74,7 +74,8 @@ impl RedisStorage { fn load_live_jobs(&self, conn: &mut redis::Connection, ids: &[String]) -> Result> { let mut jobs = Vec::new(); for id in ids { - if let Some(job) = self.load_job(conn, id)? { + if let Some(mut job) = self.load_job(conn, id)? { + strip_list_blobs(&mut job); jobs.push(job); } } @@ -84,7 +85,8 @@ impl RedisStorage { fn load_archived_jobs(&self, conn: &mut redis::Connection, ids: &[String]) -> Result> { let mut jobs = Vec::new(); for id in ids { - if let Some(job) = self.load_archived_job(conn, id)? { + if let Some(mut job) = self.load_archived_job(conn, id)? { + strip_list_blobs(&mut job); jobs.push(job); } } diff --git a/crates/taskito-core/src/storage/redis_backend/mod.rs b/crates/taskito-core/src/storage/redis_backend/mod.rs index 6fa5f17b..4bc95fff 100644 --- a/crates/taskito-core/src/storage/redis_backend/mod.rs +++ b/crates/taskito-core/src/storage/redis_backend/mod.rs @@ -119,3 +119,18 @@ impl crate::storage::notify::StorageNotifier for RedisStorage { fn map_err(e: redis::RedisError) -> QueueError { QueueError::Other(e.to_string()) } + +/// Drop the `payload`/`result` blobs from a job before it enters a listing. +/// Redis loads the whole job JSON in one read, so this saves no I/O; it exists +/// only to match the Diesel backends' narrow-projection contract — list results +/// are blob-free on every backend (fetch the full job via `get_job`). +fn strip_list_blobs(job: &mut crate::job::Job) { + job.payload = Vec::new(); + job.result = None; +} + +/// DLQ analogue of [`strip_list_blobs`]: a dead-letter entry carries only the +/// `payload` blob, dropped from listings (requeue re-reads the entry by id). +fn strip_dead_blob(dead: &mut crate::storage::DeadJob) { + dead.payload = Vec::new(); +} diff --git a/crates/taskito-core/src/storage/sqlite/archival.rs b/crates/taskito-core/src/storage/sqlite/archival.rs index f7d08248..5fd458b3 100644 --- a/crates/taskito-core/src/storage/sqlite/archival.rs +++ b/crates/taskito-core/src/storage/sqlite/archival.rs @@ -1,6 +1,6 @@ use diesel::prelude::*; -use super::super::models::ArchivedJobRow; +use super::super::models::NarrowArchivedJobRow; use super::super::schema::archived_jobs; use super::SqliteStorage; use crate::error::Result; diff --git a/crates/taskito-core/src/storage/traits.rs b/crates/taskito-core/src/storage/traits.rs index dd358811..863dfea6 100644 --- a/crates/taskito-core/src/storage/traits.rs +++ b/crates/taskito-core/src/storage/traits.rs @@ -71,6 +71,11 @@ pub trait Storage: Send + Sync + Clone { fn get_dependencies(&self, job_id: &str) -> Result>; fn get_dependents(&self, job_id: &str) -> Result>; fn update_progress(&self, id: &str, progress: i32) -> Result<()>; + /// List jobs by filter. Rows are **blob-free** on every backend: the + /// `payload`/`result` blobs come back empty (Diesel selects a narrow + /// projection; Redis strips them post-load). Fetch the full job — blobs + /// included — with [`Storage::get_job`]. The same contract holds for every + /// listing method (`list_jobs_filtered`, `list_archived`, `list_dead*`). fn list_jobs( &self, status: Option, diff --git a/crates/taskito-core/tests/rust/storage_tests.rs b/crates/taskito-core/tests/rust/storage_tests.rs index 8c468828..3ba7f01e 100644 --- a/crates/taskito-core/tests/rust/storage_tests.rs +++ b/crates/taskito-core/tests/rust/storage_tests.rs @@ -841,8 +841,9 @@ fn test_payload_roundtrip(s: &impl Storage) { /// A job run to completion is archived: its blobs move into `archived_jobs` and /// the live `jobs` row is removed. `get_job` must still resolve the full payload -/// and result from the archive, and a terminal-status list must carry the -/// blobs. Redis passes trivially. +/// and result from the archive. Listing (S13) returns a blob-free narrow +/// projection: the row is present with its metadata, but `payload`/`result` +/// come back empty on every backend (fetch the full job via `get_job`). fn test_archived_job_payload_resolves(s: &impl Storage) { let q = "q-archived-payload-resolves"; let mut nj = make_job(q, "archived_payload_task"); @@ -852,19 +853,67 @@ fn test_archived_job_payload_resolves(s: &impl Storage) { s.dequeue(q, now_millis() + 1000, None).unwrap(); s.complete(&job.id, Some(vec![0x11, 0x22])).unwrap(); - // The job now lives only in `archived_jobs`; the side-table row is gone. + // Detail lookup: the job now lives only in `archived_jobs`; the side-table + // row is gone, yet `get_job` still resolves the full payload and result. let fetched = s.get_job(&job.id).unwrap().unwrap(); assert_eq!(fetched.status, JobStatus::Complete); assert_eq!(fetched.payload, vec![0xCA, 0xFE, 0xBA, 0xBE]); assert_eq!(fetched.result, Some(vec![0x11, 0x22])); - // Listing by the terminal status reads the archive and still carries blobs. + // Listing by the terminal status reads the archive but drops the blobs: + // the row is there with its non-blob columns, payload/result are empty. let listed = s .list_jobs(Some(JobStatus::Complete as i32), Some(q), None, 50, 0, None) .unwrap(); let row = listed.iter().find(|j| j.id == job.id).unwrap(); - assert_eq!(row.payload, vec![0xCA, 0xFE, 0xBA, 0xBE]); - assert_eq!(row.result, Some(vec![0x11, 0x22])); + assert_eq!(row.task_name, "archived_payload_task"); + assert_eq!(row.status, JobStatus::Complete); + assert!( + row.payload.is_empty(), + "listing must not carry the arg blob" + ); + assert!( + row.result.is_none(), + "listing must not carry the result blob" + ); +} + +/// S13 for the live and DLQ tables: `list_jobs` on a live status and `list_dead` +/// both return blob-free rows, while `get_job` still resolves the full payload. +fn test_listing_is_blob_free(s: &impl Storage) { + // Live path: a pending job lists without its arg blob but resolves in full. + let q = "q-blob-free-listing"; + let mut nj = make_job(q, "blob_free_task"); + nj.payload = vec![0xAB, 0xCD, 0xEF]; + let job = s.enqueue(nj).unwrap(); + + let listed = s + .list_jobs(Some(JobStatus::Pending as i32), Some(q), None, 50, 0, None) + .unwrap(); + let row = listed.iter().find(|j| j.id == job.id).unwrap(); + assert_eq!(row.task_name, "blob_free_task"); + assert!( + row.payload.is_empty(), + "live listing must drop the arg blob" + ); + assert_eq!( + s.get_job(&job.id).unwrap().unwrap().payload, + vec![0xAB, 0xCD, 0xEF], + "get_job must still resolve the full payload" + ); + + // DLQ path: a dead-lettered entry lists without its arg blob. + s.dequeue(q, now_millis() + 1000, None).unwrap(); + let running = s.get_job(&job.id).unwrap().unwrap(); + s.move_to_dlq(&running, "boom", None).unwrap(); + + let dead = s.list_dead(10, 0).unwrap(); + let entry = dead.iter().find(|d| d.original_job_id == job.id).unwrap(); + assert_eq!(entry.task_name, "blob_free_task"); + assert!( + entry.payload.is_empty(), + "DLQ listing must drop the arg blob" + ); } fn due_periodic_names(s: &impl Storage) -> Vec { @@ -1245,6 +1294,7 @@ fn run_storage_tests(s: &impl Storage) { test_dependent_blocked_by_cancelled_parent(s); test_payload_roundtrip(s); test_archived_job_payload_resolves(s); + test_listing_is_blob_free(s); test_concurrent_dequeue_no_double_claim(s); test_rate_limit_token_exhaustion(s); test_task_logs_after_cursor(s); From 59acbae7be3a686a9466f4f65b2c595d403acbbf Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:14:27 +0530 Subject: [PATCH 02/16] perf(storage): chunk mass delete, purge, and cancel loops delete_job_children now chunks the id list (SQLite 999-param limit); purge_completed(_with_ttl) and the cancel/expire-pending paths run in bounded per-txn batches instead of one unbounded BEGIN IMMEDIATE, so mass ops at millions of rows never stall other writers. --- .../src/storage/diesel_common/jobs.rs | 243 +++++++++++------- .../taskito-core/src/storage/sqlite/tests.rs | 42 +++ 2 files changed, 199 insertions(+), 86 deletions(-) diff --git a/crates/taskito-core/src/storage/diesel_common/jobs.rs b/crates/taskito-core/src/storage/diesel_common/jobs.rs index 2498991c..e9d01706 100644 --- a/crates/taskito-core/src/storage/diesel_common/jobs.rs +++ b/crates/taskito-core/src/storage/diesel_common/jobs.rs @@ -5,32 +5,44 @@ macro_rules! impl_diesel_job_ops { ($storage_type:ty, $conn_type:ty) => { impl $storage_type { + /// Max archived rows deleted per txn in the batched purge loops — + /// bounds the SQLite writer-lock hold time and the `IN (...)` bind + /// count so a purge of millions of rows never stalls other writers. + const PURGE_BATCH: i64 = 500; + /// Max pending rows archived per txn in the batched cancel/expire + /// loops — same lock-hold bound for the mass-mutation paths. + const MASS_ARCHIVE_BATCH: i64 = 500; + fn delete_job_children( conn: &mut $conn_type, job_ids: &[String], ) -> diesel::result::QueryResult<()> { - if job_ids.is_empty() { - return Ok(()); - } - - diesel::delete(job_errors::table.filter(job_errors::job_id.eq_any(job_ids))) - .execute(conn)?; - diesel::delete(task_logs::table.filter(task_logs::job_id.eq_any(job_ids))) + // Chunk the id list so the bind count stays under SQLite's 999 + // parameter limit even for a mass purge. The job_dependencies + // delete binds the ids twice (job_id OR depends_on_job_id), so + // cap at 450 → ≤ 900 params per statement. + const DELETE_ID_CHUNK: usize = 450; + + for chunk in job_ids.chunks(DELETE_ID_CHUNK) { + diesel::delete(job_errors::table.filter(job_errors::job_id.eq_any(chunk))) + .execute(conn)?; + diesel::delete(task_logs::table.filter(task_logs::job_id.eq_any(chunk))) + .execute(conn)?; + diesel::delete(task_metrics::table.filter(task_metrics::job_id.eq_any(chunk))) + .execute(conn)?; + diesel::delete( + job_dependencies::table.filter( + job_dependencies::job_id + .eq_any(chunk) + .or(job_dependencies::depends_on_job_id.eq_any(chunk)), + ), + ) .execute(conn)?; - diesel::delete(task_metrics::table.filter(task_metrics::job_id.eq_any(job_ids))) + diesel::delete( + replay_history::table.filter(replay_history::original_job_id.eq_any(chunk)), + ) .execute(conn)?; - diesel::delete( - job_dependencies::table.filter( - job_dependencies::job_id - .eq_any(job_ids) - .or(job_dependencies::depends_on_job_id.eq_any(job_ids)), - ), - ) - .execute(conn)?; - diesel::delete( - replay_history::table.filter(replay_history::original_job_id.eq_any(job_ids)), - ) - .execute(conn)?; + } Ok(()) } @@ -1420,66 +1432,101 @@ macro_rules! impl_diesel_job_ops { Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) } + /// Delete one already-selected batch of archived jobs (their child + /// rows first, then the archive rows) inside the caller's txn. + fn purge_archived_id_batch( + conn: &mut $conn_type, + ids: &[String], + ) -> diesel::result::QueryResult { + if ids.is_empty() { + return Ok(0); + } + Self::delete_job_children(conn, ids)?; + let affected = + diesel::delete(archived_jobs::table.filter(archived_jobs::id.eq_any(ids))) + .execute(conn)?; + Ok(affected as u64) + } + /// Purge completed jobs older than the given timestamp. Terminal /// jobs live in `archived_jobs`, so the purge targets that table. + /// + /// Deletes in bounded batches, each its own `BEGIN IMMEDIATE` txn, + /// so a purge of millions of rows never holds the single writer lock + /// for the whole sweep (SQLite) and never builds an unbounded + /// `IN (...)` list. pub fn purge_completed(&self, older_than_ms: i64) -> Result { - self.write_transaction(|conn| { - let job_ids: Vec = archived_jobs::table - .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) - .filter(archived_jobs::completed_at.lt(older_than_ms)) - .select(archived_jobs::id) - .load(conn)?; - - Self::delete_job_children(conn, &job_ids)?; - - let affected = diesel::delete( - archived_jobs::table.filter(archived_jobs::id.eq_any(&job_ids)), - ) - .execute(conn)?; - - Ok(affected as u64) - }) + let mut total = 0u64; + loop { + let removed = self.write_transaction(|conn| { + let ids: Vec = archived_jobs::table + .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) + .filter(archived_jobs::completed_at.lt(older_than_ms)) + .select(archived_jobs::id) + .limit(Self::PURGE_BATCH) + .load(conn)?; + Ok(Self::purge_archived_id_batch(conn, &ids)?) + })?; + total += removed; + if removed < Self::PURGE_BATCH as u64 { + break; + } + } + Ok(total) } /// Purge completed jobs respecting per-job result_ttl_ms. Terminal /// jobs live in `archived_jobs`, so the purge targets that table. + /// Batched like [`Self::purge_completed`]; the global-TTL and + /// per-job-TTL rows are swept in two independent bounded loops. pub fn purge_completed_with_ttl(&self, global_cutoff_ms: i64) -> Result { let now = now_millis(); + let mut total = 0u64; + + // Rows with no per-job TTL fall back to the global cutoff. + loop { + let removed = self.write_transaction(|conn| { + let ids: Vec = archived_jobs::table + .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) + .filter(archived_jobs::result_ttl_ms.is_null()) + .filter(archived_jobs::completed_at.lt(global_cutoff_ms)) + .select(archived_jobs::id) + .limit(Self::PURGE_BATCH) + .load(conn)?; + Ok(Self::purge_archived_id_batch(conn, &ids)?) + })?; + total += removed; + if removed < Self::PURGE_BATCH as u64 { + break; + } + } - self.write_transaction(|conn| { - let global_ids: Vec = archived_jobs::table - .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) - .filter(archived_jobs::result_ttl_ms.is_null()) - .filter(archived_jobs::completed_at.lt(global_cutoff_ms)) - .select(archived_jobs::id) - .load(conn)?; - - // Push the per-job `completed_at + result_ttl_ms < now` - // check into SQL and select only the id — avoids loading - // full rows (payload + result blobs) just to filter them. - let per_job_ids: Vec = archived_jobs::table - .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) - .filter(archived_jobs::result_ttl_ms.is_not_null()) - .filter(archived_jobs::completed_at.is_not_null()) - .filter( - (archived_jobs::completed_at.assume_not_null() - + archived_jobs::result_ttl_ms.assume_not_null()) - .lt(now), - ) - .select(archived_jobs::id) - .load(conn)?; - - let all_ids: Vec = global_ids.into_iter().chain(per_job_ids).collect(); - - Self::delete_job_children(conn, &all_ids)?; - - let affected = diesel::delete( - archived_jobs::table.filter(archived_jobs::id.eq_any(&all_ids)), - ) - .execute(conn)?; + // Rows with a per-job TTL: `completed_at + result_ttl_ms < now`. + // The check is pushed into SQL, selecting only the id so no + // payload/result blob is loaded just to filter it. + loop { + let removed = self.write_transaction(|conn| { + let ids: Vec = archived_jobs::table + .filter(archived_jobs::status.eq(JobStatus::Complete as i32)) + .filter(archived_jobs::result_ttl_ms.is_not_null()) + .filter(archived_jobs::completed_at.is_not_null()) + .filter( + (archived_jobs::completed_at.assume_not_null() + + archived_jobs::result_ttl_ms.assume_not_null()) + .lt(now), + ) + .select(archived_jobs::id) + .limit(Self::PURGE_BATCH) + .load(conn)?; + Ok(Self::purge_archived_id_batch(conn, &ids)?) + })?; + total += removed; + if removed < Self::PURGE_BATCH as u64 { + break; + } + } - Ok(affected as u64) - }) + Ok(total) } /// Find stale running jobs that exceeded their timeout. @@ -1606,47 +1653,71 @@ macro_rules! impl_diesel_job_ops { Ok(count) } + /// Archive matching pending jobs in bounded batches. `select_batch` + /// loads up to `limit` pending rows to cancel; each batch runs in + /// its own txn, so cancelling a huge pending backlog never holds the + /// SQLite writer lock (or the full row set in memory) at once. The + /// archive removes each row from `jobs`, so the same filter drains + /// toward empty across iterations. + fn archive_pending_in_batches( + &self, + now: i64, + error: &str, + select_batch: S, + ) -> Result + where + S: Fn(&mut $conn_type, i64) -> diesel::result::QueryResult>, + { + let mut total = 0u64; + loop { + let archived = self.write_transaction(|conn| { + let rows = select_batch(conn, Self::MASS_ARCHIVE_BATCH)?; + Ok(Self::archive_pending_rows(conn, rows, now, error)?) + })?; + total += archived; + if archived < Self::MASS_ARCHIVE_BATCH as u64 { + break; + } + } + Ok(total) + } + /// Expire pending jobs that have passed their expires_at. pub fn expire_pending_jobs(&self, now: i64) -> Result { - self.write_transaction(|conn| { - let rows: Vec = jobs::table + self.archive_pending_in_batches(now, "expired", |conn, limit| { + jobs::table .filter(jobs::status.eq(JobStatus::Pending as i32)) .filter(jobs::expires_at.is_not_null()) .filter(jobs::expires_at.lt(now)) .select(JobRow::as_select()) - .load(conn)?; - - Ok(Self::archive_pending_rows(conn, rows, now, "expired")?) + .limit(limit) + .load(conn) }) } /// Cancel all pending jobs in a specific queue. pub fn cancel_pending_by_queue(&self, queue: &str) -> Result { let now = now_millis(); - - self.write_transaction(|conn| { - let rows: Vec = jobs::table + self.archive_pending_in_batches(now, "purged", |conn, limit| { + jobs::table .filter(jobs::status.eq(JobStatus::Pending as i32)) .filter(jobs::queue.eq(queue)) .select(JobRow::as_select()) - .load(conn)?; - - Ok(Self::archive_pending_rows(conn, rows, now, "purged")?) + .limit(limit) + .load(conn) }) } /// Cancel all pending jobs for a specific task name. pub fn cancel_pending_by_task(&self, task_name: &str) -> Result { let now = now_millis(); - - self.write_transaction(|conn| { - let rows: Vec = jobs::table + self.archive_pending_in_batches(now, "revoked", |conn, limit| { + jobs::table .filter(jobs::status.eq(JobStatus::Pending as i32)) .filter(jobs::task_name.eq(task_name)) .select(JobRow::as_select()) - .load(conn)?; - - Ok(Self::archive_pending_rows(conn, rows, now, "revoked")?) + .limit(limit) + .load(conn) }) } diff --git a/crates/taskito-core/src/storage/sqlite/tests.rs b/crates/taskito-core/src/storage/sqlite/tests.rs index 975093fa..225ff562 100644 --- a/crates/taskito-core/src/storage/sqlite/tests.rs +++ b/crates/taskito-core/src/storage/sqlite/tests.rs @@ -898,6 +898,48 @@ fn test_purge_completed_respects_per_job_ttl() { assert!(storage.get_job(&kept.id).unwrap().is_some()); } +#[test] +fn test_purge_completed_drains_across_batches() { + // 550 completed rows exceed one PURGE_BATCH (500): the batched purge loop + // must drain every row across iterations, not stop after the first batch. + let storage = test_storage(); + let now = now_millis(); + for _ in 0..550 { + storage.enqueue(make_job("purge_batch")).unwrap(); + } + for _ in 0..550 { + let job = storage + .dequeue("default", now + 1000, None) + .unwrap() + .unwrap(); + storage.complete(&job.id, None).unwrap(); + } + + let removed = storage.purge_completed(now_millis() + 10_000).unwrap(); + assert_eq!(removed, 550, "batched purge must drain every completed row"); + assert!(storage.list_archived(1000, 0).unwrap().is_empty()); +} + +#[test] +fn test_cancel_pending_by_queue_drains_across_batches() { + // 550 pending rows exceed one MASS_ARCHIVE_BATCH (500): the batched cancel + // loop must archive every pending row across iterations. + let storage = test_storage(); + for _ in 0..550 { + storage.enqueue(make_job("cancel_batch")).unwrap(); + } + + let cancelled = storage.cancel_pending_by_queue("default").unwrap(); + assert_eq!( + cancelled, 550, + "batched cancel must drain every pending row" + ); + assert!(storage + .list_jobs(Some(JobStatus::Pending as i32), None, None, 1000, 0, None) + .unwrap() + .is_empty()); +} + // ── Immediate terminal-job archival ────────────────────────────────── /// Count rows in the live `jobs` table for a given id. From 710ba8eef1d0f2877386eb5d9f17514d119276c7 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:34:41 +0530 Subject: [PATCH 03/16] perf(redis): bound history purges and log queries with batched scans Redis purge_completed(_with_ttl), purge_dead(_with_ttl), and purge_task_logs loaded the entire target set before deleting; rewrite them as SSCAN/ZSCAN/ZRANGEBYSCORE-LIMIT batches so a sweep over millions of rows stays memory-bounded. query_task_logs gains a server-side ZREVRANGEBYSCORE+LIMIT fast path when unfiltered. --- .../src/storage/redis_backend/dead_letter.rs | 141 +++++++++++------- .../storage/redis_backend/jobs/maintenance.rs | 92 +++++++----- .../src/storage/redis_backend/logs.rs | 82 +++++++--- .../src/storage/redis_backend/mod.rs | 5 + .../taskito-core/tests/rust/storage_tests.rs | 24 +++ 5 files changed, 224 insertions(+), 120 deletions(-) diff --git a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs index e0aa51de..4d09e2a6 100644 --- a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs +++ b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs @@ -1,7 +1,7 @@ use redis::Commands; use serde::{Deserialize, Serialize}; -use super::{map_err, strip_dead_blob, RedisStorage}; +use super::{map_err, strip_dead_blob, RedisStorage, SCAN_BATCH}; use crate::error::{QueueError, Result}; use crate::job::{now_millis, Job, JobStatus, NewJob}; use crate::storage::DeadJob; @@ -312,38 +312,48 @@ impl RedisStorage { pub fn purge_dead(&self, older_than_ms: i64) -> Result { let mut conn = self.conn()?; let dlq_all = self.key(&["dlq", "all"]); + let mut total = 0u64; + + // Every id in the `-inf..=cutoff` score window is eligible, and each + // batch is deleted before the next query, so re-reading the window + // drains the expired rows in bounded chunks instead of loading the + // entire below-cutoff set at once. + loop { + let ids: Vec = conn + .zrangebyscore_limit(&dlq_all, "-inf", older_than_ms as f64, 0, SCAN_BATCH) + .map_err(map_err)?; + if ids.is_empty() { + break; + } - // Get all DLQ IDs with scores <= older_than_ms - let ids: Vec = conn - .zrangebyscore(&dlq_all, "-inf", older_than_ms as f64) - .map_err(map_err)?; - - if ids.is_empty() { - return Ok(0); - } - - // Load blobs to attribute each dead row to its subscription so the - // sub:dead index shrinks with the purge (no-op for non-pub/sub rows). - let blob_keys: Vec = ids.iter().map(|id| self.key(&["dlq", id])).collect(); - let blobs: Vec> = conn.mget(&blob_keys).map_err(map_err)?; - - let pipe = &mut redis::pipe(); - for (id, blob) in ids.iter().zip(&blobs) { - pipe.del(self.key(&["dlq", id])); - pipe.zrem(&dlq_all, id.as_str()); - if let Some(d) = blob { - if let Ok(entry) = serde_json::from_str::(d) { - self.push_pubsub_dead_remove( - pipe, - entry.notes.as_deref(), - &entry.original_job_id, - ); + // Load blobs to attribute each dead row to its subscription so the + // sub:dead index shrinks with the purge (no-op for non-pub/sub rows). + let blob_keys: Vec = ids.iter().map(|id| self.key(&["dlq", id])).collect(); + let blobs: Vec> = conn.mget(&blob_keys).map_err(map_err)?; + + let pipe = &mut redis::pipe(); + for (id, blob) in ids.iter().zip(&blobs) { + pipe.del(self.key(&["dlq", id])); + pipe.zrem(&dlq_all, id.as_str()); + if let Some(d) = blob { + if let Ok(entry) = serde_json::from_str::(d) { + self.push_pubsub_dead_remove( + pipe, + entry.notes.as_deref(), + &entry.original_job_id, + ); + } } } + pipe.query::<()>(&mut conn).map_err(map_err)?; + + total += ids.len() as u64; + if (ids.len() as isize) < SCAN_BATCH { + break; + } } - pipe.query::<()>(&mut conn).map_err(map_err)?; - Ok(ids.len() as u64) + Ok(total) } pub fn delete_dead(&self, dead_id: &str) -> Result { @@ -372,42 +382,57 @@ impl RedisStorage { let mut conn = self.conn()?; let dlq_all = self.key(&["dlq", "all"]); let now = now_millis(); - - let ids: Vec = conn - .zrangebyscore(&dlq_all, "-inf", "+inf") - .map_err(map_err)?; - - // (dlq_id, notes, original_job_id) — keeps the sub:dead index in step - // with the TTL purge (no-op for non-pub/sub rows). - let mut to_delete: Vec<(String, Option, String)> = Vec::new(); - for id in &ids { - let dlq_key = self.key(&["dlq", id]); - let data: Option = conn.get(&dlq_key).map_err(map_err)?; - if let Some(d) = data { - if let Ok(entry) = serde_json::from_str::(&d) { - let expired = match entry.result_ttl_ms { - Some(ttl) => entry.failed_at + ttl <= now, - None => entry.failed_at < global_cutoff_ms, - }; - if expired { - to_delete.push((id.clone(), entry.notes, entry.original_job_id)); + let mut total = 0u64; + + // Per-entry TTL lives in the blob, so every row must be inspected — but + // ZSCAN walks the set in bounded batches instead of loading every id at + // once. Each batch's expired rows are deleted before the next step. + let mut cursor: u64 = 0; + loop { + let (next, flat): (u64, Vec) = redis::cmd("ZSCAN") + .arg(&dlq_all) + .arg(cursor) + .arg("COUNT") + .arg(SCAN_BATCH) + .query(&mut conn) + .map_err(map_err)?; + cursor = next; + + // ZSCAN returns a flat [member, score, member, score, ...] list. + let mut to_delete: Vec<(String, Option, String)> = Vec::new(); + for id in flat.iter().step_by(2) { + let dlq_key = self.key(&["dlq", id]); + let data: Option = conn.get(&dlq_key).map_err(map_err)?; + if let Some(d) = data { + if let Ok(entry) = serde_json::from_str::(&d) { + let expired = match entry.result_ttl_ms { + Some(ttl) => entry.failed_at + ttl <= now, + None => entry.failed_at < global_cutoff_ms, + }; + if expired { + to_delete.push((id.clone(), entry.notes, entry.original_job_id)); + } } } } - } - if to_delete.is_empty() { - return Ok(0); - } + if !to_delete.is_empty() { + let pipe = &mut redis::pipe(); + for (id, notes, member) in &to_delete { + pipe.del(self.key(&["dlq", id])); + pipe.zrem(&dlq_all, id.as_str()); + self.push_pubsub_dead_remove(pipe, notes.as_deref(), member); + } + pipe.query::<()>(&mut conn).map_err(map_err)?; + total += to_delete.len() as u64; + } - let pipe = &mut redis::pipe(); - for (id, notes, member) in &to_delete { - pipe.del(self.key(&["dlq", id])); - pipe.zrem(&dlq_all, id.as_str()); - self.push_pubsub_dead_remove(pipe, notes.as_deref(), member); + if cursor == 0 { + break; + } } - pipe.query::<()>(&mut conn).map_err(map_err)?; - Ok(to_delete.len() as u64) + + Ok(total) } pub fn list_dead_for_retry( diff --git a/crates/taskito-core/src/storage/redis_backend/jobs/maintenance.rs b/crates/taskito-core/src/storage/redis_backend/jobs/maintenance.rs index 45eb6223..2611891f 100644 --- a/crates/taskito-core/src/storage/redis_backend/jobs/maintenance.rs +++ b/crates/taskito-core/src/storage/redis_backend/jobs/maintenance.rs @@ -5,60 +5,72 @@ use redis::Commands; use crate::error::Result; use crate::job::{now_millis, Job, JobStatus}; -use crate::storage::redis_backend::{map_err, RedisStorage}; +use crate::storage::redis_backend::{map_err, RedisStorage, SCAN_BATCH}; impl RedisStorage { - pub fn purge_completed(&self, older_than_ms: i64) -> Result { - let mut conn = self.conn()?; - // Completed jobs are terminal and live in the archive. - let status_key = self.key(&[ + /// Key of the completed-archive index SET (`archived:status:2`). + fn completed_archive_key(&self) -> String { + self.key(&[ "archived", "status", &(JobStatus::Complete as i32).to_string(), - ]); - let job_ids: Vec = conn.smembers(&status_key).map_err(map_err)?; - - let mut count = 0u64; - for id in &job_ids { - if let Some(job) = self.load_archived_job(&mut conn, id)? { - if let Some(completed_at) = job.completed_at { - if completed_at < older_than_ms { - self.delete_archived_job(&mut conn, &job)?; - count += 1; - } - } - } - } + ]) + } - Ok(count) + pub fn purge_completed(&self, older_than_ms: i64) -> Result { + self.purge_completed_scan(|job| { + job.completed_at + .is_some_and(|completed| completed < older_than_ms) + }) } pub fn purge_completed_with_ttl(&self, global_cutoff_ms: i64) -> Result { let now = now_millis(); - let mut conn = self.conn()?; - // Completed jobs are terminal and live in the archive. - let status_key = self.key(&[ - "archived", - "status", - &(JobStatus::Complete as i32).to_string(), - ]); - let job_ids: Vec = conn.smembers(&status_key).map_err(map_err)?; + self.purge_completed_scan(|job| match (job.completed_at, job.result_ttl_ms) { + (Some(completed), Some(ttl)) => completed + .checked_add(ttl) + .is_some_and(|expiry| expiry < now), + (Some(completed), None) => completed < global_cutoff_ms, + _ => false, + }) + } + /// Walk the completed-archive index in bounded SSCAN batches, deleting + /// every job the `should_purge` predicate accepts. SSCAN bounds memory to + /// one batch of ids (plus one loaded job at a time) rather than loading the + /// whole completed set, and tolerates the concurrent deletes so nothing + /// still-eligible is skipped. + fn purge_completed_scan(&self, should_purge: F) -> Result + where + F: Fn(&Job) -> bool, + { + let mut conn = self.conn()?; + let status_key = self.completed_archive_key(); + let mut cursor: u64 = 0; let mut count = 0u64; - for id in &job_ids { - if let Some(job) = self.load_archived_job(&mut conn, id)? { - let should_purge = match (job.completed_at, job.result_ttl_ms) { - (Some(completed), Some(ttl)) => completed - .checked_add(ttl) - .is_some_and(|expiry| expiry < now), - (Some(completed), None) => completed < global_cutoff_ms, - _ => false, - }; - if should_purge { - self.delete_archived_job(&mut conn, &job)?; - count += 1; + + loop { + let (next, ids): (u64, Vec) = redis::cmd("SSCAN") + .arg(&status_key) + .arg(cursor) + .arg("COUNT") + .arg(SCAN_BATCH) + .query(&mut conn) + .map_err(map_err)?; + cursor = next; + + for id in &ids { + if let Some(job) = self.load_archived_job(&mut conn, id)? { + if should_purge(&job) { + self.delete_archived_job(&mut conn, &job)?; + count += 1; + } } } + + if cursor == 0 { + break; + } } Ok(count) diff --git a/crates/taskito-core/src/storage/redis_backend/logs.rs b/crates/taskito-core/src/storage/redis_backend/logs.rs index 3491484d..ac938a1a 100644 --- a/crates/taskito-core/src/storage/redis_backend/logs.rs +++ b/crates/taskito-core/src/storage/redis_backend/logs.rs @@ -1,7 +1,7 @@ use redis::Commands; use serde::{Deserialize, Serialize}; -use super::{map_err, RedisStorage}; +use super::{map_err, RedisStorage, SCAN_BATCH}; use crate::error::{QueueError, Result}; use crate::job::now_millis; use crate::storage::models::TaskLogRow; @@ -137,6 +137,34 @@ impl RedisStorage { let mut conn = self.conn()?; let all_key = self.key(&["logs", "all"]); + // Fast path: with no task/level filter the page is exactly the newest + // `limit` ids at or after `since_ms`, so bound the fetch server-side + // (`logs:all` is scored by logged_at) instead of loading every id since + // the cutoff and truncating in memory. + if task_name.is_none() && level.is_none() { + let ids: Vec = if limit >= 0 { + conn.zrevrangebyscore_limit(&all_key, "+inf", since_ms as f64, 0, limit as isize) + .map_err(map_err)? + } else { + conn.zrevrangebyscore(&all_key, "+inf", since_ms as f64) + .map_err(map_err)? + }; + let mut rows = Vec::with_capacity(ids.len()); + for id in ids { + let log_key = self.key(&["log", &id]); + let data: Option = conn.get(&log_key).map_err(map_err)?; + if let Some(d) = data { + let entry: LogEntry = + serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; + rows.push(TaskLogRow::from(entry)); + } + } + // ZREVRANGEBYSCORE already yields newest-first (logged_at desc). + return Ok(rows); + } + + // Filtered path: task_name/level are not indexed, so scan the cutoff + // range and filter in memory. let ids: Vec = conn .zrangebyscore(&all_key, since_ms as f64, "+inf") .map_err(map_err)?; @@ -175,31 +203,41 @@ impl RedisStorage { pub fn purge_task_logs(&self, older_than_ms: i64) -> Result { let mut conn = self.conn()?; let all_key = self.key(&["logs", "all"]); + let mut total = 0u64; + + // Every id in the `-inf..=cutoff` window is expired and gets deleted, so + // re-reading the window drains old logs in bounded batches rather than + // loading the whole below-cutoff set in one shot. + loop { + let ids: Vec = conn + .zrangebyscore_limit(&all_key, "-inf", older_than_ms as f64, 0, SCAN_BATCH) + .map_err(map_err)?; + if ids.is_empty() { + break; + } - let ids: Vec = conn - .zrangebyscore(&all_key, "-inf", older_than_ms as f64) - .map_err(map_err)?; - - if ids.is_empty() { - return Ok(0); - } - - let pipe = &mut redis::pipe(); - for id in &ids { - let log_key = self.key(&["log", id]); - // Load to get job_id for by_job cleanup - let data: Option = conn.get(&log_key).map_err(map_err)?; - if let Some(d) = data { - if let Ok(entry) = serde_json::from_str::(&d) { - let by_job_key = self.key(&["logs", "by_job", &entry.job_id]); - pipe.zrem(&by_job_key, id); + let pipe = &mut redis::pipe(); + for id in &ids { + let log_key = self.key(&["log", id]); + // Load to get job_id for by_job cleanup. + let data: Option = conn.get(&log_key).map_err(map_err)?; + if let Some(d) = data { + if let Ok(entry) = serde_json::from_str::(&d) { + let by_job_key = self.key(&["logs", "by_job", &entry.job_id]); + pipe.zrem(&by_job_key, id); + } } + pipe.del(&log_key); + pipe.zrem(&all_key, id); + } + pipe.query::<()>(&mut conn).map_err(map_err)?; + + total += ids.len() as u64; + if (ids.len() as isize) < SCAN_BATCH { + break; } - pipe.del(&log_key); - pipe.zrem(&all_key, id); } - pipe.query::<()>(&mut conn).map_err(map_err)?; - Ok(ids.len() as u64) + Ok(total) } } diff --git a/crates/taskito-core/src/storage/redis_backend/mod.rs b/crates/taskito-core/src/storage/redis_backend/mod.rs index 4bc95fff..2b01d4c3 100644 --- a/crates/taskito-core/src/storage/redis_backend/mod.rs +++ b/crates/taskito-core/src/storage/redis_backend/mod.rs @@ -120,6 +120,11 @@ fn map_err(e: redis::RedisError) -> QueueError { QueueError::Other(e.to_string()) } +/// Batch size for the bounded history scans (SSCAN/ZSCAN COUNT hint and the +/// ZRANGEBYSCORE LIMIT window). Caps how many ids a purge/list holds in memory +/// per round trip so a sweep over millions of rows never loads the whole set. +const SCAN_BATCH: isize = 500; + /// Drop the `payload`/`result` blobs from a job before it enters a listing. /// Redis loads the whole job JSON in one read, so this saves no I/O; it exists /// only to match the Diesel backends' narrow-projection contract — list results diff --git a/crates/taskito-core/tests/rust/storage_tests.rs b/crates/taskito-core/tests/rust/storage_tests.rs index 3ba7f01e..30edb236 100644 --- a/crates/taskito-core/tests/rust/storage_tests.rs +++ b/crates/taskito-core/tests/rust/storage_tests.rs @@ -1385,6 +1385,30 @@ fn redis_storage_tests() { redis_update_progress_never_resurrects_archived(&storage); redis_move_to_dlq_leaves_consistent_state(&storage); redis_move_to_dlq_skips_already_archived(&storage); + redis_purge_dead_drains_across_batches(&storage); +} + +/// S15: the batched `purge_dead` must drain more than one SCAN_BATCH (500) of +/// expired entries in a single call — proving the LIMIT-window loop iterates and +/// clears the remainder, not just the first batch. +#[cfg(feature = "redis")] +fn redis_purge_dead_drains_across_batches(s: &taskito_core::RedisStorage) { + let q = "q-redis-purge-batches"; + for _ in 0..550 { + let job = s.enqueue(make_job(q, "purge_batch_task")).unwrap(); + s.move_to_dlq(&job, "boom", None).unwrap(); + } + + // Cutoff far in the future so every dead entry is eligible. + let removed = s.purge_dead(now_millis() + 3_600_000).unwrap(); + assert!( + removed >= 550, + "batched purge_dead must remove all >500 eligible entries, got {removed}" + ); + assert!( + s.list_dead(10_000, 0).unwrap().is_empty(), + "batched purge_dead must fully drain the DLQ" + ); } /// Build a raw key under the storage's prefix, matching `RedisStorage::key`. From d8a85a5734cf9a4389056ef57e6a9771f7b5b920 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:53:47 +0530 Subject: [PATCH 04/16] feat(storage): keyset pagination for job, DLQ, and archive listings Add cursor-based (`*_after`) listing methods ordered by (sort_key, id) desc so deep pages stay O(page) and stable under concurrent inserts, unlike OFFSET. Opaque cursor via storage::cursor. Redis pages ZSET-scored listings by score+tie-bucket; the status-filtered/None job lists apply the keyset in memory (already O(N)). Also resets the taskito schema at the start of the Postgres contract so its count-exact assertions are deterministic on a persistent DB. --- crates/taskito-core/Cargo.toml | 5 + crates/taskito-core/src/storage/cursor.rs | 46 ++++ .../src/storage/diesel_common/archival.rs | 34 +++ .../src/storage/diesel_common/dead_letter.rs | 32 +++ .../src/storage/diesel_common/jobs.rs | 230 ++++++++++++++++++ crates/taskito-core/src/storage/mod.rs | 107 ++++++++ .../src/storage/redis_backend/archival.rs | 24 +- .../src/storage/redis_backend/dead_letter.rs | 24 ++ .../src/storage/redis_backend/jobs/query.rs | 127 ++++++++++ .../src/storage/redis_backend/mod.rs | 47 ++++ crates/taskito-core/src/storage/traits.rs | 37 +++ .../taskito-core/tests/rust/storage_tests.rs | 169 +++++++++++++ 12 files changed, 879 insertions(+), 3 deletions(-) create mode 100644 crates/taskito-core/src/storage/cursor.rs diff --git a/crates/taskito-core/Cargo.toml b/crates/taskito-core/Cargo.toml index 9b277e24..c7340e1e 100644 --- a/crates/taskito-core/Cargo.toml +++ b/crates/taskito-core/Cargo.toml @@ -37,3 +37,8 @@ tempfile = "3" # state and assert on internal index keys. Only used by `#[cfg(feature = "redis")]` # tests; harmless when the feature is off. redis = { workspace = true } +# Raw connection access in the Postgres contract test to reset the `taskito` +# schema up front, so the count-exact assertions are deterministic on a +# persistent test DB (the Postgres analogue of the Redis suite's FLUSHDB). Only +# used by `#[cfg(feature = "postgres")]` tests. +diesel = { workspace = true } diff --git a/crates/taskito-core/src/storage/cursor.rs b/crates/taskito-core/src/storage/cursor.rs new file mode 100644 index 00000000..b37748c1 --- /dev/null +++ b/crates/taskito-core/src/storage/cursor.rs @@ -0,0 +1,46 @@ +//! Opaque keyset-pagination cursor shared by every SDK binding. +//! +//! A cursor encodes the `(sort_key, id)` of the last row of a page — the sort +//! key being the listing's ordering column (`created_at`/`completed_at`/ +//! `failed_at`, a non-negative millis timestamp) and `id` the row's UUIDv7 / +//! DLQ id. Neither contains a `:`, so a single split round-trips unambiguously +//! with no base64. Callers must treat the string as opaque and pass it back +//! verbatim; the format may gain a version prefix later. + +use crate::error::{QueueError, Result}; + +/// Encode a `(sort_key, id)` keyset cursor as an opaque string. +pub fn encode_cursor(sort_key: i64, id: &str) -> String { + format!("{sort_key}:{id}") +} + +/// Decode a cursor produced by [`encode_cursor`]. Returns `Other` on malformed +/// input — treat it like any other bad-request error. +pub fn decode_cursor(cursor: &str) -> Result<(i64, &str)> { + let (key, id) = cursor + .split_once(':') + .ok_or_else(|| QueueError::Other(format!("invalid cursor: {cursor}")))?; + let sort_key: i64 = key + .parse() + .map_err(|_| QueueError::Other(format!("invalid cursor: {cursor}")))?; + Ok((sort_key, id)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn round_trips() { + let s = encode_cursor(1_720_000_000_123, "0190a1b2-c3d4-7e5f-8a9b-0c1d2e3f4a5b"); + let (key, id) = decode_cursor(&s).unwrap(); + assert_eq!(key, 1_720_000_000_123); + assert_eq!(id, "0190a1b2-c3d4-7e5f-8a9b-0c1d2e3f4a5b"); + } + + #[test] + fn rejects_malformed() { + assert!(decode_cursor("nocolon").is_err()); + assert!(decode_cursor("notanint:abc").is_err()); + } +} diff --git a/crates/taskito-core/src/storage/diesel_common/archival.rs b/crates/taskito-core/src/storage/diesel_common/archival.rs index baedead8..38253bd6 100644 --- a/crates/taskito-core/src/storage/diesel_common/archival.rs +++ b/crates/taskito-core/src/storage/diesel_common/archival.rs @@ -20,6 +20,40 @@ macro_rules! impl_diesel_archival_ops { Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) } + + /// Keyset-paginated `list_archived`, ordered by + /// `(completed_at, id)` descending. `completed_at` is nullable in + /// the schema (always set for a terminal row), so the cursor bound + /// compares against `Some(..)`. + pub fn list_archived_after( + &self, + limit: i64, + after: Option<(i64, &str)>, + ) -> Result> { + let mut conn = self.conn()?; + + let mut query = archived_jobs::table + .into_boxed() + .order((archived_jobs::completed_at.desc(), archived_jobs::id.desc())); + + if let Some((cursor_completed_at, cursor_id)) = after { + let cursor_id = cursor_id.to_string(); + query = query.filter( + archived_jobs::completed_at + .lt(Some(cursor_completed_at)) + .or(archived_jobs::completed_at + .eq(Some(cursor_completed_at)) + .and(archived_jobs::id.lt(cursor_id))), + ); + } + + let rows: Vec = query + .limit(limit) + .select(NarrowArchivedJobRow::as_select()) + .load(&mut conn)?; + + Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) + } } }; } diff --git a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs index 30dfe712..4a9f53eb 100644 --- a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs +++ b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs @@ -101,6 +101,38 @@ macro_rules! impl_diesel_dead_letter_ops { Ok(rows.into_iter().map(DeadJob::from_narrow).collect()) } + /// Keyset-paginated `list_dead`, ordered by `(failed_at, id)` + /// descending with a `(failed_at, id) < cursor` bound. + pub fn list_dead_after( + &self, + limit: i64, + after: Option<(i64, &str)>, + ) -> Result> { + let mut conn = self.conn()?; + + let mut query = dead_letter::table + .into_boxed() + .order((dead_letter::failed_at.desc(), dead_letter::id.desc())); + + if let Some((cursor_failed_at, cursor_id)) = after { + let cursor_id = cursor_id.to_string(); + query = query.filter( + dead_letter::failed_at + .lt(cursor_failed_at) + .or(dead_letter::failed_at + .eq(cursor_failed_at) + .and(dead_letter::id.lt(cursor_id))), + ); + } + + let rows: Vec = query + .limit(limit) + .select(NarrowDeadLetterRow::as_select()) + .load(&mut conn)?; + + Ok(rows.into_iter().map(DeadJob::from_narrow).collect()) + } + /// List dead letter entries for a single task, newest first. pub fn list_dead_by_task( &self, diff --git a/crates/taskito-core/src/storage/diesel_common/jobs.rs b/crates/taskito-core/src/storage/diesel_common/jobs.rs index e9d01706..046903b3 100644 --- a/crates/taskito-core/src/storage/diesel_common/jobs.rs +++ b/crates/taskito-core/src/storage/diesel_common/jobs.rs @@ -1104,6 +1104,21 @@ macro_rules! impl_diesel_job_ops { ) } + /// Keyset-paginated `list_jobs` — see [`list_jobs_filtered_after`]. + pub fn list_jobs_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + self.list_jobs_filtered_after( + status, queue_name, task_name, None, None, None, None, limit, after, namespace, + ) + } + /// True when `status` is a terminal status whose rows now live in /// `archived_jobs` rather than the live `jobs` table. fn is_terminal_status(status: i32) -> bool { @@ -1316,6 +1331,87 @@ macro_rules! impl_diesel_job_ops { } } + /// Keyset-paginated `list_jobs_filtered`, ordered by + /// `(created_at, id)` descending. Additive twin of the offset form. + #[allow(clippy::too_many_arguments)] + pub fn list_jobs_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + match status { + Some(s) if Self::is_terminal_status(s) => self.list_archived_filtered_after( + Some(s), + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace, + ), + Some(_) => self.list_live_filtered_after( + status, + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace, + ), + None => { + // Ask each table for its own top `limit` under the SAME + // cursor. Any row in the true merged top-`limit` has at + // most `limit-1` rows from its own table ahead of it, so + // it is already in that table's top-`limit` — no offset + // compensation is needed. + let mut live = self.list_live_filtered_after( + None, + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace, + )?; + let archived = self.list_archived_filtered_after( + None, + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace, + )?; + live.extend(archived); + // Same `(created_at, id)` descending order the SQL uses, + // so the merged cursor advances monotonically. + live.sort_by(|a, b| (b.created_at, &b.id).cmp(&(a.created_at, &a.id))); + live.truncate(limit.max(0) as usize); + Ok(live) + } + } + } + /// Query the live `jobs` table with the shared filter set. #[allow(clippy::too_many_arguments)] fn list_live_filtered( @@ -1375,6 +1471,73 @@ macro_rules! impl_diesel_job_ops { .collect()) } + /// Keyset twin of `list_live_filtered`, ordered by + /// `(created_at, id)` descending with a `(created_at, id) < cursor` + /// bound instead of an offset. + #[allow(clippy::too_many_arguments)] + fn list_live_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + let mut conn = self.conn()?; + + let mut query = jobs::table + .into_boxed() + .order((jobs::created_at.desc(), jobs::id.desc())); + + if let Some(s) = status { + query = query.filter(jobs::status.eq(s)); + } + if let Some(q) = queue_name { + query = query.filter(jobs::queue.eq(q)); + } + if let Some(t) = task_name { + query = query.filter(jobs::task_name.eq(t)); + } + if let Some(m) = metadata_like { + query = query.filter(jobs::metadata.like(format!("%{m}%"))); + } + if let Some(e) = error_like { + query = query.filter(jobs::error.like(format!("%{e}%"))); + } + if let Some(after) = created_after { + query = query.filter(jobs::created_at.ge(after)); + } + if let Some(before) = created_before { + query = query.filter(jobs::created_at.le(before)); + } + if let Some(ns) = namespace { + query = query.filter(jobs::namespace.eq(ns)); + } + if let Some((cursor_created_at, cursor_id)) = after { + let cursor_id = cursor_id.to_string(); + query = query.filter( + jobs::created_at.lt(cursor_created_at).or(jobs::created_at + .eq(cursor_created_at) + .and(jobs::id.lt(cursor_id))), + ); + } + + let rows: Vec = query + .limit(limit) + .select(NarrowJobRow::as_select()) + .load(&mut conn)?; + + Ok(rows + .into_iter() + .map(|r| Job::from_narrow(r, Vec::new(), None)) + .collect()) + } + /// Query the `archived_jobs` table with the shared filter set. #[allow(clippy::too_many_arguments)] fn list_archived_filtered( @@ -1432,6 +1595,73 @@ macro_rules! impl_diesel_job_ops { Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) } + /// Keyset twin of `list_archived_filtered`, ordered by + /// `(created_at, id)` descending. Used by the status=None merge, so + /// it must page on `created_at` (matching the live side), NOT on + /// `completed_at` (which `list_archived_after` uses). + #[allow(clippy::too_many_arguments)] + fn list_archived_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + let mut conn = self.conn()?; + + let mut query = archived_jobs::table + .into_boxed() + .order((archived_jobs::created_at.desc(), archived_jobs::id.desc())); + + if let Some(s) = status { + query = query.filter(archived_jobs::status.eq(s)); + } + if let Some(q) = queue_name { + query = query.filter(archived_jobs::queue.eq(q)); + } + if let Some(t) = task_name { + query = query.filter(archived_jobs::task_name.eq(t)); + } + if let Some(m) = metadata_like { + query = query.filter(archived_jobs::metadata.like(format!("%{m}%"))); + } + if let Some(e) = error_like { + query = query.filter(archived_jobs::error.like(format!("%{e}%"))); + } + if let Some(a) = created_after { + query = query.filter(archived_jobs::created_at.ge(a)); + } + if let Some(before) = created_before { + query = query.filter(archived_jobs::created_at.le(before)); + } + if let Some(ns) = namespace { + query = query.filter(archived_jobs::namespace.eq(ns)); + } + if let Some((cursor_created_at, cursor_id)) = after { + let cursor_id = cursor_id.to_string(); + query = query.filter( + archived_jobs::created_at.lt(cursor_created_at).or( + archived_jobs::created_at + .eq(cursor_created_at) + .and(archived_jobs::id.lt(cursor_id)), + ), + ); + } + + let rows: Vec = query + .limit(limit) + .select(NarrowArchivedJobRow::as_select()) + .load(&mut conn)?; + + Ok(rows.into_iter().map(Job::from_narrow_archived).collect()) + } + /// Delete one already-selected batch of archived jobs (their child /// rows first, then the archive rows) inside the caller's txn. fn purge_archived_id_batch( diff --git a/crates/taskito-core/src/storage/mod.rs b/crates/taskito-core/src/storage/mod.rs index bc0acca9..4b984202 100644 --- a/crates/taskito-core/src/storage/mod.rs +++ b/crates/taskito-core/src/storage/mod.rs @@ -1,3 +1,4 @@ +pub mod cursor; mod diesel_common; pub mod migrate; /// Code-first schema migrations. The files live at the crate root @@ -329,6 +330,17 @@ macro_rules! impl_storage { ) -> $crate::error::Result> { self.list_jobs(status, queue_name, task_name, limit, offset, namespace) } + fn list_jobs_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> $crate::error::Result> { + self.list_jobs_after(status, queue_name, task_name, limit, after, namespace) + } fn get_job(&self, id: &str) -> $crate::error::Result> { self.get_job(id) } @@ -386,6 +398,13 @@ macro_rules! impl_storage { ) -> $crate::error::Result> { self.list_dead(limit, offset) } + fn list_dead_after( + &self, + limit: i64, + after: Option<(i64, &str)>, + ) -> $crate::error::Result> { + self.list_dead_after(limit, after) + } fn list_dead_by_task( &self, task_name: &str, @@ -683,6 +702,13 @@ macro_rules! impl_storage { ) -> $crate::error::Result> { self.list_archived(limit, offset) } + fn list_archived_after( + &self, + limit: i64, + after: Option<(i64, &str)>, + ) -> $crate::error::Result> { + self.list_archived_after(limit, after) + } fn acquire_lock( &self, lock_name: &str, @@ -790,6 +816,32 @@ macro_rules! impl_storage { namespace, ) } + fn list_jobs_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> $crate::error::Result> { + self.list_jobs_filtered_after( + status, + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace, + ) + } fn get_setting(&self, key: &str) -> $crate::error::Result> { self.get_setting(key) } @@ -972,6 +1024,26 @@ impl Storage for StorageBackend { ) -> Result> { delegate!(self, list_jobs, status, queue_name, task_name, limit, offset, namespace) } + fn list_jobs_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + delegate!( + self, + list_jobs_after, + status, + queue_name, + task_name, + limit, + after, + namespace + ) + } fn get_job(&self, id: &str) -> Result> { delegate!(self, get_job, id) } @@ -1009,6 +1081,9 @@ impl Storage for StorageBackend { fn list_dead(&self, limit: i64, offset: i64) -> Result> { delegate!(self, list_dead, limit, offset) } + fn list_dead_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result> { + delegate!(self, list_dead_after, limit, after) + } fn list_dead_by_task(&self, task_name: &str, limit: i64, offset: i64) -> Result> { delegate!(self, list_dead_by_task, task_name, limit, offset) } @@ -1268,6 +1343,9 @@ impl Storage for StorageBackend { fn list_archived(&self, limit: i64, offset: i64) -> Result> { delegate!(self, list_archived, limit, offset) } + fn list_archived_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result> { + delegate!(self, list_archived_after, limit, after) + } fn acquire_lock(&self, lock_name: &str, owner_id: &str, ttl_ms: i64) -> Result { delegate!(self, acquire_lock, lock_name, owner_id, ttl_ms) } @@ -1340,6 +1418,35 @@ impl Storage for StorageBackend { namespace ) } + #[allow(clippy::too_many_arguments)] + fn list_jobs_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + delegate!( + self, + list_jobs_filtered_after, + status, + queue_name, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + after, + namespace + ) + } fn get_setting(&self, key: &str) -> Result> { delegate!(self, get_setting, key) } diff --git a/crates/taskito-core/src/storage/redis_backend/archival.rs b/crates/taskito-core/src/storage/redis_backend/archival.rs index 41c7eb8b..91639833 100644 --- a/crates/taskito-core/src/storage/redis_backend/archival.rs +++ b/crates/taskito-core/src/storage/redis_backend/archival.rs @@ -49,9 +49,28 @@ impl RedisStorage { ) .map_err(map_err)?; - let mut jobs = Vec::new(); + self.load_archived_by_ids(&mut conn, &ids) + } + + /// Keyset-paginated `list_archived`, ordered by `(completed_at, id)` + /// descending. `archived:all` is scored by `completed_at`, so the cursor + /// maps straight onto the ZSET keyset. + pub fn list_archived_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result> { + let mut conn = self.conn()?; + let archived_all = self.key(&["archived", "all"]); + let ids = super::zset_keyset_page(&mut conn, &archived_all, after, limit)?; + self.load_archived_by_ids(&mut conn, &ids) + } + + /// Load the given archived-job ids into blob-free [`Job`]s, preserving order. + fn load_archived_by_ids( + &self, + conn: &mut redis::Connection, + ids: &[String], + ) -> Result> { + let mut jobs = Vec::with_capacity(ids.len()); for id in ids { - let archived_key = self.key(&["archived", &id]); + let archived_key = self.key(&["archived", id]); let data: Option = conn.get(&archived_key).map_err(map_err)?; if let Some(d) = data { let mut job: Job = @@ -60,7 +79,6 @@ impl RedisStorage { jobs.push(job); } } - Ok(jobs) } } diff --git a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs index 4d09e2a6..48bdd777 100644 --- a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs +++ b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs @@ -159,6 +159,30 @@ impl RedisStorage { Ok(results) } + /// Keyset-paginated `list_dead`, ordered by `(failed_at, id)` descending. + /// `dlq:all` is scored by `failed_at`, so the cursor maps straight onto the + /// ZSET keyset. + pub fn list_dead_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result> { + let mut conn = self.conn()?; + let dlq_all = self.key(&["dlq", "all"]); + let ids = super::zset_keyset_page(&mut conn, &dlq_all, after, limit)?; + + let mut results = Vec::with_capacity(ids.len()); + for id in &ids { + let dlq_key = self.key(&["dlq", id]); + let data: Option = conn.get(&dlq_key).map_err(map_err)?; + if let Some(d) = data { + let entry: DeadJobEntry = + serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; + let mut dead = DeadJob::from(entry); + strip_dead_blob(&mut dead); + results.push(dead); + } + } + + Ok(results) + } + pub fn list_dead_by_task( &self, task_name: &str, diff --git a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs index 953a9f8d..5ba45a6a 100644 --- a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs +++ b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs @@ -60,6 +60,62 @@ impl RedisStorage { Ok(jobs[start..end].to_vec()) } + /// Keyset-paginated `list_jobs`. The live/archived status indexes are plain + /// SETs (and the None-merge spans two differently-scored ZSETs), so there is + /// no ZSET to seek — this loads the same candidate set `list_jobs` does and + /// applies the `(created_at, id) < cursor` keyset in memory. Correct and + /// stable across inserts; it does not add the Big-O win the Diesel backends + /// get (Redis `list_jobs` is already O(N) regardless of pagination). + pub fn list_jobs_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + let mut conn = self.conn()?; + let mut jobs = self.load_status_candidates(&mut conn, status)?; + jobs.retain(|job| { + queue_name.is_none_or(|q| job.queue == q) + && task_name.is_none_or(|t| job.task_name == t) + && namespace.is_none_or(|ns| job.namespace.as_deref() == Some(ns)) + }); + Ok(keyset_take(jobs, after, limit)) + } + + /// Load the candidate jobs for a status filter (terminal → archive, live → + /// active sets, None → both), shared by the keyset listing paths. + fn load_status_candidates( + &self, + conn: &mut redis::Connection, + status: Option, + ) -> Result> { + match status { + Some(s) if Self::is_terminal_status(s) => { + let key = self.key(&["archived", "status", &s.to_string()]); + let ids: Vec = conn.smembers(&key).map_err(map_err)?; + self.load_archived_jobs(conn, &ids) + } + Some(s) => { + let key = self.key(&["jobs", "status", &s.to_string()]); + let ids: Vec = conn.smembers(&key).map_err(map_err)?; + self.load_live_jobs(conn, &ids) + } + None => { + let live_key = self.key(&["jobs", "all"]); + let live_ids: Vec = conn.zrange(&live_key, 0, -1).map_err(map_err)?; + let mut all = self.load_live_jobs(conn, &live_ids)?; + let archived_key = self.key(&["archived", "all"]); + let archived_ids: Vec = + conn.zrange(&archived_key, 0, -1).map_err(map_err)?; + all.extend(self.load_archived_jobs(conn, &archived_ids)?); + Ok(all) + } + } + } + /// True when `status` is a terminal status whose jobs live in the archive. fn is_terminal_status(status: i32) -> bool { matches!( @@ -343,4 +399,75 @@ impl RedisStorage { let end = start.saturating_add(limit.max(0) as usize).min(jobs.len()); Ok(jobs[start..end].to_vec()) } + + /// Keyset-paginated `list_jobs_filtered` — same in-memory keyset approach + /// and caveats as [`Self::list_jobs_after`]. + #[allow(clippy::too_many_arguments)] + pub fn list_jobs_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result> { + let mut conn = self.conn()?; + let candidates = self.load_status_candidates(&mut conn, status)?; + + let mut jobs = Vec::new(); + for job in candidates { + if status.is_some_and(|s| job.status as i32 != s) { + continue; + } + if queue_name.is_some_and(|q| job.queue != q) { + continue; + } + if task_name.is_some_and(|t| job.task_name != t) { + continue; + } + if let Some(m) = metadata_like { + if !job.metadata.as_deref().is_some_and(|meta| meta.contains(m)) { + continue; + } + } + if let Some(e) = error_like { + if !job.error.as_deref().is_some_and(|err| err.contains(e)) { + continue; + } + } + if created_after.is_some_and(|a| job.created_at < a) { + continue; + } + if created_before.is_some_and(|b| job.created_at > b) { + continue; + } + if namespace.is_some_and(|ns| job.namespace.as_deref() != Some(ns)) { + continue; + } + jobs.push(job); + } + + Ok(keyset_take(jobs, after, limit)) + } +} + +/// Sort `jobs` by `(created_at, id)` descending, keep only rows strictly after +/// the `after` cursor in that order, and take the first `limit`. The in-memory +/// equivalent of the Diesel `(created_at, id) < cursor` keyset. +fn keyset_take(mut jobs: Vec, after: Option<(i64, &str)>, limit: i64) -> Vec { + jobs.sort_by(|a, b| (b.created_at, &b.id).cmp(&(a.created_at, &a.id))); + jobs.into_iter() + .filter(|j| match after { + Some((cursor_created_at, cursor_id)) => { + (j.created_at, j.id.as_str()) < (cursor_created_at, cursor_id) + } + None => true, + }) + .take(limit.max(0) as usize) + .collect() } diff --git a/crates/taskito-core/src/storage/redis_backend/mod.rs b/crates/taskito-core/src/storage/redis_backend/mod.rs index 2b01d4c3..7b1b38f4 100644 --- a/crates/taskito-core/src/storage/redis_backend/mod.rs +++ b/crates/taskito-core/src/storage/redis_backend/mod.rs @@ -139,3 +139,50 @@ fn strip_list_blobs(job: &mut crate::job::Job) { fn strip_dead_blob(dead: &mut crate::storage::DeadJob) { dead.payload = Vec::new(); } + +/// Keyset page of member ids from a ZSET scored so that a higher score is newer +/// (e.g. `archived:all` by `completed_at`, `dlq:all` by `failed_at`), in +/// `(score, member)` **descending** order. `after` is the `(score, id)` of the +/// previous page's last row. Matches the Diesel `(sort_key, id) < cursor` +/// keyset: Redis orders equal-score members by reverse-lexicographic id under +/// `ZREVRANGEBYSCORE`, which is exactly `id DESC`. +fn zset_keyset_page( + conn: &mut redis::Connection, + zkey: &str, + after: Option<(i64, &str)>, + limit: i64, +) -> Result> { + use redis::Commands; + if limit <= 0 { + return Ok(Vec::new()); + } + let Some((score, cursor_id)) = after else { + // First page: the newest `limit` members overall. + return conn + .zrevrangebyscore_limit(zkey, "+inf", "-inf", 0, limit as isize) + .map_err(map_err); + }; + + // Tie bucket first (same score, id < cursor_id), newest id first. Bounded by + // how many rows share one exact-millisecond score. + let same_score: Vec = conn + .zrangebyscore(zkey, score as f64, score as f64) + .map_err(map_err)?; + let mut page: Vec = same_score + .into_iter() + .filter(|m| m.as_str() < cursor_id) + .collect(); + page.reverse(); // ascending lex → descending id + page.truncate(limit as usize); + + // Then rows strictly below the cursor score, newest first. + if (page.len() as i64) < limit { + let remaining = limit - page.len() as i64; + let lower: Vec = conn + .zrevrangebyscore_limit(zkey, format!("({score}"), "-inf", 0, remaining as isize) + .map_err(map_err)?; + page.extend(lower); + } + + Ok(page) +} diff --git a/crates/taskito-core/src/storage/traits.rs b/crates/taskito-core/src/storage/traits.rs index 863dfea6..687470f6 100644 --- a/crates/taskito-core/src/storage/traits.rs +++ b/crates/taskito-core/src/storage/traits.rs @@ -85,6 +85,20 @@ pub trait Storage: Send + Sync + Clone { offset: i64, namespace: Option<&str>, ) -> Result>; + /// Keyset-paginated `list_jobs`, ordered by `(created_at, id)` descending. + /// `after` is the `(created_at, id)` of the previous page's last row; the + /// caller derives the next cursor from the returned rows' last element. + /// O(page) at any depth, and stable under concurrent inserts (unlike the + /// offset form). Rows are blob-free like every listing. + fn list_jobs_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result>; fn get_job(&self, id: &str) -> Result>; fn stats(&self) -> Result; fn purge_completed(&self, older_than_ms: i64) -> Result; @@ -103,6 +117,9 @@ pub trait Storage: Send + Sync + Clone { fn move_to_dlq(&self, job: &Job, error: &str, metadata: Option<&str>) -> Result<()>; fn list_dead(&self, limit: i64, offset: i64) -> Result>; + /// Keyset-paginated `list_dead`, ordered by `(failed_at, id)` descending. + /// See [`Storage::list_jobs_after`] for the cursor contract. + fn list_dead_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result>; /// Dead-letter entries for one task, newest first, paginated. fn list_dead_by_task(&self, task_name: &str, limit: i64, offset: i64) -> Result>; /// Delete every dead-letter entry for a task. Returns the number removed. @@ -259,6 +276,9 @@ pub trait Storage: Send + Sync + Clone { fn archive_old_jobs(&self, cutoff_ms: i64) -> Result; fn list_archived(&self, limit: i64, offset: i64) -> Result>; + /// Keyset-paginated `list_archived`, ordered by `(completed_at, id)` + /// descending. See [`Storage::list_jobs_after`] for the cursor contract. + fn list_archived_after(&self, limit: i64, after: Option<(i64, &str)>) -> Result>; // ── Distributed locking ──────────────────────────────────── @@ -315,6 +335,23 @@ pub trait Storage: Send + Sync + Clone { namespace: Option<&str>, ) -> Result>; + /// Keyset-paginated `list_jobs_filtered`, ordered by `(created_at, id)` + /// descending. See [`Storage::list_jobs_after`] for the cursor contract. + #[allow(clippy::too_many_arguments)] + fn list_jobs_filtered_after( + &self, + status: Option, + queue_name: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<(i64, &str)>, + namespace: Option<&str>, + ) -> Result>; + // ── Dashboard settings (key/value store) ───────────────────── /// Fetch a single setting value by key, or ``None`` if unset. diff --git a/crates/taskito-core/tests/rust/storage_tests.rs b/crates/taskito-core/tests/rust/storage_tests.rs index 30edb236..9ed3dfe5 100644 --- a/crates/taskito-core/tests/rust/storage_tests.rs +++ b/crates/taskito-core/tests/rust/storage_tests.rs @@ -1298,6 +1298,164 @@ fn run_storage_tests(s: &impl Storage) { test_concurrent_dequeue_no_double_claim(s); test_rate_limit_token_exhaustion(s); test_task_logs_after_cursor(s); + test_keyset_pagination_jobs(s); + test_keyset_pagination_dlq_and_archive(s); +} + +/// S12: keyset-paginated `list_jobs_after` must page through every row exactly +/// once, in `(created_at, id)` descending order, and stay stable when new rows +/// are inserted mid-pagination (the property offset pagination lacks). +fn test_keyset_pagination_jobs(s: &impl Storage) { + let q = "q-keyset-jobs"; + let total = 25; + for _ in 0..total { + s.enqueue(make_job(q, "keyset_task")).unwrap(); + } + + let page_size = 10; + let mut seen: Vec = Vec::new(); + let mut cursor: Option<(i64, String)> = None; + let mut inserted_extra = false; + loop { + let after = cursor.as_ref().map(|(k, id)| (*k, id.as_str())); + let page = s + .list_jobs_after( + Some(JobStatus::Pending as i32), + Some(q), + None, + page_size, + after, + None, + ) + .unwrap(); + if page.is_empty() { + break; + } + + // Order within the page is strictly descending by (created_at, id). + for w in page.windows(2) { + assert!( + (w[0].created_at, &w[0].id) > (w[1].created_at, &w[1].id), + "page must be strictly descending by (created_at, id)" + ); + } + + for j in &page { + seen.push(j.id.clone()); + } + let last = page.last().unwrap(); + cursor = Some((last.created_at, last.id.clone())); + + // Insert rows mid-pagination: keyset must not skip or duplicate the + // rows already paged past. The new rows are newer, so they sort ahead + // of the cursor and are correctly excluded from later pages. + if !inserted_extra { + for _ in 0..5 { + s.enqueue(make_job(q, "keyset_task")).unwrap(); + } + inserted_extra = true; + } + + if page.len() < page_size as usize { + break; + } + } + + // Every original row seen exactly once (the mid-pagination inserts are + // newer than the cursor, so they never appear). + assert_eq!( + seen.len(), + total, + "keyset must page every original row once" + ); + let unique: std::collections::HashSet<&String> = seen.iter().collect(); + assert_eq!(unique.len(), total, "keyset must never duplicate a row"); +} + +/// S12 for the DLQ and archive tables: `list_dead_after` / `list_archived_after` +/// page through every row exactly once. +fn test_keyset_pagination_dlq_and_archive(s: &impl Storage) { + let q = "q-keyset-terminal"; + let total = 15; + for _ in 0..total { + let job = s.enqueue(make_job(q, "keyset_terminal")).unwrap(); + s.dequeue(q, now_millis() + 1000, None).unwrap(); + let running = s.get_job(&job.id).unwrap().unwrap(); + s.move_to_dlq(&running, "boom", None).unwrap(); + } + + // DLQ paging. + let dlq_ids = page_all_dead(s, 6); + let dlq_from_this_q: Vec<&String> = dlq_ids.iter().collect(); + assert!( + dlq_from_this_q.len() >= total, + "keyset DLQ paging must cover every dead row" + ); + let unique: std::collections::HashSet<&String> = dlq_ids.iter().collect(); + assert_eq!(unique.len(), dlq_ids.len(), "DLQ keyset must not duplicate"); + + // Archive paging: complete a fresh batch so archived rows exist. + let qa = "q-keyset-archive"; + for _ in 0..total { + let job = s.enqueue(make_job(qa, "keyset_archive")).unwrap(); + s.dequeue(qa, now_millis() + 1000, None).unwrap(); + s.complete(&job.id, None).unwrap(); + } + let arch_ids = page_all_archived(s, 6); + let unique: std::collections::HashSet<&String> = arch_ids.iter().collect(); + assert_eq!( + unique.len(), + arch_ids.len(), + "archive keyset must not duplicate" + ); + assert!( + arch_ids.len() >= total, + "keyset archive paging must cover every archived row" + ); +} + +/// Page the whole DLQ via `list_dead_after`, returning every dead id seen. +fn page_all_dead(s: &impl Storage, page_size: i64) -> Vec { + let mut seen = Vec::new(); + let mut cursor: Option<(i64, String)> = None; + loop { + let after = cursor.as_ref().map(|(k, id)| (*k, id.as_str())); + let page = s.list_dead_after(page_size, after).unwrap(); + if page.is_empty() { + break; + } + let last = page.last().unwrap(); + cursor = Some((last.failed_at, last.id.clone())); + for d in &page { + seen.push(d.id.clone()); + } + if page.len() < page_size as usize { + break; + } + } + seen +} + +/// Page the whole archive via `list_archived_after`, returning every id seen. +fn page_all_archived(s: &impl Storage, page_size: i64) -> Vec { + let mut seen = Vec::new(); + let mut cursor: Option<(i64, String)> = None; + loop { + let after = cursor.as_ref().map(|(k, id)| (*k, id.as_str())); + let page = s.list_archived_after(page_size, after).unwrap(); + if page.is_empty() { + break; + } + let last = page.last().unwrap(); + cursor = Some((last.completed_at.unwrap_or(0), last.id.clone())); + for j in &page { + seen.push(j.id.clone()); + } + if page.len() < page_size as usize { + break; + } + } + seen } fn test_task_logs_after_cursor(s: &impl Storage) { @@ -1666,6 +1824,8 @@ fn redis_purge_preserves_reused_unique_key(s: &taskito_core::RedisStorage) { #[cfg(feature = "postgres")] #[test] fn postgres_storage_tests() { + use diesel::connection::SimpleConnection; + use diesel::{Connection, PgConnection}; use taskito_core::PostgresStorage; let url = match std::env::var("TASKITO_POSTGRES_TEST_URL") { @@ -1676,6 +1836,15 @@ fn postgres_storage_tests() { } }; + // Reset the `taskito` schema so the count-exact contract is deterministic on + // a persistent test DB (the Postgres analogue of the Redis suite's FLUSHDB). + // `PostgresStorage::new` recreates the schema and re-runs migrations. Harmless + // on a fresh CI database. + if let Ok(mut conn) = PgConnection::establish(&url) { + conn.batch_execute("DROP SCHEMA IF EXISTS taskito CASCADE") + .expect("reset taskito schema"); + } + let storage = match PostgresStorage::new(&url) { Ok(s) => s, Err(e) => { From f2db305971d6c089ed7d73c86c8b198c09e0cae6 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:54:20 +0530 Subject: [PATCH 05/16] feat(workflows): keyset pagination for workflow runs list_workflow_runs_after pages by (created_at, id) desc across all three stores. Redis pages in memory since the by_state index is re-scored on state transitions. --- crates/taskito-workflows/src/diesel_common.rs | 81 +++++++++++++++++++ crates/taskito-workflows/src/lib.rs | 17 ++++ crates/taskito-workflows/src/redis_store.rs | 59 ++++++++++++++ crates/taskito-workflows/src/storage.rs | 12 +++ .../tests/storage_contract.rs | 43 ++++++++++ 5 files changed, 212 insertions(+) diff --git a/crates/taskito-workflows/src/diesel_common.rs b/crates/taskito-workflows/src/diesel_common.rs index 925ddf6a..e7681cfd 100644 --- a/crates/taskito-workflows/src/diesel_common.rs +++ b/crates/taskito-workflows/src/diesel_common.rs @@ -416,6 +416,87 @@ macro_rules! impl_workflow_diesel_ops { .collect()) } + fn list_workflow_runs_after( + &self, + definition_name: Option<&str>, + state: Option<$crate::WorkflowState>, + limit: i64, + after: Option<(i64, &str)>, + ) -> ::taskito_core::error::Result> { + let mut conn = self.inner.conn()?; + + // No cursor → sentinel `(i64::MAX, "")`: `created_at < MAX` holds + // for every real timestamp, so the keyset clause matches all rows + // and the first page is returned. Keeps one SQL per filter combo. + let (cursor_created_at, cursor_id) = after.unwrap_or((i64::MAX, "")); + + let rows: Vec<$crate::diesel_common::RunRow> = match (definition_name, state) { + (Some(name), Some(st)) => ::diesel::sql_query( + &$prep_sql("SELECT r.id, r.definition_id, r.params, r.state, r.started_at, + r.completed_at, r.error, r.parent_run_id, r.parent_node_name, + r.created_at + FROM workflow_runs r + JOIN workflow_definitions d ON r.definition_id = d.id + WHERE d.name = ? AND r.state = ? + AND (r.created_at < ? OR (r.created_at = ? AND r.id < ?)) + ORDER BY r.created_at DESC, r.id DESC LIMIT ?"), + ) + .bind::<::diesel::sql_types::Text, _>(name) + .bind::<::diesel::sql_types::Text, _>(st.as_str()) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::Text, _>(cursor_id) + .bind::<::diesel::sql_types::BigInt, _>(limit) + .load(&mut *conn)?, + (Some(name), None) => ::diesel::sql_query( + &$prep_sql("SELECT r.id, r.definition_id, r.params, r.state, r.started_at, + r.completed_at, r.error, r.parent_run_id, r.parent_node_name, + r.created_at + FROM workflow_runs r + JOIN workflow_definitions d ON r.definition_id = d.id + WHERE d.name = ? + AND (r.created_at < ? OR (r.created_at = ? AND r.id < ?)) + ORDER BY r.created_at DESC, r.id DESC LIMIT ?"), + ) + .bind::<::diesel::sql_types::Text, _>(name) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::Text, _>(cursor_id) + .bind::<::diesel::sql_types::BigInt, _>(limit) + .load(&mut *conn)?, + (None, Some(st)) => ::diesel::sql_query( + &$prep_sql("SELECT id, definition_id, params, state, started_at, completed_at, + error, parent_run_id, parent_node_name, created_at + FROM workflow_runs WHERE state = ? + AND (created_at < ? OR (created_at = ? AND id < ?)) + ORDER BY created_at DESC, id DESC LIMIT ?"), + ) + .bind::<::diesel::sql_types::Text, _>(st.as_str()) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::Text, _>(cursor_id) + .bind::<::diesel::sql_types::BigInt, _>(limit) + .load(&mut *conn)?, + (None, None) => ::diesel::sql_query( + &$prep_sql("SELECT id, definition_id, params, state, started_at, completed_at, + error, parent_run_id, parent_node_name, created_at + FROM workflow_runs + WHERE (created_at < ? OR (created_at = ? AND id < ?)) + ORDER BY created_at DESC, id DESC LIMIT ?"), + ) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::BigInt, _>(cursor_created_at) + .bind::<::diesel::sql_types::Text, _>(cursor_id) + .bind::<::diesel::sql_types::BigInt, _>(limit) + .load(&mut *conn)?, + }; + + Ok(rows + .into_iter() + .map($crate::diesel_common::run_from_row) + .collect()) + } + fn create_workflow_node( &self, node: &$crate::WorkflowNode, diff --git a/crates/taskito-workflows/src/lib.rs b/crates/taskito-workflows/src/lib.rs index d551c2b8..f59be413 100644 --- a/crates/taskito-workflows/src/lib.rs +++ b/crates/taskito-workflows/src/lib.rs @@ -127,6 +127,23 @@ impl WorkflowStorage for WorkflowStorageBackend { ) } + fn list_workflow_runs_after( + &self, + definition_name: Option<&str>, + state: Option, + limit: i64, + after: Option<(i64, &str)>, + ) -> Result> { + delegate!( + self, + list_workflow_runs_after, + definition_name, + state, + limit, + after + ) + } + fn create_workflow_node(&self, node: &WorkflowNode) -> Result<()> { delegate!(self, create_workflow_node, node) } diff --git a/crates/taskito-workflows/src/redis_store.rs b/crates/taskito-workflows/src/redis_store.rs index 362ae15a..26fcaa5c 100644 --- a/crates/taskito-workflows/src/redis_store.rs +++ b/crates/taskito-workflows/src/redis_store.rs @@ -537,6 +537,65 @@ impl WorkflowStorage for WorkflowRedisStorage { Ok(runs) } + fn list_workflow_runs_after( + &self, + definition_name: Option<&str>, + state: Option, + limit: i64, + after: Option<(i64, &str)>, + ) -> Result> { + let mut conn = self.conn()?; + + let def_id_for_filter: Option = match definition_name { + Some(name) => { + let by_name = k_def_by_name(&self.prefix, name); + let ids: Vec = conn.zrevrange(&by_name, 0, 0).map_err(into_other)?; + match ids.into_iter().next() { + Some(id) => Some(id), + None => return Ok(Vec::new()), + } + } + None => None, + }; + + let index_key = match (def_id_for_filter.as_deref(), state) { + (Some(def_id), _) => k_runs_by_def(&self.prefix, def_id), + (None, Some(st)) => k_runs_by_state(&self.prefix, st.as_str()), + (None, None) => k_runs_all(&self.prefix), + }; + + // The `by_state` index is re-scored to the transition timestamp on every + // state change, so its score is not `created_at` — the keyset must run + // on the loaded run's real `created_at`, in memory. (Workflow-run counts + // are modest; the Big-O win the Diesel backends get is not attempted + // here, matching the Redis job listings.) + let candidate_ids: Vec = conn.zrevrange(&index_key, 0, -1).map_err(into_other)?; + + let mut runs = Vec::new(); + for id in candidate_ids { + if let Some(run) = self.get_workflow_run(&id)? { + if let Some(st) = state { + if run.state != st { + continue; + } + } + runs.push(run); + } + } + + runs.sort_by(|a, b| (b.created_at, &b.id).cmp(&(a.created_at, &a.id))); + Ok(runs + .into_iter() + .filter(|r| match after { + Some((cursor_created_at, cursor_id)) => { + (r.created_at, r.id.as_str()) < (cursor_created_at, cursor_id) + } + None => true, + }) + .take(limit.max(0) as usize) + .collect()) + } + fn create_workflow_node(&self, node: &WorkflowNode) -> Result<()> { let mut conn = self.conn()?; write_node_pipeline(&self.prefix, node, &mut conn) diff --git a/crates/taskito-workflows/src/storage.rs b/crates/taskito-workflows/src/storage.rs index a5771468..629f735d 100644 --- a/crates/taskito-workflows/src/storage.rs +++ b/crates/taskito-workflows/src/storage.rs @@ -37,6 +37,18 @@ pub trait WorkflowStorage: Send + Sync { offset: i64, ) -> Result>; + /// Keyset-paginated `list_workflow_runs`, ordered by `(created_at, id)` + /// descending. `after` is the `(created_at, id)` of the previous page's + /// last run; the caller derives the next cursor from the returned rows' + /// last element. O(page) at any depth, stable under concurrent inserts. + fn list_workflow_runs_after( + &self, + definition_name: Option<&str>, + state: Option, + limit: i64, + after: Option<(i64, &str)>, + ) -> Result>; + // ── Nodes ────────────────────────────────────────────────────── fn create_workflow_node(&self, node: &WorkflowNode) -> Result<()>; diff --git a/crates/taskito-workflows/tests/storage_contract.rs b/crates/taskito-workflows/tests/storage_contract.rs index c704de89..ecc52906 100644 --- a/crates/taskito-workflows/tests/storage_contract.rs +++ b/crates/taskito-workflows/tests/storage_contract.rs @@ -244,6 +244,48 @@ fn case_list_runs_by_state(s: &impl WorkflowStorage) { assert_eq!(pending.len(), 1); } +fn case_list_runs_after(s: &impl WorkflowStorage) { + let def = make_definition("keyset_runs"); + s.create_workflow_definition(&def).unwrap(); + + let total = 12; + for _ in 0..total { + s.create_workflow_run(&make_run(&def.id)).unwrap(); + } + + // Page through in threes; every run appears exactly once, newest first. + let page_size = 3; + let mut seen: Vec = Vec::new(); + let mut cursor: Option<(i64, String)> = None; + loop { + let after = cursor.as_ref().map(|(k, id)| (*k, id.as_str())); + let page = s + .list_workflow_runs_after(Some("keyset_runs"), None, page_size, after) + .unwrap(); + if page.is_empty() { + break; + } + for w in page.windows(2) { + assert!( + (w[0].created_at, &w[0].id) > (w[1].created_at, &w[1].id), + "page must be strictly descending by (created_at, id)" + ); + } + let last = page.last().unwrap(); + cursor = Some((last.created_at, last.id.clone())); + for r in &page { + seen.push(r.id.clone()); + } + if page.len() < page_size as usize { + break; + } + } + + assert_eq!(seen.len(), total, "keyset must page every run once"); + let unique: std::collections::HashSet<&String> = seen.iter().collect(); + assert_eq!(unique.len(), total, "keyset must never duplicate a run"); +} + fn case_create_and_get_node(s: &impl WorkflowStorage) { let def = make_definition("node_test"); s.create_workflow_definition(&def).unwrap(); @@ -645,6 +687,7 @@ fn run_contract(s: &impl WorkflowStorage) { case_set_run_started_and_completed(s); case_list_runs(s); case_list_runs_by_state(s); + case_list_runs_after(s); case_create_and_get_node(s); case_create_nodes_batch(s); case_update_node_status(s); From 3ceb82158f60f611a16697dc7fd51514a3413198 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:56:22 +0530 Subject: [PATCH 06/16] feat(python): cursor-paginated list APIs list_jobs_after / list_jobs_filtered_after / list_archived_after / dead_letters_after / list_workflow_runs_after (+ async) return a Page(items, next_cursor). next_cursor is None on the last page. Dashboard workflow-runs endpoint accepts after and echoes next_cursor. --- .../taskito-python/src/py_queue/inspection.rs | 123 +++++++++- crates/taskito-python/src/py_queue/mod.rs | 45 ++++ .../src/py_queue/workflow_ops/queries.rs | 40 +++ sdks/python/taskito/_taskito.pyi | 35 +++ sdks/python/taskito/async_support/mixins.py | 45 ++++ .../taskito/dashboard/handlers/workflows.py | 23 +- sdks/python/taskito/mixins/inspection.py | 58 +++++ sdks/python/taskito/mixins/operations.py | 16 ++ sdks/python/taskito/pagination.py | 29 +++ sdks/python/tests/core/test_pagination.py | 227 ++++++++++++++++++ 10 files changed, 638 insertions(+), 3 deletions(-) create mode 100644 sdks/python/taskito/pagination.py create mode 100644 sdks/python/tests/core/test_pagination.py diff --git a/crates/taskito-python/src/py_queue/inspection.rs b/crates/taskito-python/src/py_queue/inspection.rs index 8a499918..526fdb12 100644 --- a/crates/taskito-python/src/py_queue/inspection.rs +++ b/crates/taskito-python/src/py_queue/inspection.rs @@ -4,7 +4,7 @@ use pyo3::types::PyDict; use taskito_core::job::{now_millis, NewJob}; use taskito_core::storage::Storage; -use super::PyQueue; +use super::{next_cursor, PyQueue}; use crate::py_job::PyJob; #[pymethods] @@ -36,6 +36,36 @@ impl PyQueue { Ok(jobs.into_iter().map(PyJob::from).collect()) } + /// Keyset-paginated `list_jobs`. Returns `(jobs, next_cursor)`; pass + /// `next_cursor` back as `after` for the following page (`None` when the + /// page was the last). O(page) at any depth, stable under inserts. + #[pyo3(signature = (status=None, queue=None, task_name=None, limit=50, after=None, namespace=None))] + pub fn list_jobs_after( + &self, + status: Option<&str>, + queue: Option<&str>, + task_name: Option<&str>, + limit: i64, + after: Option<&str>, + namespace: Option<&str>, + ) -> PyResult<(Vec, Option)> { + if limit < 0 { + return Err(pyo3::exceptions::PyValueError::new_err( + "limit must be non-negative", + )); + } + let status_int = parse_status(status)?; + let cursor = decode_after(after)?; + + let jobs = self + .storage + .list_jobs_after(status_int, queue, task_name, limit, cursor, namespace) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + + let next = next_cursor(&jobs, limit, |j| (j.created_at, &j.id)); + Ok((jobs.into_iter().map(PyJob::from).collect(), next)) + } + /// List jobs with extended filters. #[allow(clippy::too_many_arguments)] #[pyo3(signature = (status=None, queue=None, task_name=None, metadata_like=None, error_like=None, created_after=None, created_before=None, limit=50, offset=0, namespace=None))] @@ -78,6 +108,50 @@ impl PyQueue { Ok(jobs.into_iter().map(PyJob::from).collect()) } + /// Keyset-paginated `list_jobs_filtered`. Returns `(jobs, next_cursor)`. + #[allow(clippy::too_many_arguments)] + #[pyo3(signature = (status=None, queue=None, task_name=None, metadata_like=None, error_like=None, created_after=None, created_before=None, limit=50, after=None, namespace=None))] + pub fn list_jobs_filtered_after( + &self, + status: Option<&str>, + queue: Option<&str>, + task_name: Option<&str>, + metadata_like: Option<&str>, + error_like: Option<&str>, + created_after: Option, + created_before: Option, + limit: i64, + after: Option<&str>, + namespace: Option<&str>, + ) -> PyResult<(Vec, Option)> { + if limit < 0 { + return Err(pyo3::exceptions::PyValueError::new_err( + "limit must be non-negative", + )); + } + let status_int = parse_status(status)?; + let cursor = decode_after(after)?; + + let jobs = self + .storage + .list_jobs_filtered_after( + status_int, + queue, + task_name, + metadata_like, + error_like, + created_after, + created_before, + limit, + cursor, + namespace, + ) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + + let next = next_cursor(&jobs, limit, |j| (j.created_at, &j.id)); + Ok((jobs.into_iter().map(PyJob::from).collect(), next)) + } + /// List dead letter queue entries. #[pyo3(signature = (limit=10, offset=0))] pub fn dead_letters(&self, limit: i64, offset: i64) -> PyResult>> { @@ -110,6 +184,45 @@ impl PyQueue { }) } + /// Keyset-paginated `dead_letters`. Returns `(entries, next_cursor)`. + #[pyo3(signature = (limit=10, after=None))] + pub fn dead_letters_after( + &self, + limit: i64, + after: Option<&str>, + ) -> PyResult<(Vec>, Option)> { + if limit < 0 { + return Err(pyo3::exceptions::PyValueError::new_err( + "limit must be non-negative", + )); + } + let cursor = decode_after(after)?; + let dead = self + .storage + .list_dead_after(limit, cursor) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + + let next = next_cursor(&dead, limit, |d| (d.failed_at, &d.id)); + + Python::attach(|py| { + let mut result = Vec::with_capacity(dead.len()); + for d in dead { + let dict = PyDict::new(py); + dict.set_item("id", d.id)?; + dict.set_item("original_job_id", d.original_job_id)?; + dict.set_item("queue", d.queue)?; + dict.set_item("task_name", d.task_name)?; + dict.set_item("error", d.error)?; + dict.set_item("retry_count", d.retry_count)?; + dict.set_item("failed_at", d.failed_at)?; + dict.set_item("metadata", d.metadata)?; + dict.set_item("dlq_retry_count", d.dlq_retry_count)?; + result.push(dict.into()); + } + Ok((result, next)) + }) + } + /// List dead letter queue entries for a single task, newest first. #[pyo3(signature = (task_name, limit=10, offset=0))] pub fn dead_letters_by_task( @@ -504,6 +617,14 @@ impl PyQueue { } } +/// Decode an opaque `after` cursor string into a `(sort_key, id)` keyset bound. +fn decode_after(after: Option<&str>) -> PyResult> { + after + .map(taskito_core::storage::cursor::decode_cursor) + .transpose() + .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string())) +} + fn parse_status(status: Option<&str>) -> PyResult> { match status { Some(s) => Ok(Some(match s { diff --git a/crates/taskito-python/src/py_queue/mod.rs b/crates/taskito-python/src/py_queue/mod.rs index 81e8d26e..7d3c2bcc 100644 --- a/crates/taskito-python/src/py_queue/mod.rs +++ b/crates/taskito-python/src/py_queue/mod.rs @@ -26,6 +26,23 @@ use taskito_core::worker::WorkerDispatcher; use crate::py_job::PyJob; +/// Build the next-page cursor from a returned page. `None` when the page is the +/// last one (fewer rows than `limit`), so callers can loop `while cursor`. +/// Shared by every `*_after` binding across the `py_queue` submodules. +pub(crate) fn next_cursor( + rows: &[T], + limit: i64, + key: impl Fn(&T) -> (i64, &str), +) -> Option { + if (rows.len() as i64) < limit { + return None; + } + rows.last().map(|r| { + let (sort_key, id) = key(r); + taskito_core::storage::cursor::encode_cursor(sort_key, id) + }) +} + /// The core queue engine exposed to Python. #[pyclass] pub struct PyQueue { @@ -626,6 +643,34 @@ impl PyQueue { Ok(jobs.into_iter().map(PyJob::from).collect()) } + /// Keyset-paginated `list_archived`, ordered by completed time. Returns + /// `(jobs, next_cursor)`; pass `next_cursor` back as `after`. + #[pyo3(signature = (limit=50, after=None))] + pub fn list_archived_after( + &self, + limit: i64, + after: Option<&str>, + ) -> PyResult<(Vec, Option)> { + if limit < 0 { + return Err(pyo3::exceptions::PyValueError::new_err( + "limit must be non-negative", + )); + } + let cursor = after + .map(taskito_core::storage::cursor::decode_cursor) + .transpose() + .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + let jobs = self + .storage + .list_archived_after(limit, cursor) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + // Archived rows always have `completed_at`; fall back to created_at. + let next = next_cursor(&jobs, limit, |j| { + (j.completed_at.unwrap_or(j.created_at), &j.id) + }); + Ok((jobs.into_iter().map(PyJob::from).collect(), next)) + } + /// Register a periodic task schedule. #[pyo3(signature = (name, task_name, cron_expr, args=None, kwargs=None, queue="default", timezone=None))] pub fn register_periodic( diff --git a/crates/taskito-python/src/py_queue/workflow_ops/queries.rs b/crates/taskito-python/src/py_queue/workflow_ops/queries.rs index 83b01771..8b39564d 100644 --- a/crates/taskito-python/src/py_queue/workflow_ops/queries.rs +++ b/crates/taskito-python/src/py_queue/workflow_ops/queries.rs @@ -114,6 +114,46 @@ impl PyQueue { result.map_err(|e| PyRuntimeError::new_err(e.to_string())) } + /// Keyset-paginated `list_workflow_runs`. Returns `(runs, next_cursor)`; + /// pass `next_cursor` back as `after`. `limit` is clamped to `[1, 500]`. + #[pyo3(signature = (definition_name=None, state=None, limit=50, after=None))] + pub fn list_workflow_runs_after( + &self, + py: Python<'_>, + definition_name: Option<&str>, + state: Option<&str>, + limit: i64, + after: Option<&str>, + ) -> PyResult<(Vec, Option)> { + let wf_storage = workflow_storage(self)?; + let limit = limit.clamp(1, 500); + let definition_owned = definition_name.map(|s| s.to_string()); + let state_parsed = match state { + Some(s) => Some( + WorkflowState::from_str_val(s) + .ok_or_else(|| PyValueError::new_err(format!("invalid workflow state: {s}")))?, + ), + None => None, + }; + let cursor = after + .map(taskito_core::storage::cursor::decode_cursor) + .transpose() + .map_err(|e| PyValueError::new_err(e.to_string()))?; + + let result: CoreResult<(Vec, Option)> = py.detach(|| { + let runs = wf_storage.list_workflow_runs_after( + definition_owned.as_deref(), + state_parsed, + limit, + cursor, + )?; + let next = crate::py_queue::next_cursor(&runs, limit, |r| (r.created_at, &r.id)); + Ok((runs.into_iter().map(PyWorkflowRun::from).collect(), next)) + }); + + result.map_err(|e| PyRuntimeError::new_err(e.to_string())) + } + /// Fetch a workflow run with full per-node detail including compensation fields. /// /// Used by the dashboard ``/api/workflows/runs/{run_id}`` endpoint. Unlike diff --git a/sdks/python/taskito/_taskito.pyi b/sdks/python/taskito/_taskito.pyi index 660a5364..714ec1b1 100644 --- a/sdks/python/taskito/_taskito.pyi +++ b/sdks/python/taskito/_taskito.pyi @@ -138,6 +138,15 @@ class PyQueue: offset: int = 0, namespace: str | None = None, ) -> list[PyJob]: ... + def list_jobs_after( + self, + status: str | None = None, + queue: str | None = None, + task_name: str | None = None, + limit: int = 50, + after: str | None = None, + namespace: str | None = None, + ) -> tuple[list[PyJob], str | None]: ... def get_job(self, job_id: str) -> PyJob | None: ... def get_job_errors(self, job_id: str) -> list[dict[str, Any]]: ... def cancel_job(self, job_id: str) -> bool: ... @@ -163,7 +172,23 @@ class PyQueue: offset: int = 0, namespace: str | None = None, ) -> list[PyJob]: ... + def list_jobs_filtered_after( + self, + status: str | None = None, + queue: str | None = None, + task_name: str | None = None, + metadata_like: str | None = None, + error_like: str | None = None, + created_after: int | None = None, + created_before: int | None = None, + limit: int = 50, + after: str | None = None, + namespace: str | None = None, + ) -> tuple[list[PyJob], str | None]: ... def dead_letters(self, limit: int = 10, offset: int = 0) -> list[dict[str, Any]]: ... + def dead_letters_after( + self, limit: int = 10, after: str | None = None + ) -> tuple[list[dict[str, Any]], str | None]: ... def dead_letters_by_task( self, task_name: str, limit: int = 10, offset: int = 0 ) -> list[dict[str, Any]]: ... @@ -256,6 +281,9 @@ class PyQueue: def list_settings(self) -> dict[str, str]: ... def archive_old_jobs(self, older_than_seconds: int) -> int: ... def list_archived(self, limit: int = 50, offset: int = 0) -> list[PyJob]: ... + def list_archived_after( + self, limit: int = 50, after: str | None = None + ) -> tuple[list[PyJob], str | None]: ... def get_metrics( self, task_name: str | None = None, @@ -317,6 +345,13 @@ class PyQueue: limit: int = 50, offset: int = 0, ) -> list[PyWorkflowRun]: ... + def list_workflow_runs_after( + self, + definition_name: str | None = None, + state: str | None = None, + limit: int = 50, + after: str | None = None, + ) -> tuple[list[PyWorkflowRun], str | None]: ... def get_workflow_run_detail( self, run_id: str ) -> tuple[PyWorkflowRun, list[PyWorkflowRunNode]]: ... diff --git a/sdks/python/taskito/async_support/mixins.py b/sdks/python/taskito/async_support/mixins.py index 4de7bb42..e29f28a8 100644 --- a/sdks/python/taskito/async_support/mixins.py +++ b/sdks/python/taskito/async_support/mixins.py @@ -16,6 +16,7 @@ from concurrent.futures import Executor from taskito.context import LogLevel + from taskito.pagination import Page from taskito.result import JobResult logger = logging.getLogger("taskito") @@ -79,6 +80,32 @@ def query_logs( limit: int = ..., ) -> list[dict]: ... def dead_letters(self, limit: int = ..., offset: int = ...) -> list[dict]: ... + def list_jobs_after( + self, + status: str | None = ..., + queue: str | None = ..., + task_name: str | None = ..., + limit: int = ..., + after: str | None = ..., + namespace: Any = ..., + ) -> Page[JobResult]: ... + def list_jobs_filtered_after( + self, + status: str | None = ..., + queue: str | None = ..., + task_name: str | None = ..., + metadata_like: str | None = ..., + error_like: str | None = ..., + created_after: int | None = ..., + created_before: int | None = ..., + limit: int = ..., + after: str | None = ..., + namespace: Any = ..., + ) -> Page[JobResult]: ... + def dead_letters_after(self, limit: int = ..., after: str | None = ...) -> Page[dict]: ... + def list_archived_after( + self, limit: int = ..., after: str | None = ... + ) -> Page[JobResult]: ... def retry_dead(self, dead_id: str) -> str: ... def requeue_job(self, job_id: str) -> bool: ... def delete_dead(self, dead_id: str) -> bool: ... @@ -176,6 +203,24 @@ async def alist_jobs_filtered(self, **kwargs: Any) -> list[JobResult]: """Async version of :meth:`list_jobs_filtered`.""" return await self._run_sync(self.list_jobs_filtered, **kwargs) + async def alist_jobs_after(self, **kwargs: Any) -> Page[JobResult]: + """Async version of :meth:`list_jobs_after`.""" + return await self._run_sync(self.list_jobs_after, **kwargs) + + async def alist_jobs_filtered_after(self, **kwargs: Any) -> Page[JobResult]: + """Async version of :meth:`list_jobs_filtered_after`.""" + return await self._run_sync(self.list_jobs_filtered_after, **kwargs) + + async def adead_letters_after(self, limit: int = 10, after: str | None = None) -> Page[dict]: + """Async version of :meth:`dead_letters_after`.""" + return await self._run_sync(self.dead_letters_after, limit=limit, after=after) + + async def alist_archived_after( + self, limit: int = 50, after: str | None = None + ) -> Page[JobResult]: + """Async version of :meth:`list_archived_after`.""" + return await self._run_sync(self.list_archived_after, limit=limit, after=after) + async def ajob_dag(self, job_id: str) -> dict[str, Any]: """Async version of :meth:`job_dag`.""" return await self._run_sync(self.job_dag, job_id) diff --git a/sdks/python/taskito/dashboard/handlers/workflows.py b/sdks/python/taskito/dashboard/handlers/workflows.py index 89c1c782..db22c15c 100644 --- a/sdks/python/taskito/dashboard/handlers/workflows.py +++ b/sdks/python/taskito/dashboard/handlers/workflows.py @@ -48,12 +48,31 @@ def _node_to_dict(node: Any) -> dict[str, Any]: def _handle_list_workflow_runs(queue: Queue, qs: dict) -> dict[str, Any]: - """``GET /api/workflows/runs`` — paginated list with optional filters.""" + """``GET /api/workflows/runs`` — paginated list with optional filters. + + Supports both offset (``limit``/``offset``) and keyset (``after``) + pagination; when ``after`` is present it takes precedence and the response + carries a ``next_cursor`` to pass back as the next ``after``. + """ definition_name = qs.get("definition_name", [None])[0] state = qs.get("state", [None])[0] limit = _parse_int_qs(qs, "limit", 50) - offset = _parse_int_qs(qs, "offset", 0) + after = qs.get("after", [None])[0] + + if after is not None: + runs, next_cursor = queue._inner.list_workflow_runs_after( + definition_name=definition_name, + state=state, + limit=limit, + after=after, + ) + return { + "runs": [_run_to_dict(r) for r in runs], + "limit": limit, + "next_cursor": next_cursor, + } + offset = _parse_int_qs(qs, "offset", 0) runs = queue._inner.list_workflow_runs( definition_name=definition_name, state=state, diff --git a/sdks/python/taskito/mixins/inspection.py b/sdks/python/taskito/mixins/inspection.py index c9ad5682..e6d75e04 100644 --- a/sdks/python/taskito/mixins/inspection.py +++ b/sdks/python/taskito/mixins/inspection.py @@ -7,6 +7,7 @@ from typing import Any from taskito.mixins._shared import _UNSET +from taskito.pagination import Page from taskito.result import JobResult @@ -81,6 +82,63 @@ def list_jobs_filtered( ) return [JobResult(py_job=pj, queue=self) for pj in py_jobs] # type: ignore[arg-type] + def list_jobs_after( + self, + status: str | None = None, + queue: str | None = None, + task_name: str | None = None, + limit: int = 50, + after: str | None = None, + namespace: Any = _UNSET, + ) -> Page[JobResult]: + """Keyset-paginated :meth:`list_jobs`. + + Returns a :class:`~taskito.pagination.Page`; pass its ``next_cursor`` + back as ``after`` to fetch the next page. O(page) at any depth and + stable under concurrent inserts, unlike offset pagination. + """ + ns = self._namespace if namespace is _UNSET else namespace + py_jobs, next_cursor = self._inner.list_jobs_after( + status=status, + queue=queue, + task_name=task_name, + limit=limit, + after=after, + namespace=ns, + ) + items = [JobResult(py_job=pj, queue=self) for pj in py_jobs] # type: ignore[arg-type] + return Page(items=items, next_cursor=next_cursor) + + def list_jobs_filtered_after( + self, + status: str | None = None, + queue: str | None = None, + task_name: str | None = None, + metadata_like: str | None = None, + error_like: str | None = None, + created_after: int | None = None, + created_before: int | None = None, + limit: int = 50, + after: str | None = None, + namespace: Any = _UNSET, + ) -> Page[JobResult]: + """Keyset-paginated :meth:`list_jobs_filtered`.""" + ns = self._namespace if namespace is _UNSET else namespace + py_jobs, next_cursor = self._inner.list_jobs_filtered_after( + status=status, + queue=queue, + task_name=task_name, + metadata_like=metadata_like, + error_like=error_like, + created_after=created_after, + created_before=created_before, + limit=limit, + after=after, + namespace=ns, + ) + items = [JobResult(py_job=pj, queue=self) for pj in py_jobs] # type: ignore[arg-type] + return Page(items=items, next_cursor=next_cursor) + def stats(self) -> dict[str, int]: """Get queue statistics as a dict of status counts.""" return self._inner.stats() # type: ignore[no-any-return] diff --git a/sdks/python/taskito/mixins/operations.py b/sdks/python/taskito/mixins/operations.py index 31753d42..e07e32d0 100644 --- a/sdks/python/taskito/mixins/operations.py +++ b/sdks/python/taskito/mixins/operations.py @@ -6,6 +6,7 @@ from taskito.context import LogLevel from taskito.events import EventType +from taskito.pagination import Page from taskito.result import JobResult @@ -20,6 +21,15 @@ def dead_letters(self, limit: int = 10, offset: int = 0) -> list[dict]: """List dead letter queue entries.""" return self._inner.dead_letters(limit=limit, offset=offset) # type: ignore[no-any-return] + def dead_letters_after(self, limit: int = 10, after: str | None = None) -> Page[dict]: + """Keyset-paginated :meth:`dead_letters`. + + Returns a :class:`~taskito.pagination.Page` of entry dicts; pass its + ``next_cursor`` back as ``after`` for the next page. + """ + entries, next_cursor = self._inner.dead_letters_after(limit=limit, after=after) + return Page(items=entries, next_cursor=next_cursor) + def retry_dead(self, dead_id: str) -> str: """Re-enqueue a dead letter job. Returns new job ID.""" return self._inner.retry_dead(dead_id) # type: ignore[no-any-return] @@ -125,3 +135,9 @@ def list_archived(self, limit: int = 50, offset: int = 0) -> list[JobResult]: """List archived jobs with pagination.""" py_jobs = self._inner.list_archived(limit=limit, offset=offset) return [JobResult(py_job=pj, queue=self) for pj in py_jobs] # type: ignore[arg-type] + + def list_archived_after(self, limit: int = 50, after: str | None = None) -> Page[JobResult]: + """Keyset-paginated :meth:`list_archived`.""" + py_jobs, next_cursor = self._inner.list_archived_after(limit=limit, after=after) + items = [JobResult(py_job=pj, queue=self) for pj in py_jobs] # type: ignore[arg-type] + return Page(items=items, next_cursor=next_cursor) diff --git a/sdks/python/taskito/pagination.py b/sdks/python/taskito/pagination.py new file mode 100644 index 00000000..c8699f45 --- /dev/null +++ b/sdks/python/taskito/pagination.py @@ -0,0 +1,29 @@ +"""Cursor-based pagination result.""" + +from __future__ import annotations + +from collections.abc import Iterator +from dataclasses import dataclass +from typing import Generic, TypeVar + +T = TypeVar("T") + + +@dataclass(frozen=True) +class Page(Generic[T]): + """One page of a keyset-paginated listing. + + Pass :attr:`next_cursor` back as ``after=`` to fetch the following page. + ``next_cursor`` is ``None`` once there are no more rows. Treat the cursor + string as opaque — do not construct or parse it yourself. + """ + + items: list[T] + next_cursor: str | None + + def __iter__(self) -> Iterator[T]: + """Iterate the page's items directly.""" + return iter(self.items) + + def __len__(self) -> int: + return len(self.items) diff --git a/sdks/python/tests/core/test_pagination.py b/sdks/python/tests/core/test_pagination.py new file mode 100644 index 00000000..f0f311f8 --- /dev/null +++ b/sdks/python/tests/core/test_pagination.py @@ -0,0 +1,227 @@ +"""Keyset (cursor) pagination — S12.""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from taskito import Queue +from taskito.pagination import Page + +PollUntil = Any # the conftest fixture's runtime type + + +def _page_all_job_ids(queue: Queue, **kwargs: Any) -> list[str]: + """Page through ``list_jobs_after`` fully, returning every job id seen.""" + seen: list[str] = [] + cursor: str | None = None + while True: + page = queue.list_jobs_after(after=cursor, **kwargs) + assert isinstance(page, Page) + seen.extend(j.id for j in page.items) + cursor = page.next_cursor + if cursor is None: + break + return seen + + +def test_page_is_iterable_and_sized() -> None: + page: Page[int] = Page(items=[1, 2, 3], next_cursor="abc") + assert len(page) == 3 + assert list(page) == [1, 2, 3] + assert page.next_cursor == "abc" + + +def test_list_jobs_after_pages_every_job_once(tmp_path: Path) -> None: + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def noop() -> None: + pass + + expected = {noop.delay().id for _ in range(25)} + + seen = _page_all_job_ids(queue, status="pending", limit=10) + assert set(seen) == expected + assert len(seen) == len(expected), "no job may appear on two pages" + + +def test_list_jobs_after_empty(tmp_path: Path) -> None: + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def noop() -> None: + pass + + page = queue.list_jobs_after(status="pending", limit=10) + assert page.items == [] + assert page.next_cursor is None + + +def test_list_jobs_after_last_page_has_no_cursor(tmp_path: Path) -> None: + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def noop() -> None: + pass + + for _ in range(3): + noop.delay() + + # Fewer rows than the limit → this is the last page, cursor is None. + page = queue.list_jobs_after(status="pending", limit=10) + assert len(page.items) == 3 + assert page.next_cursor is None + + +def test_list_jobs_after_descending_order(tmp_path: Path) -> None: + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def noop() -> None: + pass + + for _ in range(10): + noop.delay() + + page = queue.list_jobs_after(status="pending", limit=10) + # UUIDv7 ids are time-ordered, and the page is (created_at, id) descending, + # so the ids come back strictly descending (newest first). + ids = [j.id for j in page.items] + assert ids == sorted(ids, reverse=True), "page must be newest-first" + + +def test_list_jobs_after_stable_under_insert(tmp_path: Path) -> None: + """Rows inserted mid-pagination never duplicate or skip earlier rows.""" + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def noop() -> None: + pass + + original = {noop.delay().id for _ in range(20)} + + seen: list[str] = [] + cursor: str | None = None + inserted = False + while True: + page = queue.list_jobs_after(status="pending", limit=5, after=cursor) + seen.extend(j.id for j in page.items) + cursor = page.next_cursor + if not inserted: + # Insert newer rows; they sort ahead of the cursor and must not + # appear in the remaining pages of the original scan. + for _ in range(5): + noop.delay() + inserted = True + if cursor is None: + break + + # Every original row seen exactly once. Newer inserts may or may not appear + # depending on where they sort, but the originals must be complete + unique. + for job_id in original: + assert seen.count(job_id) == 1 + + +def test_list_jobs_filtered_after(tmp_path: Path) -> None: + queue = Queue(db_path=str(tmp_path / "t.db")) + + @queue.task() + def alpha() -> None: + pass + + @queue.task() + def beta() -> None: + pass + + alpha_ids = {alpha.delay().id for _ in range(8)} + for _ in range(8): + beta.delay() + + seen: list[str] = [] + cursor: str | None = None + while True: + page = queue.list_jobs_filtered_after( + status="pending", task_name=alpha.name, limit=3, after=cursor + ) + seen.extend(j.id for j in page.items) + cursor = page.next_cursor + if cursor is None: + break + + assert set(seen) == alpha_ids + + +def test_dead_letters_after(tmp_path: Path, poll_until: PollUntil) -> None: + queue = Queue(db_path=str(tmp_path / "t.db"), workers=2) + + @queue.task(max_retries=0) + def boom() -> None: + raise ValueError("boom") + + for _ in range(6): + boom.delay() + + import threading + + thread = threading.Thread(target=queue.run_worker, daemon=True) + thread.start() + try: + poll_until( + lambda: len(queue.dead_letters(limit=100)) >= 6, + timeout=15, + message="jobs never dead-lettered", + ) + finally: + queue.shutdown() + thread.join(timeout=5) + + seen: list[str] = [] + cursor: str | None = None + while True: + page = queue.dead_letters_after(limit=2, after=cursor) + assert isinstance(page, Page) + seen.extend(entry["id"] for entry in page.items) + cursor = page.next_cursor + if cursor is None: + break + + assert len(seen) >= 6 + assert len(set(seen)) == len(seen), "no DLQ entry may appear twice" + + +def test_list_archived_after(tmp_path: Path, poll_until: PollUntil) -> None: + queue = Queue(db_path=str(tmp_path / "t.db"), workers=2) + + @queue.task() + def ok(x: int) -> int: + return x + + for i in range(6): + ok.delay(i) + + import threading + + thread = threading.Thread(target=queue.run_worker, daemon=True) + thread.start() + try: + poll_until( + lambda: len(queue.list_archived(limit=100)) >= 6, + timeout=15, + message="jobs never archived", + ) + finally: + queue.shutdown() + thread.join(timeout=5) + + seen: list[str] = [] + cursor: str | None = None + while True: + page = queue.list_archived_after(limit=2, after=cursor) + seen.extend(j.id for j in page.items) + cursor = page.next_cursor + if cursor is None: + break + + assert len(seen) >= 6 + assert len(set(seen)) == len(seen), "no archived job may appear twice" From 6ca1a0a789b99a36fa37cd50305151800bacab71 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:23:20 +0530 Subject: [PATCH 07/16] fix(storage): reject malformed keyset cursors A negative sort key or empty id is not a cursor this crate encodes; decoding one paged from a position no row holds instead of erroring. --- crates/taskito-core/src/storage/cursor.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/taskito-core/src/storage/cursor.rs b/crates/taskito-core/src/storage/cursor.rs index b37748c1..7ccf1781 100644 --- a/crates/taskito-core/src/storage/cursor.rs +++ b/crates/taskito-core/src/storage/cursor.rs @@ -23,6 +23,11 @@ pub fn decode_cursor(cursor: &str) -> Result<(i64, &str)> { let sort_key: i64 = key .parse() .map_err(|_| QueueError::Other(format!("invalid cursor: {cursor}")))?; + // A cursor this crate did not encode: sort keys are non-negative millis and + // every row has an id. Reject rather than page from a position no row holds. + if sort_key < 0 || id.is_empty() { + return Err(QueueError::Other(format!("invalid cursor: {cursor}"))); + } Ok((sort_key, id)) } @@ -42,5 +47,7 @@ mod tests { fn rejects_malformed() { assert!(decode_cursor("nocolon").is_err()); assert!(decode_cursor("notanint:abc").is_err()); + assert!(decode_cursor("-1:abc").is_err()); + assert!(decode_cursor("1:").is_err()); } } From 6738db368b6096a2bc7dcba1a1e511012095508c Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:23:37 +0530 Subject: [PATCH 08/16] fix(storage): drop non-positive limits before keyset queries SQLite reads a negative LIMIT as unbounded, so a paged call became a full scan. Matches the normalization list_dead_by_task already documents. --- crates/taskito-core/src/storage/diesel_common/archival.rs | 3 +++ .../taskito-core/src/storage/diesel_common/dead_letter.rs | 3 +++ crates/taskito-core/src/storage/diesel_common/jobs.rs | 6 ++++++ 3 files changed, 12 insertions(+) diff --git a/crates/taskito-core/src/storage/diesel_common/archival.rs b/crates/taskito-core/src/storage/diesel_common/archival.rs index 38253bd6..763409ed 100644 --- a/crates/taskito-core/src/storage/diesel_common/archival.rs +++ b/crates/taskito-core/src/storage/diesel_common/archival.rs @@ -30,6 +30,9 @@ macro_rules! impl_diesel_archival_ops { limit: i64, after: Option<(i64, &str)>, ) -> Result> { + if limit <= 0 { + return Ok(Vec::new()); + } let mut conn = self.conn()?; let mut query = archived_jobs::table diff --git a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs index 4a9f53eb..5353a696 100644 --- a/crates/taskito-core/src/storage/diesel_common/dead_letter.rs +++ b/crates/taskito-core/src/storage/diesel_common/dead_letter.rs @@ -108,6 +108,9 @@ macro_rules! impl_diesel_dead_letter_ops { limit: i64, after: Option<(i64, &str)>, ) -> Result> { + if limit <= 0 { + return Ok(Vec::new()); + } let mut conn = self.conn()?; let mut query = dead_letter::table diff --git a/crates/taskito-core/src/storage/diesel_common/jobs.rs b/crates/taskito-core/src/storage/diesel_common/jobs.rs index 046903b3..f67a21db 100644 --- a/crates/taskito-core/src/storage/diesel_common/jobs.rs +++ b/crates/taskito-core/src/storage/diesel_common/jobs.rs @@ -1347,6 +1347,12 @@ macro_rules! impl_diesel_job_ops { after: Option<(i64, &str)>, namespace: Option<&str>, ) -> Result> { + // A non-positive limit yields no page on every backend. SQLite + // reads a negative LIMIT as unbounded, which would turn a paged + // call into a full scan. + if limit <= 0 { + return Ok(Vec::new()); + } match status { Some(s) if Self::is_terminal_status(s) => self.list_archived_filtered_after( Some(s), From b97e4946fa2f6812a304f7a0c698e26866728a8d Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:23:55 +0530 Subject: [PATCH 09/16] fix(redis): recheck job status after hydration in list_jobs_after The status index is read before each job, so a job can change status in between. list_jobs_filtered_after already rechecks; this sibling did not. --- crates/taskito-core/src/storage/redis_backend/jobs/query.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs index 5ba45a6a..ffbceef1 100644 --- a/crates/taskito-core/src/storage/redis_backend/jobs/query.rs +++ b/crates/taskito-core/src/storage/redis_backend/jobs/query.rs @@ -78,7 +78,10 @@ impl RedisStorage { let mut conn = self.conn()?; let mut jobs = self.load_status_candidates(&mut conn, status)?; jobs.retain(|job| { - queue_name.is_none_or(|q| job.queue == q) + // Re-check the status against the hydrated job: the index is read + // before each job, so a job can change status in between. + status.is_none_or(|s| job.status as i32 == s) + && queue_name.is_none_or(|q| job.queue == q) && task_name.is_none_or(|t| job.task_name == t) && namespace.is_none_or(|ns| job.namespace.as_deref() == Some(ns)) }); From bda427e9e99b6c327093b0a2e7decdfaa7ebaafb Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:04 +0530 Subject: [PATCH 10/16] fix(redis): use checked arithmetic for DLQ expiry failed_at + ttl could overflow and purge an entry early. Mirrors the completed-job purge, which treats an overflowing TTL as never expiring. --- .../taskito-core/src/storage/redis_backend/dead_letter.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs index 48bdd777..875c7311 100644 --- a/crates/taskito-core/src/storage/redis_backend/dead_letter.rs +++ b/crates/taskito-core/src/storage/redis_backend/dead_letter.rs @@ -429,8 +429,13 @@ impl RedisStorage { let data: Option = conn.get(&dlq_key).map_err(map_err)?; if let Some(d) = data { if let Ok(entry) = serde_json::from_str::(&d) { + // An overflowing TTL is treated as never expiring, as in + // the completed-job purge. let expired = match entry.result_ttl_ms { - Some(ttl) => entry.failed_at + ttl <= now, + Some(ttl) => entry + .failed_at + .checked_add(ttl) + .is_some_and(|expiry| expiry <= now), None => entry.failed_at < global_cutoff_ms, }; if expired { From 4c53f837c2127e83dab9c4ea1e1944c92fb2cd93 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:15 +0530 Subject: [PATCH 11/16] perf(redis): seek keyset pages by rank Mass-cancel stamps thousands of archived rows with one completed_at, so the equal-score tie bucket is unbounded. ZREVRANK seeks the cursor directly; the score scan stays as a fallback for a purged cursor row. --- .../src/storage/redis_backend/mod.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/crates/taskito-core/src/storage/redis_backend/mod.rs b/crates/taskito-core/src/storage/redis_backend/mod.rs index 7b1b38f4..f291e75f 100644 --- a/crates/taskito-core/src/storage/redis_backend/mod.rs +++ b/crates/taskito-core/src/storage/redis_backend/mod.rs @@ -163,8 +163,21 @@ fn zset_keyset_page( .map_err(map_err); }; - // Tie bucket first (same score, id < cursor_id), newest id first. Bounded by - // how many rows share one exact-millisecond score. + // Seek by rank while the cursor row is still indexed. `ZREVRANK` orders by + // (score DESC, member DESC) — exactly the page order — so the next page is + // the `limit` members that follow it, read in one bounded range. The rank is + // re-derived every call, so concurrent inserts shift it without skipping or + // repeating rows below the cursor. + let rank: Option = conn.zrevrank(zkey, cursor_id).map_err(map_err)?; + if let Some(rank) = rank { + return conn + .zrevrange(zkey, rank + 1, rank + limit as isize) + .map_err(map_err); + } + + // The cursor row was purged between pages, so there is no rank to seek from. + // Fall back to the score bounds: the tie bucket (same score, id < cursor_id) + // first, then everything strictly below that score. let same_score: Vec = conn .zrangebyscore(zkey, score as f64, score as f64) .map_err(map_err)?; @@ -175,7 +188,6 @@ fn zset_keyset_page( page.reverse(); // ascending lex → descending id page.truncate(limit as usize); - // Then rows strictly below the cursor score, newest first. if (page.len() as i64) < limit { let remaining = limit - page.len() as i64; let lower: Vec = conn From 612e51329216b38bcbb03ce3bd8ce7748a74a9aa Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:24 +0530 Subject: [PATCH 12/16] perf(redis): window and pipeline filtered log scans The filtered path hydrated every log since the cutoff with one GET each before truncating. Walk newest-first in bounded windows, hydrate each in one pipeline, and stop once the limit is met. --- .../src/storage/redis_backend/logs.rs | 61 +++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/crates/taskito-core/src/storage/redis_backend/logs.rs b/crates/taskito-core/src/storage/redis_backend/logs.rs index ac938a1a..d22adfb3 100644 --- a/crates/taskito-core/src/storage/redis_backend/logs.rs +++ b/crates/taskito-core/src/storage/redis_backend/logs.rs @@ -163,40 +163,53 @@ impl RedisStorage { return Ok(rows); } - // Filtered path: task_name/level are not indexed, so scan the cutoff - // range and filter in memory. - let ids: Vec = conn - .zrangebyscore(&all_key, since_ms as f64, "+inf") - .map_err(map_err)?; - + // Filtered path: task_name/level are not indexed, so the cutoff range is + // walked newest-first in bounded windows and filtered in memory. Each + // window is hydrated in one pipelined round trip and the walk stops as + // soon as `limit` rows match, so neither memory nor round trips scale + // with the log history. let mut rows = Vec::new(); - for id in ids { - let log_key = self.key(&["log", &id]); - let data: Option = conn.get(&log_key).map_err(map_err)?; - if let Some(d) = data { + let mut offset: isize = 0; + loop { + let ids: Vec = conn + .zrevrangebyscore_limit(&all_key, "+inf", since_ms as f64, offset, SCAN_BATCH) + .map_err(map_err)?; + if ids.is_empty() { + break; + } + offset += ids.len() as isize; + + let log_keys: Vec = ids.iter().map(|id| self.key(&["log", id])).collect(); + let mut pipe = redis::pipe(); + for key in &log_keys { + pipe.get(key); + } + let entries: Vec> = pipe.query(&mut conn).map_err(map_err)?; + + for data in entries.into_iter().flatten() { let entry: LogEntry = - serde_json::from_str(&d).map_err(|e| QueueError::Other(e.to_string()))?; + serde_json::from_str(&data).map_err(|e| QueueError::Other(e.to_string()))?; - if let Some(n) = task_name { - if entry.task_name != n { - continue; - } + if task_name.is_some_and(|n| entry.task_name != n) { + continue; } - if let Some(l) = level { - if entry.level != l { - continue; - } + if level.is_some_and(|l| entry.level != l) { + continue; } rows.push(TaskLogRow::from(entry)); + if limit >= 0 && rows.len() as i64 == limit { + return Ok(rows); + } } - } - // Sort by logged_at desc - rows.sort_by_key(|r| std::cmp::Reverse(r.logged_at)); - if limit >= 0 { - rows.truncate(limit as usize); + if (ids.len() as isize) < SCAN_BATCH { + break; + } } + + // ZREVRANGEBYSCORE walks newest-first, so the rows are already + // logged_at desc. Ok(rows) } From fc08e182a6aeb214e827e3bf4d5b6f117879cb8a Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:36 +0530 Subject: [PATCH 13/16] docs(storage): note the Redis keyset pagination exception The trait promised O(page) at any depth, which Redis does not honour for status-filtered job listings or workflow runs. A seekable index needs a backfill migration; state the exception rather than over-promise. --- crates/taskito-core/src/storage/traits.rs | 12 ++++++++++-- crates/taskito-workflows/src/storage.rs | 8 +++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/crates/taskito-core/src/storage/traits.rs b/crates/taskito-core/src/storage/traits.rs index 687470f6..af8877d5 100644 --- a/crates/taskito-core/src/storage/traits.rs +++ b/crates/taskito-core/src/storage/traits.rs @@ -88,8 +88,16 @@ pub trait Storage: Send + Sync + Clone { /// Keyset-paginated `list_jobs`, ordered by `(created_at, id)` descending. /// `after` is the `(created_at, id)` of the previous page's last row; the /// caller derives the next cursor from the returned rows' last element. - /// O(page) at any depth, and stable under concurrent inserts (unlike the - /// offset form). Rows are blob-free like every listing. + /// Stable under concurrent inserts (unlike the offset form), and O(page) at + /// any depth on the Diesel backends. Rows are blob-free like every listing. + /// + /// **Redis exception:** the job status indexes are plain SETs with no + /// ordering to seek, so this applies the keyset in memory over the same + /// candidate set the offset form loads — correct and stable, but O(matching + /// rows) rather than O(page). Redis `list_jobs` is already O(N), so this is + /// no worse; a seekable index requires a backfill migration of pre-existing + /// rows. `list_archived_after` and `list_dead_after` are seekable on Redis + /// and carry no such exception. fn list_jobs_after( &self, status: Option, diff --git a/crates/taskito-workflows/src/storage.rs b/crates/taskito-workflows/src/storage.rs index 629f735d..c8567e48 100644 --- a/crates/taskito-workflows/src/storage.rs +++ b/crates/taskito-workflows/src/storage.rs @@ -40,7 +40,13 @@ pub trait WorkflowStorage: Send + Sync { /// Keyset-paginated `list_workflow_runs`, ordered by `(created_at, id)` /// descending. `after` is the `(created_at, id)` of the previous page's /// last run; the caller derives the next cursor from the returned rows' - /// last element. O(page) at any depth, stable under concurrent inserts. + /// last element. Stable under concurrent inserts, and O(page) at any depth + /// on the Diesel backends. + /// + /// **Redis exception:** the `by_state` index is re-scored to the transition + /// timestamp on every state change, so its order is not `created_at` and + /// cannot be seeked. Redis applies the keyset in memory over the candidate + /// set — correct and stable, but O(matching runs) rather than O(page). fn list_workflow_runs_after( &self, definition_name: Option<&str>, From b626f1e2773f1333fddb9a99fc7bcf516c941ba3 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:46 +0530 Subject: [PATCH 14/16] test(storage): assert keyset paging covers every created row The aggregate count assertions let rows from earlier cases mask a skipped one. Track the created ids and require each exactly once. --- .../taskito-core/tests/rust/storage_tests.rs | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/crates/taskito-core/tests/rust/storage_tests.rs b/crates/taskito-core/tests/rust/storage_tests.rs index 9ed3dfe5..a8314d32 100644 --- a/crates/taskito-core/tests/rust/storage_tests.rs +++ b/crates/taskito-core/tests/rust/storage_tests.rs @@ -8,7 +8,7 @@ //! when all tests share a single storage instance. use taskito_core::job::{now_millis, JobCompletion, JobStatus, NewJob}; -use taskito_core::storage::Storage; +use taskito_core::storage::{DeadJob, Storage}; use taskito_core::SqliteStorage; fn make_job(queue: &str, task_name: &str) -> NewJob { @@ -1377,45 +1377,56 @@ fn test_keyset_pagination_jobs(s: &impl Storage) { fn test_keyset_pagination_dlq_and_archive(s: &impl Storage) { let q = "q-keyset-terminal"; let total = 15; + let mut dead_job_ids = Vec::new(); for _ in 0..total { let job = s.enqueue(make_job(q, "keyset_terminal")).unwrap(); s.dequeue(q, now_millis() + 1000, None).unwrap(); let running = s.get_job(&job.id).unwrap().unwrap(); s.move_to_dlq(&running, "boom", None).unwrap(); + dead_job_ids.push(job.id); } - // DLQ paging. - let dlq_ids = page_all_dead(s, 6); - let dlq_from_this_q: Vec<&String> = dlq_ids.iter().collect(); - assert!( - dlq_from_this_q.len() >= total, - "keyset DLQ paging must cover every dead row" - ); - let unique: std::collections::HashSet<&String> = dlq_ids.iter().collect(); - assert_eq!(unique.len(), dlq_ids.len(), "DLQ keyset must not duplicate"); + // DLQ paging. Assert against the rows this test created: a `>= total` count + // over the whole table would let rows from earlier cases mask a skipped one. + let dlq = page_all_dead(s, 6); + let paged_originals: Vec<&String> = dlq.iter().map(|d| &d.original_job_id).collect(); + for job_id in &dead_job_ids { + assert_eq!( + paged_originals.iter().filter(|o| **o == job_id).count(), + 1, + "keyset DLQ paging must yield every dead row exactly once" + ); + } + let unique: std::collections::HashSet<&String> = dlq.iter().map(|d| &d.id).collect(); + assert_eq!(unique.len(), dlq.len(), "DLQ keyset must not duplicate"); // Archive paging: complete a fresh batch so archived rows exist. let qa = "q-keyset-archive"; + let mut archived_job_ids = Vec::new(); for _ in 0..total { let job = s.enqueue(make_job(qa, "keyset_archive")).unwrap(); s.dequeue(qa, now_millis() + 1000, None).unwrap(); s.complete(&job.id, None).unwrap(); + archived_job_ids.push(job.id); } let arch_ids = page_all_archived(s, 6); + for job_id in &archived_job_ids { + assert_eq!( + arch_ids.iter().filter(|id| *id == job_id).count(), + 1, + "keyset archive paging must yield every archived row exactly once" + ); + } let unique: std::collections::HashSet<&String> = arch_ids.iter().collect(); assert_eq!( unique.len(), arch_ids.len(), "archive keyset must not duplicate" ); - assert!( - arch_ids.len() >= total, - "keyset archive paging must cover every archived row" - ); } -/// Page the whole DLQ via `list_dead_after`, returning every dead id seen. -fn page_all_dead(s: &impl Storage, page_size: i64) -> Vec { +/// Page the whole DLQ via `list_dead_after`, returning every row seen. +fn page_all_dead(s: &impl Storage, page_size: i64) -> Vec { let mut seen = Vec::new(); let mut cursor: Option<(i64, String)> = None; loop { @@ -1426,10 +1437,9 @@ fn page_all_dead(s: &impl Storage, page_size: i64) -> Vec { } let last = page.last().unwrap(); cursor = Some((last.failed_at, last.id.clone())); - for d in &page { - seen.push(d.id.clone()); - } - if page.len() < page_size as usize { + let page_len = page.len(); + seen.extend(page); + if page_len < page_size as usize { break; } } From 18866b748b859b622b9cd61d1aaaa7c239474d56 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:24:53 +0530 Subject: [PATCH 15/16] fix(dashboard): allow requesting the first cursor page Only an after cursor selected keyset mode, so a client with no cursor yet could never enter it. paginate=cursor opens at the first page. --- .../taskito/dashboard/handlers/workflows.py | 10 +++--- .../tests/dashboard/test_workflows_api.py | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/sdks/python/taskito/dashboard/handlers/workflows.py b/sdks/python/taskito/dashboard/handlers/workflows.py index db22c15c..9ff0313c 100644 --- a/sdks/python/taskito/dashboard/handlers/workflows.py +++ b/sdks/python/taskito/dashboard/handlers/workflows.py @@ -50,16 +50,18 @@ def _node_to_dict(node: Any) -> dict[str, Any]: def _handle_list_workflow_runs(queue: Queue, qs: dict) -> dict[str, Any]: """``GET /api/workflows/runs`` — paginated list with optional filters. - Supports both offset (``limit``/``offset``) and keyset (``after``) - pagination; when ``after`` is present it takes precedence and the response - carries a ``next_cursor`` to pass back as the next ``after``. + Supports both offset (``limit``/``offset``) and keyset pagination. Keyset + mode is selected by ``paginate=cursor`` for the first page — a client has no + cursor yet — or by an ``after`` cursor for each page after it. The response + carries a ``next_cursor`` to pass back as the next ``after``, and ``None`` + once the last page is reached. """ definition_name = qs.get("definition_name", [None])[0] state = qs.get("state", [None])[0] limit = _parse_int_qs(qs, "limit", 50) after = qs.get("after", [None])[0] - if after is not None: + if after is not None or qs.get("paginate", [None])[0] == "cursor": runs, next_cursor = queue._inner.list_workflow_runs_after( definition_name=definition_name, state=state, diff --git a/sdks/python/tests/dashboard/test_workflows_api.py b/sdks/python/tests/dashboard/test_workflows_api.py index 1ed5f4af..fd024e1f 100644 --- a/sdks/python/tests/dashboard/test_workflows_api.py +++ b/sdks/python/tests/dashboard/test_workflows_api.py @@ -87,6 +87,42 @@ def test_list_workflow_runs_returns_paginated_list( assert isinstance(run["created_at"], int) +def test_list_workflow_runs_cursor_mode_starts_without_a_cursor( + workflows_dashboard: tuple[AuthedClient, Queue], +) -> None: + """``paginate=cursor`` opens keyset mode at the first page. + + Clients have no cursor to send for page one, so without this they could + never enter cursor mode at all. + """ + client, _ = workflows_dashboard + data = client.get("/api/workflows/runs?paginate=cursor") + assert "next_cursor" in data, "cursor mode must echo a next_cursor" + assert "offset" not in data, "cursor mode does not page by offset" + assert len(data["runs"]) >= 1 + + +def test_list_workflow_runs_cursor_pages_every_run_once( + workflows_dashboard: tuple[AuthedClient, Queue], +) -> None: + """Walking from the first cursor page reaches every run exactly once.""" + client, q = workflows_dashboard + expected = {r.id for r in q._inner.list_workflow_runs(limit=100, offset=0)} + + seen: list[str] = [] + query = "/api/workflows/runs?limit=1&paginate=cursor" + while True: + data = client.get(query) + seen.extend(r["id"] for r in data["runs"]) + cursor = data["next_cursor"] + if cursor is None: + break + query = f"/api/workflows/runs?limit=1&after={cursor}" + + assert len(seen) == len(set(seen)), "no run may appear on two pages" + assert set(seen) == expected + + def test_list_workflow_runs_respects_limit_offset( workflows_dashboard: tuple[AuthedClient, Queue], ) -> None: From 95a35b82aa1a45d4837b9a5d044bbbcfc8ffbddc Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:39:16 +0530 Subject: [PATCH 16/16] test(redis): page a same-score archived batch cancel_pending_by_queue archives under one now, so 600 rows share a completed_at. Pins the rank seek against the tie bucket that motivated it. --- .../taskito-core/tests/rust/storage_tests.rs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/taskito-core/tests/rust/storage_tests.rs b/crates/taskito-core/tests/rust/storage_tests.rs index a8314d32..4865e6e9 100644 --- a/crates/taskito-core/tests/rust/storage_tests.rs +++ b/crates/taskito-core/tests/rust/storage_tests.rs @@ -1554,6 +1554,32 @@ fn redis_storage_tests() { redis_move_to_dlq_leaves_consistent_state(&storage); redis_move_to_dlq_skips_already_archived(&storage); redis_purge_dead_drains_across_batches(&storage); + redis_keyset_pages_a_large_tie_bucket(&storage); +} + +/// S12: `cancel_pending_by_queue` archives a whole batch under a single `now`, +/// so every one of these rows lands in `archived:all` with the *same* score. +/// Paging must still yield each exactly once — the tie bucket is not bounded by +/// the clock, so a page that reads it whole would degrade with the batch size. +#[cfg(feature = "redis")] +fn redis_keyset_pages_a_large_tie_bucket(s: &taskito_core::RedisStorage) { + let q = "q-redis-tie-bucket"; + let total = 600; + let mut created = Vec::new(); + for _ in 0..total { + created.push(s.enqueue(make_job(q, "tie_task")).unwrap().id); + } + // One `now` for the whole batch → 600 archived rows sharing one score. + assert_eq!(s.cancel_pending_by_queue(q).unwrap(), total as u64); + + let paged = page_all_archived(s, 50); + for job_id in &created { + assert_eq!( + paged.iter().filter(|id| *id == job_id).count(), + 1, + "every row of a same-score batch must be paged exactly once" + ); + } } /// S15: the batched `purge_dead` must drain more than one SCAN_BATCH (500) of