refactor(sync): drop batch_id protocol fields#1
Conversation
Transaction-scope HLC (see haex-vault refactor/crdt-transaction-scope-hlc) is now the semantic grouping key for a sender-side transaction. batchId / batchSeq / batchTotal were transport metadata from an earlier protocol iteration that never made it to the database; the client no longer sends them and the completeness check based on them ran against nothing. Remove the fields from pushChangeSchema and the SyncChange wire type, delete validateBatches + BatchValidationError and their call-site, and drop the corresponding unit tests. HLC-group atomicity is now enforced upstream: the client chunks pushes at HLC boundaries, and the server persists every request in a single db.transaction().
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request removes batch validation infrastructure from the sync system. The changes eliminate batch-related schema fields ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/sync.schemas.ts`:
- Line 45: The hlcTimestamp field currently uses hlcTimestamp: z.string() which
is too permissive; replace it with a structural validator—either
z.string().regex(...) with the canonical HLC pattern or z.string().refine(value
=> isValidHlc(value), { message: 'invalid HLC timestamp' }) and add a small
helper isValidHlc or parseHlcTimestamp that enforces the HLC format/components
(e.g., logical counter, wall-time, node id) so malformed values are rejected by
the schema; update the schema entry for hlcTimestamp to use that validator and
return a clear error message on failure.
In `@src/routes/sync.ts`:
- Around line 126-129: Before starting the transaction, validate that all
incoming records in the sync request have the same hlcTimestamp to enforce the
single-HLC invariant: iterate the array payload (e.g., request body field that
contains changes/ops), collect the distinct hlcTimestamp values and if more than
one is present return a 400 error (do not start the transaction) with an
explanatory message; this check should be placed in the sync route handler just
before the transaction block that uses lastHlc so lastHlc remains unambiguous.
🪄 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: 1e06ec2e-c7b8-42e7-a220-86b60ba35c7a
📒 Files selected for processing (4)
src/routes/sync.schemas.tssrc/routes/sync.tssrc/utils/syncUtils.tstests/syncBatchValidation.test.ts
💤 Files with no reviewable changes (1)
- tests/syncBatchValidation.test.ts
| rowPks: z.string(), // JSON string | ||
| columnName: sqlIdentifier.nullable(), | ||
| hlcTimestamp: z.string(), | ||
| hlcTimestamp: z.string(), // Transaction-scope HLC; shared by every change from one sender-side tx |
There was a problem hiding this comment.
Validate hlcTimestamp structurally instead of accepting any string.
On Line 45, hlcTimestamp: z.string() is too permissive now that HLC is the primary transaction/ordering key. Malformed values can break ordering assumptions in push conflict handling.
Suggested hardening
export const pushChangeSchema = z.object({
tableName: sqlIdentifier,
rowPks: z.string(), // JSON string
columnName: sqlIdentifier.nullable(),
- hlcTimestamp: z.string(), // Transaction-scope HLC; shared by every change from one sender-side tx
+ hlcTimestamp: z
+ .string()
+ .min(1)
+ // Replace with your canonical HLC regex/parser if available.
+ .regex(/^[^\\s]+$/, 'invalid hlcTimestamp'),
deviceId: z.string().optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hlcTimestamp: z.string(), // Transaction-scope HLC; shared by every change from one sender-side tx | |
| export const pushChangeSchema = z.object({ | |
| tableName: sqlIdentifier, | |
| rowPks: z.string(), // JSON string | |
| columnName: sqlIdentifier.nullable(), | |
| hlcTimestamp: z | |
| .string() | |
| .min(1) | |
| // Replace with your canonical HLC regex/parser if available. | |
| .regex(/^[^\s]+$/, 'invalid hlcTimestamp'), | |
| deviceId: z.string().optional(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/sync.schemas.ts` at line 45, The hlcTimestamp field currently uses
hlcTimestamp: z.string() which is too permissive; replace it with a structural
validator—either z.string().regex(...) with the canonical HLC pattern or
z.string().refine(value => isValidHlc(value), { message: 'invalid HLC timestamp'
}) and add a small helper isValidHlc or parseHlcTimestamp that enforces the HLC
format/components (e.g., logical counter, wall-time, node id) so malformed
values are rejected by the schema; update the schema entry for hlcTimestamp to
use that validator and return a clear error message on failure.
| // Apply changes atomically in a transaction so either ALL changes are | ||
| // applied or NONE. HLC-group atomicity is guaranteed by the client | ||
| // chunking at HLC boundaries before calling this endpoint — the server | ||
| // trusts the sender's grouping and does no completeness verification. |
There was a problem hiding this comment.
Enforce the single-HLC request invariant server-side.
Line 126-Line 129 explicitly trusts client chunking, but there is no guard against mixed hlcTimestamp values in one request. That can violate the documented contract and make lastHlc ambiguous.
Suggested guard before transaction
const changes = rawChanges as PushChange[]
+ const uniqueHlc = new Set(changes.map((c) => c.hlcTimestamp))
+ if (uniqueHlc.size > 1) {
+ return c.json({ error: 'All changes in one push request must share the same hlcTimestamp' }, 400)
+ }
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/sync.ts` around lines 126 - 129, Before starting the transaction,
validate that all incoming records in the sync request have the same
hlcTimestamp to enforce the single-HLC invariant: iterate the array payload
(e.g., request body field that contains changes/ops), collect the distinct
hlcTimestamp values and if more than one is present return a 400 error (do not
start the transaction) with an explanatory message; this check should be placed
in the sync route handler just before the transaction block that uses lastHlc so
lastHlc remains unambiguous.
src/db/index.ts throws on module load when DATABASE_URL is unset. federation.helpers.test.ts mocks ../src/db via mock.module(), but ESM imports are hoisted, so the mock is registered too late when another test file triggers the load first. CI runner order surfaces this, local runs do not. Same fix pattern as STORAGE_ENCRYPTION_KEY (2d14c95): set a dummy value in the bun preload so module-load never throws. postgres() is lazy and opens no connection until a query runs, so tests that mock the db see no real I/O.
1 participant
Summary
batchId/batchSeq/batchTotalfrompushChangeSchemaand theSyncChangewire typevalidateBatches+BatchValidationErrorand the/sync/pushcall-site that invoked themtests/syncBatchValidation.test.ts(validated a protocol the client no longer speaks)Why
Transaction-scope HLC (see haex-vault
refactor/crdt-transaction-scope-hlc) is now the semantic grouping key for a sender-side transaction. The batch_id trio was transport metadata from an older protocol iteration that never reached the database, and the client has already stopped sending it, so the server-side validator ran against nothing.HLC-group atomicity is enforced by the client instead: it chunks pushes at HLC boundaries, and the server persists every request in a single
db.transaction().Companion work
Test plan
bun test— 279/279 pass aftervalidateBatchesremovedtests/syncsuite on the companion branches — 97/97 passSummary by CodeRabbit