From 11a49a15c6befbe4d5de1cebe74b5456aeb44b99 Mon Sep 17 00:00:00 2001 From: Parafee41 Date: Sat, 24 Oct 2026 16:58:58 +0800 Subject: [PATCH] fix(cli): reap embedding sidecar after query (#3693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- gitnexus/src/cli/group.ts | 2 +- gitnexus/src/cli/tool.ts | 33 ++++++++---- gitnexus/src/mcp/local/local-backend.ts | 11 +++- .../test/unit/group-query-cli-cleanup.test.ts | 53 +++++++++++++++++++ .../test/unit/query-degraded-signal.test.ts | 23 ++++++++ gitnexus/test/unit/tool-direct-cli.test.ts | 24 +++++++++ 6 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 gitnexus/test/unit/group-query-cli-cleanup.test.ts diff --git a/gitnexus/src/cli/group.ts b/gitnexus/src/cli/group.ts index 824b13b6a..bfd613d75 100644 --- a/gitnexus/src/cli/group.ts +++ b/gitnexus/src/cli/group.ts @@ -514,7 +514,7 @@ export function registerGroupCommands(program: Command): void { } } } finally { - await backend.dispose().catch(() => {}); + await backend.disconnect().catch(() => {}); } }, ); diff --git a/gitnexus/src/cli/tool.ts b/gitnexus/src/cli/tool.ts index 30574f430..8ddc692dd 100644 --- a/gitnexus/src/cli/tool.ts +++ b/gitnexus/src/cli/tool.ts @@ -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( diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 9b697f79a..7a69b2b23 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -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. diff --git a/gitnexus/test/unit/group-query-cli-cleanup.test.ts b/gitnexus/test/unit/group-query-cli-cleanup.test.ts new file mode 100644 index 000000000..6b9029def --- /dev/null +++ b/gitnexus/test/unit/group-query-cli-cleanup.test.ts @@ -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(); + }); +}); diff --git a/gitnexus/test/unit/query-degraded-signal.test.ts b/gitnexus/test/unit/query-degraded-signal.test.ts index 7e28c5f6a..3ea25660e 100644 --- a/gitnexus/test/unit/query-degraded-signal.test.ts +++ b/gitnexus/test/unit/query-degraded-signal.test.ts @@ -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((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) => { diff --git a/gitnexus/test/unit/tool-direct-cli.test.ts b/gitnexus/test/unit/tool-direct-cli.test.ts index 7a0afd392..99a6e0b16 100644 --- a/gitnexus/test/unit/tool-direct-cli.test.ts +++ b/gitnexus/test/unit/tool-direct-cli.test.ts @@ -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