mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-11-11 08:37:35 +00:00
fix(cli): reap embedding sidecar after query (#3693)
* fix(cli): reap embedding sidecar after query * fix(cli): drain hybrid search before cleanup * fix(cli): reap sidecar after group query --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
This commit is contained in:
parent
0f3bad23b9
commit
11a49a15c6
6 changed files with 133 additions and 13 deletions
|
|
@ -514,7 +514,7 @@ export function registerGroupCommands(program: Command): void {
|
|||
}
|
||||
}
|
||||
} finally {
|
||||
await backend.dispose().catch(() => {});
|
||||
await backend.disconnect().catch(() => {});
|
||||
}
|
||||
},
|
||||
);
|
||||
|
|
|
|||
|
|
@ -142,17 +142,28 @@ export async function queryCommand(
|
|||
}
|
||||
|
||||
const backend = await getBackend();
|
||||
const result = await backend.callTool('query', {
|
||||
// #2175: canonical param is search_query; the backend still accepts legacy "query".
|
||||
search_query: resolvedQuery,
|
||||
task_context: options?.context,
|
||||
goal: options?.goal,
|
||||
limit: parseLimit(options?.limit),
|
||||
include_content: options?.content ?? false,
|
||||
repo: options?.repo,
|
||||
branch: options?.branch,
|
||||
});
|
||||
output(result);
|
||||
try {
|
||||
const result = await backend.callTool('query', {
|
||||
// #2175: canonical param is search_query; the backend still accepts legacy "query".
|
||||
search_query: resolvedQuery,
|
||||
task_context: options?.context,
|
||||
goal: options?.goal,
|
||||
limit: parseLimit(options?.limit),
|
||||
include_content: options?.content ?? false,
|
||||
repo: options?.repo,
|
||||
branch: options?.branch,
|
||||
});
|
||||
output(result);
|
||||
} finally {
|
||||
// A local embedding query may start the sidecar. Direct CLI commands are
|
||||
// one-shot, so leaving the backend connected keeps that child (and Node's
|
||||
// event loop) alive after the complete result has already been printed.
|
||||
try {
|
||||
await backend.disconnect();
|
||||
} finally {
|
||||
_backend = null;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export async function contextCommand(
|
||||
|
|
|
|||
|
|
@ -3354,10 +3354,19 @@ export class LocalBackend {
|
|||
const searchLimit = processLimit * maxSymbolsPerProcess; // fetch enough raw results
|
||||
const ftsDisabledReason = getFtsDisabledReason(meta?.capabilities?.fts);
|
||||
const vectorDegraded = { reason: undefined as string | undefined };
|
||||
const [bm25SearchResult, semanticResults] = await Promise.all([
|
||||
const [bm25Outcome, semanticOutcome] = await Promise.allSettled([
|
||||
timer.time('bm25', this.bm25Search(repo, searchQuery, searchLimit, ftsDisabledReason)),
|
||||
timer.time('vector', this.semanticSearch(repo, searchQuery, searchLimit, vectorDegraded)),
|
||||
]);
|
||||
// A rejected lane must not let the other lane outlive query(). In particular,
|
||||
// semantic search may still be importing the embedder and can start its
|
||||
// sidecar after a one-shot CLI has already run disconnect() in `finally`.
|
||||
// Settle both lanes first, then preserve Promise.all's left-to-right error
|
||||
// precedence for callers.
|
||||
if (bm25Outcome.status === 'rejected') throw bm25Outcome.reason;
|
||||
if (semanticOutcome.status === 'rejected') throw semanticOutcome.reason;
|
||||
const bm25SearchResult = bm25Outcome.value;
|
||||
const semanticResults = semanticOutcome.value;
|
||||
|
||||
// Guard against undefined results (#1489) — when FTS is entirely
|
||||
// unavailable the search helper may return an unexpected shape.
|
||||
|
|
|
|||
53
gitnexus/test/unit/group-query-cli-cleanup.test.ts
Normal file
53
gitnexus/test/unit/group-query-cli-cleanup.test.ts
Normal file
|
|
@ -0,0 +1,53 @@
|
|||
import { Command } from 'commander';
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
const initMock = vi.fn();
|
||||
const groupQueryMock = vi.fn();
|
||||
const disconnectMock = vi.fn();
|
||||
const disposeMock = vi.fn();
|
||||
|
||||
vi.mock('../../src/mcp/local/local-backend.js', () => ({
|
||||
LocalBackend: class {
|
||||
init = initMock;
|
||||
disconnect = disconnectMock;
|
||||
dispose = disposeMock;
|
||||
getGroupService() {
|
||||
return { groupQuery: groupQueryMock };
|
||||
}
|
||||
},
|
||||
}));
|
||||
|
||||
describe('group query CLI cleanup', () => {
|
||||
beforeEach(() => {
|
||||
initMock.mockReset().mockResolvedValue(true);
|
||||
groupQueryMock.mockReset().mockResolvedValue({ results: [], per_repo: [] });
|
||||
disconnectMock.mockReset().mockResolvedValue(undefined);
|
||||
disposeMock.mockReset().mockResolvedValue(undefined);
|
||||
vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||
});
|
||||
|
||||
it('disconnects the backend after a successful query', async () => {
|
||||
const { registerGroupCommands } = await import('../../src/cli/group.js');
|
||||
const program = new Command().exitOverride();
|
||||
registerGroupCommands(program);
|
||||
|
||||
await program.parseAsync(['node', 'gitnexus', 'group', 'query', 'team', 'auth flow']);
|
||||
|
||||
expect(disconnectMock).toHaveBeenCalledOnce();
|
||||
expect(disposeMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('disconnects the backend when a group query fails', async () => {
|
||||
groupQueryMock.mockRejectedValue(new Error('query failed'));
|
||||
const { registerGroupCommands } = await import('../../src/cli/group.js');
|
||||
const program = new Command().exitOverride();
|
||||
registerGroupCommands(program);
|
||||
|
||||
await expect(
|
||||
program.parseAsync(['node', 'gitnexus', 'group', 'query', 'team', 'auth flow']),
|
||||
).rejects.toThrow('query failed');
|
||||
|
||||
expect(disconnectMock).toHaveBeenCalledOnce();
|
||||
expect(disposeMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -83,6 +83,29 @@ describe('query: degraded-enrichment signal', () => {
|
|||
beforeEach(() => vi.clearAllMocks());
|
||||
afterEach(() => vi.unstubAllEnvs());
|
||||
|
||||
it('drains semantic search before returning a BM25 failure', async () => {
|
||||
const b = makeBackend(true);
|
||||
let finishSemantic!: () => void;
|
||||
const semanticPending = new Promise<void>((resolve) => {
|
||||
finishSemantic = resolve;
|
||||
});
|
||||
(b.backend as any).bm25Search = vi.fn().mockRejectedValue(new Error('invalid symbol row'));
|
||||
(b.backend as any).semanticSearch = vi.fn().mockImplementation(async () => {
|
||||
await semanticPending;
|
||||
return [];
|
||||
});
|
||||
|
||||
let querySettled = false;
|
||||
const query = runQuery(b).finally(() => {
|
||||
querySettled = true;
|
||||
});
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
expect(querySettled).toBe(false);
|
||||
finishSemantic();
|
||||
await expect(query).rejects.toThrow('invalid symbol row');
|
||||
});
|
||||
|
||||
it('a REAL enrichment failure surfaces warning + partial, and still returns the symbol', async () => {
|
||||
const b = makeBackend(true);
|
||||
executeParameterizedMock.mockImplementation(async (_repo: string, query: string) => {
|
||||
|
|
|
|||
|
|
@ -2,12 +2,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
|||
|
||||
const initMock = vi.fn();
|
||||
const callToolMock = vi.fn();
|
||||
const disconnectMock = vi.fn();
|
||||
const writeSyncMock = vi.fn();
|
||||
|
||||
vi.mock('../../src/mcp/local/local-backend.js', () => ({
|
||||
LocalBackend: class {
|
||||
init = initMock;
|
||||
callTool = callToolMock;
|
||||
disconnect = disconnectMock;
|
||||
},
|
||||
}));
|
||||
|
||||
|
|
@ -22,9 +24,11 @@ describe('direct CLI tool commands', () => {
|
|||
vi.resetModules();
|
||||
initMock.mockReset();
|
||||
callToolMock.mockReset();
|
||||
disconnectMock.mockReset();
|
||||
writeSyncMock.mockReset();
|
||||
process.exitCode = undefined;
|
||||
initMock.mockResolvedValue(true);
|
||||
disconnectMock.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
// These commands set `process.exitCode` on the real process. Clearing it after
|
||||
|
|
@ -164,6 +168,26 @@ describe('direct CLI tool commands', () => {
|
|||
|
||||
expect(writeSyncMock).toHaveBeenCalledWith(1, expect.stringContaining('not found'));
|
||||
expect(process.exitCode).toBe(1);
|
||||
expect(disconnectMock).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it('disconnects after a successful query so the embedding sidecar cannot hold the CLI open', async () => {
|
||||
callToolMock.mockResolvedValue({ definitions: [], processes: [] });
|
||||
const { queryCommand } = await import('../../src/cli/tool.js');
|
||||
|
||||
await queryCommand('auth flow');
|
||||
|
||||
expect(writeSyncMock).toHaveBeenCalledWith(1, expect.stringContaining('"definitions": []'));
|
||||
expect(disconnectMock).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it('disconnects when a query throws', async () => {
|
||||
callToolMock.mockRejectedValue(new Error('embedding failed'));
|
||||
const { queryCommand } = await import('../../src/cli/tool.js');
|
||||
|
||||
await expect(queryCommand('auth flow')).rejects.toThrow('embedding failed');
|
||||
|
||||
expect(disconnectMock).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
// `partial` is cross-tool vocabulary, not detect_changes' private flag, and the
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue