[namanjain24-sudo]

Summary

FacilityUser enforces unique_together = (("dataset", "picture_password")), and soft-deleting a learner sets date_deleted without clearing picture_password — so the row keeps holding its sequence against that constraint until cleanup_expired_deleted_users hard-deletes it 30 days later.

The pool of free sequences was read through the default manager, which hides exactly those rows:

class FacilityUserModelManager(BaseFacilityUserModelManager):
    def get_queryset(self):
        return super().get_queryset().filter(date_deleted__isnull=True)

So for 30 days after a learner is removed, their sequence was occupied in the database but reported as free, and get_available_sequence handed it straight back out — producing IntegrityError: UNIQUE constraint failed: kolibriauth_facilityuser.dataset_id, kolibriauth_facilityuser.picture_password on a sequence that had just been reported as available.

PicturePasswordCollisionOperation had the same blind spot, with a worse outcome. It repairs an incoming record that hit this constraint by reassigning the local holder, but looked that holder up through the same manager. When the holder was soft-deleted it found nothing, hit if not local_users: return COMPLETED, and returned without clearing the store error — so the incoming FacilityUser stayed un-deserialized and failed again on every later sync until the soft-deleted row was hard-deleted.

Both now read through all_objects, so the set treated as taken matches the set the constraint actually enforces.

References

Fixes #15662

Reviewer guidance

• Two regression tests, both verified red against develop and green here:
  • test_includes_soft_deleted_learners — fails with AssertionError: '1.2.3' not found in set()
  • test_soft_deleted_local_user_reassigned — fails with AssertionError: '1.2.3' == '1.2.3', i.e. the soft-deleted holder was left in place and the collision never resolved
• pytest kolibri/core/auth/test/ — 824 passed, 16 skipped, no regressions.
• prek run --files … clean on all four files.
• Both test classes needed databases = "__all__" added, because soft-deleting through save() triggers Session.delete_all_sessions, which touches the sessions database.
• I kept the existing roles__isnull=True filter rather than widening it further. Role holders should never carry a picture password — Role.save() clears it when a role is granted — so that filter is not implicated in this bug, and dropping it would be a separate change with its own reasoning.
• I also deliberately left the reassignment policy alone: a soft-deleted holder now gets a fresh sequence from the pool like any other, rather than being set to None. Freeing the slot outright may well be better, since a user due for hard deletion cannot log in anyway, but that is a behaviour decision rather than part of this fix — happy to change it if you'd prefer.
• Independent of Keep a picture password collision from breaking the caller's transaction #15660, which touches assign_picture_password in the same file but a different function; the two do not overlap textually and can land in either order.

AI usage

Claude Code (on my instruction to audit the newer code for a significant bug, then fix what it found) traced the mismatch between the unique_together constraint and the manager used to compute the free pool, and I filed it as #15662 before this fix. It worked test-first here: both regression tests went in before the change and were confirmed red, then green. Notably the sync-path test turned a claim I had only reasoned about in the issue into something demonstrated — reverting the fix leaves the soft-deleted holder untouched and the collision unresolved. It also checked morango's deserialization path (skip_erroring defaults to False and Kolibri never passes True) to confirm the stuck record really is retried on later syncs rather than dropped, so the scope of the sync impact is stated accurately. I reviewed the constraint-versus-manager reasoning, the decision to leave the roles__isnull filter and the reassignment policy alone, and every test run above before opening this PR.

[learning-equality-bot]

👋 Hi @namanjain24-sudo, thanks for contributing!

For the review process to begin, please verify that the following is satisfied:

• Contribution is aligned with our contributing guidelines

• Pull request description has correctly filled AI usage section & follows our AI guidance:

  AI guidance

  
  

  State explicitly whether you didn't use or used AI & how.

  If you used it, ensure that the PR is aligned with Using AI as well as our DEEP framework. DEEP asks you:

  • Disclose — Be open about when you've used AI for support.
  • Engage critically — Question what is generated. Review code for correctness and unnecessary complexity.
  • Edit — Review and refine AI output. Remove unnecessary code and verify it still works after your edits.
  • Process sharing — Explain how you used the AI so others can learn.

  
  

  Examples of good disclosures:

  "I used Claude Code to implement the component, prompting it to follow the pattern in ComponentX. I reviewed the generated code, removed unnecessary error handling, and verified the tests pass."

  "I brainstormed the approach with Gemini, then had it write failing tests for the feature. After reviewing the tests, I used Claude Code to generate the implementation. I refactored the output to reduce verbosity and ran the full test suite."

