fix(dest-handling): prevent concurrent clones#28
Merged
Conversation
commit: |
📝 WalkthroughWalkthroughAdds an inflight concurrency guard and refactors clone/update logic to prevent duplicate repository clones under concurrent requests; includes an end-to-end test for concurrent syncs and a pnpm workspace override requiring minimatch >= 10.2.1. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Wrapper as cloneOrUpdateRepo (Wrapper)
participant InFlightMap as In-Flight Map
participant Impl as cloneOrUpdateRepoImpl (Impl)
participant Git as Git Ops
Client->>Wrapper: request cloneOrUpdateRepo(params, outDir)
Wrapper->>InFlightMap: check cachePath
alt no in-flight entry
Wrapper->>Impl: start cloneOrUpdateRepoImpl(params, outDir, cachePath)
Wrapper->>InFlightMap: store promise for cachePath
Impl->>Git: perform clone or fetch+update
Git-->>Impl: CloneResult
Impl-->>Wrapper: resolve CloneResult
else existing promise
InFlightMap-->>Wrapper: return existing promise
Wrapper-->>Client: reuse promise result
end
Wrapper->>InFlightMap: remove cachePath entry
Wrapper-->>Client: return { usedCache, cleanup }
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR attempts to prevent concurrent clone operations to the same git repository cache by introducing an in-flight request deduplication mechanism. The implementation uses a Map to track ongoing operations and makes waiting requests recursively retry after the first operation completes.
Changes:
- Added
cloneOrUpdateInFlightMap to track concurrent operations by cache path - Refactored
cloneOrUpdateRepoto check for in-flight operations before proceeding - Extracted the implementation logic into
cloneOrUpdateRepoImplfunction
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pnpm-workspace.yaml`:
- Around line 1-2: Update the PR description to explain why the pnpm override
for minimatch is required: state that the override in pnpm-workspace.yaml (the
overrides entry for minimatch@>=10.2.1) is to remediate CVE-2026-26996, a ReDoS
vulnerability, and that the project requires minimatch versions >=10.2.1;
include the CVE identifier, the vulnerability type (ReDoS), and a brief note
that this is a security-driven change, not a functional change, so reviewers
understand the intent.
In `@src/git/fetch-source.ts`:
- Around line 564-585: The current cloneOrUpdateRepo uses
cloneOrUpdateInFlight.get(cachePath) and returns inflight.then(() =>
cloneOrUpdateRepo(params, outDir)) which only retries on fulfillment; when the
in-flight promise rejects all waiters immediately get that rejection and never
retry. Update cloneOrUpdateRepo so the returned promise for waiters attaches a
.catch handler to the inflight promise (e.g., inflight.catch(() =>
undefined).then(() => cloneOrUpdateRepo(params, outDir))) to swallow the initial
failure and allow each waiter to call cloneOrUpdateRepoImpl (via the normal code
path) independently; keep the existing finally cleanup that deletes
cloneOrUpdateInFlight and continue using cloneOrUpdateInFlight,
cloneOrUpdateRepo, cloneOrUpdateRepoImpl and cachePath as before.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
pnpm-workspace.yamlsrc/git/fetch-source.tstests/fetch-source-file-protocol.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/fetch-source-file-protocol.test.js (1)
453-466:GIT_TERMINAL_PROMPTis never restored in thefinallyblock.Line 466 sets
process.env.GIT_TERMINAL_PROMPT = "0"but thefinallyblock (lines 497-503) doesn't restore it. This is a pre-existing pattern across all tests in this file, but since you're already touching the cleanup block it would be low effort to fix here and prevent env pollution between tests.Proposed fix
const previousPath = process.env.PATH ?? process.env.Path; const previousPathExt = process.env.PATHEXT; const previousGitDir = process.env.DOCS_CACHE_GIT_DIR; + const previousGitTerminalPrompt = process.env.GIT_TERMINAL_PROMPT; const nextPath = process.platform === "win32" ? binDir : `${binDir}${path.delimiter}${previousPath ?? ""}`;} finally { process.env.PATH = previousPath; process.env.Path = previousPath; process.env.PATHEXT = previousPathExt; process.env.DOCS_CACHE_GIT_DIR = previousGitDir; + process.env.GIT_TERMINAL_PROMPT = previousGitTerminalPrompt; await rm(tmpRoot, { recursive: true, force: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fetch-source-file-protocol.test.js` around lines 453 - 466, The test sets process.env.GIT_TERMINAL_PROMPT = "0" but never restores it in the finally cleanup; capture the original value (e.g., const previousGitTerminalPrompt = process.env.GIT_TERMINAL_PROMPT) alongside the existing previousPath/previousPathExt/previousGitDir assignments and then restore process.env.GIT_TERMINAL_PROMPT = previousGitTerminalPrompt (or delete it if undefined) inside the finally block where previousPath/previousPathExt/previousGitDir are reverted so the environment is returned to its prior state after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fetch-source-file-protocol.test.js`:
- Around line 488-496: The test currently asserts that the repository is cloned
only once by checking entries parsed from logPath (logRaw → entries → clones),
but it needs an additional assertion that all three sources performed their
individual checkouts; after building entries (the array of JSON-parsed log
lines) filter for lines that include "checkout" and the repo identifier (similar
to how clones is computed) and assert that its length equals 3 to verify each
source completed its checkout; use the same variables (entries, repo, logPath)
and pattern as the existing clones check to add this assertion.
---
Nitpick comments:
In `@tests/fetch-source-file-protocol.test.js`:
- Around line 453-466: The test sets process.env.GIT_TERMINAL_PROMPT = "0" but
never restores it in the finally cleanup; capture the original value (e.g.,
const previousGitTerminalPrompt = process.env.GIT_TERMINAL_PROMPT) alongside the
existing previousPath/previousPathExt/previousGitDir assignments and then
restore process.env.GIT_TERMINAL_PROMPT = previousGitTerminalPrompt (or delete
it if undefined) inside the finally block where
previousPath/previousPathExt/previousGitDir are reverted so the environment is
returned to its prior state after the test.
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.
Summary
Prevent concurrent clones to same destination by deduplicating inflight requests.
Changes
Summary by CodeRabbit
Chores
Refactor
Tests