mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 10:39:32 +01:00
fix(git): the interval fetches one run per folder, and the rest of the review (review)
The scheduler is the part that runs with nobody watching, so it is decided per folder rather than per session: sessions sharing a folder are the same clone and share one run that both see, `inFlight`/`lastRunAt` are keyed by the resolved folder, the schedule is stamped when a folder is picked instead of when its run starts, and a tick is skipped while the previous one is still going. The click route joins a run already going for the same folder rather than starting a second fetch in one clone. The tick keeps ONE bound for the git processes of the whole run and hands it to every folder, and bounds its folders with a bound of its own: the same semaphore cannot serve both levels, because a folder holding a slot would then wait for a slot its own fetch needs. Both bounds are `FETCH_CONCURRENCY`, so the ceiling is unchanged. The bound claim in CLAUDE.md, the `Limiter` jsdoc and docs/api-reference.md now describes it as it is. Before anything is fetched the folder is probed with the bounded path probe, so a case on a mount that stopped answering is skipped and never touched. The rest of the review: the hooks are named as they really are (`reference-transaction`, `pre-auto-gc` through the maintenance a fetch triggers, config commands such as `credential.helper`) and `FETCH_CONFIG_ARGS` turns maintenance and gc off for an unattended run; the claim that an agent in the same repository is not disturbed is replaced by the ref-lock collision it actually is; the click route publishes its own run as `trigger: 'click'`; `autoSaveGitFetch` uses `_apiJson` so a 400 shows the error toast and applies the returned settings; and the two wording fixes. Tests: the scheduler pins one run per folder, the two bounds, the probe before anything runs, and that closing the session holding the override keeps the folder while another session is still open on it; the route test pins the click publish and the join, asserting the join before the held run is released — releasing first lets the run finish and clears the lane, so the joiner would be entitled to its own fetch and the test would pass for the wrong reason.
This commit is contained in:
1 parent
9432d9f9e2
commit
ea4318947b
20 files changed
+774
-209
No files matched your search
@@ -1,8 +1,12 @@
|
||||
/**
|
||||
* @fileoverview The Git fetch interval scheduler (src/git-fetch-scheduler.ts): which cases a tick runs, and
|
||||
* which it must not. Every rule is checked by calling `tick(now)` with an explicit clock, so nothing here
|
||||
* @fileoverview The Git fetch interval scheduler (src/git-fetch-scheduler.ts): which sessions a tick runs,
|
||||
* and which it must not. Every rule is checked by calling `tick(now)` with an explicit clock, so nothing here
|
||||
* depends on fake timers or on how long a real tick takes — the scheduler owns no timers itself.
|
||||
*
|
||||
* The run belongs to the FOLDER, so these tests put two sessions on one folder and expect one fetch that
|
||||
* both see, and they check the tick's two bounds separately: one shared bound for the fetches of the whole
|
||||
* tick, and the folders bounded by a bound that cannot be the same semaphore.
|
||||
*
|
||||
* Port: N/A.
|
||||
*/
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
@@ -14,11 +18,13 @@ import {
|
||||
} from '../src/git-fetch-scheduler.js';
|
||||
import {
|
||||
FETCH_CONCURRENCY,
|
||||
type GitFetchOptions,
|
||||
type GitFetchReport,
|
||||
type GitFetchSettings,
|
||||
type GitFetchStatus,
|
||||
} from '../src/git-fetch.js';
|
||||
import { DEFAULT_GIT_TIMEOUT_MS, MAX_REPOS } from '../src/git-workspace-status.js';
|
||||
import type { PathProbeKind } from '../src/utils/bounded-path-probe.js';
|
||||
|
||||
const SETTINGS: GitFetchSettings = { enabled: true, intervalSeconds: 300 };
|
||||
const VIEW = (id: string, extra: Partial<GitFetchSessionView> = {}): GitFetchSessionView => ({
|
||||
@@ -38,10 +44,19 @@ const report = (cwd: string, name: string): GitFetchReport => ({
|
||||
type Harness = {
|
||||
scheduler: GitFetchScheduler;
|
||||
ports: GitFetchPorts;
|
||||
calls: Array<{ id: string; opts: unknown }>;
|
||||
/** The folders the tick actually ran, in the order they were picked. */
|
||||
calls: Array<{ cwd: string; opts: GitFetchOptions }>;
|
||||
published: Array<{ id: string; status: GitFetchStatus }>;
|
||||
/** Every path the tick probed, in order — the probe has to happen before anything is fetched. */
|
||||
probed: string[];
|
||||
settings: GitFetchSettings;
|
||||
dockerReads: number;
|
||||
/**
|
||||
* A session that closed: the server calls `remove()` when the session is gone, so this takes it out of the
|
||||
* ports as well as out of the scheduler. Otherwise a test that removes a session still sees it on the next
|
||||
* tick, and the folder it was on looks still open.
|
||||
*/
|
||||
remove(id: string): void;
|
||||
};
|
||||
|
||||
/** A run the test decides when to let finish, so a slow repository can be held open across ticks. */
|
||||
@@ -57,21 +72,32 @@ function deferred<T>(value: T) {
|
||||
*/
|
||||
function harness(
|
||||
sessions: GitFetchSessionView[],
|
||||
opts: { settings?: GitFetchSettings; fetch?: (id: string) => Promise<GitFetchReport> } = {}
|
||||
opts: {
|
||||
settings?: GitFetchSettings;
|
||||
fetch?: (cwd: string, portOpts: GitFetchOptions) => Promise<GitFetchReport>;
|
||||
/** Default: every path answers as a folder, so a test that does not care about the probe stays quiet. */
|
||||
probe?: (path: string) => Promise<PathProbeKind>;
|
||||
} = {}
|
||||
): Harness {
|
||||
const customFetch = opts.fetch;
|
||||
const settings = opts.settings ?? { ...SETTINGS };
|
||||
const gone = new Set<string>();
|
||||
const h: Harness = {
|
||||
scheduler: undefined as unknown as GitFetchScheduler,
|
||||
ports: undefined as unknown as GitFetchPorts,
|
||||
calls: [],
|
||||
published: [],
|
||||
probed: [],
|
||||
settings,
|
||||
dockerReads: 0,
|
||||
remove: (id) => {
|
||||
gone.add(id);
|
||||
h.scheduler.remove(id);
|
||||
},
|
||||
};
|
||||
h.ports = {
|
||||
sessionIds: () => sessions.map((s) => s.id),
|
||||
getSession: (id) => sessions.find((s) => s.id === id),
|
||||
sessionIds: () => sessions.filter((s) => !gone.has(s.id)).map((s) => s.id),
|
||||
getSession: (id) => (gone.has(id) ? undefined : sessions.find((s) => s.id === id)),
|
||||
readSettings: async () => {
|
||||
if (h.settings === null) throw new Error('settings.json is mid-write');
|
||||
return h.settings;
|
||||
@@ -80,10 +106,13 @@ function harness(
|
||||
h.dockerReads++;
|
||||
return [];
|
||||
},
|
||||
probePath: async (path) => {
|
||||
h.probed.push(path);
|
||||
return opts.probe ? await opts.probe(path) : 'directory';
|
||||
},
|
||||
fetch: async (cwd, portOpts) => {
|
||||
const id = cwd.slice(3); // the '/w/<id>' the view was built with
|
||||
h.calls.push({ id, opts: portOpts });
|
||||
return customFetch ? customFetch(id) : report(cwd, id);
|
||||
h.calls.push({ cwd, opts: portOpts });
|
||||
return customFetch ? customFetch(cwd, portOpts) : report(cwd, cwd.slice(3));
|
||||
},
|
||||
publish: (id, status) => h.published.push({ id, status }),
|
||||
};
|
||||
@@ -96,7 +125,8 @@ describe('a quiet tick', () => {
|
||||
const h = harness([VIEW('a'), VIEW('b')], { settings: { enabled: false, intervalSeconds: 300 } });
|
||||
expect(await h.scheduler.tick(1000)).toBe(0);
|
||||
expect(h.calls).toEqual([]);
|
||||
// The Docker case list is only worth reading once something is actually due.
|
||||
// The probe and the Docker case list are only worth reading once something is actually due.
|
||||
expect(h.probed).toEqual([]);
|
||||
expect(h.dockerReads).toBe(0);
|
||||
});
|
||||
|
||||
@@ -125,16 +155,38 @@ describe('a quiet tick', () => {
|
||||
expect(logged).toHaveBeenCalled();
|
||||
|
||||
h.settings = SETTINGS;
|
||||
// Nothing was recorded for the failed tick, so the case is still due at once.
|
||||
// Nothing was recorded for the failed tick, so the folder is still due at once.
|
||||
expect(await h.scheduler.tick(1000)).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the bounded probe', () => {
|
||||
it('probes the folder before fetching it, and skips anything that is not a directory', async () => {
|
||||
const kinds = new Map<string, PathProbeKind>([
|
||||
['/w/file', 'file'],
|
||||
['/w/unknown', 'unknown'],
|
||||
['/w/absent', 'absent'],
|
||||
]);
|
||||
const h = harness([VIEW('file'), VIEW('unknown'), VIEW('absent'), VIEW('ok')], {
|
||||
probe: async (path) => kinds.get(path) ?? 'directory',
|
||||
});
|
||||
const logged = vi.fn();
|
||||
h.ports.log = logged;
|
||||
expect(await h.scheduler.tick(0)).toBe(1);
|
||||
expect(h.calls.map((c) => c.cwd)).toEqual(['/w/ok']);
|
||||
// The folders that answered badly were probed and left alone, never fetched.
|
||||
expect(h.probed).toEqual(['/w/file', '/w/unknown', '/w/absent', '/w/ok']);
|
||||
expect(logged).toHaveBeenCalledTimes(3);
|
||||
// `unknown` is never read as `absent`: the log says what it answered rather than what it was assumed.
|
||||
expect(logged.mock.calls.join(' ')).toContain('unknown');
|
||||
});
|
||||
});
|
||||
|
||||
describe('who runs', () => {
|
||||
it('runs every case the app setting covers, once per interval', async () => {
|
||||
it('runs every session the app setting covers, once per interval', async () => {
|
||||
const h = harness([VIEW('a'), VIEW('b')], { settings: { enabled: true, intervalSeconds: 60 } });
|
||||
await h.scheduler.tick(0);
|
||||
expect(h.calls.map((c) => c.id).sort()).toEqual(['a', 'b']);
|
||||
expect(h.calls.map((c) => c.cwd).sort()).toEqual(['/w/a', '/w/b']);
|
||||
|
||||
await h.scheduler.tick(30_000); // not due yet
|
||||
expect(h.calls.length).toBe(2);
|
||||
@@ -143,29 +195,29 @@ describe('who runs', () => {
|
||||
expect(h.calls.length).toBe(4);
|
||||
});
|
||||
|
||||
it('runs a case that turned itself on even while the app-wide switch is off — that is what a per-case switch has to mean', async () => {
|
||||
it('runs a session that turned itself on even while the app-wide switch is off — that is what a per-session switch has to mean', async () => {
|
||||
const h = harness([VIEW('plain'), VIEW('own', { gitFetchEnabled: true, gitFetchIntervalSeconds: 30 })], {
|
||||
settings: { enabled: false, intervalSeconds: 300 },
|
||||
});
|
||||
await h.scheduler.tick(0);
|
||||
expect(h.calls.map((c) => c.id)).toEqual(['own']);
|
||||
expect(h.calls.map((c) => c.cwd)).toEqual(['/w/own']);
|
||||
|
||||
// Its own interval, not the app's: due at 30 s, not at 300.
|
||||
await h.scheduler.tick(30_000);
|
||||
expect(h.calls.length).toBe(2);
|
||||
});
|
||||
|
||||
it('never fetches a remote or Docker case, exactly as the Git panel excludes them', async () => {
|
||||
it('never fetches a remote or Docker session, exactly as the Git panel excludes them', async () => {
|
||||
const h = harness([
|
||||
VIEW('ssh', { remote: { host: 'x' } }),
|
||||
VIEW('docker', { docker: { container: 'c' } }),
|
||||
VIEW('local'),
|
||||
]);
|
||||
await h.scheduler.tick(0);
|
||||
expect(h.calls.map((c) => c.id)).toEqual(['local']);
|
||||
expect(h.calls.map((c) => c.cwd)).toEqual(['/w/local']);
|
||||
});
|
||||
|
||||
it('re-checks the Docker rule every tick, so a case that became a Docker case stops fetching', async () => {
|
||||
it('re-checks the Docker rule every tick, so a session whose case became a Docker case stops fetching', async () => {
|
||||
const sessions = [VIEW('a')];
|
||||
const h = harness(sessions);
|
||||
await h.scheduler.tick(0);
|
||||
@@ -183,16 +235,78 @@ describe('who runs', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('one run at a time', () => {
|
||||
it('skips a case still being fetched instead of stacking runs on it', async () => {
|
||||
const gate = deferred<GitFetchReport>(report('/w/a', 'a'));
|
||||
const h = harness([VIEW('a')], { settings: { enabled: true, intervalSeconds: 30 }, fetch: () => gate.promise });
|
||||
const running = h.scheduler.tick(0);
|
||||
await new Promise((resolve) => setImmediate(resolve)); // the run has started, and is still held open
|
||||
expect(await h.scheduler.tick(15_000)).toBe(0);
|
||||
describe('one folder, one run', () => {
|
||||
it('fetches a shared folder once and publishes the result to every session open on it', async () => {
|
||||
const h = harness([VIEW('a'), { ...VIEW('b'), workingDir: '/w/a' }]);
|
||||
expect(await h.scheduler.tick(0)).toBe(1); // one folder, not two runs
|
||||
expect(h.calls.length).toBe(1);
|
||||
gate.resolve(report('/w/a', 'a'));
|
||||
await running;
|
||||
expect(h.published.map((p) => p.id).sort()).toEqual(['a', 'b']);
|
||||
// Both see the same run, so the panel's row says the same thing in both tabs.
|
||||
expect(new Set(h.published.map((p) => p.status.failed)).size).toBe(1);
|
||||
});
|
||||
|
||||
it('does not pick a folder again while its run is going, and does not run a second tick over the same selection', async () => {
|
||||
const ids = Array.from({ length: FETCH_CONCURRENCY + 2 }, (_, i) => `s${i}`);
|
||||
const gate = deferred<GitFetchReport>(report('/w/first', 'first'));
|
||||
const h = harness(
|
||||
ids.map((id) => VIEW(id)),
|
||||
{
|
||||
settings: { enabled: true, intervalSeconds: 30 },
|
||||
fetch: () => gate.promise,
|
||||
}
|
||||
);
|
||||
|
||||
const first = h.scheduler.tick(0);
|
||||
await new Promise((resolve) => setImmediate(resolve)); // the first runs are going, the rest are queued
|
||||
const picked = h.calls.length;
|
||||
expect(picked).toBe(FETCH_CONCURRENCY);
|
||||
|
||||
// The server fires the next tick without waiting. The schedule was stamped at selection, so the folders
|
||||
// already picked are not picked again, and this tick is skipped while the previous one is still going.
|
||||
expect(await h.scheduler.tick(15_000)).toBe(0);
|
||||
expect(h.calls.length).toBe(picked);
|
||||
|
||||
gate.resolve(report('/w/first', 'first'));
|
||||
await first;
|
||||
// Every folder ran exactly once, and no folder ran twice.
|
||||
expect(h.calls.length).toBe(ids.length);
|
||||
expect(new Set(h.calls.map((c) => c.cwd)).size).toBe(ids.length);
|
||||
});
|
||||
|
||||
it('shares one bound across the whole tick instead of giving each folder its own, and bounds the folders with a bound of its own', async () => {
|
||||
const ids = Array.from({ length: FETCH_CONCURRENCY + 1 }, (_, i) => `s${i}`);
|
||||
let fetchesLive = 0;
|
||||
let fetchesPeak = 0;
|
||||
let foldersLive = 0;
|
||||
let foldersPeak = 0;
|
||||
const hold = () => new Promise((resolve) => setImmediate(resolve));
|
||||
const h = harness(
|
||||
ids.map((id) => VIEW(id)),
|
||||
{
|
||||
// Every folder is handed the SAME limiter for its work, so that work cannot get its own bound: a
|
||||
// per-call bound would let 5 folders × 6 items run at once. The folders themselves are bounded by a
|
||||
// DIFFERENT bound, because one semaphore cannot serve both levels — a folder holding a slot and then
|
||||
// waiting for one for its own fetch is waiting on itself, and the folders that fill the bound are
|
||||
// exactly the ones blocking it. If the tick used one semaphore for both, this test would hang and fail
|
||||
// as a timeout rather than as an assertion.
|
||||
fetch: async (cwd, opts) => {
|
||||
foldersLive++;
|
||||
foldersPeak = Math.max(foldersPeak, foldersLive);
|
||||
await opts.limiter?.([0, 1, 2, 3, 4, 5], async () => {
|
||||
fetchesLive++;
|
||||
fetchesPeak = Math.max(fetchesPeak, fetchesLive);
|
||||
await hold();
|
||||
fetchesLive--;
|
||||
});
|
||||
foldersLive--;
|
||||
return report(cwd, cwd.slice(3));
|
||||
},
|
||||
}
|
||||
);
|
||||
await h.scheduler.tick(0);
|
||||
expect(fetchesPeak).toBe(FETCH_CONCURRENCY); // the tick's ceiling, not each folder's own
|
||||
expect(foldersPeak).toBe(FETCH_CONCURRENCY);
|
||||
expect(h.calls.length).toBe(ids.length); // every folder ran, none was left waiting forever
|
||||
});
|
||||
|
||||
it('times the interval from the START of a run, so a slow repository does not push the next one out', async () => {
|
||||
@@ -203,34 +317,13 @@ describe('one run at a time', () => {
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
expect(h.calls.length).toBe(1);
|
||||
|
||||
// The run takes 200 s, longer than its own interval. Once it lands, the case is already past due: the
|
||||
// The run takes 200 s, longer than its own interval. Once it lands, the folder is already past due: the
|
||||
// next tick runs it again instead of waiting another 30 s from when the slow one finished.
|
||||
slow.resolve(report('/w/a', 'a'));
|
||||
await first;
|
||||
expect(await h.scheduler.tick(200_000)).toBe(1);
|
||||
expect(h.calls.length).toBe(2);
|
||||
});
|
||||
|
||||
it('runs at most FETCH_CONCURRENCY cases at a time, because each one is a git process', async () => {
|
||||
const ids = Array.from({ length: FETCH_CONCURRENCY + 3 }, (_, i) => `s${i}`);
|
||||
let live = 0;
|
||||
let peak = 0;
|
||||
const h = harness(
|
||||
ids.map((id) => VIEW(id)),
|
||||
{
|
||||
fetch: async () => {
|
||||
live++;
|
||||
peak = Math.max(peak, live);
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
live--;
|
||||
return report('/w/x', 'x');
|
||||
},
|
||||
}
|
||||
);
|
||||
await h.scheduler.tick(0);
|
||||
expect(peak).toBe(FETCH_CONCURRENCY);
|
||||
expect(h.calls.length).toBe(ids.length);
|
||||
});
|
||||
});
|
||||
|
||||
describe('what a run reports', () => {
|
||||
@@ -245,7 +338,7 @@ describe('what a run reports', () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("reports a failure instead of retrying harder, and keeps the case's own next run at its own interval", async () => {
|
||||
it("reports a failure instead of retrying harder, and keeps the folder's next run at its own interval", async () => {
|
||||
const h = harness([VIEW('a')], {
|
||||
settings: { enabled: true, intervalSeconds: 30 },
|
||||
fetch: () => Promise.reject(new Error('spawn git ENOENT')),
|
||||
@@ -261,7 +354,7 @@ describe('what a run reports', () => {
|
||||
});
|
||||
|
||||
describe('the effective choice', () => {
|
||||
it("a case's own override wins over the app setting", () => {
|
||||
it("a session's own override wins over the app setting", () => {
|
||||
const settings = { enabled: false, intervalSeconds: 600 };
|
||||
expect(effectiveGitFetch(VIEW('a', { gitFetchEnabled: true, gitFetchIntervalSeconds: 45 }), settings)).toEqual({
|
||||
enabled: true,
|
||||
@@ -286,19 +379,30 @@ describe('the effective choice', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('a case that goes away', () => {
|
||||
it('takes its schedule with it, so a reopened case starts on its own clock', async () => {
|
||||
const h = harness([VIEW('a')]);
|
||||
describe('a session that goes away', () => {
|
||||
it('takes its own choice with it, but keeps the folder while another session is still open on it', async () => {
|
||||
const sessions = [VIEW('a'), { ...VIEW('b'), workingDir: '/w/a' }];
|
||||
const h = harness(sessions);
|
||||
await h.scheduler.tick(0);
|
||||
expect(h.scheduler.tracked()).toBe(1);
|
||||
|
||||
h.scheduler.remove('a');
|
||||
h.remove('a');
|
||||
// The run belongs to the clone, so 'b' keeps the folder's schedule and its next run is still on time:
|
||||
// the choice that got the folder going was 'a's, but the folder is not 'a's.
|
||||
expect(h.scheduler.tracked()).toBe(1);
|
||||
await h.scheduler.tick(1000); // not due yet, and 'b' is still open on the folder
|
||||
expect(h.calls.length).toBe(1);
|
||||
|
||||
// The last session open on the folder goes, so the folder is forgotten: a session coming back on it is due
|
||||
// at once rather than on the schedule the closed one left behind.
|
||||
h.remove('b');
|
||||
expect(h.scheduler.tracked()).toBe(0);
|
||||
await h.scheduler.tick(1000);
|
||||
sessions.push({ ...VIEW('c'), workingDir: '/w/a' });
|
||||
await h.scheduler.tick(2000);
|
||||
expect(h.calls.length).toBe(2); // remembered again, from now
|
||||
});
|
||||
|
||||
it('stops everything after stop(), including a case that was already due', async () => {
|
||||
it('stops everything after stop(), including a folder that was already due', async () => {
|
||||
const h = harness([VIEW('a')]);
|
||||
h.scheduler.stop();
|
||||
expect(await h.scheduler.tick(0)).toBe(0);
|
||||
|
||||
+181
-6
@@ -1,16 +1,20 @@
|
||||
/**
|
||||
* @fileoverview Tests for the Git panel's Fetch button (src/git-fetch.ts): the one git command Codeman
|
||||
* runs that writes, and the one that runs a repository's own hooks — which is why it is a click, not a poll.
|
||||
* runs that writes, and the one that can run a repository's own hooks — which is why it is a click, not a
|
||||
* poll.
|
||||
*
|
||||
* Two halves, mirroring the module:
|
||||
* Three halves, mirroring the module:
|
||||
*
|
||||
* 1. The PURE half — the fixed argv, the failure classification, and the two reads the interval scheduler
|
||||
* depends on: what the settings say (`gitFetchSettingsFrom`) and what a run did
|
||||
* (`gitFetchStatusFrom`). No spawning.
|
||||
* 1. The PURE half — the fixed argv, the `-c` overrides the runner adds ahead of it, the failure
|
||||
* classification, and the two reads the interval scheduler depends on: what the settings say
|
||||
* (`gitFetchSettingsFrom`) and what a run did (`gitFetchStatusFrom`). No spawning.
|
||||
* 2. The IO half — a REAL git fetching a REAL local bare repo, so that "remote-tracking refs only" is
|
||||
* proven against git's actual behavior: HEAD, the index and the working tree must be exactly what they
|
||||
* were while the remote-tracking ref moves. The rest of the half uses an injected runner over real
|
||||
* repositories on disk, because what is being pinned is WHICH repositories git is allowed to touch.
|
||||
* 3. The join (`fetchWorkspaceOnce`) — one run per folder at a time, because two fetches in one clone
|
||||
* collide on git's ref lock, and the bound is ONE limiter shared with the scheduler's tick rather than a
|
||||
* bound each run gets for itself.
|
||||
*
|
||||
* Port: N/A (no server).
|
||||
*/
|
||||
@@ -23,14 +27,17 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
import { classifyGitFailure, isGitAvailable } from '../src/git-clone.js';
|
||||
import {
|
||||
FETCH_CONCURRENCY,
|
||||
FETCH_CONFIG_ARGS,
|
||||
buildFetchArgs,
|
||||
clearGitFetchLane,
|
||||
describeFetchFailure,
|
||||
fetchGitWorkspaceOverview,
|
||||
fetchWorkspaceOnce,
|
||||
gitFetchSettingsFrom,
|
||||
gitFetchStatusFrom,
|
||||
type GitFetchReport,
|
||||
} from '../src/git-fetch.js';
|
||||
import { runGit, type GitRunner } from '../src/git-workspace-status.js';
|
||||
import { createLimiter, runGit, type GitRunner } from '../src/git-workspace-status.js';
|
||||
|
||||
const ENV = {
|
||||
...process.env,
|
||||
@@ -175,6 +182,36 @@ describe('gitFetchStatusFrom', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('FETCH_CONFIG_ARGS', () => {
|
||||
it('turns off the hooks and the maintenance a fetch can otherwise start, pinned without spawning git', () => {
|
||||
expect(FETCH_CONFIG_ARGS).toEqual([
|
||||
'-c',
|
||||
'core.fsmonitor=false',
|
||||
'-c',
|
||||
'maintenance.auto=false',
|
||||
'-c',
|
||||
'gc.auto=0',
|
||||
]);
|
||||
// Read as pairs: a `-c` and the setting it overrides, and every override is one of these three.
|
||||
for (let i = 0; i < FETCH_CONFIG_ARGS.length; i += 2) {
|
||||
expect(FETCH_CONFIG_ARGS[i]).toBe('-c');
|
||||
expect(FETCH_CONFIG_ARGS[i + 1]).toMatch(/^(core\.fsmonitor|maintenance\.auto|gc\.auto)=/);
|
||||
}
|
||||
expect(FETCH_CONFIG_ARGS.filter((a) => a !== '-c').sort()).toEqual([
|
||||
'core.fsmonitor=false',
|
||||
'gc.auto=0',
|
||||
'maintenance.auto=false',
|
||||
]);
|
||||
});
|
||||
|
||||
it('is kept apart from the pinned argv so both can be read without spawning, and the runner composes them in that order', () => {
|
||||
// The argv the recorded calls pin stays the verb alone: the overrides come from the runner, not from here.
|
||||
for (const arg of buildFetchArgs()) expect(arg).not.toBe('-c');
|
||||
// Overrides before the verb, which is where git will take them.
|
||||
expect([...FETCH_CONFIG_ARGS, ...buildFetchArgs()]).toEqual([...FETCH_CONFIG_ARGS, 'fetch', '--quiet']);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Real git, real local repository ─────────────────────────────────────────
|
||||
|
||||
const gitPresent = isGitAvailable();
|
||||
@@ -347,3 +384,141 @@ describe.skipIf(!gitPresent)('fetchGitWorkspaceOverview (real git)', () => {
|
||||
expect(calls.filter(([, args]) => args[0] === 'fetch')).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── One run per folder, and one bound for the whole tick ────────────────────
|
||||
|
||||
describe.skipIf(!gitPresent)('fetchWorkspaceOnce (the join)', () => {
|
||||
// The resolver still runs real git, so these need real repositories on disk; what is injected is only the
|
||||
// fetch, because what is being pinned here is the joining, not git's answer.
|
||||
beforeEach(() => {
|
||||
// The lane is module state: a run left in it by the previous test would be joined by the next one.
|
||||
clearGitFetchLane();
|
||||
});
|
||||
|
||||
it('joins a run already going for the same folder instead of starting a second git fetch in the same clone', async () => {
|
||||
const folder = join(dir, 'folder');
|
||||
initRepo(join(folder, 'api'));
|
||||
|
||||
let releaseRun = () => {};
|
||||
const held = new Promise<void>((resolve) => (releaseRun = resolve));
|
||||
let running = () => {};
|
||||
const runStarted = new Promise<void>((resolve) => (running = resolve));
|
||||
let firstRunFetches = 0;
|
||||
const firstRunner: GitRunner = async (cwd, args) => {
|
||||
if (args[0] === 'fetch') {
|
||||
firstRunFetches++;
|
||||
running();
|
||||
await held;
|
||||
}
|
||||
return '';
|
||||
};
|
||||
const first = fetchWorkspaceOnce(folder, { git: firstRunner });
|
||||
await runStarted; // the run is going, and still going
|
||||
|
||||
// A click while that run is working the folder: it joins, so it never touches git at all — not even the
|
||||
// resolver's own probe, because it does not resolve anything for itself.
|
||||
let joinerCalls = 0;
|
||||
const joiner = fetchWorkspaceOnce(folder, {
|
||||
git: async () => {
|
||||
joinerCalls++;
|
||||
return '';
|
||||
},
|
||||
});
|
||||
|
||||
releaseRun();
|
||||
const answer = await first;
|
||||
// The joiner gets that same run's answer, including its limits: the point is not to fetch twice.
|
||||
expect(await joiner).toBe(answer);
|
||||
expect(joinerCalls).toBe(0);
|
||||
expect(firstRunFetches).toBe(1);
|
||||
});
|
||||
|
||||
it('keys the join on the resolved folder, so the same folder written two ways is one run', async () => {
|
||||
const folder = join(dir, 'folder');
|
||||
initRepo(join(folder, 'api'));
|
||||
|
||||
let releaseRun = () => {};
|
||||
const held = new Promise<void>((resolve) => (releaseRun = resolve));
|
||||
let running = () => {};
|
||||
const runStarted = new Promise<void>((resolve) => (running = resolve));
|
||||
let fetches = 0;
|
||||
const firstRunner: GitRunner = async (cwd, args) => {
|
||||
if (args[0] === 'fetch') {
|
||||
fetches++;
|
||||
running();
|
||||
await held;
|
||||
}
|
||||
return '';
|
||||
};
|
||||
const first = fetchWorkspaceOnce(folder, { git: firstRunner });
|
||||
await runStarted;
|
||||
// Two sessions on one folder reach it by different spellings; the scheduler keys by `resolve`.
|
||||
let joinerCalls = 0;
|
||||
const second = fetchWorkspaceOnce(join(dir, 'folder', '.'), {
|
||||
git: async () => {
|
||||
joinerCalls++;
|
||||
return '';
|
||||
},
|
||||
});
|
||||
releaseRun();
|
||||
expect(await second).toBe(await first);
|
||||
expect(joinerCalls).toBe(0);
|
||||
expect(fetches).toBe(1);
|
||||
});
|
||||
|
||||
it('releases the folder when a run lands, so the next click is a new run and not an old answer', async () => {
|
||||
const folder = join(dir, 'folder');
|
||||
initRepo(join(folder, 'api'));
|
||||
let fetches = 0;
|
||||
const runner: GitRunner = async (cwd, args) => {
|
||||
if (args[0] === 'fetch') fetches++;
|
||||
return '';
|
||||
};
|
||||
await fetchWorkspaceOnce(folder, { git: runner });
|
||||
expect(fetches).toBe(1);
|
||||
await fetchWorkspaceOnce(folder, { git: runner });
|
||||
expect(fetches).toBe(2);
|
||||
});
|
||||
|
||||
it('bounds a whole tick with the limiter it is handed, because two runs given a bound each would get twice the bound', async () => {
|
||||
const left = join(dir, 'left');
|
||||
const right = join(dir, 'right');
|
||||
for (let i = 0; i < FETCH_CONCURRENCY + 1; i++) {
|
||||
initRepo(join(left, `l${i}`));
|
||||
initRepo(join(right, `r${i}`));
|
||||
}
|
||||
const held = () => new Promise((resolve) => setTimeout(resolve, 10));
|
||||
// Only the fetches go through the limiter, so only they are counted here: the resolver's own probe is
|
||||
// not a bounded git process.
|
||||
const counting =
|
||||
(state: { live: number; peak: number }): GitRunner =>
|
||||
async (cwd, args) => {
|
||||
if (args[0] !== 'fetch') return '';
|
||||
state.live++;
|
||||
state.peak = Math.max(state.peak, state.live);
|
||||
await held();
|
||||
state.live--;
|
||||
return '';
|
||||
};
|
||||
|
||||
// The scheduler hands every folder in the tick the SAME limiter, so the bound is the tick's, not the run's.
|
||||
const shared = { live: 0, peak: 0 };
|
||||
const sharedRunner = counting(shared);
|
||||
const limiter = createLimiter(FETCH_CONCURRENCY);
|
||||
await Promise.all([
|
||||
fetchGitWorkspaceOverview(left, { git: sharedRunner, limiter }),
|
||||
fetchGitWorkspaceOverview(right, { git: sharedRunner, limiter }),
|
||||
]);
|
||||
expect(shared.peak).toBe(FETCH_CONCURRENCY);
|
||||
|
||||
// Without a shared limiter each run bounds only its own call, which is the failure the scheduler is there
|
||||
// to prevent: two folders at FETCH_CONCURRENCY each, so twice as many git processes as the rule says.
|
||||
const alone = { live: 0, peak: 0 };
|
||||
const aloneRunner = counting(alone);
|
||||
await Promise.all([
|
||||
fetchGitWorkspaceOverview(left, { git: aloneRunner }),
|
||||
fetchGitWorkspaceOverview(right, { git: aloneRunner }),
|
||||
]);
|
||||
expect(alone.peak).toBe(2 * FETCH_CONCURRENCY);
|
||||
});
|
||||
});
|
||||
@@ -11,6 +11,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
import { registerGitStatusRoutes } from '../../src/web/routes/git-status-routes.js';
|
||||
import { clearGitStatusCache, runGit, type GitRunner } from '../../src/git-workspace-status.js';
|
||||
import { clearGitFetchLane } from '../../src/git-fetch.js';
|
||||
|
||||
const ENV = {
|
||||
...process.env,
|
||||
@@ -44,6 +45,8 @@ async function setup(
|
||||
|
||||
beforeEach(() => {
|
||||
clearGitStatusCache();
|
||||
// The fetch lane is module state: a run left in it by the previous test would be joined by the next one.
|
||||
clearGitFetchLane();
|
||||
dir = mkdtempSync(join(tmpdir(), 'git-status-route-'));
|
||||
});
|
||||
afterEach(() => {
|
||||
@@ -467,6 +470,54 @@ describe('POST /api/sessions/:id/git-fetch', () => {
|
||||
expect(fetches()).toEqual([]);
|
||||
});
|
||||
|
||||
it("publishes what the click got as the session's own fetch state, naming the trigger the user asked for", async () => {
|
||||
git(dir, 'init', '-q', '-b', 'main');
|
||||
const { runner } = recordingRunner();
|
||||
const { app, ctx } = await setup({ git: runner });
|
||||
const res = await app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
expect(res.statusCode).toBe(200);
|
||||
// The response body is one tab's copy of the run; the state is what every other tab open on the same
|
||||
// folder sees, and a number the user did not ask for has to read differently from one they did.
|
||||
expect(session.gitFetch).toMatchObject({ trigger: 'click', state: 'ok', repos: 1, failed: 0 });
|
||||
expect(ctx.persistSessionState).toHaveBeenCalled();
|
||||
expect(ctx.broadcast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('joins a run already going for the same folder instead of starting a second git process in the same clone', async () => {
|
||||
git(dir, 'init', '-q', '-b', 'main');
|
||||
let releaseRun = () => {};
|
||||
const held = new Promise<void>((resolve) => (releaseRun = resolve));
|
||||
let running = () => {};
|
||||
const runStarted = new Promise<void>((resolve) => (running = resolve));
|
||||
const fetches: string[] = [];
|
||||
const runner: GitRunner = async (cwd, args) => {
|
||||
if (args[0] === 'fetch') {
|
||||
fetches.push(cwd);
|
||||
running();
|
||||
await held;
|
||||
}
|
||||
return execFileSyncGit(cwd, args);
|
||||
};
|
||||
const { app } = await setup({ git: runner });
|
||||
|
||||
const first = app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
await runStarted; // the run is going, and still going
|
||||
// A click from another tab open on the same clone. Two fetches there would collide on git's ref lock, so
|
||||
// this one joins the run already going rather than starting its own.
|
||||
const second = app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
// Assert the join BEFORE releasing the run: the lane entry goes away when the run finishes, so releasing
|
||||
// first would let the run complete before this request reached the lane, and it would then be entitled to
|
||||
// start its own fetch — a test that passes for the wrong reason. One tick of the loop is enough for both
|
||||
// handlers to reach the lane while the run is still held open.
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
expect(fetches).toHaveLength(1); // one run, and the second request is waiting on it, not working
|
||||
|
||||
releaseRun();
|
||||
const a = (await first).json().data;
|
||||
const b = (await second).json().data;
|
||||
expect(b).toEqual(a); // both tabs get the same run's answer, not a partial one
|
||||
});
|
||||
|
||||
it('404s an unknown session, running no fetch', async () => {
|
||||
const { calls, fetches, runner } = recordingRunner();
|
||||
const { app } = await setup({ git: runner });
|
||||
|
||||
@@ -88,29 +88,36 @@ describe('Session Options modal structure', () => {
|
||||
expect(modal.match(/<button[^>]*data-tab="context"[^>]*>/)?.[0]).not.toContain('data-claude-only');
|
||||
});
|
||||
|
||||
it('keeps the Git rail entry for every local case, and hides it for the ones nothing is ever fetched for', () => {
|
||||
it('keeps the Git rail entry for every local session, and hides it for the ones nothing is ever fetched for', () => {
|
||||
const modal = optionsModal();
|
||||
// A shell case has repositories too, so Git must not ride the Claude-only flag.
|
||||
// A shell session has repositories too, so Git must not ride the Claude-only flag.
|
||||
expect(modal.match(/<button[^>]*data-tab="git"[^>]*>/)?.[0]).not.toContain('data-claude-only');
|
||||
const open = methodBody('openSessionOptions(sessionId)');
|
||||
expect(open).toContain('.set-rail-item[data-tab="git"]');
|
||||
// Remote and Docker cases answer 'unsupported' on both git routes and the scheduler never fetches them,
|
||||
// Remote and Docker sessions answer 'unsupported' on both git routes and the scheduler never fetches them,
|
||||
// so the entry goes the same way the Claude-only ones do.
|
||||
expect(open).toMatch(/gitTabBtn\.style\.display = session\.remote \|\| session\.docker \? 'none' : ''/);
|
||||
});
|
||||
|
||||
it('makes the Git rows inert while a case follows the app, the way the keep-warm rows go inert', () => {
|
||||
it('makes the Git rows inert while a session follows the app, the way the keep-warm rows go inert', () => {
|
||||
const modal = optionsModal();
|
||||
const update = methodBody('updateGitFetchStatus(sessionId)');
|
||||
expect(update).toContain('set-row-disabled');
|
||||
expect(update).toContain('el.disabled = follows');
|
||||
// Decided from BOTH fields: a half-choice means the case is not following, and the half that still follows
|
||||
// shows the app's value while the other shows its own — a switch-only check would gray out a live row.
|
||||
// Decided from BOTH fields: a half-choice means the session is not following, and the half that still
|
||||
// follows shows the app's value while the other shows its own — a switch-only check would gray out a live row.
|
||||
expect(update).toMatch(
|
||||
/follows = session\.gitFetchEnabled === undefined && session\.gitFetchIntervalSeconds === undefined/
|
||||
);
|
||||
// Following saves null on both halves, which is what clears the override on the route.
|
||||
expect(methodBody('async autoSaveGitFetch()')).toContain('{ enabled: null, intervalSeconds: null }');
|
||||
const save = methodBody('async autoSaveGitFetch()');
|
||||
expect(save).toContain('{ enabled: null, intervalSeconds: null }');
|
||||
// Saves through the helper that says whether the call went through: `_apiPost` hands back the Response
|
||||
// without checking it, so a body the route refused (a 400) read as a save, and the panel showed a choice
|
||||
// the server never took.
|
||||
expect(save).toContain('this._apiJson(`/api/sessions/${this.editingSessionId}/git-fetch-settings`');
|
||||
expect(save).toContain('if (!data)');
|
||||
expect(save).not.toMatch(/await this\._apiPost\(/);
|
||||
// The status row is runtime text this method writes, so translating it would rewrite the stamp.
|
||||
expect(modal).toContain('id="gitFetchStatus" data-i18n-skip hidden');
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user