diff --git a/electron/remote-sync.cjs b/electron/remote-sync.cjs index 57394f65..ae3196ca 100644 --- a/electron/remote-sync.cjs +++ b/electron/remote-sync.cjs @@ -481,26 +481,28 @@ class RemoteSyncEngine { } } - // Apply tombstones to whichever side hasn't already deleted the row. + // Apply every tombstone to the other side. Gating this on the row being + // present in localBySyncId/remoteBySyncId would skip the common case: + // once both sides have converged, the surviving copy of a deleted row + // has not been touched, so it is not in the incremental window at all. + // The receiving end ignores a deletion for a row it no longer has and + // records no tombstone for it, so re-sending one costs a no-op request + // and nothing ping-pongs back. for (const tombstone of localTombstones) { - if (remoteBySyncId.has(tombstone.syncId)) { - await this.pushTombstone( - remoteBaseUrl, - remoteJwt, - entityType, - tombstone.syncId, - ); - } + await this.pushTombstone( + remoteBaseUrl, + remoteJwt, + entityType, + tombstone.syncId, + ); } for (const tombstone of remoteTombstones) { - if (localBySyncId.has(tombstone.syncId)) { - await this.pushTombstone( - EMBEDDED_BASE_URL, - this.localJwt, - entityType, - tombstone.syncId, - ); - } + await this.pushTombstone( + EMBEDDED_BASE_URL, + this.localJwt, + entityType, + tombstone.syncId, + ); } return { syncedAt }; diff --git a/src/backend/database/routes/sync.ts b/src/backend/database/routes/sync.ts index b12a5a79..fbc4bf5e 100644 --- a/src/backend/database/routes/sync.ts +++ b/src/backend/database/routes/sync.ts @@ -84,6 +84,24 @@ export function isValidEntityType(value: unknown): value is SyncEntityType { return typeof value === "string" && VALID_ENTITY_TYPES.has(value); } +// updatedAt/deletedAt are TEXT columns written by CURRENT_TIMESTAMP, so they +// read as "2026-07-29 10:11:21" while clients send an ISO 8601 `since` like +// "2026-07-29T10:06:55.172Z". Both comparisons are lexical, and ' ' (0x20) +// sorts below 'T' (0x54), so a newer row loses the comparison at position 10 +// and the incremental window comes back empty forever. Rewrite `since` into +// the stored shape. Dropping the fraction can re-send rows from the same +// second, which is harmless — every apply path upserts by syncId. +export function normalizeSince(value: unknown): string | null { + if (typeof value !== "string" || !value) return null; + // Already stored-shaped. Parsing it would be worse than leaving it alone: + // Date treats a string with no zone as local time, which shifts the cursor + // by the offset and, west of UTC, moves it forward past unsynced rows. + if (/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/.test(value)) return value; + const parsed = new Date(value); + if (Number.isNaN(parsed.getTime())) return null; + return parsed.toISOString().slice(0, 19).replace("T", " "); +} + async function findReferenceSyncId( context: RepositoryContext, entityType: SyncReferenceEntity, @@ -213,6 +231,82 @@ export function stripWritePayload( * 500: * description: Failed to fetch rows. */ +/** + * @openapi + * /sync/tombstones: + * post: + * summary: Report a deletion from the other side of a sync pair + * description: Applies a remote deletion locally (if the row still exists) and records the tombstone so future pulls stay consistent. + * tags: + * - Sync + * responses: + * 200: + * description: Deletion applied (or row already absent). + * 400: + * description: Unknown entity type or missing syncId. + * 500: + * description: Failed to apply deletion. + */ +// Must stay ahead of the "/:entityType" routes below: Express matches in +// registration order and "tombstones" is a valid :entityType value, so a +// wildcard registered first would answer this path with "Unknown entity type". +router.post( + "/tombstones", + authenticateJWT, + async (req: Request, res: Response) => { + const userId = (req as AuthenticatedRequest).userId; + const entityType = req.body?.entityType; + const syncId = req.body?.syncId; + if ( + !isValidEntityType(entityType) || + typeof syncId !== "string" || + !syncId + ) { + return res.status(400).json({ error: "Missing entityType or syncId" }); + } + + try { + const { table, singleton } = ENTITY_CONFIG[entityType]; + const context = createCurrentRepositoryContext(); + const match = singleton + ? eq(table.userId, userId) + : and( + eq((table as typeof hosts).syncId, syncId), + eq(table.userId, userId), + ); + + const existing = await context.drizzle + .select() + .from(table as typeof hosts) + .where(match) + .limit(1); + + // Only a deletion that actually removed something earns a tombstone. + // Recording one unconditionally would hand the sender a fresh tombstone + // to push back on the next pass, and the two sides would trade the same + // deletion forever. + if (existing.length > 0) { + await context.drizzle.delete(table as typeof hosts).where(match); + await createCurrentSyncTombstoneRepository().record( + userId, + entityType, + syncId, + ); + await DatabaseSaveTrigger.forceSave("sync_tombstone_applied"); + } + + res.json({ success: true }); + } catch (err) { + databaseLogger.error("Failed to apply sync tombstone", err, { + operation: "sync_tombstone_apply", + entityType, + userId, + }); + res.status(500).json({ error: "Failed to apply deletion" }); + } + }, +); + router.get( "/:entityType", authenticateJWT, @@ -222,10 +316,7 @@ router.get( if (!isValidEntityType(entityType)) { return res.status(400).json({ error: "Unknown entity type" }); } - const since = - typeof req.query.since === "string" && req.query.since - ? req.query.since - : null; + const since = normalizeSince(req.query.since); try { const { table, singleton } = ENTITY_CONFIG[entityType]; @@ -415,10 +506,7 @@ router.get( if (!isValidEntityType(entityType)) { return res.status(400).json({ error: "Unknown entity type" }); } - const since = - typeof req.query.since === "string" && req.query.since - ? req.query.since - : null; + const since = normalizeSince(req.query.since); try { const tombstones = await createCurrentSyncTombstoneRepository().listSince( @@ -438,69 +526,4 @@ router.get( }, ); -/** - * @openapi - * /sync/tombstones: - * post: - * summary: Report a deletion from the other side of a sync pair - * description: Applies a remote deletion locally (if the row still exists) and records the tombstone so future pulls stay consistent. - * tags: - * - Sync - * responses: - * 200: - * description: Deletion applied (or row already absent). - * 400: - * description: Unknown entity type or missing syncId. - * 500: - * description: Failed to apply deletion. - */ -router.post( - "/tombstones", - authenticateJWT, - async (req: Request, res: Response) => { - const userId = (req as AuthenticatedRequest).userId; - const entityType = req.body?.entityType; - const syncId = req.body?.syncId; - if ( - !isValidEntityType(entityType) || - typeof syncId !== "string" || - !syncId - ) { - return res.status(400).json({ error: "Missing entityType or syncId" }); - } - - try { - const { table, singleton } = ENTITY_CONFIG[entityType]; - const context = createCurrentRepositoryContext(); - - await context.drizzle - .delete(table as typeof hosts) - .where( - singleton - ? eq(table.userId, userId) - : and( - eq((table as typeof hosts).syncId, syncId), - eq(table.userId, userId), - ), - ); - - await createCurrentSyncTombstoneRepository().record( - userId, - entityType, - syncId, - ); - await DatabaseSaveTrigger.forceSave("sync_tombstone_applied"); - - res.json({ success: true }); - } catch (err) { - databaseLogger.error("Failed to apply sync tombstone", err, { - operation: "sync_tombstone_apply", - entityType, - userId, - }); - res.status(500).json({ error: "Failed to apply deletion" }); - } - }, -); - export default router; diff --git a/src/backend/tests/database/routes/sync.test.ts b/src/backend/tests/database/routes/sync.test.ts index 377a3b18..fa15afc0 100644 --- a/src/backend/tests/database/routes/sync.test.ts +++ b/src/backend/tests/database/routes/sync.test.ts @@ -1,9 +1,31 @@ import { describe, expect, it } from "vitest"; -import { +import syncRouter, { isValidEntityType, + normalizeSince, stripWritePayload, } from "../../../database/routes/sync.js"; +describe("sync route registration order", () => { + // Express matches in registration order and "tombstones" is a perfectly + // good :entityType value, so a POST /:entityType registered first swallows + // POST /sync/tombstones and answers it with 400 "Unknown entity type". + const paths = ( + syncRouter as unknown as { + stack: { route?: { path: string; methods: Record } }[]; + } + ).stack + .filter((layer) => layer.route?.methods.post) + .map((layer) => layer.route!.path); + + it("registers POST /tombstones before the POST /:entityType wildcard", () => { + expect(paths).toContain("/tombstones"); + expect(paths).toContain("/:entityType"); + expect(paths.indexOf("/tombstones")).toBeLessThan( + paths.indexOf("/:entityType"), + ); + }); +}); + describe("isValidEntityType", () => { it("accepts every whitelisted sync entity type", () => { for (const type of [ @@ -29,6 +51,41 @@ describe("isValidEntityType", () => { }); }); +describe("normalizeSince", () => { + it("rewrites an ISO cursor into the shape CURRENT_TIMESTAMP stores", () => { + expect(normalizeSince("2026-07-29T10:06:55.172Z")).toBe( + "2026-07-29 10:06:55", + ); + }); + + it("makes a newer row win the lexical comparison against the cursor", () => { + // The bug: a row written five minutes after the cursor still compared as + // older, because ' ' (0x20) sorts below 'T' (0x54) at position 10. + const storedRow = "2026-07-29 10:11:21"; + const rawCursor = "2026-07-29T10:06:55.172Z"; + expect(storedRow > rawCursor).toBe(false); + expect(storedRow > normalizeSince(rawCursor)!).toBe(true); + }); + + it("keeps a Postgres timestamp with fraction and offset above the cursor", () => { + expect( + "2026-07-29 10:06:55.123456+00" > + normalizeSince("2026-07-29T10:06:55.172Z")!, + ).toBe(true); + }); + + it("treats a missing, empty, or unparsable cursor as no filter", () => { + expect(normalizeSince(undefined)).toBeNull(); + expect(normalizeSince("")).toBeNull(); + expect(normalizeSince("not a date")).toBeNull(); + expect(normalizeSince(1753783615172)).toBeNull(); + }); + + it("is idempotent, so an already-normalized cursor survives a round trip", () => { + expect(normalizeSince("2026-07-29 10:06:55")).toBe("2026-07-29 10:06:55"); + }); +}); + describe("stripWritePayload", () => { it("strips id, userId, and syncId from every entity type", () => { const payload = {