Repository navigation
perf(mountsync): linear bulk batch sizing, single read of new files, bounded concurrent uploads - #552
Conversation
…bounded concurrent uploads A 13k-file --push-local-once seed spent ~100s producing no requests and then sent its 8 MiB /fs/bulk chunks strictly one at a time. - chunkOutboxRecords, WriteFilesBulk and chunkPendingBulkWrites re-marshalled the whole pending batch for every appended file (O(n^2) bytes). Encode each file once and track the request size incrementally; boundaries are byte-identical to the old algorithm (kept as a test reference). - pushLocal's scan now reads small untracked files whole once (1 MiB per file, 64 MiB per scan, never in low-memory mode) instead of hashing and then re-reading them. Outbox durability is unchanged. - PushLocalAndFlushOnce derives the public state from pushLocal's scan rather than re-hashing the tree a third time. - Outbox flushes that drain several chunks pipeline up to 2 POSTs (RELAYFILE_MOUNT_BULK_UPLOAD_CONCURRENCY, 1 = serial). State transitions stay serial and in chunk order under s.mu; same-path chunks never overlap; acks only follow server responses; in-flight uploads are still applied after an error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; the change is mergeable after normal checks. Pre-merge checks |
|
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f31608e786
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ailure, removed paths) - Captured untracked-file content is used only if the file is provably unchanged since the scan read it (same inode, size, mtime, mode as the pre-read Lstat); otherwise it is re-read so the latest bytes upload. - flushOutboxChunks inspects every completed upload, not only the head, before refilling: a transport error or a failed receipt stops dispatch for good, and a completed chunk with per-path errors pauses dispatch until it is applied. No chunk is sent once a failure is observable. - PushLocalAndFlushOnce drops paths whose files the flush removed (e.g. a schema-rejected new draft) from the reused scan before publishing public state; only untracked/DeletePending entries are Lstat'ed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
In production,
relayfile-mount --push-local-onceseeds a 13,068-file (41 MB) tree in 206–218s. The server sees about 100s with no requests, then roughly 6/fs/bulkPOSTs, one at a time.The local backlog benchmark below shows the same pattern: 6 POSTs, the first one at 61–66s, and 119 GB allocated. Almost all of that is
chunkOutboxRecordsre-marshalling the whole pending chunk each time it adds one record.WriteFilesBulkdoes the same thing again for each request. On an Apple M3 the baseline drain overruns the default 60s outbox flush budget (context deadline exceeded).Changes
chunkOutboxRecords,WriteFilesBulk(viabulkWriteChunkBounds) andchunkPendingBulkWritesnow encode each file once. They track the size as{"files":[+ items + commas +]}, andbulkWriteRequestSizercopies the old "0 on encode error" result. The old functions are kept in the tests as reference implementations. Over a mixed corpus (empty, HTML-escaped<&>, U+2028, multibyte, base64, symlinks, draft content identities, a near-cap file and an over-cap file), with caps from 0 to 8 MiB, the tests assert that files per request and bytes per request are identical. A mutation check (an off-by-one in the comma count) fails these tests.pushLocalscans, it reads small untracked regular files whole and reuses that content instead of hashing first and reading again. Limits: 1 MiB per file, 64 MiB per scan, never in low-memory mode, never for lazy-skipped files. The scan map keeps its hash-only shape, built from the same read, so the hash and the content always match. Tracked files still go through hash first, then read. Outbox writes and durability are unchanged.flushOutboxChunkskeeps up toRELAYFILE_MOUNT_BULK_UPLOAD_CONCURRENCYPOSTs in flight (default 2,1= serial, clamped to 8). The default is 2 because Cloud's per-workspace in-flight body budget is 16 MiB, so a third 8 MiB request would only buyworkspace_busyretries.flushOutboxRecordChunkis split into three steps: prepare, upload, apply.s.muthe whole time, in chunk order.unlockDuringUpload).doJSONretries with Retry-After, and once retries run out,incrementOutboxAttemptapplies backoff to that chunk only.PushLocalAndFlushOncebuilds the public state frompushLocal's scan instead of callingsaveState(), which re-hashes every file.Not changed
pushLocalflushes every 256 files, and each of those flushes is a single chunk of about 768 KB. Concurrency only helps when one flush drains several chunks: the outbox backlog drain used by--push-local-onceand--flush-outbox-once, and the drain during a--notify-flushreconcile. OverlappingpushLocal's own batches would mean applying batch results later whilepushLocalkeeps mutating state. That is a bigger semantic change, so I left it as a possible follow-up.ensureOutboxDirsalso re-readscapabilities.jsonon every ack. Both are now the largest local costs, and I didn't touch them because they are part of how durability works.Benchmarks (Apple M3, real
httpapihandler +relayfile.Store, 13,000 files × 3 KiB,-benchtime 1x)FlushOutboxOnceLargeBacklogwall/fs/bulkafterPushLocalAndFlushOnceLargeTreewallThe backlog benchmark sets
outboxFlushTimeout = 1hso that main can finish; with the default 60s budget main fails.Commands
New tests:
TestChunkOutboxRecordsMatchesLegacyBoundaries,TestBulkWriteChunkBoundsMatchesLegacyBoundaries,TestChunkPendingBulkWritesMatchesLegacyBoundaries,TestBulkWriteRequestSizerMatchesMarshalTestFlushOutboxChunksBoundsInFlightAndAcksEachRecordOnce: exactly 3 in flight, every path uploaded and acked once.TestFlushOutboxChunksWorkspaceBusyKeepsAccountingExact: a 429workspace_busychunk stays pending with backoff, the in-flight chunk next to it is acked, later chunks are not dispatched, and recovery uploads each path exactly once.TestFlushOutboxChunksSerializesSamePathAcrossChunksTestFlushOutboxChunksSerialWhenConcurrencyIsOneTestPushLocalAndFlushOnceReadsNewFileOnce: before, a new file was read 3 times; now once. An oversized file is still reported aswriteback-skipped.TestPushLocalCapturesOnlyUntrackedFiles,TestPushLocalLowMemoryDoesNotCaptureContentMutation checks: removing the in-flight drain after an error fails the 429 test, and removing the same-path gate fails the same-path test.
Reviewer notes / risks
opIdand dispatches it asynchronously. With 2 POSTs in flight, the receipt GET sometimes runs before the store has marked a few ops as done (14–25 of 13k in some runs).FlushOutboxOncethen returnsoutbox pending remains. Those records keep their opId and have zero failed attempts, and polling again 2–5s later settles all of them with no re-upload. The same race was possible with serial uploads, just less likely. Cloud'sbulkWriteresults carry noopId(they are acked the legacy way, with no GETs), so production should not see this. Self-hosted Go servers may hit it more often; settingRELAYFILE_MOUNT_BULK_UPLOAD_CONCURRENCY=1restores the old timing. The benchmark tolerates only this one outcome (settleLaggingReceipts).PushLocalAndFlushOncenow reflects thepushLocalscan, not a scan taken after the flush. This only affects observability.newMountsyncAPIHandler,mustMountsyncTestJWTandensureMountsyncJWTVerifiernow taketesting.TBso the benchmarks can use them.No version bump or release.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Medium Risk
Concurrent outbox flushing and reused push scans change timing and observability while preserving ack ordering in tests; mis-handling could duplicate uploads or miss errors on large drains.
Overview
Speeds up mount writeback for large
--push-local-once/ outbox drains by cutting redundant JSON sizing work, overlapping/fs/bulkuploads, and avoiding extra local file reads.Bulk batch sizing encodes each file once and tracks request size incrementally (
bulkWriteRequestSizer,bulkWriteChunkBounds) inchunkOutboxRecords,chunkPendingBulkWrites, andHTTPClient.WriteFilesBulk, replacing repeated full-chunk marshaling that was O(n²) on big backlogs.Concurrent outbox flush adds
flushOutboxChunkswith a configurable cap viaRELAYFILE_MOUNT_BULK_UPLOAD_CONCURRENCY(default 2,1= serial, max 8). Prepare/apply stay ordered unders.mu; only the POST runs on workers, with same-path serialization and stop-on-first-error semantics matching the old serial loop. Chunk flush is split intoprepareOutboxChunkUpload/applyOutboxChunkResponse.Local push captures small untracked files during
scanLocalFilesCapturing(withfresh()revalidation) so new files are read once;PushLocalAndFlushOncereuses that scan for public state instead of a third tree hash, dropping entries removed during flush.Docs add the new env var; tests/benchmarks assert legacy-identical chunk boundaries and concurrency/error behavior.
Reviewed by Cursor Bugbot for commit 8d12343. Bugbot is set up for automated code reviews on this repo. Configure here.
Agent Relay sessions
claudesessiona26b88d9· opened viagh pr create· last active 2026-10-10claudesession5d87b716· contributor · rangh prcommands · last active 2026-10-10