Skip to content

Commit 701cc12

Browse files
insipxclaude
andauthored
feat: key-package maintenance as TaskRunner recurring tasks (#3814)
Stacked on #3806 (generic recurrence); the KpRotation/KpDeletion proto variants land in #3805. Replaces the 5s-poll `KeyPackageCleaner` worker with two recurring TaskRunner tasks. ## What - **`KpRotation`** — recurring singleton: rotates + uploads a fresh key package when the identity's rotation deadline (`next_key_package_rotation_ns`) is due, then reschedules to the live column (never a hardcoded +30d). On rotation it self-heals the deletion singleton and pulls it in to the earliest pending `delete_at_ns`. - **`KpDeletion`** — recurring singleton: sweeps expired local key-package material, reschedules to the next pending deadline. - **Welcome nudge** — after `queue_key_package_rotation` lowers the column (~5s debounce, a security property), a durable `PullInDeadline` pulls the rotation task in to match. Closes a real race: without it, a seed that dispatched before the column write parks ~30d out and the debounce is silently lost (reproduced; regression-tested). - **Builder** seeds both singletons + reconciles deadlines to the live DB columns on every startup (idempotent; also repairs rows stranded by a crash between a column write and its pull-in). - **Gutted** the old 5s-poll worker: loop/factory/rotate path deleted; the struct + local-deletion helpers remain as the `KpDeletion` arm's implementation. ## Behavior notes - KP maintenance no longer has its own toggle — it is a critical MLS function, deliberately coupled to the TaskRunner (no standalone fallback). `WorkerKind::KeyPackageCleaner` remains in core and all three bindings' enums for FFI compatibility, but enabling/disabling it is now a no-op. Disabling the TaskRunner itself — per-kind via worker config or globally via `disable_workers` — disables KP maintenance with it. - Pre-registration (fresh client, no identity row): readers return `None`/false, the rotation seed parks ~30d and converges silently once registration writes the column. No transient errors. - Zero migrations; zero reaper changes — recurring rows are never-expiring seeds (`expires_at_ns = i64::MAX`, `max_attempts = i32::MAX`), so the existing reaper predicate can't fire on them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> > [!NOTE] > ### Replace key-package cleaner worker with TaskRunner recurring tasks for rotation and deletion > - Removes the legacy `KeyPackagesCleanerWorker` and replaces it with durable `KpRotation` and `KpDeletion` task rows managed by the `TaskRunner` in [key_package_maintenance.rs](https://github.com/xmtp/libxmtp/pull/3814/files#diff-1d4be0f0b8283620c52870358d20c933cc8086d19b8c804906ce1fc6d99af0eb). > - `KpRotation` tasks reschedule to the DB-backed `next_key_package_rotation_ns` deadline; `KpDeletion` tasks reschedule to the earliest pending `delete_at_ns` across marked key packages. > - `seed_and_reconcile_kp_tasks` is called at startup (after `TaskWorker` registration) to seed both recurring tasks and pull in deadlines from live DB state. > - `queue_key_rotation` is now synchronous and atomically updates the rotation column and enqueues a `PullInDeadline` nudge; `nudge_deletion` does the same for deletion. > - `is_identity_needs_rotation` now returns `false` pre-registration and treats a `NULL` rotation column on an existing row as due immediately, fixing a bug where migrated DBs with `NULL` deadlines were skipped. > - Behavioral Change: key-package maintenance no longer runs as a standalone polling worker; it is now driven entirely by the `TaskRunner` dispatch loop. > > <!-- Macroscope's changelog starts here --> > #### Changes since #3814 opened > > - Modified key rotation queueing to insert-or-ignore rotation seed tasks before enqueuing pull-in tasks [3cb7a61] > - Added tests verifying rotation seed self-healing behavior during key rotation nudges [3cb7a61] > - Changed `KpDeletion` task rescheduling to use rotation interval constant instead of fixed 30-day value [3cb7a61] > - Updated mock implementation to match new trait signature [3cb7a61] > <!-- Macroscope's changelog ends here --> > > <!-- Macroscope's review summary starts here --> > > <sup><a href="https://app.macroscope.com">Macroscope</a> summarized 74a0d28.</sup> > <!-- Macroscope's review summary ends here --> > > <!-- macroscope-ui-refresh --> <!-- Macroscope's pull request summary ends here --> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent f46d623 commit 701cc12

14 files changed

Lines changed: 852 additions & 299 deletions

File tree

crates/xmtp_db/src/encrypted_store/identity.rs

Lines changed: 196 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,26 @@ impl StoredIdentity {
4747
}
4848
pub trait QueryIdentity {
4949
fn queue_key_package_rotation(&self) -> Result<(), StorageError>;
50+
/// Atomically lower/initialize the rotation column (5s debounce) AND enqueue a
51+
/// `PullInDeadline` task targeting `rotation_task_hash` at the resulting column
52+
/// value — one transaction, so neither write can land without the other.
53+
/// `rotation_seed` is insert-or-ignored first so the pull-in always has a live
54+
/// target (commit-target-first), even if startup seeding never ran.
55+
/// Callers wake the TaskWorker AFTER this returns (never inside a tx).
56+
fn queue_key_rotation_with_nudge(
57+
&self,
58+
rotation_task_hash: &crate::tasks::TaskDataHash,
59+
rotation_seed: crate::tasks::NewTask,
60+
) -> Result<(), StorageError>;
5061
fn reset_key_package_rotation_queue(
5162
&self,
5263
rotation_interval_ns: i64,
5364
) -> Result<(), StorageError>;
5465
fn is_identity_needs_rotation(&self) -> Result<bool, StorageError>;
66+
/// The identity's absolute rotation deadline (`next_key_package_rotation_ns`).
67+
/// `None` if NULL or if no identity row exists yet (indistinguishable to callers;
68+
/// treat as "no scheduled deadline").
69+
fn next_key_package_rotation_ns(&self) -> Result<Option<i64>, StorageError>;
5570
}
5671

5772
impl<T> QueryIdentity for &T
@@ -62,6 +77,14 @@ where
6277
(**self).queue_key_package_rotation()
6378
}
6479