📢✨ Before we assign a reviewer:

• @rtibblesbot will pre-review this pull request. Its comments are generated by an LLM, and should be evaluated accordingly.
• We'll also invite community pre-review. See the community review guidance for both authors and reviewers.

Also check that issue requirements are satisfied & you ran pre-commit locally.

Pull requests that don't follow the guidelines will be closed.

Reviewer assignment can take up to 2 weeks.

[namanjain24-sudo]

Confirming both checklist items:

• Contributing guidelines: narrowly-scoped bug fix with two regression tests, linked to Soft-deleted learners keep their picture password, so it is handed out again while the constraint still holds it #15662 which describes the reproduction, and I ran pytest kolibri/core/auth/test/ (824 passed, 16 skipped) and prek locally before opening it. Same flag as on Keep a picture password collision from breaking the caller's transaction #15660: Soft-deleted learners keep their picture password, so it is handed out again while the constraint still holds it #15662 is a bug I reported myself and it hasn't been triaged or assigned yet, so this isn't against an assigned help wanted issue — I opened the fix alongside the report because it's a two-line change and the report is more useful with it attached, but say the word and I'll close this and wait for triage.
• AI usage: the section in the description is accurate per DEEP. Claude Code found this while auditing the picture-password code and wrote the fix and tests on my instruction, test-first — both tests went in before the change and were confirmed red ('1.2.3' not found in set() and '1.2.3' == '1.2.3'), then green. I reviewed the constraint-versus-manager reasoning, checked that Role.save() clearing picture passwords is why the roles__isnull filter isn't implicated here, and made the call to leave the reassignment policy for soft-deleted holders alone rather than widen the change. Two judgement calls I'd genuinely welcome your view on are in the reviewer guidance.

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.20.0a2.dev1693_gdd4cf8ceb.pex
Windows Installer (EXE)	kolibri-setup-0.20.0a2.dev1693+gdd4cf8ceb-unsigned.exe
Debian Package	kolibri_0.20.0alpha2dev1693-0ubuntu1_all.deb
Debian Package	kolibri-server_0.20.0alpha2dev1693-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.20.0a2.dev1693+gdd4cf8ceb.dmg
Android Package (APK)	kolibri-0.20.0a2.dev1693+gdd4cf8ceb-debug.apk
Flatpak bundle (GNOME)	org.learningequality.Kolibri.Devel.flatpak
Raspberry Pi Image	kolibri-pi-image-0.20.0alpha2dev1693.zip
TAR file	kolibri-0.20.0a2.dev1693+gdd4cf8ceb.tar.gz
WHL file	kolibri-0.20.0a2.dev1693+gdd4cf8ceb-py3-none-any.whl

Smoke test screenshot

Download screenshot

[namanjain24-sudo]

Pushed the reclaim half that @rtibbles asked for on #15662, so this PR now covers the whole of the described outcome:

• soft-deleted learners keep their sequence (already here — get_assigned_sequences reads through all_objects)
• the sync collision resolver finds a soft-deleted holder (already here)
• new: when every sequence is taken, get_available_sequence reclaims one instead of failing

The one assumption, flagged so it is easy to overturn: I read "least recently deleted" as LRU-style — reclaim from the learner deleted longest ago first — on the grounds their sequence is least likely to be wanted back. That is a single .order_by("date_deleted") in _reclaim_sequence_from_deleted_learner; if you meant the opposite it is a one-character change to -date_deleted and I'll push it. I asked on the issue and went ahead rather than leave the PR half-done, since the rest stands either way.

NoAvailableSequences still raises when there is genuinely nothing to reclaim — the existing test_raises_no_available_sequences_when_pool_exhausted passes unchanged, because no learner is soft-deleted in that scenario. The new test fills the pool, soft-deletes two learners ten days and one day ago, and asserts the ten-day-old sequence is the one handed over while the one-day-old learner keeps theirs.

pytest kolibri/core/auth/test/ — 825 passed, 16 skipped. prek clean.

