Skip to content

fix(xmtp_mls): defer unknown task kinds instead of deleting them - #4020

Merged
tylerhawkes merged 1 commit into
mainfrom
tyler/defer-unknown-tasks
Aug 20, 2026
Merged

tylerhawkes merged 1 commit into
mainfrom
tyler/defer-unknown-tasks

Conversation

@tylerhawkes

@tylerhawkes tylerhawkes commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #4018, addressing a Macroscope finding raised on #4011.

#4018 added a KpLiveness arm to run_task for the task variant the proto regen introduced. Nothing in this crate schedules that kind yet, so the arm deleted the row.

A row of an unknown kind can only appear one way: a newer client wrote it into a database this build also opens. Deleting it discards work that newer client still expects to run, and it is not recoverable — the scheduling decision lived in the row.

Reschedule instead. The row survives for whichever client understands it, and expires_at_ns (checked at the top of run_and_reschedule_task) still bounds how long an unclaimed row can sit there, so an old client running alone cannot accumulate them.

One hour was picked so an old client sharing the database wakes on it rarely, while a newer client still picks it up promptly after a restart.

Independent of the app-data callback stack — it touches only worker/tasks.rs and is based directly on main.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AdY7WKkNbJmdzpmUWvW1my

Note

Defer unknown task kinds instead of deleting them in MLS worker

Previously, unrecognized task kinds (such as KpLiveness in older builds) were deleted when encountered. Now they are rescheduled 1 hour into the future using a new UNKNOWN_TASK_DEFER_NS constant, preserving tasks for future processing by a compatible build.

Behavioral Change: unknown tasks are no longer permanently deleted; they persist and retry every hour until a build that recognizes them is deployed.

Macroscope summarized 38ff2e2.

An older client that finds a task kind it does not understand — only
possible when a newer client shares the same local database — deleted the
row. That discards work the newer client still expects to run, and
`run_and_reschedule_task` already treats `TaskOutcome::Done` as a delete.

Reschedule instead. The row survives for whichever client does understand
it, and `expires_at_ns` still bounds how long an unclaimed row can linger,
so an old client running alone cannot accumulate them forever.
@tylerhawkes
tylerhawkes requested a review from a team as a code owner August 19, 2026 20:07
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @tylerhawkes's task —— View job


Code Review Completed

Review Date: August 19, 2026 at 20:07 UTC

  • Read the modified file
  • Analyze the implementation
  • Provide review feedback

Review Feedback

The implementation is solid and well-reasoned. The approach of deferring unknown task kinds instead of deleting them correctly handles the multi-version database sharing scenario.

Strengths:

  • Clear constant naming (UNKNOWN_TASK_DEFER_NS) with excellent inline documentation explaining the timing rationale (lines 22-25)
  • The 1-hour deferral strikes a good balance between avoiding hot-loops on old clients and allowing prompt pickup by newer clients
  • Leverages existing expires_at_ns bound to prevent unbounded accumulation
  • Appropriate logging with tracing::warn! that includes context (task ID, task kind)

Minor observations:

  1. Pattern consistency: The KpLiveness arm is the only one that returns early with a reschedule. Consider if a None case should also defer (lines 466-469) - though deletion makes sense there since truly empty tasks are malformed.

  2. Generic applicability: This pattern will need to be repeated for each new proto variant. The code could benefit from a comment noting that future proto additions should follow this pattern if they're not immediately implemented. The current comment (lines 451-457) is excellent but focuses on KpLiveness specifically.

  3. Test coverage: No test explicitly validates the defer behavior for unknown task kinds. Consider adding a test that:

    • Seeds a task with a recognizable-but-unhandled variant
    • Calls run_and_reschedule_task
    • Asserts the task survives with next_attempt_at_ns advanced by ~1 hour

No blocking issues. The change addresses the stated problem correctly and follows the codebase's patterns for task handling.


@macroscopeapp

macroscopeapp Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 38ff2e2

Macroscope's review found this PR approvable — Small, self-contained bug fix by the designated code owner. Changes unknown task handling from deletion to 1-hour deferral for multi-version database compatibility, with clear rationale documented in comments.

You can add or adjust custom eligibility rules. Learn more.

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.92%. Comparing base (e3f9c62) to head (38ff2e2).

Files with missing lines Patch % Lines
crates/xmtp_mls/src/worker/tasks.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4020      +/-   ##
==========================================
- Coverage   85.93%   85.92%   -0.01%     
==========================================
  Files         417      417              
  Lines       68170    68172       +2     
==========================================
- Hits        58579    58575       -4     
- Misses       9591     9597       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tylerhawkes
tylerhawkes merged commit 6095502 into main Aug 20, 2026
65 of 69 checks passed
@tylerhawkes
tylerhawkes deleted the tyler/defer-unknown-tasks branch August 20, 2026 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants