TASK-012: Implement Sync Orchestrator

[Gabriel-Pereira1788]

Summary

Implements SyncOrchestrator::triggerSync (previously a stub) end-to-end, plus two foundational gaps discovered while analyzing the issue that neither the issue nor the docs covered:

• Sync contract capture: MigrationEngine::parseSchemaJson only read sync.enabled, discarding endpoint/pagination/request/response. Added SyncDefinitionRegistry to keep the full contract available to the orchestrator.
• Cursor persistence: no table stored a sync cursor across sessions. Added _salve_sync_cursors + SyncCursorStore.
• updatedAt requirement: registerSchema now requires a datetime column named updatedAt on any sync-enabled schema, used for lastWriteWins.
• SyncQueueReader.readPage: filters by entity and returns the row id needed to safely clear only what was sent.
• SyncHttpCaller: builds the arbitrary sync-page HTTP request (method/path/headers/auth) on top of the existing platform::httpExecute transport — no injectable IHttpClient (that design was abandoned on the unmerged feat/http-client branch).
• SyncOperationApplier: applies server operations with lastWriteWins, always inside SyncApplyGuard's bypass transaction.
• SyncOrchestrator::triggerSync: the real pagination loop — fixed retry (3x/5s, test-only seam for the delay), transparent 401→refresh→retry, atomic apply+cursor+queue-cleanup per page, stops at maxPagesPerSession and resumes from the persisted cursor.

A full write-up of the analysis (status inconsistencies, design gaps, architecture proposal) was posted as a comment on the issue before implementation started.

Closes #13

Test plan

• npm run test:native — 315 assertions / 123 test cases passing
• New unit coverage for every new collaborator (SyncDefinitionRegistry, SyncCursorStore, SyncQueueReader.readPage, SyncHttpCaller, SyncOperationApplier)
• SyncOrchestratorTests.cpp mapped 1:1 to the issue's 7 acceptance criteria (full cycle, multi-page, maxPagesPerSession cutoff + resume, network retry exhaustion, 401 refresh, no queue re-entry, mid-apply rollback)
• HybridSalveDatabaseSyncTests.cpp — same flow end-to-end through the real JSI bridge
• Pre-existing tests updated where the new updatedAt requirement affected them

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added end-to-end data synchronization with paginated requests, retries, credential refresh, cursor persistence, and conflict resolution.
  • Sync operations can now insert, update, and delete local records while reporting results.
  • Added support for custom request headers and JSON-based sync endpoints.
  • Sync-enabled schemas now require an updatedAt datetime column.

• Bug Fixes

  • Prevented stale sync configuration from persisting across database reopenings.

• Documentation

  • Updated schema documentation with the required updatedAt field and synchronization guidance.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 30102554-ca04-4f0a-8609-579db5da7c08

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/sync-orchestrator

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (3)

cpp/tests/sync/SyncOperationApplierTests.cpp (1)

86-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a tie-case test for the delete boundary.

Test name says "not older than local," but the data (remote=150, local=100) only exercises the strictly-newer case, not the remote == local boundary that the wording implies.

+TEST_CASE("apply deletes a row when the remote updatedAt equals local (tie goes to remote)", "[sync][SyncOperationApplier]") {
+  auto conn = openWithCustomers("applier_delete_tie");
+  conn->execute("INSERT INTO customers (id, name, updatedAt) VALUES ('1', 'a', 150)", {});
+  SyncOperationApplier applier(conn);
+
+  auto ops = json::parse(R"([
+    { "operation": "delete", "entity": "customers", "primaryKey": "1",
+      "payload": { "id": "1" }, "updatedAt": 150 }
+  ])").asArray();
+
+  auto stats = applier.apply(ops);
+  REQUIRE(stats.deleted == 1);
+}

🤖 Prompt for 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.

In `@cpp/tests/sync/SyncOperationApplierTests.cpp` around lines 86 - 99, Add a
tie-case test alongside the existing SyncOperationApplier delete test, using
equal local and remote updatedAt values while retaining the delete operation and
asserting stats.deleted is 1 and the row is absent. Keep the existing
strictly-newer test unchanged.

cpp/sync/SyncQueueReader.cpp (2)

41-45: 🚀 Performance & Scalability | 🔵 Trivial

Consider an index on sync_queue(entity, id).

readPage now filters by entity and orders by id on every page fetch, but sync_queue (per the MigrationEngine schema) only has a primary key on id. As the shared queue table accumulates rows across multiple sync-enabled schemas, this query becomes a full-table scan per page.

🤖 Prompt for 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.

In `@cpp/sync/SyncQueueReader.cpp` around lines 41 - 45, Update the
MigrationEngine schema definition for sync_queue to add a composite index on
(entity, id), supporting the entity filter and ascending id ordering used by
SyncQueueReader::readPage. Keep the existing primary key and queue behavior
unchanged.

---

9-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract shared row→operation conversion to reduce duplication.

readOperations and readPage build near-identical JSON operation objects, differing only by a one-column offset (readPage adds id at index 0). This is copy/paste that's already drifted; a future column change is easy to apply to one method and miss the other.

Consider a shared private helper that converts a row (given a column offset) into the json::Object, called from both methods.

Also applies to: 36-64

🤖 Prompt for 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.

