Skip to content

Commit b879988

Browse files
committed
perf(sync): read the team's MCP history once per pull, in one git call (#993)
historicalContents read each historical blob with its own git process (about 370 ms for 60 versions); it now reads them all with one git cat-file --batch (about 30 ms). The MCP reconcile reads that history once per run for every target, and only when a target file holds an unrecorded entry; with no team server, nothing recorded and no server in any target file it returns without reading it. A dry run names an unrecorded copy of a server the team deleted that a pull would remove.
1 parent 3a4cf2f commit b879988

3 files changed

Lines changed: 82 additions & 15 deletions

File tree

‎src/__tests__/e2e/config-entry-ownership-993.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,11 @@ describe('ownership of unrecorded MCP servers and hook entries (#993 bug 12)', (
261261
init(t, pulled, 'claude');
262262
removeMcpManifests();
263263
t.publish({ 'mcp/mcp.yaml': 'servers: []\n' }, 'drop');
264+
const preview = pull(pulled, '--dry-run');
265+
expect(preview.output).toContain(
266+
`Would remove MCP server plain-api from ${path.join(pulled, '.mcp.json')}: it equals a server the team has removed.`,
267+
);
268+
expect(mcpServer(pulled, 'plain-api')).toEqual({ type: 'http', url: 'https://team.example.com/v1' });
264269
pull(pulled);
265270
expect(mcpServer(pulled, 'plain-api')).toBeUndefined();
266271

@@ -282,6 +287,18 @@ describe('ownership of unrecorded MCP servers and hook entries (#993 bug 12)', (
282287
expect(mcpServer(dir, 'plain-api')).toEqual(mine);
283288
});
284289

290+
it('keeps a member\'s server whose name the team history never had', () => {
291+
const t = team('never-team', { 'mcp/mcp.yaml': mcpYaml('https://team.example.com/v1') });
292+
const mine = { type: 'http', url: 'https://mine.example.com/mcp' };
293+
const dir = business('never-team-biz', { '.mcp.json': JSON.stringify({ mcpServers: { 'my-own': mine } }) });
294+
init(t, dir, 'claude');
295+
removeMcpManifests();
296+
t.publish({ 'mcp/mcp.yaml': 'servers: []\n' }, 'drop');
297+
pull(dir);
298+
expect(mcpServer(dir, 'plain-api')).toBeUndefined();
299+
expect(mcpServer(dir, 'my-own')).toEqual(mine);
300+
});
301+
285302
it('removes an unrecorded copy of a deleted server from Codex config too', () => {
286303
const t = team('removed-codex', { 'mcp/mcp.yaml': mcpYaml('https://team.example.com/v1') });
287304
const dir = business('removed-codex-biz', { '.codex/.keep': '' });

‎src/mcp-reconcile.ts‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@ export interface McpChange {
122122

123123
const UNRECORDED_SERVER_REASON = 'a server with this name already exists and is not managed by teamai';
124124

125+
/** The dry-run line for an unrecorded copy of a server the team removed, which a run would remove (#993). */
126+
function describeRemovedServerPreview(target: McpTarget, name: string): string {
127+
return `Would remove MCP server ${name} from ${target.file}: it equals a server the team has removed.`;
128+
}
129+
125130
/** The dry-run line for an unrecorded server pull would record as teamai's without rewriting it (#993). */
126131
function describeAdoptionPreview(target: McpTarget, name: string): string {
127132
return `Would record MCP server ${name} in ${target.file} as teamai's: it already holds the team's ${name}.`;
@@ -726,13 +731,14 @@ function renderMcpEntry(
726731
return { entry: { entry, hash: entryHash(entry), resolvedValue }, passthrough };
727732
}
728733

729-
/** Whether any revision of the team repo's MCP files defines a server (#993). Never for an HTTP-mode team. */
730-
async function teamMcpHistoryHasServers(localConfig: LocalConfig): Promise<boolean> {
731-
if (localConfig.repo.kind === 'http') return false;
732-
const layout = entryLayout('mcp');
733-
const versions = await historicalContents(localConfig.repo.localPath, layout.dir);
734-
return (versions ?? []).some((version) => path.posix.basename(version.path) === layout.file
735-
&& (parseTeamMcpServers(version.content.toString('utf8'))?.length ?? 0) > 0);
734+
/** Every revision of the team repo's MCP files (#993), or null when unreadable or an HTTP-mode team. */
735+
type TeamMcpHistory = Array<{ path: string; content: Buffer }> | null;
736+
737+
/** The team's MCP history, read once, on first use. One per reconcile run, shared by every target. */
738+
function teamMcpHistory(localConfig: LocalConfig): () => Promise<TeamMcpHistory> {
739+
return once(async () => (localConfig.repo.kind === 'http'
740+
? null
741+
: historicalContents(localConfig.repo.localPath, entryLayout('mcp').dir)));
736742
}
737743

738744
/** Whose an entry is that `target`'s own MCP record does not claim (#993). */
@@ -770,11 +776,11 @@ function judgeUnrecordedMcpEntry(
770776
desired: ReadonlyMap<string, DesiredMcpEntry>,
771777
vars: Record<string, string>,
772778
claimed: ReadonlySet<string>,
779+
history: () => Promise<TeamMcpHistory> = teamMcpHistory(localConfig),
773780
): McpEntryJudge {
774781
const renders = once(async (): Promise<Map<string, unknown[]> | null> => {
775-
if (localConfig.repo.kind === 'http') return null;
776782
const layout = entryLayout('mcp');
777-
const versions = await historicalContents(localConfig.repo.localPath, layout.dir);
783+
const versions = await history();
778784
if (versions === null) return null;
779785
const entries = new Map<string, unknown[]>();
780786
for (const version of versions) {
@@ -978,6 +984,14 @@ export async function unclaimedMcpServers(target: McpTarget, claimed: readonly s
978984
return unclaimed.length === 0 || (await gitTracks(target.file)).kind === 'tracked' ? [] : unclaimed;
979985
}
980986

987+
/** Whether any target's file holds an MCP server, recorded or not. */
988+
async function someMcpEntryInstalled(targets: readonly McpTarget[]): Promise<boolean> {
989+
for (const target of targets) {
990+
if (((await installedMcpEntries(target))?.size ?? 0) > 0) return true;
991+
}
992+
return false;
993+
}
994+
981995
/** `load`, run once, on the first call. */
982996
function once<T>(load: () => Promise<T>): () => Promise<T> {
983997
let value: Promise<T> | undefined;
@@ -1833,9 +1847,10 @@ async function reconcileTargets(
18331847
// An empty desired set still has to run: it is how servers dropped from
18341848
// mcp.yaml get cleaned out of the tools we previously injected them into.
18351849
const nothingOwned = Object.values(manifest).every((r) => r.length === 0);
1836-
// A server the team deleted can still sit unrecorded in a tool's file (#993): only a team
1837-
// history with no MCP server at all proves there is nothing to look for.
1838-
if (teamDefs.length === 0 && nothingOwned && !await teamMcpHistoryHasServers(localConfig)) return { changes, wrote };
1850+
// A server the team deleted can still sit unrecorded in a tool's file (#993): only target
1851+
// files with no server at all prove there is nothing to look for, without reading the history.
1852+
if (teamDefs.length === 0 && nothingOwned && !await someMcpEntryInstalled(targets)) return { changes, wrote };
1853+
const history = teamMcpHistory(localConfig);
18391854
// The files an earlier pull recorded, and each record this run rebuilds after it was lost (#882).
18401855
const ledger = localConfig.scope === 'project' && !options.dryRun ? (await readResolvedMcpFiles(localConfig)).files : {};
18411856
const listed = new Set(Object.keys(ledger));
@@ -1864,7 +1879,7 @@ async function reconcileTargets(
18641879
// Which of this team's servers apply to this tool, and in what rendered form.
18651880
const { desired, skipped, kept } = desiredMcpForTarget(resolved, teamDefs, desiredContext);
18661881
changes.push(...skipped);
1867-
const judge = judgeUnrecordedMcpEntry(localConfig, resolved, desired, desiredContext.vars, claimedByOtherTools(targets, resolved, manifest));
1882+
const judge = judgeUnrecordedMcpEntry(localConfig, resolved, desired, desiredContext.vars, claimedByOtherTools(targets, resolved, manifest), history);
18681883
// A file an earlier teamai created that hides a later one, holding only teamai's servers, is left (#993).
18691884
const leaving = removeAll ? null : await leaveFormerMcpFile(resolved, manifest[manifestKey] ?? [], judge);
18701885
const target = leaving ? { ...resolved, file: leaving.next } : resolved;
@@ -2147,6 +2162,7 @@ async function applyJson(
21472162
// server from the team history (#993), and goes like any other server teamai no longer delivers.
21482163
for (const [name, entry] of Object.entries(doc.servers)) {
21492164
if (desired.has(name) || ownedNames.has(name) || await judge(name, entry) !== 'teamai') continue;
2165+
if (options.dryRun) log.info(describeRemovedServerPreview(target, name));
21502166
delete doc.servers[name];
21512167
dirty = true;
21522168
changes.push({ tool: target.tool, server: name, action: 'removed' });
@@ -2336,6 +2352,7 @@ async function applyCodex(
23362352
if (desired.has(name) || ownedNames.has(name) || await judge(name, codexBlockIn(source, name)) !== 'teamai') continue;
23372353
const next = spliceCodexBlock(source, name, null);
23382354
if (next === source) continue;
2355+
if (options.dryRun) log.info(describeRemovedServerPreview(target, name));
23392356
source = next;
23402357
dirty = true;
23412358
changes.push({ tool: target.tool, server: name, action: 'removed' });

‎src/utils/team-history.ts‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { execFile } from 'node:child_process';
12
import { createHash } from 'node:crypto';
23
import { createGit } from './git.js';
34
import { log } from './logger.js';
@@ -113,10 +114,42 @@ export async function historicalContents(
113114
): Promise<Array<HistoricalVersion & { content: Buffer }> | null> {
114115
const versions = await historicalVersions(repoPath, pathspec);
115116
if (versions === null) return null;
117+
const blobs = await readBlobs(repoPath, [...new Set(versions.map((version) => version.blob))]);
116118
const contents: Array<HistoricalVersion & { content: Buffer }> = [];
117119
for (const version of versions) {
118-
const content = await readBlob(repoPath, version.blob);
119-
if (content !== null) contents.push({ ...version, content });
120+
const content = blobs.get(version.blob);
121+
if (content !== undefined) contents.push({ ...version, content });
120122
}
121123
return contents;
122124
}
125+
126+
/**
127+
* The bytes of each of `blobs` in `repoPath`, read by one `git cat-file --batch`
128+
* rather than a git process per blob. A blob git does not have is left out.
129+
*/
130+
async function readBlobs(repoPath: string, blobs: readonly string[]): Promise<Map<string, Buffer>> {
131+
const found = new Map<string, Buffer>();
132+
if (blobs.length === 0) return found;
133+
const out = await new Promise<Buffer | null>((resolve) => {
134+
const child = execFile('git', ['-C', repoPath, 'cat-file', '--batch'], { encoding: 'buffer', maxBuffer: 256 * 1024 * 1024 },
135+
(error, stdout) => resolve(error ? null : stdout));
136+
child.stdin?.end(`${blobs.join('\n')}\n`);
137+
});
138+
if (out === null) {
139+
log.debug(`Could not read ${blobs.length} historical blob(s) in ${repoPath}`);
140+
return found;
141+
}
142+
// Each answer: `<id> <type> <size>\n<bytes>\n`, or `<id> missing\n`.
143+
let at = 0;
144+
while (at < out.length) {
145+
const eol = out.indexOf(0x0a, at);
146+
if (eol === -1) break;
147+
const [id, type, size] = out.subarray(at, eol).toString('utf8').split(' ');
148+
at = eol + 1;
149+
if (type === 'missing' || size === undefined) continue;
150+
const length = Number(size);
151+
if (type === 'blob') found.set(id, Buffer.from(out.subarray(at, at + length)));
152+
at += length + 1;
153+
}
154+
return found;
155+
}

0 commit comments

Comments
 (0)