Adopt supervisor ownership and reconciliation in the Android task layer - #15051
Merged
rtibbles merged 5 commits intoJul 24, 2026
Merged
Conversation
github-actions
Bot
added
DEV: backend
Contributor
Build Artifacts
Smoke test screenshot |
rtibblesbot
marked this pull request as ready for review
July 23, 2026 01:11
rtibblesbot
force-pushed
the
issue-15014-3d4576
branch
2 times, most recently
from
July 23, 2026 21:23
c581e59 to
81ced0d
Compare
rtibbles
reviewed
Jul 24, 2026
rtibblesbot
force-pushed
the
issue-15014-3d4576
branch
from
July 24, 2026 05:04
466a734 to
a7931b2
Compare
rtibbles
reviewed
Jul 24, 2026
rtibbles
left a comment
rtibbles
left a comment
Member
There was a problem hiding this comment.
Clean, well-documented change. Verified the three glue points against core and the Task.enqueueOnce wrapper. CI green including the new android_platform_tests job. No blocking findings, just two suggestions I'd like resolved before merge.
Please tidy up the commit history afterwards in preparation for merge.
Verified on an emulator (android-35, this PR's debug APK):
- Killed a
remotecontentimportmid-run; DB showed it frozenRUNNINGowned by the dashless-hex of its WorkManager request id,Noneafter completion. - WorkManager restarted its own persisted work under the same request id → reconcile left it untouched.
- With WorkManager's DB cleared (WM eviction), reconcile logged
Requeuing job … with no live supervisor (owner: <dashless hex>), the QUEUED hook re-enqueued, and it resumed under a new request id →COMPLETED. - The QUEUED hook and the membership sync both re-enqueued on that pass;
enqueueUniqueWork(REPLACE)collapsed it to one worker.
| taskworker.execute_job(job_id, "3f2504e0-4f89-41d3-9a0c-0305e82c3301") | ||
|
|
||
| orm_job = job_storage.get_orm_job(job_id) | ||
| self.assertEqual(orm_job.state, State.COMPLETED) |
Member
There was a problem hiding this comment.
suggestion: Asserts the terminal state but not "owned while RUNNING". Enforced by core's fence and tested there, so it's a gap in the Android-side proof, not a correctness hole. Reading back orm_job.supervisor_id mid-execution would close it.
Contributor
Author
There was a problem hiding this comment.
Done in 452c2ad. The task now reads back orm_job.supervisor_id mid-execution and asserts the job is RUNNING and owned by supervisor_id_from_request(request_id), in addition to the terminal COMPLETED/None assertions — so the Android side now proves ownership is held for the duration, not just cleared afterwards.
rtibblesbot
force-pushed
the
issue-15014-3d4576
branch
from
July 24, 2026 15:13
452c2ad to
1bc00d9
Compare
Contributor
Author
|
Commit history tidied for merge as requested. The two review-response fixups are folded into their logical parents (tree unchanged):
Final history is five logical commits, one per behavioural change:
Post-tidy tree is byte-identical to the reviewed state; Android host tests pass (11 passed). |
The WorkManager task worker now forwards its request id to Kolibri's execute_job as supervisor_id, so a job run through WorkManager is owned by its request id for the duration of execution and cleared on completion. The request id survives WorkManager's own retries, so a retried request keeps owning its job. Add a host test harness that stubs the Chaquopy Java bridge so the Android python modules can be tested off-device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AndroidJobHook now enqueues to WorkManager whenever a job transitions into QUEUED, not only on schedule. The enqueue body moves into a shared _enqueue_task helper called by both schedule() and update(); update() invokes it when state == State.QUEUED. Core's identical-re-mark guard suppresses the hook for a no-op QUEUED->QUEUED update, so an already-queued row is not re-enqueued. A reconcile requeue therefore re-enters WorkManager immediately, exactly as a freshly scheduled job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_do_reconciliation now runs reconcile_stalled_jobs first, passing the active WorkManager request ids as the live supervisor set, so SELECTED and RUNNING jobs whose owner is no longer live (including NULL owners left by the pre-update worker) are requeued, and stalled CANCELING jobs are finalized to CANCELED. Clearing the dead owner first is required or the fenced execute_job would refuse to re-run the job; the requeue then re-enters WorkManager via the QUEUED hook. The existing WorkManager<->queue sync is retained as a second, post-reconcile pass. _get_workmanager_job_ids becomes _get_active_work (returning both job ids and request ids) over a pure _extract_work_ids helper. The live set is a pre-reconcile snapshot; the membership sync compares against a post-reconcile snapshot so reconcile's requeues are already visible and not double-enqueued. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The task worker stamped a job's supervisor_id with the WorkManager request id in its canonical java.util.UUID.toString() (dashed) form, and the reconciler built its live set the same way. But Kolibri's morango UUIDField normalizes every stored supervisor_id to the dashless uuid4().hex form, and the ownership fence compares stored-vs-expected owner as plain strings. So after the claim write, the job's own completion/worker-info writes compared unequal to the stored owner, were discarded as disowned, and the job was stranded in RUNNING forever — the first-run setup task never completed, so onboarding never reached the library (the Android smoke test's "Welcome to Kolibri!" assertion). Convert the request id to the dashless hex form via a shared supervisor_id_from_request helper at both use sites (the ownership claim and the reconcile live set) so they agree with each other and with what morango stores. Add an end-to-end lifecycle test that runs a job through the real storage and asserts it reaches COMPLETED with ownership cleared. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The host tests under platforms/android/app/src/main/python/tests had no CI coverage and, living under the Chaquopy source dir, would have been bundled into the shipped APK. Add an android_platform_tests job to the Python tests workflow that runs them from the repo root against the root pytest.ini (they need Kolibri's Django test settings and kolibri/dist pythonpath), and exclude tests/** from the Chaquopy python source set so they stay out of the built app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rtibblesbot
force-pushed
the
issue-15014-3d4576
branch
from
July 24, 2026 15:27
1bc00d9 to
5000c5c
Compare
rtibbles
approved these changes
Jul 24, 2026
rtibbles
left a comment
rtibbles
left a comment
Member
There was a problem hiding this comment.
Changes as expected, manual QA in an API 35 Android emulator showed the task reconciliation behaving as expected.
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
reconcile_stalled_jobsagainst the live request-id set to requeue stalled RUNNING/CANCELING jobs.task_identity.supervisor_id_from_requesthelper normalizes WorkManager's dashed request UUIDs to the dashless hex form morango stores assupervisor_id, so the worker's claim and the reconciler's live set compare equal against the same owner value.References
Implements #15014. Consumes the supervisor-ownership +
reconcile_stalled_jobsAPI already ondevelop(kolibri/core/tasks/worker.py,storage.py).Reviewer guidance
task_identity.py— the worker'ssupervisor_id(taskworker.py:36) and the live-set entries fromtask_reconciler._extract_work_idsmust be the same canonical string. Both now route throughsupervisor_id_from_request; the correctness of the whole change hinges on that single helper, so confirm no call site bypasses it. Any divergence (one side dashed, one dashless) makes reconcile disown every running job.kolibri_plugin.py:92— the QUEUED guard inupdate()relies on core suppressing the hook for a no-op QUEUED→QUEUED re-mark; check an already-queued row isn't re-enqueued.task_reconciler.py:140— the live set is a pre-reconcile snapshot; the membership sync at:156uses a fresh post-reconcile snapshot. Confirm both queries are required (reconcile's requeues must be visible to the sync) and not collapsible.Reviewer testing
adb devicesempty); needs a manual on-device pass before merge. This is the one outstanding acceptance criterion.platforms/android/app/src/main/python/tests, which stub only the Chaquopy Java bridge. These now run in CI (tox.yml) and are excluded from the packaged APK (build.gradle).AI usage
Claude Code implemented the three glue changes and their host tests against a reviewed plan and the fixed Kolibri-core API contract. Verified with the Android python test suite and prek; on-device verification remains outstanding.
@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 15:28 UTC