Skip to content

[codex] skip unchanged read-only sync - #297

Merged
TimKleindick merged 1 commit into
mainfrom
codex/read-only-sync-fast-path
Jun 18, 2026
Merged

TimKleindick merged 1 commit into
mainfrom
codex/read-only-sync-fast-path

Conversation

@TimKleindick

@TimKleindick TimKleindick commented Jun 18, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a conservative DB-computed fingerprint fast path for read-only interface syncs.
  • Skip the expensive full reconcile when active database rows already match the configured _data payload.
  • Add regression coverage for the no-op path and the conservative fallback branches.

Why

Read-only startup hooks currently run on management commands and always perform the full synchronization path, even when data is already present and unchanged. That makes command startup slower than necessary.

Notes

The fast path intentionally falls back to the existing full sync for relation payloads, non-concrete measurement descriptors, duplicate unique identifiers, malformed payload rows, unsafe JSON serialization, or fields that cannot be compared directly.

Validation

  • python -m pytest tests/unit/test_read_only_interface.py tests/integration/test_readonly_interface_manager.py tests/integration/test_database_manager.py::DatabaseIntegrationTest::test_readonly_interface_sync
  • python -m pytest tests/unit/test_read_only_interface.py tests/integration/test_readonly_interface_manager.py tests/integration/test_database_manager.py::DatabaseIntegrationTest::test_readonly_interface_sync --cov=general_manager.interface.capabilities.read_only.management --cov-report=term-missing:skip-covered
  • ruff check src/general_manager/interface/capabilities/read_only/management.py tests/unit/test_read_only_interface.py
  • mypy src/general_manager/interface/capabilities/read_only/management.py
  • pre-commit during commit: ruff lint, ruff format, mypy, whitespace/end-of-file checks

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@TimKleindick, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 40 minutes and 20 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c8d0b167-89ec-4d42-af0e-d488176e1253

📥 Commits

Reviewing files that changed from the base of the PR and between 8abf679 and 84c5920.

📒 Files selected for processing (2)
  • src/general_manager/interface/capabilities/read_only/management.py
  • tests/unit/test_read_only_interface.py

Walkthrough

sync_data gains a fast-path that computes deterministic SHA-256 fingerprints of the incoming payload and the active database rows. When the fingerprints match, all create/update/deactivate operations are skipped. The implementation adds eligibility guards (relation fields, measurement fields, non-dict items, duplicate/missing unique keys) and new derived field sets. Ten new tests cover the skip case and all fallback scenarios.

Changes

sync_data fingerprint fast-path

Layer / File(s) Summary
Imports and derived field sets for fast-path eligibility
src/general_manager/interface/capabilities/read_only/management.py
Adds sha256 import and derives local_field_names and measurement_field_names sets used to decide whether a payload is eligible for the fingerprint comparison.
_active_database_matches_payload implementation
src/general_manager/interface/capabilities/read_only/management.py
Adds _fingerprint() (SHA-256 over stable JSON) and _row_sort_key() helpers; validates eligibility constraints; builds and compares sorted normalized snapshots of payload and active DB rows; returns early from sync_data on a fingerprint match.
Fast-path and fallback tests
tests/unit/test_read_only_interface.py
Captures atomic_mock in setUp; adds one test confirming the reconcile path is skipped on a matching second sync; adds nine tests confirming fallback behavior (exceptions, atomic_mock calls, drift deactivation) for each guard condition.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A hop through the hash, a skip past the sync,
If the data's the same, no need for the ink!
SHA-256 whispers: "All clear, friend!"
No updates, no writes — just a fast-path end.
The rabbit saves cycles, then naps in the hay. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title '[codex] skip unchanged read-only sync' accurately summarizes the main optimization: skipping synchronization when the database already matches the payload.
Docstring Coverage ✅ Passed Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/read-only-sync-fast-path

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@TimKleindick
TimKleindick force-pushed the codex/read-only-sync-fast-path branch from 71e81ab to 8abf679 Compare June 18, 2026 11:43
@TimKleindick
TimKleindick marked this pull request as ready for review June 18, 2026 12:30

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/general_manager/interface/capabilities/read_only/management.py`:
- Around line 516-535: Remove the `default=str` parameter from both json.dumps
calls in the `_fingerprint` and `_row_sort_key` methods to prevent silent
fingerprint collisions. Wrap each json.dumps call in a try-except block to catch
TypeErrors and handle them gracefully (such as returning None or raising a
meaningful exception). Then locate the fingerprint comparison logic around lines
625-627 and add None checks before comparing fingerprints to ensure unsafe
serialization failures are caught rather than producing unreliable results.

In `@tests/unit/test_read_only_interface.py`:
- Around line 724-728: The current test in the sync_data test method passes due
to an empty fake DB triggering an active-row count mismatch, not because the
duplicate-key guard is actually working. To strengthen the test, set up the fake
DB state to have data that would satisfy the active-row count check (matching
the number of records being synced), and then add instrumentation or mocking to
directly observe that the duplicate detection logic is being executed when
sync_data is called with DummyInterface. This ensures the test fails if the
duplicate-key guard is removed rather than passing for the wrong reason.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9ac40f5e-4cc8-47e3-9203-dfebc9a3cea5

📥 Commits

Reviewing files that changed from the base of the PR and between 970347a and 8abf679.

📒 Files selected for processing (2)
  • src/general_manager/interface/capabilities/read_only/management.py
  • tests/unit/test_read_only_interface.py

Comment thread src/general_manager/interface/capabilities/read_only/management.py Outdated
Comment thread tests/unit/test_read_only_interface.py
@TimKleindick
TimKleindick force-pushed the codex/read-only-sync-fast-path branch from 8abf679 to 84c5920 Compare June 18, 2026 12:49
@TimKleindick
TimKleindick marked this pull request as draft June 18, 2026 12:49
@TimKleindick
TimKleindick marked this pull request as ready for review June 18, 2026 12:50
@TimKleindick
TimKleindick merged commit 08334eb into main Jun 18, 2026
11 checks passed
@TimKleindick
TimKleindick deleted the codex/read-only-sync-fast-path branch June 18, 2026 12:54
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.

1 participant