Still outstanding, deliberately not in this push: LearnerCounterCache.count uses FacilityUser.objects, so are_picture_passwords_exhausted undercounts soft-deleted holders against the 1300 limit, as @rtibblesbot noted. I left it because the same counter feeds the bulk-assignment task's progress total, where counting soft-deleted learners would make the progress wrong — so it probably wants a separate count rather than a change to this one. Happy to do that here or separately, whichever you prefer.

[namanjain24-sudo]

CI shows two reds on the reclaim commit. Both trace to one infrastructure hang rather than the change, and I checked rather than assumed, because the same two jobs passed on this PR's previous commit:

• Browser smoke test was cancelled, not failed. The log shows Playwright's pip install completing at 12:16:25 ("Successfully installed … playwright-1.64.1"), then nothing for ten minutes until 12:26:15 ##[error]The operation was canceled and Terminate orphan process: pid (2204) (playwright). That is the browser-binary download hanging after the package install succeeded — the Kolibri app is never started in that window.
• WHL smoke tests is a four-second gate job whose only step is "Fail if any needed job failed or was cancelled". It is red purely because the browser job was cancelled.

The change in this commit is confined to kolibri/core/auth/utils/picture_passwords.py and its tests, and the browser job never reached the point of running Kolibri, so there is no path from one to the other.

Locally: pytest kolibri/core/auth/test/ is 825 passed, 16 skipped, and prek is clean on the changed files.

I can't re-run jobs on this repo from a fork, and I'd rather not force-push a no-op commit just to clear a red that isn't telling you anything. Happy to push a rebase if you'd like a clean run, or it will sort itself out on the next push.

[namanjain24-sudo]

A dependency between this PR and #15660 that I found while testing the new reclaim path, and that is worth knowing before either lands.

assign_picture_password retries once on an IntegrityError, and that retry calls get_available_sequence again. Before this PR that second call only read; with the reclaim it can now also write, because freeing the longest-deleted learner's sequence updates that row. The retry runs after a collision has already raised inside the caller's atomic block, so whether that write can happen at all depends on #15660.

Measured both ways on a tree with develop plus the branch(es) named:

tree	reclaim after a collision
this PR alone	TransactionManagementError: … can't execute queries until the end of the 'atomic' block
this PR + #15660	succeeds, returns the reclaimed sequence

To be precise about what this is and isn't: the retry path is already broken on develop without #15660 — that is exactly #15594, and the reads in the old retry failed the same way. So this PR does not introduce the breakage, it just means the reclaim-on-exhaustion path inherits it. Nothing here regresses a path that works today.

The practical upshot is only about ordering: if #15660 lands first, or they land together, the reclaim works as intended. If this one lands alone, reclaim-on-exhaustion will fail for any caller that hits a collision first — which is the narrow case of a facility whose pool is full and that races on the same sequence.

Both branches merge together cleanly; I ran the combined tree at 886 passed, 16 skipped.

[learning-equality-bot]

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

[rtibblesbot]

🟡 Waiting for changes

Outcome: reviewed e609267

Last updated: 2026-10-29 11:19 UTC

[rtibblesbot]

PR #15664: the reclaim sits in get_available_sequence, not in assign_picture_password as #15662 asked. Counting soft-deleted learners via all_objects fixes #15662 as filed.

CI: only "Build Assets" failed, and it was cancelled. Test jobs pass. Tests not run locally (no venv).

• blocking: move the reclaim into assign_picture_password, per the maintainer (inline).
• suggestion: restored learners can come back without a picture password (inline).
• suggestion: the sync collision path falls back to None instead of reclaiming (inline).
• suggestion: no test covers the read-only contract (inline).
• suggestion: are_picture_passwords_exhausted undercounts, because LearnerCounterCache.count still uses FacilityUser.objects. The #15662 thread planned to fix this here. Is that fix still coming?
• nitpick: date_deleted ties need a tie-break (inline).
• nitpick: comments explain the change rather than the code (inline).

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Check that the reclaim sits in assign_picture_password and runs in one transaction, as requested on #15662.
• Decide whether restoring a learner whose sequence was reclaimed should assign a new one, and whether sync collisions should reclaim too.

---

Written by rtibblesbot, an LLM-based coding agent.

