A reconnecting device kept appending alongside its stale entry instead of
replacing it, so userSet would fill to MAX_CONNECTIONS_PER_USER and reject
legitimate reconnects with 4008 until the 30s+10s heartbeat cycle caught up.
addConnection now evicts any existing entry with the same clientId before
adding the new socket; the cap still applies to genuinely distinct clientIds.
The DUPLICATE_OPERATION->success conversion in the snapshot route looked
up the existing op by primary key alone. `isSameDuplicateOperation`
already enforces ownership upstream, so this can't be tripped today, but
keeping the userId filter on the route's own lookup keeps the
idempotency conversion correct even if that upstream invariant ever
changes. Switches `findUnique({id})` to `findFirst({id, userId})`;
`id` is still the primary key so the plan is unchanged.
Two correctness fixes on top of the persisted-full-state-clock optimization.
1. Persist the aggregate of prior history (merged with the snapshot op's own
clock) instead of just the snapshot op's clock. BACKUP_IMPORT uses a fresh
`{ newClient: 1 }` clock by design, so storing it raw left downloading
clients with a baseline that did not cover pre-import ops still alive in
the server-side conflict-detection set; their first post-restore edit could
be rejected as CONCURRENT. The aggregate scan moves from per-download to
per-snapshot, which is a net win — full-state ops are rare.
2. Look up the SYNC_IMPORT idempotency target by exact opId first, then fall
back to an ordered (`serverSeq DESC`) findFirst. Postgres ordering is
undefined without ORDER BY, so the previous `findFirst` could non-
deterministically return a different full-state op (e.g. a later
BACKUP_IMPORT) and flip the idempotency check incorrectly.
Two related improvements to snapshot/full-state handling:
- Persist the incoming op's vector clock as `user_sync_state.latest_full_state_vector_clock`
so downloads can skip the expensive aggregate scan when the snapshot clock is
still valid; falls back to the legacy aggregate when missing or invalid.
- Treat retried snapshot uploads (same opId) as idempotent successes instead of
409/DUPLICATE_OPERATION so a dropped response doesn't push clients into the
download-and-merge path. Per-namespace request dedup keeps ops and snapshot
caches isolated.
Previously, the storage-counter reconcile marker was set only after the
per-user drain loop finished. With the new multi-batch flow, a transient
DB failure between batch 1 and batch 2 commits batch 1's deletes but
exits before the marker call — leaving the counter stale-high until the
next daily pass or process restart, instead of self-healing on the next
sync request.
Move the marker (and affectedUserIds push) to fire after the first
successful batch so any survivor deletes are reconciled. Cover with a
test that spies the private batch helper to throw on the second call
and asserts the user is still marked.
Daily retention cleanup previously issued one unbounded `deleteMany` per user
that covered every operation older than the retention cutoff. On large
backlogs this monopolized Postgres and made user-facing sync slow during the
window, which matched the production "sync is still slow" reports.
- Cap per-batch deletes (default 5k) and per-run total deletes (default 25k),
with a select-then-delete-by-id pattern that keeps each statement short.
- Drain each user across batches before moving on so a fresh user with a tiny
backlog isn't blocked behind a large one until next pass.
- Order candidates by `snapshotAt asc` so the stalest users win the budget
when it's tight.
- Defer the initial cleanup pass after startup to 30 min in production (was
10s) so retention work doesn't compete with deploy/restart warmup; call
`unref()` on cleanup timers so they never hold the process open.
- Expose all knobs via env vars wired through docker-compose, env examples,
and the Helm chart, with a strict positive-integer parser and clamped
maxima so a misconfiguration cannot unwind the bound.
S1: replace the Fisher-Yates random shuffle in cleanup.ts with a
deterministic orderBy snapshotAt asc at the source. When the affected
user list exceeds the reconcile budget, the most-drifted users are
reconciled first; the freshest tail rolls over to the next pass. No
more Math.random, no probabilistic starvation, and the test stub for
Math.random goes away.
C1 regression tests: add coverage that proves the prototype-pollution
guard works for CRT, UPD, MOV, BATCH single-entity, and BATCH entities
map. Each test asserts state.TASK's prototype is still Object.prototype
and the malicious payload doesn't leak via the prototype chain.
C1: guard CRT/UPD/MOV/BATCH (and BATCH-entities) replay against
__proto__/constructor/prototype entityIds — bracket-assignment to those
keys invokes the __proto__ setter and swaps the entity map's prototype.
Shares the new isUnsafeEntityKey helper with the full-state branch.
W2: stop deleting storageUsageLocks[userId] in clearForUser. The
identity-guarded finally in runWithStorageUsageLock self-deletes when
the chain drains, and removing the head while a follower is queued lets
a fresh caller see no previous and race the in-flight chain. Replaces
the deprecated assertion with a race regression test.
W4: when _generateSnapshotImpl's regenerated blob exceeds the hard
MAX_SNAPSHOT_SIZE_BYTES ceiling, mirror cacheSnapshot's clear-stale
logic via a race-safe updateMany. Without this an oversize regen left
a poisoned older blob serving GET /snapshot.
W9: reject layered or non-gzip Content-Encoding values with 415 in the
content-type parser instead of falling through to JSON.parse on raw
gzip bytes (which produced a misleading 400 invalid-json).
W11: computeOpStorageBytes now returns { bytes, fallback }; route gate
and uploadOps both Logger.warn the count of unserializable ops (never
the content) so the previously-lost observability is back.
S6: drop the no-op Math.max around estimatedOpStorageBytes for
uncacheable snapshots — stateBytes is always > MAX_SNAPSHOT_SIZE_BYTES
when \!cacheable, so the floor never engaged.
The previous SYNC_IMPORT prototype-pollution regression test asserted
`Object.prototype.polluted === undefined` and `({}).polluted === undefined`.
Both assertions are tautological: `Object.assign({}, {__proto__: x})` uses
[[Set]] on the target, which triggers the `__proto__` setter only on the
target itself — it swaps `state`'s prototype reference, but does NOT
pollute the global `Object.prototype`. The old test therefore passed even
against the buggy implementation it was meant to catch.
Assert the real invariant instead:
- `Object.getPrototypeOf(result) === Object.prototype` (state's prototype
was not swapped — this is what flips under the buggy code).
- `result.polluted` is undefined via the prototype chain.
- `__proto__` is not an own property on the result.
Verified the new assertions fail against `Object.assign(state, fullState)`
and pass against the current key-by-key copy with a __proto__/constructor/
prototype skip list. Kept the global-pollution checks as defense in depth
and the `finally` cleanup in case of future regressions.
B3: remove now-redundant post-commit `applyStorageUsageDelta` writes in
the routes. Wave-1 commit 9af17e460e moved the storage-counter write
INTO `uploadOps`'s `$transaction`, so the route adding a second delta
would double-count every accepted op. /ops drops the post-commit
increment entirely; POST /snapshot now only writes `cacheResult.deltaBytes`
(the snapshot-cache portion). The route's `computeOpsStorageBytes` now
delegates to the shared `computeOpStorageBytes` helper in `sync.const.ts`
to guarantee gate-vs-increment parity.
B4: when `preparedSnapshot.cacheable === false`, do not subtract
`previousBytes` from the gate, and use `Math.max(stateBytes,
MAX_SNAPSHOT_SIZE_BYTES)` as the gate value so the worst-case op-row
write cannot slip past on a raw-JSON estimate that doesn't capture
JSONB/TOAST overhead.
B5: GET /snapshot now passes a `maxCacheBytes` cap (the user's remaining
quota) down through `generateSnapshot`. When the freshly compressed
snapshot would exceed the cap, the cache is NOT written but the
in-memory snapshot is still returned to the caller. A user already at
quota can still catch up reads without growing on-disk storage further.
Adds a regression test in `snapshot.service.spec.ts`.
B8: add `rateLimit: { max: 10, timeWindow: '15 minutes' }` to POST
/snapshot to match the other write-heavy operations (DELETE /data,
/restore/:seq). Snapshot uploads are expensive (30MB body,
`prepareSnapshotCache` zlib + JSON.stringify, full-state op replay)
so burst-uploads from one user can pin a worker.
B9: split `MAX_DECOMPRESSED_SIZE` into per-endpoint caps
(30 MB for /ops, 60 MB for /snapshot) so a 10 MB compressed body cannot
expand to 100 MB of JSON.parse work and stall the event loop.
B10: normalize `Content-Encoding` via a shared `isSingleTokenGzipEncoding`
helper so RFC-valid values like `Gzip`, ` gzip `, or `['gzip']` match.
Layered encodings (`gzip, deflate`) are rejected. Replaces all three
strict `=== 'gzip'` comparisons in `sync.routes.ts`.
Updates `sync-compressed-body.routes.spec.ts` for the new post-commit
delta contract: the snapshot route only writes the cache portion now;
the op-row portion is owned by `uploadOps`'s transaction.
Two negative-path generateSnapshot tests declared `update` on the mock tx
and asserted `expect(update).not.toHaveBeenCalled()`. Production calls
`tx.userSyncState.upsert` (not update) inside `_generateSnapshotImpl`, so
the assertion was tautological — `update` is never invoked regardless of
whether the rejection guard ran. Renaming to `upsert` makes the
"no cache write happened" assertion meaningful again.
The module-level `prisma.userSyncState.update` mock stays — production
still calls `update` from `_invalidateCachedSnapshot`, exercised by the
decompression-error test.
- B7: replace leaky `Transaction rolled back: <raw Prisma msg>` per-op
error with a generic 'Transaction failed - please retry'. The raw
exception text is still logged via `Logger.error`, but never returned
to the client (could leak SQL fragments, FK / column names).
- B6: in `deleteOldestRestorePointAndOps`, measure the exact bytes for
the deleted full-state op (SYNC_IMPORT / BACKUP_IMPORT / REPAIR) row
via `pg_column_size` BEFORE deletion. Bounded to 0-1 rows per call,
so it doesn't reintroduce the DoS from scanning every delta op. Delta
ops keep the cheap `count * APPROX_BYTES_PER_OP` approximation;
full-state payloads (up to 20MB) no longer undercount by ~20000x and
leave the counter permanently low if reconcile fails. Docstring on
APPROX_BYTES_PER_OP clarifies it is ONLY valid for delta cleanup.
- I6: inside `deleteOldSyncedOpsForAllUsers`, call
`storageQuotaService.markNeedsReconcile(userId)` for each user whose
cleanup deleted rows. A crash mid-loop now leaves surviving users
with a forced-reconcile marker so their next request self-heals,
instead of waiting for the daily pass. TODO left for persisting the
marker in a DB column for cross-restart / multi-instance safety.
- B3 (service side): write `users.storage_used_bytes` atomically
inside the upload `$transaction` using `$executeRaw` with the same
GREATEST(...) clamp pattern as `storage-quota.service.ts`. Shared
`computeOpStorageBytes` helper keeps the gate and the increment in
agreement about per-op size. Clean-slate path SETs the counter to
the new total rather than incrementing onto the just-reset row.
- B2 companion: add `RequestDeduplicationService.clearForUser(userId)`
and call it from both `deleteAllUserData` and the uploadOps
clean-slate path so a retry from before the wipe cannot return
cached results that reference now-deleted state.
The minSeq-based gap check compared the raw sinceSeq, so a client
requesting from before retention purge would get gapDetected=true
even when the snapshot already supersedes that purged history. Case 3
already used effectiveSinceSeq; this aligns Case 2 so the snapshot
fast-forward suppresses the false positive and avoids triggering an
unnecessary full restore on the client.
The deferred-reconcile budget caps at 720 users per pass
(RECONCILE_BUDGET_MS / RECONCILE_INTERVAL_MS = 3.6M / 5K). With a stable
DB iteration order, the first 720 affectedUserIds got reconciled every
daily cleanup and any tail beyond that drifted indefinitely.
Shuffle the affected-user list with Fisher-Yates before slicing the
budget window so each pass picks a different random subset, and include
both numbers in the warn log so operators can spot servers that have
outgrown the per-pass budget.
clearForUser now also clears forcedReconciles (would force a spurious
extra scan on the next quota check) and storageUsageLocks (orphaned
queued lock chain) alongside the existing inflightReconciles cleanup.
Also document the known byte-counting mismatch between pg_column_size
(TOAST-compressed) and the hot-path Buffer.byteLength(JSON.stringify)
in calculateStorageUsage. The proper fix needs a payload_bytes column
populated at insert time; in-place switching to octet_length(text)
would re-detoast every row and resurrect the disk-I/O DoS.
Address three review findings on snapshot.service.ts:
- B1: _generateSnapshotImpl no longer unconditionally upserts the cache.
A concurrent upload that bumped lastSnapshotSeq beyond our latestSeq
could be clobbered. Switched to the same race-safe updateMany+create
pattern as cacheSnapshot, and pin cacheDelta to 0 when the race is
lost so the onCacheDelta hook does not over-credit storage.
- I2: Always run _assertCachedSnapshotBaseReplayable before serving a
cached snapshot, including the startSeq>=latestSeq fast path.
A legacy build that failed to reject encrypted ops could have
produced a poisoned cache that the fast path would serve forever.
- I4: replayOpsToState no longer uses Object.assign for full-state op
payloads. A malicious SYNC_IMPORT/BACKUP_IMPORT/REPAIR payload with
a __proto__ key would pollute Object.prototype via the [[Set]]
semantics of Object.assign. Copy keys explicitly and skip
__proto__/constructor/prototype.
I3 (RepeatableRead transaction spans gzip/replay/gzip and can starve
the DB pool under load) is acknowledged but left for a focused PR —
restructuring requires moving the cache write outside the transaction
and updating multiple integration tests.
Updated unit-test mocks to provide updateMany/create on the
transaction client, and added regression coverage for the prototype-
pollution and fast-path-validation cases.
- B11: Remove orphan deploy-db-scalar.mjs and dead RUN_POST_MIGRATION_INDEXES
references in env.example and README. Rewrite README deploy section to
match what scripts/deploy.sh actually does (drop false claims about
empty migration SQL, failed-migration recovery, and optional index builds).
- B12: Default RUN_MIGRATIONS_ON_STARTUP to false in docker-compose.yml so
the compose, Dockerfile ENV, and env.example agree on the safer default
(manual deploy.sh runs migrations once before app restart). Update the
migration-sql.spec assertion to match.
- I9: Wrap prisma migrate deploy in deploy.sh with a MIGRATION_TIMEOUT
(default 900s). CREATE INDEX CONCURRENTLY can block on long-running
transactions; on timeout (exit 124) the script now fails loudly with a
clear error rather than hanging.
Deadlock: updateStorageUsage now bypasses the inflightReconciles dedupe
when already inside the per-user lock, so a reentrant caller cannot
await a non-reentrant promise queued behind the lock it holds.
Counter drift: GET /snapshot's cache rewrite feeds its byte delta into
the storage-usage counter via an onCacheDelta hook; failed counter
writes mark the user for forced reconcile on next quota check;
oversized snapshots clear the stale row and credit the freed bytes.
Event loop: zlib gzip/gunzip moved off the synchronous fast path so
large snapshots stop blocking other tenants.
Hardening: /ops bodyLimit set to MAX_COMPRESSED_SIZE_OPS; snapshot
pre-gate reconciles before rejecting a stale-high counter; all 413
responses funnel through one helper using errorCode consistently;
reconcileTimers leak fixed by self-removing fired timers.
Includes regression tests for each new behavior.
- Use upsert for userSyncState to avoid RecordNotFound on first-time users
- Reset (not merge) state on full-state ops; throw on malformed payloads
- Add typed EncryptedOpsNotSupportedError; stop leaking op count to clients
- Split JSON.parse vs decompress error paths (new invalid-json reason)
- Measure base64 transport size against decoded binary; decode once
- Guard concurrent cacheSnapshot writes against stale overwrite
- Move encryption-skip policy from route into cacheSnapshotIfReplayable
- Invalidate corrupt cached snapshot blobs instead of re-logging forever
- Use Buffer.byteLength for replay state size (was UTF-16 vs byte limit)
- Skip cache-replayability assert on the cache-hit fast-path
- Extract async gunzip/gzip into gzip.ts; stop blocking the event loop
- Replace inline tx structural types with Prisma.TransactionClient
- deploy.sh: set -euo pipefail with guarded vars and pipeline-safe diagnostics
- Accept leading-gap replays only when first surviving op is a full-state op
(SYNC_IMPORT / BACKUP_IMPORT / REPAIR); fixes SNAPSHOT_REPLAY_INCOMPLETE after
clean-slate uploads and retention pruning. Mid-stream gaps still throw.
- Snapshot transactions now run at RepeatableRead so encrypted-op guards
cannot be raced by a concurrent writer.
- Restore getParsedBodySizeBytes for storage-quota accounting: JSON.stringify
.length counts UTF-16 code units and re-serializes the payload; prefer the
parser-reported decompressed byte count, then Content-Length, then a
Buffer.byteLength fallback.
- Extend in-memory tx mocks with count + opType-aware findFirst so the new
encrypted-op guards exercise cleanly under vitest.
- Snapshot replay throws on gaps, max-ops overflow, and encrypted ops
instead of silently truncating state.
- Operation download bounds all queries by userSyncState.lastSeq.
- Compressed body parser enforces maxOutputLength to reject zip bombs.
`updateStorageUsage` runs a SUM(pg_column_size) scan that TOAST-detoasts
every operation row for a user — slow under load. When several concurrent
requests for the same user near quota hit the cache-miss path in
`enforceStorageQuota`, each independently triggered its own full scan,
multiplying disk I/O.
Add an in-flight promise map keyed by userId so concurrent callers share
a single scan; the lock is released in `finally` so sequential callers
(notably the iterative reconcile inside `freeStorageForUpload`) still
see fresh results. Wire the new `clearForUser` into the clean-slate
cleanup path next to the existing rate-limit and snapshot lock clears.
Adds tests covering: concurrent dedupe, sequential refresh, lock release
on scan failure, and `clearForUser` no-op.
decrementStorageUsage now runs $executeRaw with a clamped UPDATE
(storage-quota.service.ts), so cleanup tests that exercise
deleteOldSyncedOpsForAllUsers crashed with "prisma.$executeRaw is not a
function". Add the missing prisma mock entries to keep the cleanup tests
green.
Picked up the remaining items from the multi-agent review:
- Hoist Operation / ServerOperation imports in sync.routes.ts so the
upload path no longer needs `as unknown as import('./sync.types').*`
double casts. The Set + filter pattern for accepted ops becomes an
index-zip loop, dropping two iterations + the Set allocation.
- Pull APPROX_BYTES_PER_OP out of two inline locals in sync.service.ts
into sync.const.ts so the constant is a single source of truth and
the trade-off (over-estimate vs observed median op size) is documented
in one place.
- Log when computeOpsStorageBytes silently skips an op whose payload
fails JSON.stringify (BigInt, circular refs). Counter still
under-counts but the cause is visible in logs.
- Update storage-quota docstrings to reflect that calculateStorageUsage /
updateStorageUsage are now legitimately called from the
freeStorageForUpload reconcile path (rare cleanup events), not only
offline.
Tests:
- cleanup.spec.ts: assert updateStorageUsage is NOT called after
deleteOldSyncedOpsForAllUsers (the per-user SUM loop was the DoS).
- storage-quota-cleanup.spec.ts: drop the assertion that
deleteOldestRestorePointAndOps issues a pg_column_size query (it does
not anymore); add a $executeRaw mock that mutates testUsers so the
iterative-cleanup test exercises the real decrement path; tune
starting storage to within a few KB of quota so cleanup's
approximate decrement can detect progress within a few iterations.
- storage-quota.service.spec.ts: add the $executeRaw prisma mock and
unit tests for incrementStorageUsage / decrementStorageUsage
(positive path, Math.floor of non-integer deltas, no-op on
NaN/Infinity/zero/negative, clamped UPDATE for decrement).
The "just 1ms over max clock drift boundary" test captured Date.now()
once in the test and again inside uploadOps. If the second sample
advanced by >=1ms, the op was within the drift window from the
service's perspective, clamping never fired, and toBeLessThan failed
with equal values.
Freeze time with vi.useFakeTimers/setSystemTime so both samples
match, and tighten the assertion to the exact clamped value.
