fix(git): retain prefetch limits until requests settle
Diff prefetch released concurrency slots after its UI deadline while the underlying requests kept running, allowing overlapping batches to accumulate work. Retain per-runtime directory slots until transport completion, preserve outstanding requests across cache resets, and skip prefetch in hidden Git views. Clear deadline timers and avoid invalidating active batches on duplicate demand. Validated 43 focused tests, UI type-check and ESLint. Reproduced four outstanding requests against a limit of two before the fix. Reviewed existing oxlint findings; Windows process failure was not reproduced.
This commit is contained in:
@@ -1041,7 +1041,7 @@ export const GitView: React.FC<GitViewProps> = ({ isActive }) => {
|
|||||||
);
|
);
|
||||||
|
|
||||||
React.useEffect(() => {
|
React.useEffect(() => {
|
||||||
if (!gitDirectory || changeEntries.length === 0) {
|
if (!isActive || !gitDirectory || changeEntries.length === 0) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1071,7 +1071,7 @@ export const GitView: React.FC<GitViewProps> = ({ isActive }) => {
|
|||||||
return () => {
|
return () => {
|
||||||
window.clearTimeout(timeoutId);
|
window.clearTimeout(timeoutId);
|
||||||
};
|
};
|
||||||
}, [changeEntries, gitDirectory, git, prefetchDiffs, stagedChangeEntries, visibleChangePaths]);
|
}, [isActive, changeEntries, gitDirectory, git, prefetchDiffs, stagedChangeEntries, visibleChangePaths]);
|
||||||
|
|
||||||
const getPushedRemoteName = (result?: Awaited<ReturnType<typeof git.gitPush>>) => {
|
const getPushedRemoteName = (result?: Awaited<ReturnType<typeof git.gitPush>>) => {
|
||||||
return result?.pushed[0]?.remote
|
return result?.pushed[0]?.remote
|
||||||
|
|||||||
@@ -243,6 +243,8 @@ Important properties:
|
|||||||
- branch persistence is versioned, bounded, runtime-scoped, and claims the ambiguous legacy cache once
|
- branch persistence is versioned, bounded, runtime-scoped, and claims the ambiguous legacy cache once
|
||||||
- diff data has per-directory and aggregate count/UTF-8-byte limits; oversized single entries are rejected
|
- diff data has per-directory and aggregate count/UTF-8-byte limits; oversized single entries are rejected
|
||||||
|
|
||||||
|
Diff prefetch admits at most two outstanding transport requests per runtime and directory across overlapping batches. Its 15-second deadline stops waiting for a result; it does not cancel server work. A timed-out request retains its path and concurrency slot until the transport settles, including across cache resets, so later batches cannot repeat it or exceed the limit. Saturated prefetch skips further work instead of queueing retries. Late timed-out results never enter the cache, and successful or rejected transport completion releases capacity. Duplicate or saturated demand does not invalidate a batch already running. The Git view schedules prefetch only while active; explicit file opens remain independent of background prefetch capacity.
|
||||||
|
|
||||||
### `useGitHubPrStatusStore.ts`
|
### `useGitHubPrStatusStore.ts`
|
||||||
|
|
||||||
`useGitHubPrStatusStore` is a centralized PR cache keyed by a collision-safe tuple of runtime, directory, branch, and requested remote.
|
`useGitHubPrStatusStore` is a centralized PR cache keyed by a collision-safe tuple of runtime, directory, branch, and requested remote.
|
||||||
|
|||||||
@@ -91,6 +91,114 @@ describe('useGitStore', () => {
|
|||||||
useGitStore.getState().resetForRuntimeSwitch(getRuntimeKey());
|
useGitStore.getState().resetForRuntimeSwitch(getRuntimeKey());
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('keeps timed-out diff requests inside the concurrency limit until they settle', async () => {
|
||||||
|
const paths = ['one.ts', 'two.ts', 'three.ts', 'four.ts'];
|
||||||
|
setDirectoryStatus(createStatus({}, paths.map((path) => ({ path, index: ' ', working_dir: 'M' }))));
|
||||||
|
const pending: Array<{ path: string; request: Deferred<Awaited<ReturnType<GitAPI['getGitFileDiff']>>> }> = [];
|
||||||
|
const git = createGitApi(async () => createStatus());
|
||||||
|
git.getGitFileDiff = (_directory, { path }) => {
|
||||||
|
const request = createDeferred<Awaited<ReturnType<GitAPI['getGitFileDiff']>>>();
|
||||||
|
pending.push({ path, request });
|
||||||
|
return request.promise;
|
||||||
|
};
|
||||||
|
|
||||||
|
const prefetch = useGitStore.getState().prefetchDiffs('/repo', git, paths);
|
||||||
|
try {
|
||||||
|
expect(pending.length).toBe(2);
|
||||||
|
// Exercise the real 15-second prefetch deadline. Expiring the UI wait
|
||||||
|
// does not settle the injected transport or stop its server-side work.
|
||||||
|
await new Promise((resolve) => setTimeout(resolve, 15_100));
|
||||||
|
expect(pending.length).toBe(2);
|
||||||
|
await useGitStore.getState().prefetchDiffs('/repo', git, paths);
|
||||||
|
expect(pending.length).toBe(2);
|
||||||
|
expect(useGitStore.getState().getDirectoryState('/repo')?.diffCache.size).toBe(0);
|
||||||
|
} finally {
|
||||||
|
for (const { path, request } of pending) request.resolve({ path, original: 'before', modified: 'after' });
|
||||||
|
await prefetch;
|
||||||
|
}
|
||||||
|
// Late responses were discarded, but their real completion frees capacity.
|
||||||
|
expect(useGitStore.getState().getDirectoryState('/repo')?.diffCache.size).toBe(0);
|
||||||
|
git.getGitFileDiff = async (_directory, { path }) => ({ path, original: 'fresh', modified: 'fresh' });
|
||||||
|
await useGitStore.getState().prefetchDiffs('/repo', git, paths);
|
||||||
|
expect(useGitStore.getState().getDirectoryState('/repo')?.diffCache.size).toBe(4);
|
||||||
|
}, 40_000);
|
||||||
|
|
||||||
|
test('limits overlapping diff batches per directory and retains capacity across a cache reset', async () => {
|
||||||
|
const paths = ['one.ts', 'two.ts', 'three.ts'];
|
||||||
|
const status = createStatus({}, paths.map((path) => ({ path, index: ' ', working_dir: 'M' })));
|
||||||
|
const directories = ['/repo-a', '/repo-b', '/repo-c'];
|
||||||
|
const populate = () => useGitStore.setState({
|
||||||
|
directories: new Map(directories.map((directory) => [directory, createDirectoryState(status)])),
|
||||||
|
});
|
||||||
|
populate();
|
||||||
|
const request = createDeferred<Awaited<ReturnType<GitAPI['getGitFileDiff']>>>();
|
||||||
|
const calls: string[] = [];
|
||||||
|
const git = createGitApi(async () => status);
|
||||||
|
git.getGitFileDiff = (directory) => {
|
||||||
|
calls.push(directory);
|
||||||
|
return request.promise;
|
||||||
|
};
|
||||||
|
const batches = directories.flatMap((directory) => paths.map((path) => (
|
||||||
|
useGitStore.getState().prefetchDiffs(directory, git, [path])
|
||||||
|
)));
|
||||||
|
try {
|
||||||
|
expect(calls.length).toBe(6);
|
||||||
|
for (const directory of directories) expect(calls.filter((value) => value === directory).length).toBe(2);
|
||||||
|
useGitStore.getState().resetForRuntimeSwitch(getRuntimeKey());
|
||||||
|
populate();
|
||||||
|
await Promise.all(directories.map((directory) => useGitStore.getState().prefetchDiffs(directory, git, paths)));
|
||||||
|
expect(calls.length).toBe(6);
|
||||||
|
} finally {
|
||||||
|
request.resolve({ path: 'one.ts', original: '', modified: '' });
|
||||||
|
await Promise.all(batches);
|
||||||
|
}
|
||||||
|
for (const directory of directories) expect(useGitStore.getState().getDirectoryState(directory)?.diffCache.size).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('repeated status refreshes for three directories share their outstanding requests', async () => {
|
||||||
|
const status = createStatus();
|
||||||
|
const directories = ['/status-a', '/status-b', '/status-c'];
|
||||||
|
useGitStore.setState({ directories: new Map(directories.map((directory) => [directory, createDirectoryState(status)])) });
|
||||||
|
const request = createDeferred<GitStatus>();
|
||||||
|
let calls = 0;
|
||||||
|
const git = createGitApi(async () => {
|
||||||
|
calls += 1;
|
||||||
|
return request.promise;
|
||||||
|
});
|
||||||
|
const refreshes = Array.from({ length: 20 }, () => directories.map((directory) => (
|
||||||
|
useGitStore.getState().fetchStatus(directory, git, { silent: true })
|
||||||
|
))).flat();
|
||||||
|
try {
|
||||||
|
expect(calls).toBe(3);
|
||||||
|
} finally {
|
||||||
|
request.resolve(status);
|
||||||
|
await Promise.all(refreshes);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a duplicate diff demand does not discard the first batch or leave failure slots occupied', async () => {
|
||||||
|
const paths = ['one.ts', 'two.ts', 'three.ts'];
|
||||||
|
setDirectoryStatus(createStatus({}, paths.map((path) => ({ path, index: ' ', working_dir: 'M' }))));
|
||||||
|
const request = createDeferred<Awaited<ReturnType<GitAPI['getGitFileDiff']>>>();
|
||||||
|
const git = createGitApi(async () => createStatus());
|
||||||
|
let calls = 0;
|
||||||
|
git.getGitFileDiff = async (_directory, { path }) => {
|
||||||
|
calls += 1;
|
||||||
|
if (path === 'one.ts') return request.promise;
|
||||||
|
if (path === 'two.ts') throw new Error('failed read');
|
||||||
|
return { path, original: '', modified: 'fresh' };
|
||||||
|
};
|
||||||
|
const first = useGitStore.getState().prefetchDiffs('/repo', git, paths);
|
||||||
|
await useGitStore.getState().prefetchDiffs('/repo', git, paths);
|
||||||
|
request.resolve({ path: 'one.ts', original: '', modified: 'fresh' });
|
||||||
|
await first;
|
||||||
|
expect(calls).toBe(3);
|
||||||
|
const cache = useGitStore.getState().getDirectoryState('/repo')?.diffCache;
|
||||||
|
expect(cache?.has('one.ts')).toBe(true);
|
||||||
|
expect(cache?.has('two.ts')).toBe(false);
|
||||||
|
expect(cache?.has('three.ts')).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
test('does not reuse an in-flight light status request for full status', async () => {
|
test('does not reuse an in-flight light status request for full status', async () => {
|
||||||
setDirectoryStatus(createStatus());
|
setDirectoryStatus(createStatus());
|
||||||
const requests: Deferred<GitStatus>[] = [];
|
const requests: Deferred<GitStatus>[] = [];
|
||||||
|
|||||||
@@ -657,7 +657,11 @@ export const useGitStore = create<GitStore>()(
|
|||||||
inFlightStatusFetches.clear();
|
inFlightStatusFetches.clear();
|
||||||
inFlightEnsureAllByDirectory.clear();
|
inFlightEnsureAllByDirectory.clear();
|
||||||
inFlightNestedRepoDiscovery.clear();
|
inFlightNestedRepoDiscovery.clear();
|
||||||
inFlightDiffFetchesByDirectory.clear();
|
// Outstanding transports still consume capacity on their captured
|
||||||
|
// runtime, even after its visible cache has been reset.
|
||||||
|
for (const [key, requests] of inFlightDiffFetchesByDirectory) {
|
||||||
|
if (requests.size === 0) inFlightDiffFetchesByDirectory.delete(key);
|
||||||
|
}
|
||||||
diffFetchGenerationByDirectory.clear();
|
diffFetchGenerationByDirectory.clear();
|
||||||
set({
|
set({
|
||||||
runtimeKey,
|
runtimeKey,
|
||||||
@@ -1141,7 +1145,6 @@ export const useGitStore = create<GitStore>()(
|
|||||||
},
|
},
|
||||||
|
|
||||||
prefetchDiffs: async (directory, git, filePaths, options = {}) => {
|
prefetchDiffs: async (directory, git, filePaths, options = {}) => {
|
||||||
const token = startRequest(directory, 'diff');
|
|
||||||
const dirState = get().directories.get(directory);
|
const dirState = get().directories.get(directory);
|
||||||
if (!dirState?.status?.files || dirState.status.files.length === 0 || filePaths.length === 0) return;
|
if (!dirState?.status?.files || dirState.status.files.length === 0 || filePaths.length === 0) return;
|
||||||
|
|
||||||
@@ -1175,7 +1178,7 @@ export const useGitStore = create<GitStore>()(
|
|||||||
}
|
}
|
||||||
|
|
||||||
const limitedFilePaths = dedupedPaths.slice(0, Math.max(1, maxFiles));
|
const limitedFilePaths = dedupedPaths.slice(0, Math.max(1, maxFiles));
|
||||||
if (limitedFilePaths.length === 0) return;
|
if (limitedFilePaths.length === 0 || inFlight.size >= DIFF_PREFETCH_CONCURRENCY) return;
|
||||||
|
|
||||||
const generation = getDiffFetchGeneration(directory);
|
const generation = getDiffFetchGeneration(directory);
|
||||||
|
|
||||||
@@ -1183,7 +1186,7 @@ export const useGitStore = create<GitStore>()(
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
limitedFilePaths.forEach((path) => inFlight.add(path));
|
const token = startRequest(directory, 'diff');
|
||||||
|
|
||||||
let nextIndex = 0;
|
let nextIndex = 0;
|
||||||
const results: Array<{ path: string; diff: { original: string; modified: string; isBinary?: boolean } }> = [];
|
const results: Array<{ path: string; diff: { original: string; modified: string; isBinary?: boolean } }> = [];
|
||||||
@@ -1195,30 +1198,44 @@ export const useGitStore = create<GitStore>()(
|
|||||||
};
|
};
|
||||||
|
|
||||||
const fetchWithTimeout = async (filePath: string) => {
|
const fetchWithTimeout = async (filePath: string) => {
|
||||||
const fetchPromise = git.getGitFileDiff(directory, { path: filePath });
|
inFlight.add(filePath);
|
||||||
|
const fetchPromise = (async () => {
|
||||||
|
try {
|
||||||
|
return await git.getGitFileDiff(directory, { path: filePath });
|
||||||
|
} finally {
|
||||||
|
// A UI deadline only stops waiting. Keep the path and capacity
|
||||||
|
// reserved until the transport actually settles.
|
||||||
|
inFlight.delete(filePath);
|
||||||
|
}
|
||||||
|
})();
|
||||||
|
let timeout: ReturnType<typeof setTimeout> | undefined;
|
||||||
const timeoutPromise = new Promise<never>((_, reject) => {
|
const timeoutPromise = new Promise<never>((_, reject) => {
|
||||||
setTimeout(() => reject(new Error(`Timed out after ${DIFF_PREFETCH_TIMEOUT_MS}ms`)), DIFF_PREFETCH_TIMEOUT_MS);
|
timeout = setTimeout(() => reject(new Error(`Timed out after ${DIFF_PREFETCH_TIMEOUT_MS}ms`)), DIFF_PREFETCH_TIMEOUT_MS);
|
||||||
});
|
});
|
||||||
const response = await Promise.race([fetchPromise, timeoutPromise]);
|
try {
|
||||||
return {
|
const response = await Promise.race([fetchPromise, timeoutPromise]);
|
||||||
path: filePath,
|
return {
|
||||||
diff: { original: response.original ?? '', modified: response.modified ?? '', isBinary: response.isBinary },
|
path: filePath,
|
||||||
};
|
diff: { original: response.original ?? '', modified: response.modified ?? '', isBinary: response.isBinary },
|
||||||
|
};
|
||||||
|
} finally {
|
||||||
|
clearTimeout(timeout);
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
const worker = async () => {
|
const worker = async () => {
|
||||||
for (;;) {
|
for (;;) {
|
||||||
if (generation !== getDiffFetchGeneration(directory) || !isRequestCurrent(token, directory)) {
|
if (generation !== getDiffFetchGeneration(directory) || !isRequestCurrent(token, directory)
|
||||||
|
|| inFlight.size >= DIFF_PREFETCH_CONCURRENCY) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const next = takeNext();
|
const next = takeNext();
|
||||||
if (!next) return;
|
if (!next) return;
|
||||||
|
if (inFlight.has(next)) continue;
|
||||||
try {
|
try {
|
||||||
results.push(await fetchWithTimeout(next));
|
results.push(await fetchWithTimeout(next));
|
||||||
} catch {
|
} catch {
|
||||||
// Ignore individual failures/timeouts during prefetch.
|
// Ignore individual failures/timeouts during prefetch.
|
||||||
} finally {
|
|
||||||
inFlight.delete(next);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
@@ -1226,8 +1243,6 @@ export const useGitStore = create<GitStore>()(
|
|||||||
const workerCount = Math.min(DIFF_PREFETCH_CONCURRENCY, limitedFilePaths.length);
|
const workerCount = Math.min(DIFF_PREFETCH_CONCURRENCY, limitedFilePaths.length);
|
||||||
await Promise.allSettled(Array.from({ length: workerCount }, () => worker()));
|
await Promise.allSettled(Array.from({ length: workerCount }, () => worker()));
|
||||||
|
|
||||||
limitedFilePaths.forEach((path) => inFlight.delete(path));
|
|
||||||
|
|
||||||
if (generation !== getDiffFetchGeneration(directory) || !isRequestCurrent(token, directory)) {
|
if (generation !== getDiffFetchGeneration(directory) || !isRequestCurrent(token, directory)) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user