Skip to content
This repository was archived by the owner on Jul 9, 2026. It is now read-only.

Correct behavior for background initialization, only useful for pulls - #15

Merged
bjester merged 3 commits into
mainfrom
update-fields-testing
Jul 9, 2026
Merged

bjester merged 3 commits into
mainfrom
update-fields-testing

Conversation

@bjester

@bjester bjester commented Jul 8, 2026 •

Copy link
Copy Markdown
Member

Summary

The BackgroundSessionContext was hardcoded to assume it was the receiver. Background initialization is really only useful when the local instance is the producer, not the receiver. The hardcoded state meant the sync went through sync operations in the wrong flow, as the receiver not the producer, meaning the queuing process would never occur.

This also moves the morango requirement from the core deps to only the developer group. For developers, this helps IDEs and static analysis find morango. In the tests, it ends up loading morango from Kolibri's bundled version, meaning the core requirement just created confusion. This is why the expected test failure is still extant, because upgrading that core morango requirement had no effect.

References

found in QA of learningequality/kolibri#14884 when this plugin was utilized

closes #14

Reviewer guidance

This was tested on a Kolibri instance, with background queuing enabled:

export KOLIBRI_SYNC_BACKGROUND_INITIALIZATION="True"
export KOLIBRI_SYNC_BACKGROUND_INITIALIZATION_STAGES="queuing"

A separate instance then synced with the above instance, and verified that

AI usage

AI was used to review the changes and increase the test coverage (the second commit). It also found an issue in Morango's SessionContext which has an expected failing test in test_is_receiver__client_pull. The Morango fix is happening in tandem.

@rtibblesbot

rtibblesbot commented Jul 9, 2026 •

Copy link
Copy Markdown

🔵 Review posted

Last updated: 2026-07-09 00:23 UTC

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #15 correctly fixes issue #14: BackgroundSessionContext no longer hardcodes is_server/is_receiver, so a server servicing a pull now resolves to is_producer and runs the queuing stage that populates records_total. Verified against morango's LocalSessionContext receiver/producer logic; the push flow is preserved. Good server/client × push/pull test matrix, and the expectedFailure xfail cleanly documents the upstream morango or-collapse quirk.

CI passing. No UI files — Phase 3 skipped.

Non-blocking:

  • nitpick — redundant INFO log line, see inline.
  • suggestion — worth a note on BackgroundSessionContext that it's server-only, see inline.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran a phased review pipeline over the pull request diff:

  • Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
  • Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
  • Specialized frontend/backend review passes applied framework-specific lenses where those files changed
  • For UI changes: manual QA and an accessibility audit against a live dev server, when available
  • Checked CI status and linked issue acceptance criteria
  • Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence

@bjester
bjester force-pushed the update-fields-testing branch from 588cb21 to d57a2db Compare July 9, 2026 00:31
@bjester bjester changed the title Correct behavior for background initialization, only useful for pulls [DO NOT MERGE] Correct behavior for background initialization, only useful for pulls Jul 9, 2026
@bjester bjester changed the title [DO NOT MERGE] Correct behavior for background initialization, only useful for pulls Correct behavior for background initialization, only useful for pulls Jul 9, 2026
@bjester
bjester force-pushed the update-fields-testing branch from 78ecffd to fa6c491 Compare July 9, 2026 17:52
@bjester
bjester force-pushed the update-fields-testing branch from fa6c491 to 19cdbe3 Compare July 9, 2026 17:55
@bjester
bjester merged commit 023716a into main Jul 9, 2026
8 checks passed
@bjester
bjester deleted the update-fields-testing branch July 9, 2026 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Background queuing causes int vs None comparison exception

3 participants