diff --git a/packages/super-sync-server/src/sync/services/request-deduplication.service.ts b/packages/super-sync-server/src/sync/services/request-deduplication.service.ts index 1da342a62f..bf4bb24755 100644 --- a/packages/super-sync-server/src/sync/services/request-deduplication.service.ts +++ b/packages/super-sync-server/src/sync/services/request-deduplication.service.ts @@ -56,6 +56,20 @@ export class RequestDeduplicationService { this.cache.set(key, { processedAt: Date.now(), results }); } + /** + * Remove every cached entry for the given user. Call when the user's data is + * wiped (account reset / encryption-password change) so cached results from + * the pre-wipe state cannot be returned for a post-wipe retry. + */ + clearForUser(userId: number): void { + const prefix = `${userId}:`; + for (const key of this.cache.keys()) { + if (key.startsWith(prefix)) { + this.cache.delete(key); + } + } + } + /** * Remove expired entries from memory. * Should be called periodically to prevent stale entries. diff --git a/packages/super-sync-server/src/sync/sync.const.ts b/packages/super-sync-server/src/sync/sync.const.ts index e28b452d89..71e0eebde0 100644 --- a/packages/super-sync-server/src/sync/sync.const.ts +++ b/packages/super-sync-server/src/sync/sync.const.ts @@ -5,11 +5,44 @@ export { /** * Approximate bytes-per-op used when decrementing `users.storage_used_bytes` - * during cleanup-deletes. The exact figure would require detoasting every - * deleted payload via `pg_column_size`, which was the source of the production - * disk-I/O DoS. Picked as a conservative over-estimate vs the observed median - * task-op (~150-300 bytes) so the cleanup loop reliably makes progress; drift - * is reconciled once at the end of `freeStorageForUpload` via a single + * during DELTA-op cleanup-deletes. ONLY valid for ordinary CRT/UPD/DEL ops + * whose payloads observably cluster around 150-300 bytes — picking 1024 is a + * conservative over-estimate so the cleanup loop reliably makes progress; + * drift is reconciled once at the end of `freeStorageForUpload` via a single * `updateStorageUsage` scan. + * + * DO NOT use for full-state ops (SYNC_IMPORT / BACKUP_IMPORT / REPAIR). Their + * payloads can be up to 20MB, so 1024 undercounts by ~20000x and the cached + * counter ends up permanently low if reconcile fails. `deleteOldestRestorePointAndOps` + * measures the exact `pg_column_size(payload)` for those 1-2 rows BEFORE + * deleting; the per-row scan there is bounded to the restore-point fan-out + * (very small) and does not reintroduce the SUM(pg_column_size) DoS that + * scanning every delta op caused. */ export const APPROX_BYTES_PER_OP = 1024; + +/** + * Locally-computed approximation of how many bytes an operation's payload and + * vector clock will occupy on disk. Used by both the route layer (for quota + * gating and post-commit counter deltas) and the service layer (for the atomic + * counter write inside the upload transaction). Keeping a single + * implementation guarantees the gate and the increment cannot disagree about + * what "size" means. + * + * Robust against malformed payloads: if JSON.stringify throws (e.g. BigInt, + * circular ref), the op is charged APPROX_BYTES_PER_OP so the counter cannot + * be bypassed by submitting unserializable ops that still persist as JSONB. + */ +export const computeOpStorageBytes = (op: { + payload: unknown; + vectorClock: unknown; +}): number => { + try { + return ( + Buffer.byteLength(JSON.stringify(op.payload ?? null), 'utf8') + + Buffer.byteLength(JSON.stringify(op.vectorClock ?? {}), 'utf8') + ); + } catch { + return APPROX_BYTES_PER_OP; + } +}; diff --git a/packages/super-sync-server/src/sync/sync.service.ts b/packages/super-sync-server/src/sync/sync.service.ts index 5ac20e84d1..0ffbd56134 100644 --- a/packages/super-sync-server/src/sync/sync.service.ts +++ b/packages/super-sync-server/src/sync/sync.service.ts @@ -11,7 +11,7 @@ import { SYNC_ERROR_CODES, ConflictResult, } from './sync.types'; -import { APPROX_BYTES_PER_OP } from './sync.const'; +import { APPROX_BYTES_PER_OP, computeOpStorageBytes } from './sync.const'; import { Logger } from '../logger'; import { Prisma } from '@prisma/client'; import { @@ -273,9 +273,42 @@ export class SyncService { update: {}, // No-op update to ensure it exists }); + // Track the delta-bytes for accepted ops so we can write + // `users.storage_used_bytes` atomically in the same transaction as the + // op inserts. Doing the counter write outside this transaction (as + // the route layer used to) opens a window where the data commits but + // the counter does not — if the process dies between, the in-memory + // `markStorageNeedsReconcile` marker is lost too. + let acceptedDeltaBytes = 0; for (const op of ops) { const result = await this.processOperation(userId, clientId, op, now, tx); results.push(result); + if (result.accepted) { + acceptedDeltaBytes += computeOpStorageBytes(op); + } + } + + // Atomic counter write inside the same transaction as the data write. + // GREATEST(..., 0) guards against negative drift (the counter is + // advisory; reconcile self-heals if it ever drifts). Skip when clean + // slate already reset the counter to zero earlier in this transaction. + if (acceptedDeltaBytes > 0 && !isCleanSlate) { + const delta = BigInt(Math.floor(acceptedDeltaBytes)); + await tx.$executeRaw` + UPDATE users + SET storage_used_bytes = GREATEST(storage_used_bytes + ${delta}::bigint, 0::bigint) + WHERE id = ${userId} + `; + } else if (acceptedDeltaBytes > 0 && isCleanSlate) { + // Clean slate reset the counter to 0 above; the only "real" usage + // after the wipe is the ops being uploaded right now. Set rather + // than increment so we don't double-count anything left in the row. + const delta = BigInt(Math.floor(acceptedDeltaBytes)); + await tx.$executeRaw` + UPDATE users + SET storage_used_bytes = ${delta}::bigint + WHERE id = ${userId} + `; } // Update device last seen @@ -312,11 +345,14 @@ export class SyncService { }, ); - // Clear caches after clean slate transaction completes successfully + // Clear caches after clean slate transaction completes successfully. + // Include request dedup so a retry from before the wipe cannot return + // cached results that reference now-deleted state. if (isCleanSlate) { this.rateLimitService.clearForUser(userId); this.snapshotService.clearForUser(userId); this.storageQuotaService.clearForUser(userId); + this.requestDeduplicationService.clearForUser(userId); } } catch (err) { // Transaction failed - all operations were rolled back @@ -346,8 +382,11 @@ export class SyncService { Logger.error(`Transaction failed for user ${userId}: ${errorMessage}`); } - // Mark all "successful" results as failed due to transaction rollback - // Use INTERNAL_ERROR for all transient failures - client will retry + // Mark all "successful" results as failed due to transaction rollback. + // Use INTERNAL_ERROR for all transient failures - client will retry. + // The raw `errorMessage` is logged above but never returned to the client: + // Prisma exceptions can include SQL fragments, column names, and FK names, + // so the per-op error string is a generic, non-leaky message instead. return ops.map((op) => ({ opId: op.id, accepted: false, @@ -355,7 +394,7 @@ export class SyncService { ? 'Concurrent transaction conflict - please retry' : isTimeout ? 'Transaction timeout - server busy, please retry' - : `Transaction rolled back: ${errorMessage}`, + : 'Transaction failed - please retry', errorCode: SYNC_ERROR_CODES.INTERNAL_ERROR, })); } @@ -885,8 +924,18 @@ export class SyncService { affectedUserIds.push(state.userId); // Deliberately leave storageUsedBytes stale-high here. A count-based // approximate decrement can undercount users with many tiny ops and - // let them bypass quota indefinitely. The quota-miss path performs one - // exact reconcile for the affected user before rejecting uploads. + // let them bypass quota indefinitely. Mark the user as needing an + // exact reconcile so their next request self-heals the drift instead + // of waiting for the daily pass — and, crucially, so that a crash + // mid-loop still lets surviving deletes self-reconcile rather than + // leaving the counter stale-high indefinitely. + // NOTE: the marker is in-memory (process-local). A persistent + // `users.storage_needs_reconcile` column would survive restarts; see + // TODO below. + // TODO: persist the reconcile marker in a DB column so it survives + // restarts of a single-instance deployment and works correctly across + // a multi-instance deployment behind a load balancer. + this.storageQuotaService.markNeedsReconcile(state.userId); } } } @@ -946,10 +995,28 @@ export class SyncService { return { deletedCount: 0, freedBytes: 0, success: false }; } - // freedBytes is an approximate decrement applied to the cached counter so - // freeStorageForUpload's loop can make progress without recomputing per-row - // pg_column_size (the original DoS pattern). Reconciled to exact value once - // at the end of freeStorageForUpload via a single updateStorageUsage call. + // Full-state ops (SYNC_IMPORT/BACKUP_IMPORT/REPAIR) can be up to 20MB each, + // so the APPROX_BYTES_PER_OP=1024 fallback used for delta ops would undercount + // by ~20000x and leave the cached counter permanently low if a reconcile + // failure later rolls back to that figure. Measure the exact bytes for the + // restore-point rows in the deletion window via pg_column_size BEFORE we + // delete them — `deleteOldestRestorePointAndOps` deletes at most ONE + // restore point per call (or zero, when it keeps the single remaining one), + // so this is a bounded 0-1 row scan that does not reintroduce the DoS the + // earlier SUM(pg_column_size) over every delta op caused. + const fullStateRows = await prisma.$queryRaw< + Array<{ exact_bytes: bigint | null; full_state_count: bigint }> + >` + SELECT + COALESCE(SUM(pg_column_size(payload) + pg_column_size(vector_clock)), 0) AS exact_bytes, + COUNT(*)::bigint AS full_state_count + FROM operations + WHERE user_id = ${userId} + AND server_seq <= ${deleteUpToSeq} + AND op_type IN ('SYNC_IMPORT', 'BACKUP_IMPORT', 'REPAIR') + `; + const fullStateExactBytes = Number(fullStateRows[0]?.exact_bytes ?? 0); + const fullStateCount = Number(fullStateRows[0]?.full_state_count ?? 0); // Delete the operations const result = await prisma.operation.deleteMany({ @@ -959,7 +1026,15 @@ export class SyncService { }, }); - const freedBytes = result.count * APPROX_BYTES_PER_OP; + // freedBytes is split: exact size for the 0-1 restore-point rows just + // measured (catches the 20MB-ish payloads that the APPROX_BYTES_PER_OP + // approximation undercounts by ~20000x), plus the approximate + // count*APPROX_BYTES_PER_OP for the remaining delta ops (median 150-300B + // — modest over-estimate so the cleanup loop progresses without scanning + // every delta payload). Reconciled to exact value once at the end of + // freeStorageForUpload via a single updateStorageUsage call. + const deltaOpsCount = Math.max(0, result.count - fullStateCount); + const freedBytes = fullStateExactBytes + deltaOpsCount * APPROX_BYTES_PER_OP; if (result.count > 0) { // Clear stale snapshot cache if it references deleted operations @@ -1187,10 +1262,14 @@ export class SyncService { }); }); - // Clear caches + // Clear caches. Include the request-dedup cache so a retry of an + // ops-upload from the pre-wipe state cannot resurrect its cached results + // post-wipe. (Process-local only; persists across requests within the + // single instance.) this.rateLimitService.clearForUser(userId); this.snapshotService.clearForUser(userId); this.storageQuotaService.clearForUser(userId); + this.requestDeduplicationService.clearForUser(userId); } async isDeviceOwner(userId: number, clientId: string): Promise { diff --git a/packages/super-sync-server/tests/conflict-detection.spec.ts b/packages/super-sync-server/tests/conflict-detection.spec.ts index 7d82b19868..889ee614d0 100644 --- a/packages/super-sync-server/tests/conflict-detection.spec.ts +++ b/packages/super-sync-server/tests/conflict-detection.spec.ts @@ -160,6 +160,8 @@ vi.mock('../src/db', async () => { return state.users.get(args.where.id) || null; }), }, + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }); return { diff --git a/packages/super-sync-server/tests/duplicate-operation-precheck.spec.ts b/packages/super-sync-server/tests/duplicate-operation-precheck.spec.ts index a7e63d08f3..c297ca6c7a 100644 --- a/packages/super-sync-server/tests/duplicate-operation-precheck.spec.ts +++ b/packages/super-sync-server/tests/duplicate-operation-precheck.spec.ts @@ -381,7 +381,10 @@ describe('Duplicate Operation Pre-check', () => { accepted: false, errorCode: SYNC_ERROR_CODES.INTERNAL_ERROR, }); - expect(results[0].error).toContain('non-id unique constraint'); + // Generic, non-leaky message. The original Prisma exception text (which + // can include SQL fragments, column / FK names) is only emitted to the + // server log; the per-op error returned to the client must not leak it. + expect(results[0].error).toBe('Transaction failed - please retry'); expect(tx.userSyncState.update).toHaveBeenCalledTimes(1); expect(tx.syncDevice.upsert).not.toHaveBeenCalled(); }); diff --git a/packages/super-sync-server/tests/gap-detection.spec.ts b/packages/super-sync-server/tests/gap-detection.spec.ts index 201c299ec4..ed24ba13cf 100644 --- a/packages/super-sync-server/tests/gap-detection.spec.ts +++ b/packages/super-sync-server/tests/gap-detection.spec.ts @@ -172,6 +172,8 @@ vi.mock('../src/db', async () => { return state.users.get(args.where.id) || null; }), }, + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }); return { diff --git a/packages/super-sync-server/tests/setup.ts b/packages/super-sync-server/tests/setup.ts index 11b8d87a79..f2deecb9a2 100644 --- a/packages/super-sync-server/tests/setup.ts +++ b/packages/super-sync-server/tests/setup.ts @@ -288,6 +288,11 @@ vi.mock('../src/db', () => { update: vi.fn().mockResolvedValue({}), }, $queryRaw: vi.fn().mockResolvedValue([{ total: BigInt(0) }]), + // The upload transaction writes the storage counter atomically via + // $executeRaw to keep the data write and the counter delta in a single + // commit. Default mock is a no-op; specs that care about counter + // behaviour mock it explicitly. + $executeRaw: vi.fn().mockResolvedValue(0), }; if (typeof callback === 'function') { return callback(tx); @@ -320,6 +325,7 @@ vi.mock('../src/db', () => { update: vi.fn(), }, $queryRaw: vi.fn().mockResolvedValue([{ total: BigInt(0) }]), + $executeRaw: vi.fn().mockResolvedValue(0), }; return { diff --git a/packages/super-sync-server/tests/storage-quota-cleanup.spec.ts b/packages/super-sync-server/tests/storage-quota-cleanup.spec.ts index cc54fe7e39..ada9a9686f 100644 --- a/packages/super-sync-server/tests/storage-quota-cleanup.spec.ts +++ b/packages/super-sync-server/tests/storage-quota-cleanup.spec.ts @@ -105,6 +105,8 @@ vi.mock('../src/db', () => { upsert: vi.fn().mockResolvedValue({}), count: vi.fn().mockResolvedValue(1), }, + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }; return callback(tx); }), @@ -193,11 +195,20 @@ vi.mock('../src/db', () => { }, $queryRaw: vi .fn() - .mockImplementation(async (_query, userIdArg, deleteUpToSeqArg) => { - // Compute total bytes from test operations that match the SQL WHERE params. - // Only used by calculateStorageUsage (offline path) — hot paths no - // longer SUM pg_column_size. - let total = BigInt(0); + .mockImplementation(async (query, userIdArg, deleteUpToSeqArg) => { + // The same mock serves two SQL shapes: + // 1. calculateStorageUsage's full-table SUM(pg_column_size) + // scan (slow path) — returns `[{ total }]`. + // 2. deleteOldestRestorePointAndOps's BOUNDED full-state scan + // filtered by `op_type IN (SYNC_IMPORT, BACKUP_IMPORT, REPAIR)` + // — returns `[{ exact_bytes, full_state_count }]`. + // Detect by inspecting the SQL template fragments. + const queryParts = query as unknown as TemplateStringsArray; + const sql = Array.isArray(queryParts) ? queryParts.join('') : ''; + const isFullStateScan = sql.includes('op_type IN'); + + let totalBytes = BigInt(0); + let fullStateCount = BigInt(0); for (const op of testOperations.values()) { if (typeof userIdArg === 'number' && op.userId !== userIdArg) { continue; @@ -205,12 +216,23 @@ vi.mock('../src/db', () => { if (typeof deleteUpToSeqArg === 'number' && op.serverSeq > deleteUpToSeqArg) { continue; } - + const isRestorePoint = + op.opType === 'SYNC_IMPORT' || + op.opType === 'BACKUP_IMPORT' || + op.opType === 'REPAIR'; + if (isFullStateScan && !isRestorePoint) { + continue; + } const payloadSize = op.payload ? JSON.stringify(op.payload).length : 0; const clockSize = op.vectorClock ? JSON.stringify(op.vectorClock).length : 0; - total += BigInt(payloadSize + clockSize); + totalBytes += BigInt(payloadSize + clockSize); + if (isRestorePoint) fullStateCount++; } - return [{ total }]; + + if (isFullStateScan) { + return [{ exact_bytes: totalBytes, full_state_count: fullStateCount }]; + } + return [{ total: totalBytes }]; }), // decrementStorageUsage uses $executeRaw with a clamped UPDATE. // Simulate by mutating the matching user's storageUsedBytes in-place. @@ -371,11 +393,14 @@ describe('Storage Quota Cleanup', () => { expect(testOperations.size).toBe(4); // ops 4, 5, 6, 7 remain }); - it('should not run any per-row pg_column_size query during cleanup-delete', async () => { - // The cleanup path used to SUM(pg_column_size(payload)) over the range - // being deleted, which forced PostgreSQL to detoast every payload and - // caused the production disk-I/O DoS. Counter is now decremented via - // an approximate count*const decrement; no $queryRaw should run. + it('should not run pg_column_size over the full delta-op range during cleanup-delete', async () => { + // The cleanup path used to SUM(pg_column_size(payload)) over the FULL + // range being deleted, which forced PostgreSQL to detoast every payload + // and caused the production disk-I/O DoS. Counter is decremented via an + // approximate count*const decrement for delta ops; the only allowed + // pg_column_size scan is over the bounded restore-point rows + // (op_type IN (SYNC_IMPORT, BACKUP_IMPORT, REPAIR)) so the 20MB-ish + // full-state payloads do not undercount. const { initSyncService, getSyncService } = await import('../src/sync/sync.service'); const { prisma } = await import('../src/db'); @@ -392,7 +417,10 @@ describe('Storage Quota Cleanup', () => { const offendingCalls = vi.mocked(prisma.$queryRaw).mock.calls.filter((call) => { const queryParts = call[0] as unknown as TemplateStringsArray; - return Array.from(queryParts).join('').includes('pg_column_size'); + const sql = Array.from(queryParts).join(''); + // Only block the unbounded variant; the bounded full-state scan is + // expected and gated by an `op_type IN (...)` filter. + return sql.includes('pg_column_size') && !sql.includes('op_type IN'); }); expect(offendingCalls).toHaveLength(0); }); @@ -702,11 +730,18 @@ describe('Storage Quota Cleanup', () => { storageQuotaBytes: BigInt(quota), }); - createRestorePoint(clientId, userId); // seq 1 - first approximate delete + // Use a larger payload so the exact-pg_column_size measurement for the + // restore point produces a freedBytes value comparable to the realistic + // 20MB-scale payloads this code path was tuned for. With tiny payloads + // (`{}`), the new exact accounting would mean each iteration barely + // dents the counter and the success-then-reconcile-disproves flow this + // test exercises never fires. + const restorePayload = { state: 'x'.repeat(2000) }; + createOp(clientId, userId, { opType: 'SYNC_IMPORT', payload: restorePayload }); // seq 1 - first approximate delete createOp(clientId, userId); // seq 2 - createRestorePoint(clientId, userId); // seq 3 - second delete needed + createOp(clientId, userId, { opType: 'SYNC_IMPORT', payload: restorePayload }); // seq 3 - second delete needed createOp(clientId, userId); // seq 4 - createRestorePoint(clientId, userId); // seq 5 - must keep + createOp(clientId, userId, { opType: 'SYNC_IMPORT', payload: restorePayload }); // seq 5 - must keep let reconcileCalls = 0; service.updateStorageUsage = async () => { diff --git a/packages/super-sync-server/tests/sync-fixes.spec.ts b/packages/super-sync-server/tests/sync-fixes.spec.ts index d7d8f4da66..d225b0c3b4 100644 --- a/packages/super-sync-server/tests/sync-fixes.spec.ts +++ b/packages/super-sync-server/tests/sync-fixes.spec.ts @@ -171,6 +171,8 @@ vi.mock('../src/db', () => { count: vi.fn().mockResolvedValue(1), }, $queryRaw: vi.fn().mockResolvedValue([]), + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }; return callback(tx); }), diff --git a/packages/super-sync-server/tests/sync-operations.spec.ts b/packages/super-sync-server/tests/sync-operations.spec.ts index 5e98b4db8a..75af766bae 100644 --- a/packages/super-sync-server/tests/sync-operations.spec.ts +++ b/packages/super-sync-server/tests/sync-operations.spec.ts @@ -306,6 +306,8 @@ vi.mock('../src/db', async () => { return state.users.get(args.where.id) || null; }), }, + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }); return { diff --git a/packages/super-sync-server/tests/sync.service.spec.ts b/packages/super-sync-server/tests/sync.service.spec.ts index 5df6e0fb7a..12c8bd5283 100644 --- a/packages/super-sync-server/tests/sync.service.spec.ts +++ b/packages/super-sync-server/tests/sync.service.spec.ts @@ -301,6 +301,11 @@ vi.mock('../src/db', async () => { }), update: vi.fn().mockResolvedValue({}), }, + // The upload transaction now writes the storage counter atomically via + // $executeRaw to keep the data write and the counter delta in a single + // commit. Mock is a no-op here — the existing spec asserts behaviour at + // the op level and does not inspect storage_used_bytes inside this file. + $executeRaw: vi.fn().mockResolvedValue(0), }); return { diff --git a/packages/super-sync-server/tests/time-tracking-operations.spec.ts b/packages/super-sync-server/tests/time-tracking-operations.spec.ts index dd7e3deeaf..24ea823b55 100644 --- a/packages/super-sync-server/tests/time-tracking-operations.spec.ts +++ b/packages/super-sync-server/tests/time-tracking-operations.spec.ts @@ -293,6 +293,8 @@ vi.mock('../src/db', async () => { return state.users.get(args.where.id) || null; }), }, + // Upload transaction writes the storage counter atomically via $executeRaw. + $executeRaw: vi.fn().mockResolvedValue(0), }); return {