80+
fn queue_key_rotation_with_nudge(
81+
&self,
82+
rotation_task_hash: &crate::tasks::TaskDataHash,
83+
rotation_seed: crate::tasks::NewTask,
84+
) -> Result<(), StorageError> {
85+
(**self).queue_key_rotation_with_nudge(rotation_task_hash, rotation_seed)
86+
}
87+
6588
fn reset_key_package_rotation_queue(
6689
&self,
6790
rotation_interval_ns: i64,
@@ -72,14 +95,24 @@ where
7295
fn is_identity_needs_rotation(&self) -> Result<bool, StorageError> {
7396
(**self).is_identity_needs_rotation()
7497
}
98+
99+
fn next_key_package_rotation_ns(&self) -> Result<Option<i64>, StorageError> {
100+
(**self).next_key_package_rotation_ns()
101+
}
75102
}
76103

77104
impl<C: ConnectionExt> QueryIdentity for DbConnection<C> {
78105
fn queue_key_package_rotation(&self) -> Result<(), StorageError> {
79106
self.raw_query(|conn| {
80107
let rotate_at_ns = now_ns() + KEY_PACKAGE_QUEUE_INTERVAL_NS;
108+
// NULL (migrated DBs) counts as unscheduled: initialize it here so the
109+
// 5s debounce applies and nudge payloads stay stable (coalescing).
81110
diesel::update(dsl::identity)
82-
.filter(dsl::next_key_package_rotation_ns.gt(rotate_at_ns))
111+
.filter(
112+
dsl::next_key_package_rotation_ns
113+
.gt(rotate_at_ns)
114+
.or(dsl::next_key_package_rotation_ns.is_null()),
115+
)
83116
.set(dsl::next_key_package_rotation_ns.eq(rotate_at_ns))
84117
.execute(conn)?;
85118

@@ -89,6 +122,69 @@ impl<C: ConnectionExt> QueryIdentity for DbConnection<C> {
89122
Ok(())
90123
}
91124

125+
fn queue_key_rotation_with_nudge(
126+
&self,
127+
rotation_task_hash: &crate::tasks::TaskDataHash,
128+
rotation_seed: crate::tasks::NewTask,
129+
) -> Result<(), StorageError> {
130+
use crate::schema::tasks;
131+
use diesel::Connection;
132+
use xmtp_proto::xmtp::mls::database::{PullInDeadline, Task as TaskProto, task::Task};
133+
134+
let hash = rotation_task_hash.to_vec();
135+
self.raw_query(|conn| {
136+
conn.transaction::<_, diesel::result::Error, _>(|conn| {
137+
let rotate_at_ns = now_ns() + KEY_PACKAGE_QUEUE_INTERVAL_NS;
138+
diesel::update(dsl::identity)
139+
.filter(
140+
dsl::next_key_package_rotation_ns
141+
.gt(rotate_at_ns)
142+
.or(dsl::next_key_package_rotation_ns.is_null()),
143+
)
144+
.set(dsl::next_key_package_rotation_ns.eq(rotate_at_ns))
145+
.execute(conn)?;
146+
147+
// Read back inside the tx: the column is stable between rotations,
148+
// so repeat calls produce byte-identical pull-ins that coalesce.
149+
let deadline: Option<Option<i64>> = dsl::identity
150+
.select(dsl::next_key_package_rotation_ns)
151+
.first::<Option<i64>>(conn)
152+
.optional()?;
153+
// Pre-registration (no identity row): match the old zero-rows-
154+
// matched no-op instead of erroring; nothing to rotate yet.
155+
let Some(deadline) = deadline else {
156+
return Ok(());
157+
};
158+
159+
// Ensure the pull-in's target exists (no-op when already seeded):
160+
// a client whose startup seeding never ran must not enqueue a
161+
// dropped-on-miss nudge.
162+
diesel::insert_or_ignore_into(tasks::table)
163+
.values(rotation_seed)
164+
.execute(conn)?;
165+
166+
let pull_in = crate::tasks::NewTask::builder()
167+
.originating_message_sequence_id(0)
168+
.originating_message_originator_id(0)
169+
.expires_at_ns(crate::tasks::NEVER_EXPIRES)
170+
.max_attempts(i32::MAX)
171+
.build(TaskProto {
172+
task: Some(Task::PullInDeadline(PullInDeadline {
173+
target_data_hash: hash,
174+
not_later_than_ns: deadline.unwrap_or(rotate_at_ns),
175+
})),
176+
})
177+
// All required builder fields are set above; unreachable.
178+
.map_err(|_| diesel::result::Error::RollbackTransaction)?;
179+
diesel::insert_or_ignore_into(tasks::table)
180+
.values(pull_in)
181+
.execute(conn)?;
182+
Ok(())
183+
})
184+
})?;
185+
Ok(())
186+
}
187+
92188
fn reset_key_package_rotation_queue(
93189
&self,
94190
rotation_interval_ns: i64,
@@ -113,17 +209,33 @@ impl<C: ConnectionExt> QueryIdentity for DbConnection<C> {
113209
fn is_identity_needs_rotation(&self) -> Result<bool, StorageError> {
114210
use crate::schema::identity::dsl;
115211

116-
let next_rotation_opt: Option<i64> = self.raw_query(|conn| {
212+
let next_rotation_opt: Option<Option<i64>> = self.raw_query(|conn| {
117213
dsl::identity
118214
.select(dsl::next_key_package_rotation_ns)
119215
.first::<Option<i64>>(conn)
216+
.optional()
120217
})?;
121218

122219
Ok(match next_rotation_opt {
123-
Some(rotate_at) => now_ns() >= rotate_at,
124-
None => true,
220+
// No identity row (pre-registration): nothing to rotate yet.
221+
None => false,
222+
// NULL column on an existing row: rotation is due now.
223+
Some(None) => true,
224+
Some(Some(rotate_at)) => now_ns() >= rotate_at,
125225
})
126226
}
227+
228+
fn next_key_package_rotation_ns(&self) -> Result<Option<i64>, StorageError> {
229+
use crate::schema::identity::dsl;
230+
// Use optional() so an empty table (pre-registration) returns Ok(None).
231+
let v: Option<Option<i64>> = self.raw_query(|conn| {
232+
dsl::identity
233+
.select(dsl::next_key_package_rotation_ns)
234+
.first::<Option<i64>>(conn)
235+
.optional()
236+
})?;
237+
Ok(v.flatten())
238+
}
127239
}
128240

129241
#[cfg(test)]
@@ -132,6 +244,86 @@ pub(crate) mod tests {
132244
use crate::{Store, XmtpTestDb};
133245
use xmtp_common::rand_vec;
134246

247+
/// A stand-in rotation seed for exercising `queue_key_rotation_with_nudge`
248+
/// (the real seed payload lives in xmtp_mls).
249+
fn test_rotation_seed() -> crate::tasks::NewTask {
250+
use xmtp_proto::xmtp::mls::database::{KpRotation, Task as TaskProto, task::Task};
251+
crate::tasks::NewTask::builder()
252+
.originating_message_sequence_id(0)
253+
.originating_message_originator_id(0)
254+
.expires_at_ns(crate::tasks::NEVER_EXPIRES)
255+
.max_attempts(i32::MAX)
256+
.next_attempt_at_ns(0)
257+
.build(TaskProto {
258+
task: Some(Task::KpRotation(KpRotation {})),
259+
})
260+
.unwrap()
261+
}
262+
263+
#[xmtp_common::test]
264+
fn queue_with_nudge_is_noop_before_registration() {
265+
use crate::prelude::{QueryIdentity, QueryTasks};
266+
use crate::test_utils::with_connection;
267+
with_connection(|conn| {
268+
// Empty identity table (pre-registration): must be a no-op like the
269+
// old column-only path, not a NotFound error. The seed must NOT be
270+
// inserted either — pre-registration means zero writes.
271+
let hash = crate::tasks::TaskDataHash::try_from([0x11u8; 32].as_slice()).unwrap();
272+
conn.queue_key_rotation_with_nudge(&hash, test_rotation_seed())
273+
.unwrap();
274+
assert!(
275+
conn.get_tasks().unwrap().is_empty(),
276+
"no pull-in without an identity row"
277+
);
278+
})
279+
}
280+
281+
#[xmtp_common::test]
282+
fn queue_with_nudge_selfheals_missing_seed() {
283+
use crate::prelude::{QueryIdentity, QueryTasks};
284+
use crate::test_utils::with_connection;
285+
with_connection(|conn| {
286+
StoredIdentity::new("".to_string(), rand_vec::<24>(), rand_vec::<24>())
287+
.store(conn)
288+
.unwrap();
289+
let seed = test_rotation_seed();
290+
let hash = crate::tasks::TaskDataHash::try_from(seed.data_hash.as_slice()).unwrap();
291+
conn.queue_key_rotation_with_nudge(&hash, seed).unwrap();
292+
let tasks = conn.get_tasks().unwrap();
293+
assert!(
294+
tasks.iter().any(|t| t.data_hash == hash.as_ref()),
295+
"nudge must insert the missing rotation seed (pull-in target)"
296+
);
297+
assert_eq!(tasks.len(), 2, "seed + pull-in");
298+
})
299+
}
300+
301+
#[xmtp_common::test]
302+
fn queue_initializes_null_rotation_column() {
303+
use crate::prelude::QueryIdentity;
304+
use crate::test_utils::with_connection;
305+
use xmtp_configuration::KEY_PACKAGE_QUEUE_INTERVAL_NS;
306+
with_connection(|conn| {
307+
StoredIdentity::new("".to_string(), rand_vec::<24>(), rand_vec::<24>())
308+
.store(conn)
309+
.unwrap();
310+
311+
// Migrated DBs have NULL here; queueing must initialize it (5s
312+
// debounce) rather than skip the row.
313+
conn.queue_key_package_rotation().unwrap();
314+
let v = conn
315+
.next_key_package_rotation_ns()
316+
.unwrap()
317+
.expect("NULL column must be initialized");
318+
let now = xmtp_common::time::now_ns();
319+
assert!(v > now && v <= now + KEY_PACKAGE_QUEUE_INTERVAL_NS);
320+
321+
// Lower-only: a later queue call never raises the deadline.
322+
conn.queue_key_package_rotation().unwrap();
323+
assert_eq!(conn.next_key_package_rotation_ns().unwrap().unwrap(), v);
324+
})
325+
}
326+
135327
#[xmtp_common::test]
136328
async fn can_only_store_one_identity() {
137329
let store = crate::TestDb::create_ephemeral_store().await;

crates/xmtp_db/src/encrypted_store/key_package_history.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ pub trait QueryKeyPackageHistory {
4747

4848
fn get_expired_key_packages(&self) -> Result<Vec<StoredKeyPackageHistoryEntry>, StorageError>;
4949

50+
/// Soonest pending `delete_at_ns` across all key packages marked for deletion,
51+
/// or `None` if none are marked. The KpDeletion task's reschedule source.
52+
fn min_key_package_delete_at_ns(&self) -> Result<Option<i64>, StorageError>;
53+
5054
fn delete_key_package_history_up_to_id(&self, id: i32) -> Result<(), StorageError>;
5155

5256
fn delete_key_package_entry_with_id(&self, id: i32) -> Result<(), StorageError>;
@@ -86,6 +90,10 @@ where
8690
(**self).get_expired_key_packages()
8791
}
8892

93+
fn min_key_package_delete_at_ns(&self) -> Result<Option<i64>, StorageError> {
94+
(**self).min_key_package_delete_at_ns()
95+
}
96+
8997
fn delete_key_package_history_up_to_id(&self, id: i32) -> Result<(), StorageError> {
9098
(**self).delete_key_package_history_up_to_id(id)
9199
}
@@ -163,6 +171,18 @@ impl<C: ConnectionExt> QueryKeyPackageHistory for DbConnection<C> {
163171
.map_err(StorageError::from) // convert ConnectionError into StorageError
164172
}
165173

174+
fn min_key_package_delete_at_ns(&self) -> Result<Option<i64>, StorageError> {
175+
use crate::schema::key_package_history::dsl;
176+
use diesel::dsl::min;
177+
let v: Option<i64> = self.raw_query(|conn| {
178+
dsl::key_package_history
179+
.filter(dsl::delete_at_ns.is_not_null())
180+
.select(min(dsl::delete_at_ns))
181+
.first::<Option<i64>>(conn)
182+
})?;
183+
Ok(v)
184+
}
185+
166186
fn delete_key_package_history_up_to_id(&self, id: i32) -> Result<(), StorageError> {
167187
self.raw_query(|conn| {
168188
diesel::delete(
@@ -194,6 +214,14 @@ mod tests {
194214
use crate::test_utils::with_connection;
195215
use xmtp_common::rand_vec;
196216

217+
#[xmtp_common::test]
218+
fn min_key_package_delete_at_ns_none_when_empty() {
219+
with_connection(|conn| {
220+
// Aggregate MIN over an empty/unmarked table is NULL -> None.
221+
assert_eq!(conn.min_key_package_delete_at_ns().unwrap(), None);
222+
})
223+
}
224+
197225
#[xmtp_common::test]
198226
fn test_store_key_package_history_entry() {
199227
with_connection(|conn| {

crates/xmtp_db/src/encrypted_store/tasks.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ pub fn data_hash_for(task: &TaskProto) -> TaskDataHash {
131131
TaskDataHash(xmtp_common::sha256_array(&bytes))
132132
}
133133

134+
/// Never reaped by expiry: the row's lifetime is bounded by other means (a
135+
/// recurring row lives forever; an applied pull-in self-deletes).
136+
pub const NEVER_EXPIRES: i64 = i64::MAX;
137+
134138
pub trait QueryTasks {
135139
fn create_task(&self, task: NewTask) -> Result<Task, StorageError>;
136140

crates/xmtp_db/src/mock.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,10 +501,13 @@ mock! {
501501

502502
impl QueryIdentity for DbQuery {
503503
fn queue_key_package_rotation(&self) -> Result<(), StorageError>;
504+
fn queue_key_rotation_with_nudge(&self, rotation_task_hash: &crate::tasks::TaskDataHash, rotation_seed: crate::tasks::NewTask) -> Result<(), StorageError>;
504505

505506
fn reset_key_package_rotation_queue(&self, rotation_interval: i64) -> Result<(), StorageError>;
506507

507508
fn is_identity_needs_rotation(&self) -> Result<bool, StorageError>;
509+
510+
fn next_key_package_rotation_ns(&self) -> Result<Option<i64>, StorageError>;
508511
}
509512

510513
impl QueryIdentityCache for DbQuery {
@@ -545,6 +548,8 @@ mock! {
545548
&self,
546549
) -> Result<Vec<crate::key_package_history::StoredKeyPackageHistoryEntry>, StorageError>;
547550

551+
fn min_key_package_delete_at_ns(&self) -> Result<Option<i64>, StorageError>;
552+
548553
fn delete_key_package_history_up_to_id(&self, id: i32) -> Result<(), StorageError>;
549554

550555
fn delete_key_package_entry_with_id(&self, id: i32) -> Result<(), StorageError>;

0 commit comments

Comments
 (0)