[srikar-naidu]

Summary

Adds SyncDatabase so sync code can explicitly choose which database it uses. The default handle keeps Django’s existing ORM routing, while a pinned handle uses the specified alias for ORM queries, raw SQL, transactions, and instance counter updates.

Also adds a test guard that reports the SQL when a query accidentally reaches the default database.

TODO

• Added tests for the new code
• Documentation reviewed. The new behavior is described in the code docstrings.
• Dependency changes reviewed. This PR adds no dependencies.

Reviewer guidance

The main thing to check is the difference between the two handles. The default handle leaves ORM routing to Django and uses default for raw SQL and transactions. A pinned handle keeps those operations on its own alias, even when the router points elsewhere.

All seven new contract tests and all four existing async and legacy network characterization tests passed. The characterization tests ran without edits. Full SQLite and PostgreSQL verification is still outstanding.

Issues addressed

Closes #365.

Documentation

The SyncDatabase and query guard docstrings describe the new behavior. There’s no separate documentation page for this change.

AI usage

I used Codex to help map the issue requirements to the existing sync code, implement the handle and query guard, and run the tests. I provided the maintainer’s branch and scope requirements, reviewed the proposed approach, and decided to proceed after discussing how to preserve default routing and verify that pinned operations stay on their alias.

[learning-equality-bot]

👋 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.

[rtibblesbot]

🟡 Waiting for changes

Outcome: reviewed fb59b94

Last updated: 2026-11-10 01:35 UTC

[rtibblesbot]

PR #419: SyncDatabase covers #365 and keeps the default handle's behaviour. CI passing.

• suggestion: the diff carries 10 commits from open PRs #410, #411 and #415, because the base is release-v0.9.x. Merging this first lands their changes without their own review. Retarget to net-characterization-363 so the diff shows only 984f435.
• suggestion: a pinned handle's backend keeps the first reading thread's connection (inline).
• suggestion: guard violations in controller-driven syncs surface only as ERRORED (inline).
• suggestion: pinned transaction test never runs the non-isolated branch (inline).
• nitpick: module begin_transaction rebuilds the backend per call (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.
• Decide whether pinned SyncDatabase handles may cross threads, since the sync stack (#366/#368) will copy this API.
• Confirm the stacked PRs #410, #411 and #415 merge before this, or retarget it so only 984f435 lands.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

PR #419: all 4 prior findings are resolved. No new findings. CI is passing.

Prior-finding status

RESOLVED — morango/sync/db.py — pinned backend keeps first thread's connection
RESOLVED — morango/sync/db.py — begin_transaction rebuilds default backend on every call
RESOLVED — tests/testapp/tests/helpers.py — guard violation inside controller-driven sync shows only ERRORED
RESOLVED — tests/testapp/tests/sync/test_db.py — non-isolated branch never run

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 the SyncDatabase handle API is the pattern the sync refactor (#365) should build on, since later sync code will copy it.
• Check the module-global default backend relying on Django's per-thread connection proxy suits how sync runs across threads.

---

Written by rtibblesbot, an LLM-based coding agent.

[bjesterbot]

This covers the #365 contract well. The routed default handle keeps today's behaviour: backend returns the module DBBackend, so existing patches of morango.sync.db.DBBackend still work. The pinned handle sends ORM calls, raw SQL, and transactions to its own alias. The diff now holds only the three intended files, because the base is db-handle-365. I found no blockers. The two suggestions below matter mostly because #366/#368 will copy these test patterns.

• ⚠️ Should-fix: PinnedSyncDatabaseTestCase keeps TestingRouter active. Spec §5 asks pinned-handle tests to run with DATABASE_ROUTERS=[] (inline).
• 💡 Suggestion: the guard has two blind spots that its docstring should state: it only watches the calling thread, and on PostgreSQL it does not see a top-level atomic() on default (inline).
• 💡 Nit: the parameter docs for begin_transaction were dropped in the move (inline).
• ✅ The per-thread backend cache, which rebuilds when connections[alias] changes, fixes the cross-thread problem without needing ConnectionProxy. The guard re-raises a rejected query even when the code under test catches it, which keeps the SQL visible inside SessionController.proceed_to.

Verification

• pytest tests/testapp/tests/sync/test_db.py: 10 passed, 1 skipped (the existing PostgreSQL-only isolation test).
• Full SQLite suite (pytest tests/testapp/tests): 666 passed, 2 skipped. This includes the async and legacy network characterization tests, unedited.
• The 4 pinned tests also pass with @override_settings(DATABASE_ROUTERS=[]) added locally. So the first suggestion is a one-line change.
• I did not run PostgreSQL locally.

Generated by Claude Code using model Claude Opus 5.5

[rtibblesbot]

PR #419: all 4 prior findings resolved. One new nitpick inline. CI passing.

Prior-finding status

RESOLVED — morango/sync/db.py — pinned backend keeps the first thread's connection
RESOLVED — morango/sync/db.py — begin_transaction rebuilds the default backend per call
RESOLVED — tests/testapp/tests/helpers.py — guard violation in controller sync shows only ERRORED
RESOLVED — tests/testapp/tests/sync/test_db.py — non-isolated branch never exercised

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 legacy-instance matching on _calculate_0_4_uuid in morango/models/core.py is right for every 0.4/0.5 instance already stored on deployed devices.
• Check that SyncDatabase is the pattern other sync callers should copy for pinned-alias access.

---

Written by rtibblesbot, an LLM-based coding agent.

[bjester]

Overall, looks great! Although I'm requesting the removal of the resolution of the MAC-less pathway. I'm aware of those failures, but haven't quite determined the right change for it. For now, we can ignore it.

Thank you for your assistance!

[bjester]

I suspect this was to resolve the failing test in CI? We already have an issue tracked for it. Unfortunately, this change isn't quite appropriate. Please remove it. The tests are flip-flopping so I'll just keep running them until we get a green.

[bjester]

Obviously, this can be removed as well without corresponding change to its target.

[srikar-naidu]

Thanks for the review!!! I pushed fb59b94 to remove the MAC-less identity change and its regression test. The PR diff is back to the three files for #365. The focused database tests passed locally (10 passed, 1 PostgreSQL-only skip).

I’d also be interested in taking on #366 once this PR is merged. If it’s open to outside contributors, would you be willing to assign it to me?

[rtibblesbot]

PR #419: all 5 prior findings are resolved or acknowledged. The legacy-UUID revert is clean and adds no new findings. CI is passing.

Prior-finding status

RESOLVED — morango/sync/db.py — pinned handle's backend keeps first thread's connection
RESOLVED — morango/sync/db.py — begin_transaction rebuilds default backend on every call
RESOLVED — tests/testapp/tests/helpers.py — guard violation in controller-driven sync shows only ERRORED
RESOLVED — tests/testapp/tests/sync/test_db.py — non-isolated branch never exercised
ACKNOWLEDGED — morango/sync/db.py:96 — duplicated :param block (maintainer accepted)

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 routing sync operations through the new SyncDatabase handle keeps transaction and connection behaviour unchanged for existing default-database deployments.
• Confirm that the pinned-handle-per-thread connection model is the pattern you want other sync code to copy.

---

Written by rtibblesbot, an LLM-based coding agent.

[bjester]

LGTM! Thank you for your contribution

[rtibblesbot]

Quick check on rtibblesbot's review of this PR: react 👍 helpful · 😕 mixed · 👎 not helpful. Reply with anything specific.