fix: speed up mount bootstrap fallback#185
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces content-hash computation (SHA-256) for files and uses it to parallelize bootstrap reads. Content hashes are computed in the store layer, surfaced through tree/event APIs and WebSocket payloads, and then leveraged during bootstrap to skip remote reads when local file content already matches the remote entry hash, while parallelizing remaining reads within a configurable worker bound. ChangesContent Hash and Bootstrap Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/relayfile/store.go (1)
1075-1083:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid recomputing full-content SHA-256 during every tree read.
ListTree/listTreeFromFilesnow hash file bytes on-demand for each entry, which makes listing CPU-cost scale with total file bytes. This can become a hot path under large workspaces and frequent polls. Prefer storingcontentHashat write/upsert time and reusing it in list/event responses.Also applies to: 4237-4245, 4778-4788
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/relayfile/store.go` around lines 1075 - 1083, ListTree/listTreeFromFiles is recalculating full-file SHA-256 on every read via contentHashForFile, causing CPU work proportional to file bytes; instead compute and persist the content hash at write/upsert time and read that stored value when constructing TreeEntry. Update the upsert/write paths that create or modify files to set the file.ContentHash (or Provider/ProviderObjectID-backed hash field) once, then change ListTree/listTreeFromFiles and the code paths that build TreeEntry (where ContentHash currently calls contentHashForFile) to use the persisted file.ContentHash property; also replace other similar callsites (the other occurrences around the repository that call contentHashForFile) to read the stored hash rather than recomputing.
🧹 Nitpick comments (1)
internal/mountsync/syncer.go (1)
2444-2450: 💤 Low valueSilent error swallowing in optimization path may mask filesystem issues.
The function correctly returns
(false, nil)forErrNotExist(file doesn't exist locally, so we can't skip). However, line 2449 also returns(false, nil)for other errors (permission denied, I/O errors, etc.), which could mask unexpected filesystem problems during bootstrap.Consider logging non-
ErrNotExisterrors at debug level so operators can diagnose bootstrap issues:snapshot, err := readLocalSnapshot(localPath, false) if err != nil { if errors.Is(err, os.ErrNotExist) { return false, nil } + s.logf("trySkipBootstrapRead: local snapshot read failed for %s: %v", remotePath, err) return false, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mountsync/syncer.go` around lines 2444 - 2450, The current handling of readLocalSnapshot(localPath, false) swallows all errors by returning (false, nil) for any non-nil err; change it so only os.ErrNotExist returns (false, nil) silently, and for any other error you emit a debug-level log entry including the error and localPath (using the package's logger, e.g. logger.Debugf or s.log.Debugf) before returning (false, nil) so operators can diagnose filesystem/permission issues while preserving the existing return behavior; reference readLocalSnapshot and localPath to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/relayfile-v1.openapi.yaml`:
- Around line 201-208: The new query parameter "path" on the exportWorkspace
operation should be constrained to absolute paths like other filesystem
endpoints: update the parameter schema for name "path" (in the exportWorkspace
operation) to include a pattern '^/.*' (and optionally minLength: 1) so only
values starting with '/' are accepted; ensure the schema remains type: string
and keep the existing default and description.
---
Outside diff comments:
In `@internal/relayfile/store.go`:
- Around line 1075-1083: ListTree/listTreeFromFiles is recalculating full-file
SHA-256 on every read via contentHashForFile, causing CPU work proportional to
file bytes; instead compute and persist the content hash at write/upsert time
and read that stored value when constructing TreeEntry. Update the upsert/write
paths that create or modify files to set the file.ContentHash (or
Provider/ProviderObjectID-backed hash field) once, then change
ListTree/listTreeFromFiles and the code paths that build TreeEntry (where
ContentHash currently calls contentHashForFile) to use the persisted
file.ContentHash property; also replace other similar callsites (the other
occurrences around the repository that call contentHashForFile) to read the
stored hash rather than recomputing.
---
Nitpick comments:
In `@internal/mountsync/syncer.go`:
- Around line 2444-2450: The current handling of readLocalSnapshot(localPath,
false) swallows all errors by returning (false, nil) for any non-nil err; change
it so only os.ErrNotExist returns (false, nil) silently, and for any other error
you emit a debug-level log entry including the error and localPath (using the
package's logger, e.g. logger.Debugf or s.log.Debugf) before returning (false,
nil) so operators can diagnose filesystem/permission issues while preserving the
existing return behavior; reference readLocalSnapshot and localPath to locate
the code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fd8dc3a-8296-4f41-b6bc-bd6ce308529b
📒 Files selected for processing (7)
internal/httpapi/websocket.gointernal/mountsync/bootstrap_test.gointernal/mountsync/syncer.gointernal/mountsync/syncer_test.gointernal/relayfile/store.gointernal/relayfile/store_test.goopenapi/relayfile-v1.openapi.yaml
Contributor
|
Opened follow-up PR for the post-merge CodeRabbit OpenAPI feedback: #187 |
This was referenced May 21, 2026
Closed
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.
Summary
contentHashto tree entries and filesystem events, including websocket forwarding and OpenAPI coveragepushLocalcan echo it backReadFilecalls when local bytes already match treecontentHash, and fetch remaining tree files through bounded parallelism (RELAYFILE_BOOTSTRAP_READ_CONCURRENCY, default 16, max 64)Review Notes
main.pushLocalbefore bootstrap, defeating skip-on-hash and risking noisy writeback. Added a regression for that exact shape.origin/mainso it does not include the older issue-182/lifecycle branch work.Tests
go test ./...scripts/check-contract-surface.shFixes #183