[namanjain24-sudo]

Thanks — pushed the blocking change and the two nitpicks. Taking the points in order.

blocking — reclaim moved into assign_picture_password. get_available_sequence raises NoAvailableSequences again and writes nothing; assign_picture_password catches that and takes the sequence from the oldest soft-deleted holder. Clearing the holder and saving the user happen in one transaction.atomic(), so the case you flagged — an IntegrityError on the user's save after the holder was already cleared, leaving the sequence assigned to nobody — rolls back instead. Being a savepoint, it also stays safe inside the callers' own atomic blocks.

suggestion — no test covers the read-only contract. Added test_get_available_sequence_does_not_reclaim: it fills the pool, soft-deletes a holder, asserts get_available_sequence raises, and asserts the holder still has its sequence afterwards. I checked it is red against the previous commit rather than assuming.

nitpick — date_deleted ties. Now order_by("date_deleted", "id"). You are right about bulk soft-delete: perform_bulk_destroy writes a single now() across the whole selection, so every row in one deletion ties.

nitpick — comments that explain the change. Deleted four: the all_objects one in sync_operations.py and the three in the tests, including the "30 days" one, which duplicated the retention window in tasks.py and would have gone stale. I kept two and would rather argue for them than delete quietly: the get_assigned_sequences docstring saying soft-deleted learners are included, and the policy sentence in the reclaim helper. Both document behaviour a caller cannot see from the code — that the set includes rows the default manager hides, and why "longest deleted" is the criterion. Happy to cut them too if you disagree.

suggestion — restored learners come back without a sequence. Agreed, and it is already open as #15690 against develop (issue #15689). It reproduces without the reclaim too, because a learner soft-deleted before picture login is switched on is skipped by both backfills, which is why it is a separate PR rather than another commit here.

suggestion — the sync collision path falls back to None. You are right that it is the opposite policy, and I would rather not decide it unilaterally. Reclaiming there means a sync can silently take a sequence from a soft-deleted learner on the receiving device, which is a different trade-off from a coach creating a learner. It is also the one path where the holder may be a user this device has never seen. Happy to make it reclaim in this PR if that is the call.

suggestion — are_picture_passwords_exhausted undercounts. Still happy to do it. The reason it is not here yet is the one I raised on #15662: LearnerCounterCache.count also feeds the bulk-assignment task's progress total, where counting soft-deleted learners would make the progress wrong, so it probably wants a second count rather than a change to that one. Say which you prefer and I will add it here or separately.

pytest kolibri/core/auth/test/ — 826 passed, 16 skipped. prek clean.

[rtibblesbot]

PR #15664: 4 of 6 prior findings resolved; 2 suggestions still open on their existing threads.

CI pending.

• suggestion: a restored learner whose sequence was reclaimed has no picture password, so can't use picture login. Please handle this on restore, or reply on the existing thread with why it is acceptable.
• suggestion: sync_operations.py:396: an active conflicting local learner gets None once soft-deleted holders fill the pool. Please handle the exhausted pool there, or reply on the existing thread with why None is acceptable.
• nitpick: new docstring lines restate the code (inline).

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py — get_available_sequence writes another user's row
UNADDRESSED — kolibri/core/auth/utils/picture_passwords.py — restored learner whose sequence was reclaimed can't use picture login
UNADDRESSED — kolibri/core/auth/sync_operations.py:396 — active local learner gets None once soft-deleted holders fill the pool
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:180 — no test that get_available_sequence stays read-only
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — .first() tie on date_deleted
RESOLVED — kolibri/core/auth/sync_operations.py — comments explaining the change

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Decide whether taking a picture password from the learner deleted longest ago is acceptable, since restoring that learner leaves them without picture login.
• Check that sync collision handling, which now also matches soft-deleted local learners, behaves correctly once the pool is exhausted.

---

Written by rtibblesbot, an LLM-based coding agent.

Superseded by my review of bb21baf: no blocking findings remain.

[namanjain24-sudo]

One red check on this run, and it is not the change — leaving the evidence here so nobody has to dig.

DEB install tests / Install test (ubuntu:20.04, ubuntu-24.04-arm) fails at:

PASS: Kolibri is responding on port 8080 (attempt 1)
FAIL: Kolibri server runs on '', expected /opt/kolibri/python/bin/python3.*

