From ca7abf8426aaa547510b62ba0019beb96d05f321 Mon Sep 17 00:00:00 2001 From: ZacharyZcR Date: Fri, 31 Jul 2026 16:43:11 +0800 Subject: [PATCH] fix remote sync stalling after the first pass and never propagating deletions (#1140) The incremental cursor never matched. updated_at/deleted_at are TEXT columns written by CURRENT_TIMESTAMP ("2026-07-29 10:11:21"), while the client sends an ISO 8601 since ("2026-07-29T10:06:55.172Z"). Both comparisons are lexical and ' ' sorts below 'T', so a newer row lost at position 10 and every ?since= query came back empty. Pass 1 syncs everything (since is null) and persists a cursor; every pass after it returns nothing with lastError: null and reports success. Normalize since into the stored shape on the way in, leaving an already-normalized value alone -- parsing that would treat it as local time and, west of UTC, push the cursor past unsynced rows. POST /sync/tombstones was unreachable. It was registered after POST /:entityType, and "tombstones" is a valid :entityType, so the wildcard answered it with 400 "Unknown entity type" and the handler never ran. The pass has no per-entity error handling, so that 400 also discarded the state of every entity type already synced in the same pass. Move it ahead of the wildcards. The tombstone guard consulted the incremental window. A row deleted on one side and untouched on the other -- the shape every ordinary deletion takes once the two sides converge -- is not in that window, so the tombstone was skipped, and skipped again on each later pass as it slid out of its own window. The guard cannot just be dropped: recording a tombstone for a row that was already gone hands the sender a fresh one to push back, and the two trade the same deletion forever. So only a delete that removed something records a tombstone, which makes the endpoint idempotent and lets the client push every tombstone unconditionally. Deletions missed while the cursor was broken stay missed -- their tombstones predate the persisted cursor. Ordinary edits do come through, since the row's updatedAt is still newer than it. Fixes Termix-SSH/Support#1050 Fixes Termix-SSH/Support#1051 --- electron/remote-sync.cjs | 36 ++-- src/backend/database/routes/sync.ts | 169 ++++++++++-------- .../tests/database/routes/sync.test.ts | 59 +++++- 3 files changed, 173 insertions(+), 91 deletions(-) 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 = {