In `@cpp/sync/SyncQueueReader.cpp` around lines 9 - 34, Extract the duplicated
row-to-operation mapping from SyncQueueReader::readOperations and readPage into
a shared private helper that accepts the row and column offset. Have both
methods call this helper, using offset 0 for readOperations and the offset
needed for readPage’s leading id column, while preserving the existing JSON
field names and value conversions.

🤖 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 `@cpp/database/MigrationEngine.cpp`:
- Around line 391-404: The sync-enabled schema validation in the root sync
parsing block must also reject nullable updatedAt columns. Update the
schema.columns lookup condition alongside the existing datetime type check to
require the updatedAt column’s not-null constraint, preserving the current error
path for invalid sync schemas.

In `@cpp/http/SyncHttpCaller.cpp`:
- Around line 51-66: Update the response handling in SyncHttpCaller so invalid
or empty JSON does not throw for non-success HTTP responses; preserve and return
the original response.statusCode with an appropriate placeholder payload,
allowing SyncOrchestrator’s 401 refresh and non-2xx retry/error branches to
execute. Keep normal JSON parsing for successful responses and retain
network-error handling.

In `@cpp/sync/SyncOperationApplier.cpp`:
- Around line 62-71: Align the delete branch in SyncOperationApplier with the
upsert lastWriteWins rule by changing its eligibility check so deletes are
skipped when remoteUpdatedAt is equal to or older than localUpdatedAt. Preserve
deletion for newer remote timestamps and the existing handling when
localUpdatedAt is absent.
- Around line 12-21: Restrict sync SQL construction to the registered schema
contract: validate each operation’s entity, pkCol, and payload columns against
the registered schema, and escape embedded quotes before using them as SQL
identifiers. Apply this validation before building statements that use
primaryKeyColumn and the operation-selected identifiers. Make lastWriteWins use
the same tie-breaking rule for delete, insert, and update, including equal
updatedAt values.

In `@cpp/sync/SyncOrchestrator.cpp`:
- Around line 55-91: Decouple the retry counter in sendPageWithRetryAnd401 from
the 401 refresh cycle so refreshing credentials and reissuing the same page does
not consume a genuine network-send attempt. Preserve the single-refresh behavior
and ensure a post-refresh network failure still receives the full kMaxAttempts
budget; add coverage for the combined 401-then-network-failure sequence if the
surrounding tests support it.

---

Nitpick comments:
In `@cpp/sync/SyncQueueReader.cpp`:
- Around line 41-45: Update the MigrationEngine schema definition for sync_queue
to add a composite index on (entity, id), supporting the entity filter and
ascending id ordering used by SyncQueueReader::readPage. Keep the existing
primary key and queue behavior unchanged.
- Around line 9-34: Extract the duplicated row-to-operation mapping from
SyncQueueReader::readOperations and readPage into a shared private helper that
accepts the row and column offset. Have both methods call this helper, using
offset 0 for readOperations and the offset needed for readPage’s leading id
column, while preserving the existing JSON field names and value conversions.

In `@cpp/tests/sync/SyncOperationApplierTests.cpp`:
- Around line 86-99: Add a tie-case test alongside the existing
SyncOperationApplier delete test, using equal local and remote updatedAt values
while retaining the delete operation and asserting stats.deleted is 1 and the
row is absent. Keep the existing strictly-newer test unchanged.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: da676c21-0849-41a5-bf2d-defc275c93f6

📥 Commits

Reviewing files that changed from the base of the PR and between 339dce8 and 988e131.

📒 Files selected for processing (30)

• android/CMakeLists.txt
• cpp/database/DatabaseManager.cpp
• cpp/database/MigrationEngine.cpp
• cpp/database/MigrationEngine.hpp
• cpp/http/SyncHttpCaller.cpp
• cpp/http/SyncHttpCaller.hpp
• cpp/sync/SyncCursorStore.cpp
• cpp/sync/SyncCursorStore.hpp
• cpp/sync/SyncDefinitionRegistry.cpp
• cpp/sync/SyncDefinitionRegistry.hpp
• cpp/sync/SyncOperationApplier.cpp
• cpp/sync/SyncOperationApplier.hpp
• cpp/sync/SyncOrchestrator.cpp
• cpp/sync/SyncOrchestrator.hpp
• cpp/sync/SyncQueueReader.cpp
• cpp/sync/SyncQueueReader.hpp
• cpp/tests/CMakeLists.txt
• cpp/tests/database/HybridSalveDatabaseSyncTests.cpp
• cpp/tests/database/MigrationEngineTests.cpp
• cpp/tests/expression/RequestExpressionEvaluatorTests.cpp
• cpp/tests/http/SyncHttpCallerTests.cpp
• cpp/tests/query/QueryExecutorTests.cpp
• cpp/tests/sync/README.md
• cpp/tests/sync/SyncApplyGuardTests.cpp
• cpp/tests/sync/SyncCursorStoreTests.cpp
• cpp/tests/sync/SyncDefinitionRegistryTests.cpp
• cpp/tests/sync/SyncOperationApplierTests.cpp
• cpp/tests/sync/SyncOrchestratorTests.cpp
• cpp/tests/sync/SyncQueueReaderTests.cpp
• docs/project.md