That assertion is check_server_interpreter in platforms/debian/tests/install_smoke_test.sh:114, which does readlink /proc/$(head -1 ~/.kolibri/server.pid)/exe with || true. An empty result means the pid file or the readlink was not ready, not that the server is on the wrong interpreter — the server had already started and answered on port 8080 in the line above.

Three things say it is environmental rather than this branch:

• The same ubuntu:20.04 image passes on the x86 runner in this very run; only the ubuntu-24.04-arm variant fails. The other seven DEB variants pass.
• That same ARM variant passes on Keep a picture password collision from breaking the caller's transaction #15660, Require an attendance record's user to share its session's dataset #15667, Move date_modified forward when an attendance session is edited #15671 and Reassign a picture password when a learner is restored #15690 right now.
• Keep a picture password collision from breaking the caller's transaction #15660 edits the same file this PR does, picture_passwords.py, and passes it.

The change here is Python logic in kolibri/core/auth/; it cannot affect which interpreter a packaged server runs under, or whether a pid file has landed yet. I cannot re-run the job from a fork, so I have left it rather than pushing a no-op commit to retrigger — happy to if you would rather see it green.

[namanjain24-sudo]

One more push, from re-reading the restructure rather than from a review finding.

Moving the reclaim out of get_available_sequence left an inconsistency I introduced: the retry inside assign_picture_password still calls get_available_sequence, which now raises instead of reclaiming. So a collision that happened to coincide with the pool emptying raised NoAvailableSequences, where the first attempt in the same call would have taken a sequence back — and that contradicted the docstring I had just written on that function. Before the restructure both paths reclaimed, because the reclaim was inside the function they both called.

Both paths reclaim again now. test_retry_reclaims_when_the_pool_empties_mid_assignment covers it, and I checked it is red without the change, failing with the NoAvailableSequences it used to raise.

pytest kolibri/core/auth/test/ — 827 passed, 16 skipped. prek clean.

Also, thank you for the four withdrawals — the sync-path and docstring reasoning landing rather than being argued down was useful.

[rtibblesbot]

