Improve buffer serialization performance by performing bulk counter lookup

[bjester]

Summary

Improves the performance of sync transfer for a large database by performing a single query for counter records (for associated buffers being transferred). For the same DB referenced in the issue, where a chunk would take approximately 10 seconds to transfer, the same chunk takes less than 1 second-- a 90%+ decrease in the processing time.

Tests already cover the API and serializer integration with multiple buffers.

Reviewer guidance

Is the existing test coverage sufficient or should unit tests be added for the serializer?

Issues addressed

Closes #344

AI Usage

AI was used to verify the performance issue, and with that context, implement the fix. The fix was minimal and existing tests covered the modified code. Small changes were made to the implementation to loop over transfer sessions, to make the code a little cleaner, even though it's only ever called with one transfer session.

[rtibblesbot]

🔵 Review posted

Last updated: 2026-07-07 18:59 UTC

[rtibblesbot]

PR #345 review. Clean, minimal N+1 fix for BufferListSerializer — bulk-fetches RecordMaxCounterBuffer rows once per transfer session and caches them on each Buffer instance via _rmcb_list, with a backward-compatible fallback in Buffer.rmcb_list() for callers outside the list path. Correctness of the (transfer_session_id, model_uuid) bucketing checks out.

Note: new commits landed after this review's passes were run, so the latest changes on the branch have not been re-reviewed — please re-request review after this if anything material changed since. CI also shows failures/pending checks that should resolve before merge.

Findings:

• suggestion: model_uuid__in set isn't scoped to the current transfer_session_id inside the per-session loop, undercutting the comment's stated rationale (see inline).
• suggestion: no test locks in the new batched-query behavior for the many=True path (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

[rtibbles]

Code updates make sense, and code comments explain why this isn't resulting in a large number of queries still, as this will only usually operate over a single transfer session.

Existing test coverage gives further confidence of no regression.

[rtibblesbot]

PR #345 delta re-review.

2 of 2 prior findings RESOLVED. No new correctness issues from the core pass. Two unresolved items from this round below; CI also still has several checks queued/pending as of this review.

• important: model_uuid__in set size scales with page size, which is driven by the caller-configurable sync chunk_size — see inline comment.
• suggestion: the multi-transfer_session_id branch of the new loop has no test coverage — see inline comment.

Prior-finding status

RESOLVED — morango/api/serializers.py:178 — model_uuid set not scoped to current transfer_session_id
RESOLVED — morango/api/serializers.py (test_api.py:1076) — missing test for batched-query behavior on many=True path

---

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

How was this generated?

Compared the current PR state against findings from a prior review:

• Retrieved prior bot reviews via the GitHub API
• Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
• Only raised NEW findings for newly introduced code
• Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
• Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence

[rtibblesbot]

important: This IN clause is sized to the number of buffers in the current page, and page size is driven by the caller-configurable sync chunk_size (NetworkSyncConnection.chunk_size, default 500 but overridable via create_sync_session/resume_sync_session). Since the codebase's only tested backend is SQLite, a caller that sets chunk_size above SQLite's compiled SQLITE_MAX_VARIABLE_NUMBER (999 on many stock builds) would hit OperationalError: too many SQL variables here — a failure mode that didn't exist when each buffer issued its own single-value filter. Consider sub-chunking the IN clause independent of chunk_size, or documenting/enforcing a max safe chunk_size.

[rtibblesbot]

suggestion: This loop is written to handle multiple transfer_session_ids in one call (per the comment above), which is exactly the shape where the previously-fixed scoping bug lived. The new regression test only covers a single transfer session, so a future scoping regression on this branch wouldn't be caught. If this path is kept as future-proofing, add a multi-session test; otherwise consider simplifying to the single-session case that's actually exercised.