Keep repeating tasks visible after each run - #15093
Conversation
Build Artifacts
Smoke test screenshot |
405925b to
6d8f79e
Compare
035d28c to
d28a69b
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Generally looking OK, some thoughts about cleanup, and there may be other redundancies I've not spotted in a first look.
If there is other unnecessary indirection in the code, that should be cleaned up too.
Also, 9 commits for this seems too much - once you've made the relevant changes compress it to a logical series of commits.
| 'Status of a task run that was canceled, with a relative time such as "5 minutes ago".', | ||
| }, | ||
| taskStatusWithNextRun: { | ||
| message: '{status} · {nextRun}', |
There was a problem hiding this comment.
I think this would be better constructed as an RTL sensitive text join rather than a string.
There was a problem hiding this comment.
Done — taskStatusWithNextRun is gone, and syncFacilityTaskDisplayInfo now joins the two clauses with formatList from kolibri/utils/i18n (the Intl.ListFormat wrapper), so the separator and the ordering come from the locale rather than a hard-coded string.
One consequence worth flagging: it drops the middot the issue described, because Intl.ListFormat uses the locale's own list separator (a, b in en, a وb in ar). Say the word if you would rather keep the middot and I will build the join a different way.
There was a problem hiding this comment.
Still in place after the rebase onto develop — no code change this cycle. The join is formatList([getLastRunStatusMsg(task), nextRunMsg(task)]) at packages/kolibri-common/utils/syncTaskUtils.js:208; tasks.js only holds the two independently-translatable clauses.
There was a problem hiding this comment.
No change this cycle — the join is still formatList([getLastRunStatusMsg(task), nextRunMsg(task)]) (syncTaskUtils.js:208), with the two clauses translated independently in tasks.js.
There was a problem hiding this comment.
Unchanged — the join is still formatList([getLastRunStatusMsg(task), nextRunMsg(task)]) (syncTaskUtils.js:214), with the two clauses translated separately in tasks.js.
There was a problem hiding this comment.
No code change this cycle — the only work was diagnosing the Morango/PostgreSQL CI failure. The RTL-sensitive join is unchanged: formatList([getLastRunStatusMsg(task), nextRunMsg(task)]) at packages/kolibri-common/utils/syncTaskUtils.js:214, with the two clauses translated independently in tasks.js.
There was a problem hiding this comment.
No change this cycle — this cycle was CI-only (diagnosing and fixing the Morango/PostgreSQL job). The RTL-sensitive join is still formatList([getLastRunStatusMsg(task), nextRunMsg(task)]) at packages/kolibri-common/utils/syncTaskUtils.js:214, with the two clauses translated independently in tasks.js.
| retryInterval = null, | ||
| }) { | ||
| const now = Date.now(); | ||
| return { |
There was a problem hiding this comment.
Isn't this very similar to the data in the syncSchedule.js test helper you already created? You did that based on two usages, so I'm confused why a third wasn't integrated.
There was a problem hiding this comment.
Fair — that was my miss. The helper has moved to packages/kolibri-common/utils/__tests__/syncSchedule.js (next to the module it describes) and all three specs now build their rows from it. It grew hoursUntilNext, syncState and retryInterval knobs to cover this file's cases, and makeRescheduledTask is gone.
There was a problem hiding this comment.
Still in place after the rebase — no code change this cycle. The fixture is packages/kolibri-common/utils/__tests__/syncSchedule.js and all three specs build their rows from it (syncTaskUtils.spec.js:13, FacilitiesTasksPage.spec.js:5, FacilitiesPage.spec.js:5); no local task-literal builders remain.
There was a problem hiding this comment.
No change this cycle — all three specs still build their rows from packages/kolibri-common/utils/__tests__/syncSchedule.js (syncTaskUtils.spec.js:13, FacilitiesTasksPage.spec.js:5, FacilitiesPage.spec.js:11).
There was a problem hiding this comment.
Unchanged — all three specs still build their rows from packages/kolibri-common/utils/__tests__/syncSchedule.js (syncTaskUtils.spec.js:13, FacilitiesTasksPage.spec.js:5, FacilitiesPage.spec.js:11).
There was a problem hiding this comment.
No code change this cycle (CI diagnosis only). All three specs still build their rows from the shared packages/kolibri-common/utils/__tests__/syncSchedule.js fixture — syncTaskUtils.spec.js:13, FacilitiesTasksPage.spec.js:5, FacilitiesPage.spec.js:11.
There was a problem hiding this comment.
No change this cycle (CI-only work). All three specs still build their rows from the shared packages/kolibri-common/utils/__tests__/syncSchedule.js fixture — syncTaskUtils.spec.js:13, FacilitiesTasksPage.spec.js:5, FacilitiesPage.spec.js:11.
| } | ||
|
|
||
| const relativeStatusToDescriptionMap = { | ||
| [TaskStatuses.COMPLETED]: relativeTime => |
There was a problem hiding this comment.
Couldn't these just be a mapping from a task type to a string id? They all use the same relative time argument, so repeating the function is pointless.
There was a problem hiding this comment.
Done — it is now a status-to-string-id map and getLastRunStatusMsg formats the relative time once:
const relativeStatusToStringMap = {
[TaskStatuses.COMPLETED]: 'taskFinishedRelativeStatus',
[TaskStatuses.FAILED]: 'taskFailedRelativeStatus',
[TaskStatuses.CANCELED]: 'taskCanceledRelativeStatus',
};There was a problem hiding this comment.
Still in place after the rebase — no code change this cycle. relativeStatusToStringMap (syncTaskUtils.js:143) maps status to string id, and getLastRunStatusMsg formats the relative time at the single call site.
There was a problem hiding this comment.
No change this cycle — still the relativeStatusToStringMap status→string-id map at syncTaskUtils.js:143, with the relative time formatted once in getLastRunStatusMsg.
There was a problem hiding this comment.
Unchanged — still the relativeStatusToStringMap status→string-id map (syncTaskUtils.js:149), with the relative time formatted once in getLastRunStatusMsg.
There was a problem hiding this comment.
No code change this cycle (CI diagnosis only). Still the relativeStatusToStringMap status→string-id map at packages/kolibri-common/utils/syncTaskUtils.js:149, with the relative time formatted once in getLastRunStatusMsg.
There was a problem hiding this comment.
No change this cycle (CI-only work). Still the relativeStatusToStringMap status→string-id map at packages/kolibri-common/utils/syncTaskUtils.js:149, with the relative time formatted once in getLastRunStatusMsg.
| ); | ||
| } | ||
| const syncStep = syncTaskStatusToStepMap[task.extra_metadata.sync_state]; | ||
| // extra_metadata carries over between runs, so a live run's sync_state can |
There was a problem hiding this comment.
Cut to two lines: extra_metadata carries over between runs, so a live run still holds the previous run's sync_state until morango reports a step.
There was a problem hiding this comment.
Minor follow-up: the QA fix in c6bb01d reworded this comment slightly, since the stale-sync_state reset now also covers a queued row that is due to run. Still two lines:
// extra_metadata carries over between runs, so a run that has not reported a
// step yet still holds the previous run's sync_state.There was a problem hiding this comment.
Still two lines after the rebase — no code change this cycle (syncTaskUtils.js:194): extra_metadata carries over between runs, so a run that has not reported a step yet still holds the previous run's sync_state.
There was a problem hiding this comment.
No change this cycle — the comment is still the two-line version at syncTaskUtils.js:194.
There was a problem hiding this comment.
Unchanged — still the two-line comment at syncTaskUtils.js:200.
There was a problem hiding this comment.
No code change this cycle (CI diagnosis only). Still the two-line comment at packages/kolibri-common/utils/syncTaskUtils.js:200.
There was a problem hiding this comment.
No change this cycle (CI-only work). Still the two-line comment at packages/kolibri-common/utils/syncTaskUtils.js:200.
d28a69b to
c5de2b9
Compare
|
Addressed the review and force-pushed. Inline replies on each thread; the two structural ones:
Commits: 9 → 6. The two trailing fixups are folded into the commits they fix, and the two storage commits are now one:
The tree is byte-identical to the pre-recompose tree plus the review changes. Verified: @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
|
On the remaining part of the review — "if there is other unnecessary indirection in the code, that should be cleaned up too" — I went back over the diff for it and did not find anything further worth removing:
The three you named are all done; nothing new pushed for this pass. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
rtibbles
left a comment
There was a problem hiding this comment.
No further concerns from the code.
c5de2b9 to
e996417
Compare
|
The issues observed in the latest build while manually testing:
same-task.mp4
After clicking the clear.completed.mp4Logs: logs.zip |
|
Forgot to mention that the |
e996417 to
c6bb01d
Compare
|
Thanks @pcenov — one real bug in there, which is fixed in c6bb01d. Taking them in turn: 1. Sync re-runs the same task instead of creating a new one Two things bundled together here. Re-using the row is intentional: #15052 gave the manual sync the same deterministic But what the panel said while that happened was wrong, and that is what the video shows. Between pressing Sync and the runner picking the job up, the row still read:
It was announcing the previous run plus a nonsense countdown, because the row is re-queued and my code only treated 2. No This one is deliberate — it is an acceptance criterion on #15092:
For a repeating sync the task row is the schedule; there is no separate record. Clearing it would delete the schedule, so the daily sync would silently stop. The one-off If the 3. No Pre-existing, not from this PR. Whether facility syncs should be cancellable is a real question, but it's a backend change (morango has to be able to unwind a part-finished transfer session) and belongs in its own issue rather than here. Happy to file it. While I was in there I did tighten the related guard: Cancel is now only offered for a genuinely live run, because cancelling a queued repeating row clears the job — which would delete the schedule. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
c6bb01d to
f28842f
Compare
|
CI note: the only red check is Android smoke test (API 24), which is an infra hang, not a change here. The job stuck on |
0f898f4 to
42a8398
Compare
…lean A repeating job is re-queued the instant a run ends, so its own state stops describing that run and the task manager loses the only record of what the user just watched. Stamp the outcome onto two dedicated columns on every transition into a finished state, which covers all three exits - recurrence, failure retry and a task-requested retry_in delay - in the same write that ends the run, before any reschedule re-queues the row. A job that is not re-scheduled stamps the same columns with its own finished state, so the snapshot never contradicts the row it sits on. progress, total_progress, exception, traceback and result live in saved_job and survive the reschedule, so a second run would otherwise inherit the previous run's progress bar and exception. They have to survive until the next run actually starts - until then the queued row is still describing the run that just ended - so reset them on the transition into RUNNING rather than at reschedule time. A bare re-mark of an already-RUNNING job is not a new run and stays a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
last_finished_status and last_finished_datetime mirror the existing state -> status and scheduled_time -> scheduled_datetime naming. clearable stays state-derived, so a re-scheduled recurring row is not clearable and "Clear completed" cannot delete a sync schedule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A re-queued schedule is QUEUED, so the panel rendered a full progress bar and inferred "is running" from extra_metadata.sync_state, which is stale (COMPLETED) on that row. Centralise "which run is being shown" in syncTaskUtils so the panel, the tasks bar and the schedule table agree: Finished 2 minutes ago, next sync in 1 day Failed 5 minutes ago, retrying in 1 hour The two clauses are joined with formatList so the separator and the ordering follow the locale rather than a hard-coded string. A failed run is only announced as retrying when the schedule has a retry_interval; without one it re-schedules onto its ordinary recurrence. The same staleness makes a second automatic run read as finished until morango reports its first step, because a recurrence re-persists the previous run's extra_metadata; treat a terminal sync_state as PENDING, which is what a fresh enqueue writes. Neither cancel nor retry is offered on a re-queued schedule: cancelling a QUEUED job clears it, which would delete the schedule, and the schedule already retries itself. The unused nextSync string goes — the next-run clause now lives in TaskStrings, where the panel and the schedule table both reach it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The task manager hid every repeat === null row that was not RUNNING, so a manual KDP sync vanished the instant it finished. List one that has a last finished run too; a schedule that has never run stays out, because it belongs on Manage sync schedule. With recurring rows always present, "not clearable" stops being a proxy for "syncing" — it would pin every registered facility to a permanent spinner, and it would leave the tasks bar reading "0 of 1 complete" forever. Both now go off the run being displayed. The empty state tested facilityTasks rather than the filtered list, so a device whose only task was a hidden schedule rendered neither tasks nor the "no tasks" message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SyncInterface polled a single task until it became clearable, which a repeating row never does — so after a manual KDP sync from this page the row spun forever and the 2-second poll never stopped. It also cleared that id on completion, which for a schedule would delete the schedule. Poll against the last finished run recorded when the sync was started instead, and only clear a task that is genuinely one-off. That baseline lives until the next sync starts: clearing it on teardown made an in-flight poll response read as "run finished" against a null baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An empty placeholder stands in for a schedule that has never run. The empty-state cell was already under-spanning its four columns; with the new one it spans five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A server that fails to start left the ones before it running and holding open connections to their test databases, so every later test failed to create its own.
42a8398 to
4a8220f
Compare
|
Thanks @rtibblesbot, manually tested and confirmed that the acceptance criteria is met in the latest build so that:
|
rtibbles
left a comment
There was a problem hiding this comment.
Code changes check out, manual QA passes.
The concerns about new syncs not appearing as new tasks we will carry into follow up issues, but not needed here.

