Skip to content

Dequeue transferred records using an explicit database handle - #421

Open
srikar-naidu wants to merge 12 commits into
learningequality:release-v0.9.xfrom
srikar-naidu:dequeue-records-366
Open

srikar-naidu wants to merge 12 commits into
learningequality:release-v0.9.xfrom
srikar-naidu:dequeue-records-366

Conversation

@srikar-naidu

Copy link
Copy Markdown

Summary

Passes a required database handle through dequeue and legacy self-referential ordering, so SQL, transactions, and instance counter updates use the selected database.

Adds SessionContext.db, defaults it to routed access, and restores the default handle after unpickling. The receiver operation passes this handle to dequeue.

TODO

  • Added tests for the new code
  • Updated code docstrings
  • No new dependencies required

Reviewer guidance

This PR targets dequeue-records-366.

Following Task 5, DatabaseMaxCounter.update_fsics() remains unchanged for #367. The pinned tests mock that helper while checking dequeue SQL, counters, ordering, and rollback under assert_no_default_db_queries() with routers disabled. Full pinned FSIC routing therefore still depends on #367. If that work should be included here, I can adjust the scope.

Verification completed locally:

  • All 99 context and operation tests passed.
  • All four unchanged async and legacy network characterization tests passed.
  • Repository hooks passed.
  • The broader run reported two timestamp failures in unchanged TransferSession tests on Windows. Both passed on rerun.
  • PostgreSQL was not run locally.

Issues addressed

Addresses #366, with FSIC helper routing deferred to #367 as specified in Task 5.

Documentation

Updated the SessionContext and dequeue docstrings. No separate documentation files were added.

AI usage

I used Codex as a coding assistant for implementation, test generation, and running checks. I provided the issue requirements and maintainer’s instructions, discussed the proposed approach, and evaluated the scope decision around FSIC routing. I chose to keep that work in #367 according to Task 5 and document the limitation for the reviewer. I also used the lessons and review feedback from my previous contribution to guide this PR.

bjester and others added 12 commits November 3, 2026 10:25
Deserialization streams partitions in the order of the sync filter
(learningequality#392), so a record whose FK target is in a later partition fails.
Scope listed the read-write partitions first, which put a single-user
scope's logs before the user they reference.

The read filter now lists the read template's partitions, then the
read-write template's, and the write filter does the same with the
write template. Each template keeps its own order.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Builds a scope in the shape of Kolibri's single-user scope, with the
user in a read-only partition and its log in the read-write partition,
named so the read-write partition also sorts first. The scope's read
filter deserializes both. A second test keeps the old read-write-first
order as a counter-example, where the log fails on its missing user.

Refs learningequality#398

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Explains how a scope's filters order the partitions of its templates,
and how implementers should order their templates so deserialization
puts shared records before the records that reference them.

The changelog notes that a server receiving a push from an older
client still deserializes in the client's filter order.

Closes learningequality#398

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
cleanupsyncs matched --sync-filter against the stored filter string with
startswith. Kolibri passes the full filter string of the current sync,
so after Scope began listing read-write partitions last, the command no
longer matched transfer sessions stored in the old order, and left their
buffers behind.

A transfer session now matches if its filter starts with the given
filter, as before, or has the same partitions in any order. The match
runs in Python, after the other options narrow the candidates in the
database.

The command now loads the transfer sessions to close in batches of 500
IDs, so no query binds more parameters than older versions of SQLite
allow.

Refs learningequality#398

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_get_client_ip_for_server opens a UDP socket to learn which local
address routes to the server. AppArmor or a sandbox can refuse to
create that socket, which raised outside the existing try block. It
now logs a warning and returns 127.0.0.1, as it already did when the
connect failed. The bare except is narrowed to OSError.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A MORANGO_NODE_ID set in the environment overrides the node ID, so
test_consistent_0_5_instance_id got a different instance ID than the
one it expects.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A sandbox can deny binding to port 0, which LiveServerTestCase uses by
default and get_free_tcp_port used to find a free port.
get_free_tcp_port moves to the test helpers and falls back to a fixed
list of ports when port 0 is denied. A new FreePortLiveServerTestCase
picks its port with it in setUpClass, so each class gets a port that
is free when its server starts. The client test cases now extend it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The second server, second_environment and the sync session setup move
from test_syncsession.py into integration/helpers.py, with the setup in
a new BasePushPullClientTestCase. PushPullClientTestCase extends it and
keeps its tests and the activity timestamp checks they use.

Other test modules can now reuse the harness without importing
PushPullClientTestCase, which would collect and run its tests again.

Refs learningequality#363

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pins what a full push and a full pull with ASYNC_OPERATIONS send and
store, so the refactors of the sync layer can show they change neither.

A spy on SessionWrapper.request records each request, then passes the
call through to the real method. For each request it records the
method, path, query params, body encoding, body keys, the values of
the body fields that are stable between runs, the record count of each
pushed chunk, and the is_retryable flag. Generated IDs are normalized
to <id>. The tests compare the recording with fixed expected lists for
every transfer stage.

The tests also assert the final local transfer session, the Buffer,
Store and app rows on both sides, and the byte counts the server
stores. The spy fails a sync that sends more than 100 requests or runs
longer than 120 seconds, so a stalled sync fails instead of hanging.

Closes learningequality#363

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pins what a full push and a full pull send and store when the server
lacks ASYNC_OPERATIONS, so the refactors of the LegacyNetwork*
operations can show they change neither.

The tests run against a live server that emulates a morango version
before 0.6.0: the MORANGO_TEST_LEGACY_SERVER variable turns off both
ASYNC_OPERATIONS and FSIC_V2_FORMAT on the server, and LiveServer now
takes extra environment variables for that. The client keeps its full
set of capabilities, as a current Kolibri does when it syncs with an
older one. Reducing only the client capabilities, as learningequality#364 first
proposed, fails today on the server for both push and pull (learningequality#416).

The expected lists show that the client creates the transfer session
and sends no stage updates for serializing, queuing, dequeuing, or
deserializing. The legacy pull moves 11 records instead of 10: the
client creates the transfer session before it serializes, so it sends
an empty client_fsic, and the server queues the user from the initial
pull again.

Closes learningequality#364

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Add explicit database handle for sync access

* Address sync database handle review

* Apply Ruff formatting to sync database helper

* Address sync database documentation and test review

* Identify legacy instances by their original UUID

* Revert "Identify legacy instances by their original UUID"

This reverts commit 8940331.
Pass the context database through dequeue SQL, counter updates, and legacy ordering. Restore routed default handles after context pickling. Keep FSIC helper routing for Task 6 (learningequality#367), as specified by Task 5.
@srikar-naidu
srikar-naidu requested review from a team and leq-bot November 11, 2026 14:51
@learning-equality-bot

Copy link
Copy Markdown
Contributor

👋 Hi @srikar-naidu, 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.

📎 We couldn't find an issue linked to this pull request. Please edit the description and link one under References, for example Fixes #123 or Closes #123. A plain #123 mention doesn't link the PR to the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants