Deduplicate scheduled syncs so a manual sync reschedules instead of doubling#15052
Open
rtibblesbot wants to merge 5 commits into
Open
Deduplicate scheduled syncs so a manual sync reschedules instead of doubling#15052rtibblesbot wants to merge 5 commits into
rtibblesbot wants to merge 5 commits into
Conversation
Storage.schedule()'s upsert overwrote interval/repeat/retry_interval with the incoming call's values. So enqueuing a plain one-off onto an existing recurring row (e.g. a manual "Sync now" landing on a scheduled recurring sync) silently cancelled the recurrence. Keep the existing schedule when the incoming enqueue is a plain one-off, moving only scheduled_time forward. After the immediate run completes the worker reschedules to now + interval, re-anchoring the recurrence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
DEV: backend
rtibblesbot
marked this pull request as ready for review
July 23, 2026 01:54
Contributor
Build Artifacts
Smoke test screenshot |
rtibblesbot
force-pushed
the
issue-14988-1dc828
branch
from
July 23, 2026 03:51
e8f2074 to
135f744
Compare
rtibbles
requested changes
Jul 23, 2026
rtibbles
left a comment
rtibbles
left a comment
Member
There was a problem hiding this comment.
This makes sense, I would prefer to keep the changes more localized rather than changing global schedule semantics for the sake of two tasks.
| } | ||
|
|
||
| if orm_job: | ||
| # Bringing a recurring job forward for an immediate run (e.g. a |
Member
There was a problem hiding this comment.
I would prefer we handle this internally to the syncing tasks only, rather than changing the semantics of the global schedule method for every single task.
Contributor
Author
There was a problem hiding this comment.
Agreed — reverted the Storage.schedule() change so global scheduling semantics are untouched, and moved the recurrence handling into the sync tasks. SyncJobValidator now brings an already-scheduled recurring sync forward via enqueue_args (re-asserting the existing interval/repeat/retry_interval) when a manual sync converges onto its deterministic job_id; a running scheduled job is left alone so the plain enqueue dedups onto it. Only syncs get this behaviour now. See 14148ad.
A manual "Sync" to KDP minted a random job id, so it never collided with the scheduled recurring sync (deterministic id "kdp_sync_<facility>"). Both ran at once and morango's deserializing stage failed (learningequality#14988). Give the manual dataportalsync path the same deterministic id via a new DataPortalSyncJobValidator, so a second sync collapses onto the scheduled row and brings it forward instead of running concurrently. resumesync keeps its own id — it targets one specific session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply the same deterministic-id dedup to peerfacilitysync: a manual "Sync
now" to a peer with a scheduled recurring sync now collapses onto the
existing job ("peer_sync_<facility>_<device>") when a concrete device is
known. Peer facility imports are one-off pulls and explicitly drop any
inherited id so they never collapse onto a schedule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The readable dedup seeds "kdp_sync_<facility>" (41 chars) and "peer_sync_<facility>_<device>" (75 chars) overflow Job.id, which is capped at varchar(36). SQLite ignores the length so it passed locally, but Postgres rejects the insert with StringDataRightTruncation, failing the real-storage dedup tests. Hash the seed to a stable 32-char id (same approach as discovery.tasks.generate_job_id). Both the scheduled and manual paths go through the shared helpers, so dedup convergence is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revert the change to the global Storage.schedule() upsert and instead keep a scheduled recurring sync's recurrence in the syncing tasks themselves. When a manual sync converges onto an already-scheduled recurring sync (they share a deterministic job_id), SyncJobValidator now brings the scheduled job forward via enqueue_args that re-assert its interval/repeat, rather than relying on schedule() to preserve recurrence for every task. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rtibblesbot
force-pushed
the
issue-14988-1dc828
branch
from
July 23, 2026 21:29
14148ad to
19a00bd
Compare
rtibbles
reviewed
Jul 24, 2026
rtibbles
left a comment
rtibbles
left a comment
Member
There was a problem hiding this comment.
Code looks good - just needs manual verification of the fix.
Contributor
Author
|
Ran a manual end-to-end verification against the real task queue ( 1. Scheduled recurring sync waiting for its next daily run + manual "Sync"
2. Scheduled sync already
Not covered here: a live KDP registration push, which needs a real portal token — worth one QA pass against staging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The first manual "Sync" to Kolibri Data Portal spawned a second, concurrent
dataportalsynctask alongside the scheduled recurring one; the two ran at once and morango'sdeserializingstage failed. This gives the manual sync the same deterministicjob_idas the scheduled recurring job, so the manual request reschedules the existing job to run now instead of minting a duplicate, and preserves the job's recurrence when it is brought forward. The same convergence is extended to scheduled peer facility syncs.References
Fixes #14988. First seen: v0.19.5-rc0 (reproduces on 0.19.4).
Reviewer guidance
Verify fix of #14988
AI usage
Used Claude Code to implement the fix from a pre-approved plan with test-driven development. Verified with the auth-task and task-storage pytest suites and prek/ruff.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-07-24 03:00 UTC