fix: Underlay Push scale updates - #3677
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the scalability and reliability of “push to Underlay” for large communities by reducing memory blowups during hydration/mapping, streaming file uploads with bounded concurrency, adding checkpointed resumability, and ensuring push logs are finalized even when worker processes die.
Changes:
- Chunked pub hydration +
separate: truehasMany includes to avoid cartesian explosion and reduce heap usage. - Stream files during mapping with bounded upload concurrency, checkpoint cache entries during the push, and negotiate/commit with async commit polling.
- Improve push-log correctness/observability: adoption of stale
runninglogs, crash-finalization, and warning truncation caps.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| workers/tasks/pushToUnderlay.ts | Chunked pub hydration, early ensureCollection, streaming uploads + periodic cache checkpoints, capped warning logging |
| workers/queue.ts | Adds worker-thread heap limit support and crash-finalization for underlay push logs |
| server/underlayPushLog/queries.ts | Caps stored warnings, fixes running-log adoption, adds “fail log by workerTaskId” finalizer |
| server/underlayPushEntry/queries.ts | Extracts cache-entry upsert so it can be called mid-push (checkpointing) |
| server/underlay/mapping.ts | Adds fileHashes to support streaming pushes without retaining bytes |
| server/underlay/incremental.ts | Streaming upload pipeline, upload pool concurrency, checkpoint hooks, avoids memoizing file bytes |
| server/underlay/client.ts | Authenticated reads for private collections; async commit + polling; missing-file recovery helpers |
| server/underlay/tests/pushLogWarnings.test.ts | Tests warning truncation/message behavior |
| server/underlay/tests/pushLogAdoption.test.ts | Tests adoption behavior repointing workerTaskId |
| server/underlay/tests/incremental.test.ts | Adds streaming/concurrency/checkpointing tests; updates memoization expectations |
| server/underlay/tests/client.test.ts | Tests async commit handshake + authenticated reads |
| server/envSchema.ts | Adds WORKER_MAX_OLD_SPACE_MB env var schema |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+700
to
+706
| const response = await this.request( | ||
| `${this.collectionPath()}/versions/negotiate/${sessionId}`, | ||
| { method: 'GET' }, | ||
| ); | ||
| if (!response.ok) { | ||
| continue; | ||
| } |
Comment on lines
809
to
812
| if (body.filesNeeded && body.filesNeeded.length > 0) { | ||
| for (const ref of body.filesNeeded) { | ||
| const hash = ref.replace(/^sha256:/, ''); | ||
| let file = filesByHash.get(hash); | ||
| if (!file && resolveFileByHash) { | ||
| // biome-ignore lint/performance/noAwaitInLoops: bounded retry | ||
| file = (await resolveFileByHash(hash)) ?? undefined; | ||
| } | ||
| if (file) { | ||
| // biome-ignore lint/performance/noAwaitInLoops: bounded retry | ||
| await this.uploadFile(file); | ||
| } | ||
| } | ||
| await this.uploadMissingFiles(body.filesNeeded, filesByHash, resolveFileByHash); | ||
| response = await doCommit(); | ||
| } else { |
| // the same task OOMs at different points on different machines and the failure is not | ||
| // reproducible locally. Setting WORKER_MAX_OLD_SPACE_MB pins it. Left unset by default so this | ||
| // change alters nothing until someone chooses a value. | ||
| const maxOldSpaceMb = Number(env.WORKER_MAX_OLD_SPACE_MB) || undefined; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Large communities could not push to Underlay. The push died after 33 seconds with
JS heap out of memory, and showed as "Running" in the UI indefinitely — the worker recorded the error on theWorkerTask, but nothing ever finalized theUnderlayPushLog, so the failure was invisible.Root cause:
pushToUnderlayloaded every pub with fourhasManyassociations eagerly joined in one query. SiblinghasManyincludes produce a cartesian product, each repeating the pub's full payload (description,htmlDescription,metadata). One pub alone expands to 4,368 rows.Changes
Query / memory
separate: trueon all fourhasManyincludes — makes association cost additive, notmultiplicative.
mapped, then drop. Peak file memory is one pub's worth, not the collection's.
Throughput
latency.
?async=true) and polls the session. Previously amulti-minute commit blew the 60s client timeout and was retried four times, re-running
the work server-side each attempt. Falls back cleanly to a synchronous
201, so this doesnot require Underlay and PubPub to deploy in a particular order.
Resumability / observability
times out or crashes resumes instead of restarting from zero.
UnderlayPushLogis finalized when the worker process dies, not just when the task throws.runningrow is re-pointed at the task actually running it — otherwise it wasinvisible to the crash-finalizer and stuck at
runningforever.Checkpointed N pub(s); M file(s) uploaded).getBaseVersion/ensureCollectionnow send credentials. Unauthenticated, they 404 on aprivate collection, which was read as "no versions yet" and caused every push after the
first to fail with a misleading 409 conflict.
Verification
Run against a local copy of the real database:
fingerprinting every association incl. nested
user/externalPublication:identical — 4,043 attributions, 1,324 exports, 161 edges, 224 releases.
distinct, so the cascade genuinely varies).
autogeddon, 18 pubs): committedv1.0.0, 218 records / 419 files, integrity confirmed on the Underlay side.Deploy notes
infra/stack.yml'sworkerservice has no memory limit. Consider settingWORKER_MAX_OLD_SPACE_MB(added here) and/or a container limit, so a runaway push fails one task rather than the host.Follow-ups (pre-existing, not addressed)
.epubexports 403 even after the presigned-URL retry; every one was skippedlocally. Worth confirming whether the fallback works against prod S3
cleanup is ever added, exclude recently-uploaded blobs.