fix(server): clean status fetch temporary packs #4338
+302
−16
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,11 @@ const STATUS_UPSTREAM_REFRESH_TIMEOUT = Duration.seconds(5); | |
|
|
||
| const STATUS_UPSTREAM_REFRESH_FAILURE_COOLDOWN = Duration.seconds(5); | ||
| const STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY = 2_048; | ||
| const GIT_TEMPORARY_PACK_PREFIX = "tmp_pack_"; | ||
| const GIT_TEMPORARY_PACK_STALE_AGE = Duration.hours(1); | ||
| const GIT_STATUS_FETCH_OBJECT_DIRECTORY = "t3-status-fetch"; | ||
| const GIT_STATUS_FETCH_REF_PREFIX = "refs/t3-status-fetch"; | ||
| const GIT_ZERO_OID = "0".repeat(40); | ||
| const STATUS_UPSTREAM_REFRESH_ENV = Object.freeze({ | ||
| GCM_INTERACTIVE: "never", | ||
| GIT_ASKPASS: "", | ||
|
|
@@ -93,6 +98,7 @@ type TraceTailState = { | |
| class StatusRemoteRefreshCacheKey extends Data.Class<{ | ||
| gitCommonDir: string; | ||
| remoteName: string; | ||
| branchName: string; | ||
| }> {} | ||
|
|
||
| interface ExecuteGitOptions { | ||
|
|
@@ -227,6 +233,19 @@ export function splitNullSeparatedGitStdoutPaths( | |
| return splitNullSeparatedPaths(result.stdout, result.stdoutTruncated); | ||
| } | ||
|
|
||
| export function isStaleGitTemporaryPackFile(input: { | ||
| entry: string; | ||
| modifiedAtMs: number | null; | ||
| nowMs: number; | ||
| staleAfterMs: number; | ||
| }): boolean { | ||
| return ( | ||
| input.entry.startsWith(GIT_TEMPORARY_PACK_PREFIX) && | ||
| input.modifiedAtMs !== null && | ||
| input.nowMs - input.modifiedAtMs >= input.staleAfterMs | ||
| ); | ||
| } | ||
|
|
||
| function sanitizeRemoteName(value: string): string { | ||
| const sanitized = value | ||
| .trim() | ||
|
|
@@ -929,20 +948,226 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* | |
| const fetchRemoteForStatus = ( | ||
| gitCommonDir: string, | ||
| remoteName: string, | ||
| branchName: string, | ||
| ): Effect.Effect<void, GitCommandError> => | ||
| Effect.gen(function* () { | ||
| const fetchCwd = | ||
| path.basename(gitCommonDir) === ".git" ? path.dirname(gitCommonDir) : gitCommonDir; | ||
| const objectDirectory = path.join(gitCommonDir, "objects"); | ||
| const packDirectory = path.join(objectDirectory, "pack"); | ||
| // Older builds wrote interrupted fetches into the shared pack directory. | ||
| // New fetches use the isolated object directory below; this only reaps legacy leftovers. | ||
| const cleanupStaleTemporaryPacks = Effect.gen(function* () { | ||
| if (!(yield* fileSystem.exists(packDirectory))) return; | ||
| const now = yield* DateTime.now; | ||
| const nowMs = DateTime.toEpochMillis(now); | ||
| const entries = yield* fileSystem.readDirectory(packDirectory, { recursive: false }); | ||
| yield* Effect.forEach( | ||
| entries, | ||
| (entry) => { | ||
| if (!entry.startsWith(GIT_TEMPORARY_PACK_PREFIX)) return Effect.void; | ||
| const temporaryPackPath = path.join(packDirectory, entry); | ||
| return fileSystem.stat(temporaryPackPath).pipe( | ||
| Effect.flatMap((info) => { | ||
| const modifiedAtMs = Option.getOrNull(info.mtime)?.getTime() ?? null; | ||
| return isStaleGitTemporaryPackFile({ | ||
| entry, | ||
| modifiedAtMs, | ||
| nowMs, | ||
| staleAfterMs: Duration.toMillis(GIT_TEMPORARY_PACK_STALE_AGE), | ||
| }) | ||
| ? fileSystem.remove(temporaryPackPath, { force: true }) | ||
| : Effect.void; | ||
| }), | ||
| Effect.ignore, | ||
| ); | ||
| }, | ||
| { concurrency: "unbounded", discard: true }, | ||
| ); | ||
| }).pipe( | ||
| Effect.catch((cause) => | ||
| Effect.logWarning("Failed to clean stale temporary Git pack files.", { cause }), | ||
| ), | ||
| ); | ||
|
|
||
| yield* cleanupStaleTemporaryPacks; | ||
| const operationId = yield* crypto.randomUUIDv4.pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new GitCommandError({ | ||
| operation: "GitVcsDriver.fetchRemoteForStatus", | ||
| command: "crypto.randomUUIDv4", | ||
| cwd: fetchCwd, | ||
| detail: "Failed to create an isolated status fetch identifier.", | ||
| cause, | ||
| }), | ||
| ), | ||
| ); | ||
| const temporaryRoot = path.join( | ||
| objectDirectory, | ||
| GIT_STATUS_FETCH_OBJECT_DIRECTORY, | ||
| operationId, | ||
| ); | ||
| const temporaryObjectDirectory = path.join(temporaryRoot, "objects"); | ||
| const temporaryPackDirectory = path.join(temporaryObjectDirectory, "pack"); | ||
| const temporaryRef = `${GIT_STATUS_FETCH_REF_PREFIX}/${operationId}`; | ||
| const targetRef = `refs/remotes/${remoteName}/${branchName}`; | ||
|
|
||
| yield* fileSystem.makeDirectory(temporaryPackDirectory, { recursive: true }).pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new GitCommandError({ | ||
| operation: "GitVcsDriver.fetchRemoteForStatus", | ||
| command: "filesystem.makeDirectory", | ||
| cwd: fetchCwd, | ||
| detail: "Failed to create an isolated Git object directory for status fetch.", | ||
| cause, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| const cleanupIsolatedFetch = Effect.all( | ||
| [ | ||
| executeGit( | ||
| "GitVcsDriver.fetchRemoteForStatus.cleanupRef", | ||
| fetchCwd, | ||
| ["--git-dir", gitCommonDir, "update-ref", "-d", temporaryRef], | ||
| { allowNonZeroExit: true, timeoutMs: 2_000 }, | ||
| ).pipe(Effect.ignore), | ||
| fileSystem.remove(temporaryRoot, { recursive: true, force: true }).pipe(Effect.ignore), | ||
| ], | ||
| { concurrency: "unbounded", discard: true }, | ||
| ); | ||
|
|
||
| const promoteFile = (source: string, destination: string) => | ||
| fileSystem | ||
| .rename(source, destination) | ||
| .pipe( | ||
| Effect.catch((cause) => | ||
| fileSystem | ||
| .exists(destination) | ||
| .pipe( | ||
| Effect.flatMap((exists) => | ||
| exists ? fileSystem.remove(source, { force: true }) : Effect.fail(cause), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| const promoteObjectFiles = Effect.gen(function* () { | ||
| const objectEntries = yield* fileSystem.readDirectory(temporaryObjectDirectory, { | ||
| recursive: false, | ||
| }); | ||
| for (const entry of objectEntries) { | ||
| if (entry === "pack") { | ||
| const packEntries = yield* fileSystem.readDirectory(temporaryPackDirectory, { | ||
| recursive: false, | ||
| }); | ||
| yield* Effect.forEach( | ||
| packEntries.filter((packEntry) => packEntry.startsWith("pack-")), | ||
| (packEntry) => | ||
| promoteFile( | ||
| path.join(temporaryPackDirectory, packEntry), | ||
| path.join(packDirectory, packEntry), | ||
| ), | ||
| { concurrency: 1, discard: true }, | ||
| ); | ||
| continue; | ||
| } | ||
| if (!/^[0-9a-f]{2}$/u.test(entry)) continue; | ||
| const sourceDirectory = path.join(temporaryObjectDirectory, entry); | ||
| const destinationDirectory = path.join(objectDirectory, entry); | ||
| yield* fileSystem.makeDirectory(destinationDirectory, { recursive: true }); | ||
| const looseObjects = yield* fileSystem.readDirectory(sourceDirectory, { | ||
| recursive: false, | ||
| }); | ||
| yield* Effect.forEach( | ||
| looseObjects, | ||
| (looseObject) => | ||
| promoteFile( | ||
| path.join(sourceDirectory, looseObject), | ||
| path.join(destinationDirectory, looseObject), | ||
| ), | ||
| { concurrency: 1, discard: true }, | ||
| ); | ||
| } | ||
| }).pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new GitCommandError({ | ||
| operation: "GitVcsDriver.fetchRemoteForStatus", | ||
| command: "filesystem.rename", | ||
| cwd: fetchCwd, | ||
| detail: "Failed to promote isolated Git objects.", | ||
| cause, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| yield* Effect.gen(function* () { | ||
| const previousTarget = yield* executeGit( | ||
| "GitVcsDriver.fetchRemoteForStatus.readTarget", | ||
| fetchCwd, | ||
| ["--git-dir", gitCommonDir, "rev-parse", "--verify", targetRef], | ||
| { allowNonZeroExit: true }, | ||
| ); | ||
| const expectedTarget = | ||
| previousTarget.exitCode === 0 ? previousTarget.stdout.trim() : GIT_ZERO_OID; | ||
| const result = yield* executeGit( | ||
| "GitVcsDriver.fetchRemoteForStatus", | ||
| fetchCwd, | ||
| [ | ||
| "--git-dir", | ||
| gitCommonDir, | ||
| "-c", | ||
| "fetch.unpackLimit=0", | ||
| "fetch", | ||
| "--quiet", | ||
| "--no-tags", | ||
| "--no-write-fetch-head", | ||
| "--refmap=", | ||
| remoteName, | ||
| `+refs/heads/${branchName}:${temporaryRef}`, | ||
|
Contributor
There was a problem hiding this comment. Default-branch tip goes staleMedium Severity Status refresh now fetches only Additional Locations (2)  Reviewed by Cursor Bugbot for commit 46ac662. Configure here. |
||
| ], | ||
| { | ||
| allowNonZeroExit: true, | ||
| env: { | ||
| ...STATUS_UPSTREAM_REFRESH_ENV, | ||
| GIT_OBJECT_DIRECTORY: temporaryObjectDirectory, | ||
| GIT_ALTERNATE_OBJECT_DIRECTORIES: objectDirectory, | ||
| }, | ||
| timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT), | ||
| }, | ||
| ); | ||
| if (result.exitCode !== 0) return; | ||
|
|
||
| yield* fileSystem.makeDirectory(packDirectory, { recursive: true }).pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new GitCommandError({ | ||
| operation: "GitVcsDriver.fetchRemoteForStatus", | ||
| command: "filesystem.makeDirectory", | ||
| cwd: fetchCwd, | ||
| detail: "Failed to prepare the Git pack directory.", | ||
| cause, | ||
| }), | ||
| ), | ||
| ); | ||
| yield* promoteObjectFiles; | ||
| const fetchedCommit = yield* runGitStdout( | ||
| "GitVcsDriver.fetchRemoteForStatus.resolve", | ||
| fetchCwd, | ||
| ["--git-dir", gitCommonDir, "rev-parse", "--verify", temporaryRef], | ||
| ).pipe(Effect.map((stdout) => stdout.trim())); | ||
| yield* runGit("GitVcsDriver.fetchRemoteForStatus.promoteRef", fetchCwd, [ | ||
| "--git-dir", | ||
| gitCommonDir, | ||
| "update-ref", | ||
| targetRef, | ||
| fetchedCommit, | ||
| expectedTarget, | ||
| ]); | ||
| }).pipe(Effect.ensuring(cleanupIsolatedFetch)); | ||
|
Contributor
There was a problem hiding this comment. Orphaned fetch dirs never reapedHigh Severity The new isolated status-fetch mechanism creates temporary directories under Reviewed by Cursor Bugbot for commit 46ac662. Configure here. |
||
| }); | ||
|
|
||
| const resolveGitCommonDir = Effect.fn("resolveGitCommonDir")(function* (cwd: string) { | ||
| const gitCommonDir = yield* runGitStdout("GitVcsDriver.resolveGitCommonDir", cwd, [ | ||
|
|
@@ -955,7 +1180,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* | |
| const refreshStatusRemoteCacheEntry = Effect.fn("refreshStatusRemoteCacheEntry")(function* ( | ||
| cacheKey: StatusRemoteRefreshCacheKey, | ||
| ) { | ||
| yield* fetchRemoteForStatus(cacheKey.gitCommonDir, cacheKey.remoteName, cacheKey.branchName); | ||
| return true as const; | ||
| }); | ||
|
|
||
|
|
@@ -979,6 +1204,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* | |
| new StatusRemoteRefreshCacheKey({ | ||
| gitCommonDir, | ||
| remoteName: upstream.remoteName, | ||
| branchName: upstream.branchName, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong remote refspec assumed
Medium Severity
The new status fetch builds
+refs/heads/${branchName}:…from the remote-tracking shorthand. That assumes a 1:1refs/heads/*mapping, unlike the previousgit fetch <remote>which honored configured refspecs, so custom upstreams (for example pull-request refs) can fail to refresh and leavebehindCountstale.Additional Locations (1)
apps/server/src/vcs/GitVcsDriverCore.ts#L1012-L1014Reviewed by Cursor Bugbot for commit 46ac662. Configure here.