dofs: link staged chunks during sync apply - #87
Merged
Conversation
Sync receivers already stage chunk payloads in content-addressed storage before applying file entries. Reading them back and concatenating a whole-file buffer makes peak isolate memory roughly twice the file size. Link each file's existing chunk references directly while preserving metadata and validating declared sizes. This keeps payload bytes out of the apply path and relies on vfs_chunks reachability to protect linked blobs from collection.
linkStagedChunksSync writes chunk rows from a list it did not produce, so the checks writeFile used to run on the way into the sync apply path have to live in the helper itself. Positional reads find the chunk covering an offset by dividing that offset by CHUNK_SIZE, and take a chunk's start offset to be its index times CHUNK_SIZE. A local writer chunks with chunksOf and satisfies that by construction, but a chunk list that arrived over the wire only satisfies it while both sides window at the same size. Reject a list whose interior chunks are short or whose chunks overflow a window, so a sender that windows differently fails loudly instead of producing a file whose bytes read back from the wrong offsets. Also restore the read-only mount check. applyChanges gates read-only mount roots itself and reports the skipped entries, so behavior over sync is unchanged, but the exported helper is a write primitive and the guard belongs at the data layer. Run both checks in applyFileEntry before the existing entry at the path is removed. A batch is a sequence of independent transactions, so a throw after the removal would drop a file and put nothing in its place.
馃 Changeset detectedLatest commit: 91139bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your interest in Cloudflare Computer. This repository does not accept unsolicited pull requests. Please use one of the accepted contribution paths instead:
If a maintainer asked you to open this pull request, they can add the |
Open
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
2 participants
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.
dofs: link staged chunks during sync apply
Pulling a file into the durable object used to hold that file in memory twice.
applyChangesread every chunk back out of storage, joined them into one whole-file buffer, and handed the buffer towriteFile, which then split it into chunks again. Both copies are live at the same time, so a 64 MiB file costs about 128 MiB inside an isolate whose budget is 128 MiB. One large file in a sync was enough to run the isolate out of memory.By the time apply runs, the chunks are already in content-addressed storage: the sender staged them through
pushObjects, or the receiver staged them fromfetchObjects. So apply now links the file's inode directly to those staged chunks, keeping the mode, modification time, and chunk list the change entry declares. Payload bytes never enter the isolate at all. Declared sizes are still checked against what is really staged, so an interrupted or truncated write is caught before anything gets linked, and a linked chunk is reachable fromvfs_chunks, which is what keeps garbage collection off it.Peak
ArrayBufferbytes during a 64 MiB apply, sampled on every database call:Linking a chunk list the receiver did not produce needs two guards that came free when the bytes went through
writeFile. The first is layout: positional reads find the chunk covering an offset by dividing that offset by the chunk size, and treat a chunk's start as its index times the chunk size. A local writer satisfies that by construction, but a chunk list off the wire only satisfies it while both sides use the same window size, solinkStagedChunksSyncnow rejects a list whose interior chunks are short or whose chunks overflow a window. The second is the read-only mount check. Sync already reports entries under a read-only mount as skipped, so behavior over the wire is unchanged, but the guard belongs in the write primitive rather than in one of its callers. Both checks run before the existing entry at the path is removed, because a batch is a sequence of independent transactions and a late failure would otherwise delete a file and put nothing in its place.To see the old behavior and the new one for yourself, run the package tests under both backends:
npm run build --workspace @cloudflare/dofs npm test --workspace @cloudflare/dofs npm run test:workers --workspace @cloudflare/dofsTwo of the new tests spy on the database handle and assert that no query reads chunk payloads, which fails against the old code. Three cover the layout guard: a ragged chunk list is rejected, an oversized chunk is rejected, and a file already at the path survives a rejected entry. One covers the read-only mount guard, and one proves garbage collection leaves a linked blob alone even when its staging timestamp is old enough to sweep. The memory numbers came from a throwaway probe; asserting on peak isolate memory is too brittle to keep in the suite.
One thing this change does not fix: nothing verifies that a staged blob's bytes really hash to the hash they arrived under. The receive loops trust the sender, as
stageBlobdocuments. Apply used to launder that trust by re-hashing the joined buffer, which turned a mislabeled blob into a manifest mismatch and an endless resync; now the declared hash is taken at face value, and a mislabeled blob would leave both sides agreeing on manifests while holding different bytes. The sender is the durable object's own container over a trusted channel, so this is not urgent. If we want the check back it belongs inpushObjectsand thefetchObjectsreceive loop, and it costs one hash pass over every synced byte.The fix in the first commit comes from a patch by Caio Nogueira; the second commit adds the two guards and the ordering fix.