Summary
A re-queued repeating job's own state no longer describes the run that just ended.
Incidental, hit while running the auth suite:
multiple_kolibri_serversnow shuts down the servers it already started when a later one fails to start. They were left holding connections to their test databases, so one failed start turned into a failure in every test after it.References
Fixes #15092. First seen: #15052 (comment).
Reviewer guidance
Screenshots
Walked every acceptance criterion on two live Kolibri servers — device A (
:8451) owning "QA Facility", device B (:8452) having imported it — with a real peer sync schedule between them.Finished 30 seconds ago, next sync in 1 hour,10 KB sent • 23 KB received, no Clear/Retry/CancelFailed 20 seconds ago, retrying in 5 minutesrepeat: 0entry, still clearablestatus === RUNNING; tasks bar arithmeticAlso confirmed by sampling the API through each run:
POST /api/tasks/tasks/clearall/returns 200 and leaves both the row and the schedule in place.RUNNINGthe row reportsexception: nullandpercentage: 0; the previous run'sCommandErrorand traceback are gone, and the panel reads "Waiting" rather than "Finished" even thoughextra_metadata.sync_statestill holds the previous run'sCOMPLETED.last_finished_datetimechanges), andrepeat_interval/retry_intervalsurvive, soTaskResource.clearis correctly not called on a schedule.axe-core audit on the task manager and Manage sync schedule: 0 violations at WCAG AA.
AI usage
Used Claude Code to implement the change from a pre-approved plan, writing the failing tests before each piece. Verified with the tasks and auth Python suites, the kolibri-common/device/facility Jest suites,
prek, and manual QA in a browser against a seeded device.@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Deviations from the issue spec
_write_job_update, not inreschedule_finished_job_if_neededandmark_job_as_runningas the spec named them. One write point covers all three reschedule exits and both mark-as-running paths without duplicating the condition.showsLastRunrequires a non-terminalstatusbefore it reads the snapshot.🟡 Waiting for feedback
Last updated: 2026-07-31 14:29 UTC