PR #15664: no new findings. All 8 prior findings are resolved or acknowledged. CI is pending.

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py — get_available_sequence writes another user's row
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:180 — no test covers get_available_sequence staying read-only
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — .first() picks an arbitrary row on date_deleted ties
RESOLVED — kolibri/core/auth/sync_operations.py — comments explain the change, not the code
ACKNOWLEDGED — kolibri/core/auth/utils/picture_passwords.py — restored learner whose sequence was reclaimed (moved to #15690)
RESOLVED — kolibri/core/auth/sync_operations.py:396 — sync collision path doesn't reclaim (withdrawn)
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:135 — docstring restates body / Raises sentence
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:182 — docstring paragraph restates the except branch (withdrawn)

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Confirm reclaiming a picture password from the longest-deleted learner (writing another user's soft-deleted row) is the intended product behaviour.
• Check the reclaimed row's change syncs cleanly when that learner's facility data is held on other devices.

---

Written by rtibblesbot, an LLM-based coding agent.

[namanjain24-sudo]

Taking the third item in your next-step list — whether the reclaimed row's change syncs cleanly when that learner's facility data is held on other devices — since I could check it rather than leave it open.

The narrow save does mark the row for sync. holder.save(update_fields=["picture_password"]) looks like it would skip the dirty bit, but morango's SyncableModel.save adds it to the set (morango/models/core.py:865-867):

if update_dirty_bit_to is not None and kwargs.get("update_fields") is not None:
    kwargs["update_fields"] = set(kwargs["update_fields"]) | {"_morango_dirty_bit"}

Clearing to NULL cannot collide. The constraint is (dataset, picture_password), and NULLs do not participate in it — which the codebase already relies on, since most users in a facility have no sequence at all, and PicturePasswordCollisionOperation itself already sets several local users to None in one pass when it runs out of spares.

The interesting case is ordering, and this PR already covers it. The reclaim produces two records: the holder losing its sequence and the new user gaining it. If the gaining record deserializes on another device before the clearing one, it hits the unique constraint against a holder that is still soft-deleted there. That is exactly the case the first commit on this branch fixes — before it, PicturePasswordCollisionOperation looked the local holder up through FacilityUser.objects, found nothing because the row was soft-deleted, and returned COMPLETED leaving the store error in place. With all_objects it finds the holder, re-sequences it, and the incoming record lands.

So the reclaim and the sync-side fix in this PR are two halves of the same thing, which I had not spelled out. Worth a human eye on the claim, but that is what the code does.

[rtibblesbot]

Checked and agreed — that closes the sync question from my review:

• morango's SyncableModel.save adds _morango_dirty_bit to update_fields, so the narrow save is picked up for sync.
• Out-of-order arrival is handled by the all_objects lookup in PicturePasswordCollisionOperation from the first commit.

Nothing further from me on this item.

[namanjain24-sudo]

A merge-order note, since this PR and #15660 now overlap and that is a consequence of the restructure rather than something either PR set out to do.

Moving the reclaim into assign_picture_password put this PR into the same function #15660 edits. At 77b2cd3 — before the restructure — the two branches merged cleanly, because this one did not touch that function at all. They no longer do.

I checked rather than just flagging it, and it is mild:

• picture_passwords.py auto-merges correctly. The result keeps Keep a picture password collision from breaking the caller's transaction #15660's savepoints around both saves and this PR's reclaim fallback on both paths, in the right places.
• The only conflict is in test_picture_passwords.py, where both branches add a test at the same anchor. Resolution is keeping both.
• With that resolved, pytest kolibri/core/auth/test/ is 829 passed, 16 skipped on the combined tree.

So whichever lands second needs a trivial rebase and no behavioural decision. Happy to rebase this one onto #15660 now if you would rather carry the conflict here than discover it at merge time — I have left both branches as they are for now rather than guessing at the order you want.

[namanjain24-sudo]

Closing a loop on my own claim rather than leaving it asserted.

When I moved the reclaim I said the transaction.atomic() means "an IntegrityError on the user's save after the holder was already cleared rolls back instead", and that "being a savepoint, it also stays safe inside the callers' own atomic blocks". Both were true, but nothing in the branch exercised either — the atomic you asked for was the point of the blocking change and had no test behind it. That is now covered:

• test_failed_reclaim_leaves_the_sequence_with_its_holder — the user's save fails after the holder has been cleared; the holder must still have its sequence afterwards. Confirmed red with the atomic() removed, where the holder is left at None and the sequence belongs to nobody, which is exactly the case you flagged.
• test_reclaim_works_inside_a_caller_atomic_block — the reclaim run the way FacilityUserSerializer.create and Role.delete run it, asserting the caller's transaction is still usable afterwards. This one is a shape test and I will say so plainly: it does not go red if the atomic() is simply deleted, since there is no error in that path. It guards against a future change making the reclaim incompatible with an enclosing transaction, which no other test here would catch.

pytest kolibri/core/auth/test/ — 829 passed, 16 skipped. prek clean.

[rtibblesbot]

PR #15664: all 8 prior findings are resolved. One new nitpick. CI is pending.

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py — get_available_sequence writes another user's row
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — restored learner can't use picture login (moved to #15690)
RESOLVED — kolibri/core/auth/sync_operations.py:396 — active conflicting learner gets None (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:181 — no test that get_available_sequence stays read-only
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — .first() arbitrary on date_deleted ties
RESOLVED — kolibri/core/auth/sync_operations.py — comments explain the change
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:135 — docstring restates body
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:182 — docstring paragraph (withdrawn)

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

PR #15664: all 9 prior findings resolved. CI still pending.

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py — get_available_sequence writes another user's row
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — Restored learner with a reclaimed sequence can't use picture login (moved to #15690)
RESOLVED — kolibri/core/auth/sync_operations.py:396 — Conflicting active learner gets None once the pool is full (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:181 — No test covers get_available_sequence staying read-only
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — .first() picks an arbitrary row when date_deleted ties
RESOLVED — kolibri/core/auth/sync_operations.py — Comments explain the change, not the code
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:135 — Docstring restates the body / Raises line
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:182 — Docstring paragraph restates the except branch (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py — Test docstring restates the name and the assertion

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Confirm that reclaiming a soft-deleted user's picture sequence (writing that user's row) is acceptable, given it syncs to other devices.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibbles]

Please trim this to the all_objects fix plus a single reclaim path.

• Keep the all_objects changes in get_assigned_sequences and PicturePasswordCollisionOperation.
• Reclaim only when the first get_available_sequence call raises NoAvailableSequences: a helper clears the picture_password of the learner deleted longest ago and returns that sequence, then the existing save and retry run unchanged.
• Drop the reclaim from the retry branch and the helper's own transaction.atomic().
• Tests: keep test_includes_soft_deleted_learners, test_exhausted_pool_reclaims_from_longest_deleted_learner and the sync operation test. Drop the other four.
• Leave LearnerCounterCache as is. Counting soft-deleted learners would refuse new learners at the limit before reclaim could run.
• Cut the docstring additions.

[rtibblesbot]

Deferring to @rtibbles's review: withdrawing my earlier asks for transaction.atomic() around the reclaim, the read-only test, and counting soft-deleted learners in LearnerCounterCache.

[namanjain24-sudo]

Trimmed to that shape — the PR is now 4 files, +111/-3.

• get_assigned_sequences and PicturePasswordCollisionOperation keep the all_objects change.
• _reclaim_sequence_from_deleted_learner(facility) clears the longest-deleted learner's sequence and returns it. assign_picture_password calls it only when the first get_available_sequence raises; the save and retry are byte-for-byte as they were.
• The reclaim is gone from the retry branch, and the helper has no transaction.atomic() of its own. The transaction import went with it.
• Tests: test_includes_soft_deleted_learners, test_exhausted_pool_reclaims_from_longest_deleted_learner and the sync operation test. The other four are dropped. I checked the reclaim test is still red without the fallback.
• Docstring additions cut.

On LearnerCounterCache — thank you for the reason, which I had not seen. I had been treating the undercount as a straightforward bug; counting soft-deleted learners would make are_picture_passwords_exhausted true at the limit and refuse new learners before the reclaim could run, which inverts the behaviour this PR is adding. Leaving it alone, and I will not raise it again.

pytest kolibri/core/auth/test/ — 825 passed, 16 skipped. prek clean.

[namanjain24-sudo]

Correcting my own merge-order note above. It no longer applies.

I flagged that this PR and #15660 had started to conflict, because moving the reclaim into assign_picture_password put both branches in that function. The trim you asked for took the overlap back out — the reclaim is now a single line in the except of the first get_available_sequence call, and the save and retry are untouched, which is exactly where #15660 works.

Re-checked rather than assumed: git merge-tree reports clean, and a trial merge produces 0 conflicted files. The merged assign_picture_password keeps this PR's reclaim on the first call and #15660's savepoints around both saves, and pytest kolibri/core/auth/test/ is 827 passed, 16 skipped on the combined tree.

So there is no merge-order constraint between the two any more, and nothing needs rebasing whichever lands first. Sorry for the noise — the note was accurate when I wrote it and the trim is what undid it.

[rtibblesbot]

PR #15664: the trim removed two docstrings that still carry needed facts (2 inline suggestions). All 8 prior findings are resolved or acknowledged.

• CI failure: the Mac job hit a network timeout, unrelated to this change.

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py — get_available_sequence writes another user's row
ACKNOWLEDGED — kolibri/core/auth/utils/picture_passwords.py:145 — restored learner can't use picture login (moved to #15690)
RESOLVED — kolibri/core/auth/sync_operations.py:396 — reclaim on the sync path (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:179 — no test that get_available_sequence stays read-only (moot: it has no write path now)
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — .first() arbitrary on date_deleted ties
RESOLVED — kolibri/core/auth/sync_operations.py — comments explain the change
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — docstring line / Raises restate the body
RESOLVED — kolibri/core/auth/utils/picture_passwords.py — docstring paragraph restates the except branch (withdrawn)

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Confirm that taking a soft-deleted learner's picture password, so that a restore leaves them without one (handled in #15690), is an acceptable product call.
• Check that the reclaim writing the holder's row outside transaction.atomic is safe: if the user's save fails, the holder has already lost the sequence.

---

Written by rtibblesbot, an LLM-based coding agent.

[namanjain24-sudo]

Three red checks on this run, none of them the change. Evidence rather than assertion, since the trim is exactly the commit they landed on.

Python unit tests on Mac OS (3.10) — the runner could not resolve DNS, so the checkout never completed and no tests ran:

fatal: unable to access 'https://github.com/learningequality/kolibri/': Could not resolve host: github.com
Manifest fetch failed; retrying in 1000ms ...
##[error]The operation was aborted due to timeout

Python tests — the aggregate. Its job list shows exactly two failures: the Mac job above and itself.

No zombies — Kolibri started and stopped cleanly all ten times ("Kolibri stopped in a maximum of 3 seconds"), and the port check that follows then found a listener still on 8082. The last stop was logged at "0 seconds", so the check appears to have raced the socket being released.

That one is worth more than a shrug, because the workflow has 24 successes and 1 failure in its last 25 runs repo-wide and the failure is mine. What rules it out as this branch's doing is the branch's own history: this job has passed eight times here, including on ea71d768 and 1925bd44, which carried strictly more code than 8cf0f843 does — the trim removes the transaction.atomic(), the retry-branch reclaim and four tests. A commit that only deletes code cannot introduce a shutdown hang the fuller versions did not have, and nothing in this PR runs during server start or stop; it is auth utility code reached from user creation, role deletion and sync.

I cannot re-run jobs from a fork. Happy to push a no-op to retrigger if you would rather see it green than take my word for it.

[namanjain24-sudo]

Small correction to the note above. I wrote "a commit that only deletes code". 8cf0f843 is 8 insertions and 144 deletions, not pure deletion — the additions are the helper's new signature, its return sequence, and the two call-site lines.

The conclusion is unchanged and does not rest on that phrasing: those eight lines are sequence-selection logic in utils/picture_passwords.py, reached only from user creation, role deletion and sync, and nothing in them runs while the server starts or stops. The branch's own eight prior passes of that job remain the stronger evidence.

[namanjain24-sudo]

Both restored — you were right that the trim took facts with it, and one of them had gone stale rather than merely missing.

get_assigned_sequences — the paragraph is back, trimmed to the load-bearing part: soft-deleted learners' sequences keep occupying the (dataset, picture_password) constraint until they are hard-deleted. Your reasoning is what convinced me: without it all_objects reads like a slip, and "fixing" it back to objects is #15662 again.

assign_picture_password — the Raises line was simply wrong, and I had not noticed when I trimmed it. It said NoAvailableSequences is raised when no sequence is free, which is now the case where the function reclaims. It escapes only when there is no soft-deleted holder to take from, or when the retry's get_available_sequence finds the pool full. Both are documented now, along with the fact that the reclaim writes another learner's row.

@rtibbles — flagging a tension rather than deciding it: your review said to cut the docstring additions, and this puts two of them back on @rtibblesbot's suggestion. If you would rather they stay out, say so and they come out again; the Raises line should still be corrected either way, since it is inaccurate as it stands.

pytest kolibri/core/auth/test/ — 825 passed, 16 skipped. ruff format --check clean.

[rtibblesbot]

PR #15664: all 11 prior findings are resolved. No new findings. CI is still pending.

Prior-finding status

RESOLVED — kolibri/core/auth/utils/picture_passwords.py:157 — get_available_sequence writes another user's row
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:148 — restored learner can't use picture login (moved to #15690)
RESOLVED — kolibri/core/auth/sync_operations.py:396 — reclaim for conflicting active local learner (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py:179 — no test that get_available_sequence stays read-only
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:139 — .first() tie ordering on date_deleted
RESOLVED — kolibri/core/auth/sync_operations.py — comments restating the change
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:154 — docstring restating body / Raises (summary kept, withdrawn)
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:171 — docstring paragraph restating except (withdrawn)
RESOLVED — kolibri/core/auth/test/test_picture_passwords.py — test docstring restating test name
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:119 — trim dropped the soft-deleted-holder paragraph
RESOLVED — kolibri/core/auth/utils/picture_passwords.py:177 — Raises line wrong once reclaim exists

Next step: Needs a full code review by a human engineer.

• The author is outside the team: confirm this should be built at all.
• Confirm that silently taking a soft-deleted learner's picture password once the pool is full is the intended product behaviour.
• Check the reclaim writing a deleted learner's row is safe for sync, given that row may later be restored or synced from another device.

---

Written by rtibblesbot, an LLM-based coding agent.