From 886e3d5e505884e33e479aaf8ca046b643f17bfb Mon Sep 17 00:00:00 2001 From: Gallay Lajos Date: Mon, 1 Jun 2026 11:58:46 +0200 Subject: [PATCH 1/3] feat: Async git improvements --- .../actions/validate-repo-action.spec.ts | 32 +-- .../actions/validate-repo-action.ts | 6 +- .../actions/check-prerequisite-action.spec.ts | 53 ++-- .../actions/check-prerequisite-action.ts | 42 ++- service/src/mcp/tools/repository-tools.ts | 7 +- .../src/services/git-service-runner.spec.ts | 230 +++++++++++++++ service/src/services/git-service.ts | 98 +++++-- service/src/utils/run-cli.spec.ts | 270 ++++++++++++++++++ service/src/utils/run-cli.ts | 142 +++++++++ 9 files changed, 786 insertions(+), 94 deletions(-) create mode 100644 service/src/services/git-service-runner.spec.ts create mode 100644 service/src/utils/run-cli.spec.ts create mode 100644 service/src/utils/run-cli.ts diff --git a/service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts b/service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts index 766603b..60452f6 100644 --- a/service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts +++ b/service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts @@ -2,27 +2,25 @@ import { GitHubRepositoryDataSet } from '../../data-store/tokens.js' import { getDataSetFor } from '@furystack/repository' import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { GitService } from '../../../services/git-service.js' import { createMockActionContext, withTestInjector } from '../../../test-helpers.js' import { ValidateRepoAction } from './validate-repo-action.js' -const execFileMock = vi.hoisted(() => - vi.fn<(cmd: string, args: string[], options: { timeout: number }) => Promise<{ stdout: string; stderr: string }>>(), -) -vi.mock('child_process', () => ({ - execFile: (...args: unknown[]) => execFileMock(...(args as [string, string[], { timeout: number }])), -})) +const lsRemoteMock = vi.fn<(url: string) => Promise>() -vi.mock('util', () => ({ - promisify: () => execFileMock, -})) +const bindGitServiceStub = (injector: { bind: (token: typeof GitService, factory: () => GitService) => void }) => { + injector.bind(GitService, () => ({ lsRemote: lsRemoteMock }) as unknown as GitService) +} describe('ValidateRepoAction', () => { beforeEach(() => { - vi.clearAllMocks() + lsRemoteMock.mockReset() }) it('should return accessible: true when git ls-remote succeeds', async () => { await withTestInjector(async ({ elevated }) => { + bindGitServiceStub(elevated) const ts = new Date().toISOString() await getDataSetFor(elevated, GitHubRepositoryDataSet).add(elevated, { id: 'repo-1', @@ -34,7 +32,7 @@ describe('ValidateRepoAction', () => { updatedAt: ts, }) - execFileMock.mockResolvedValue({ stdout: 'abc123\tHEAD\n', stderr: '' }) + lsRemoteMock.mockResolvedValue(undefined) const result = await ValidateRepoAction( createMockActionContext({ injector: elevated, urlParams: { id: 'repo-1' } }), @@ -42,18 +40,13 @@ describe('ValidateRepoAction', () => { const body = result.chunk as { accessible: boolean } expect(body.accessible).toBe(true) - expect(execFileMock).toHaveBeenCalledWith( - 'git', - ['ls-remote', '--exit-code', 'https://github.com/user/repo.git'], - { - timeout: 15000, - }, - ) + expect(lsRemoteMock).toHaveBeenCalledWith('https://github.com/user/repo.git') }) }) it('should return accessible: false when git ls-remote fails', async () => { await withTestInjector(async ({ elevated }) => { + bindGitServiceStub(elevated) const ts = new Date().toISOString() await getDataSetFor(elevated, GitHubRepositoryDataSet).add(elevated, { id: 'repo-2', @@ -65,7 +58,7 @@ describe('ValidateRepoAction', () => { updatedAt: ts, }) - execFileMock.mockRejectedValue(new Error('Repository not found')) + lsRemoteMock.mockRejectedValue(new Error('Repository not found')) const result = await ValidateRepoAction( createMockActionContext({ injector: elevated, urlParams: { id: 'repo-2' } }), @@ -79,6 +72,7 @@ describe('ValidateRepoAction', () => { it('should throw 404 when repository does not exist', async () => { await withTestInjector(async ({ elevated }) => { + bindGitServiceStub(elevated) await expect( ValidateRepoAction(createMockActionContext({ injector: elevated, urlParams: { id: 'nonexistent' } })), ).rejects.toThrow('Repository not found') diff --git a/service/src/app-models/github-repositories/actions/validate-repo-action.ts b/service/src/app-models/github-repositories/actions/validate-repo-action.ts index 44781f3..f0cf230 100644 --- a/service/src/app-models/github-repositories/actions/validate-repo-action.ts +++ b/service/src/app-models/github-repositories/actions/validate-repo-action.ts @@ -5,9 +5,7 @@ import { RequestError } from '@furystack/rest' import { JsonResult, type RequestAction } from '@furystack/rest-service' import type { ValidateRepoEndpoint } from 'common' -import { execFile } from 'child_process' -import { promisify } from 'util' -const execFileAsync = promisify(execFile) +import { GitService } from '../../../services/git-service.js' export const ValidateRepoAction: RequestAction = async ({ injector, getUrlParams }) => { const logger = getLogger(injector).withScope('ValidateRepo') @@ -22,7 +20,7 @@ export const ValidateRepoAction: RequestAction = async ({ } try { - await execFileAsync('git', ['ls-remote', '--exit-code', repo.url], { timeout: 15000 }) + await injector.get(GitService).lsRemote(repo.url) await logger.information({ message: `Repository validated: ${repo.url}` }) return JsonResult({ accessible: true }) } catch (error) { diff --git a/service/src/app-models/prerequisites/actions/check-prerequisite-action.spec.ts b/service/src/app-models/prerequisites/actions/check-prerequisite-action.spec.ts index 40bc225..242a825 100644 --- a/service/src/app-models/prerequisites/actions/check-prerequisite-action.spec.ts +++ b/service/src/app-models/prerequisites/actions/check-prerequisite-action.spec.ts @@ -12,16 +12,21 @@ import { describe, expect, it, vi } from 'vitest' import { runCheck, CheckPrerequisiteAction } from './check-prerequisite-action.js' import { legacyRepository as getRepository } from '../../../utils/legacy-repository.js' -const execFileMock = vi.hoisted(() => - vi.fn<(cmd: string, args: string[], options: { timeout: number }) => Promise<{ stdout: string; stderr: string }>>(), +const runCliMock = vi.hoisted(() => + vi.fn< + ( + cmd: string, + args: readonly string[], + options: { timeoutMs: number; env?: Record }, + ) => Promise<{ stdout: string; stderr: string }> + >(), ) -vi.mock('child_process', () => ({ - execFile: (...args: unknown[]) => execFileMock(...(args as [string, string[], { timeout: number }])), -})) - -vi.mock('util', () => ({ - promisify: () => execFileMock, +vi.mock('../../../utils/run-cli.js', () => ({ + runCli: (...args: unknown[]) => + runCliMock( + ...(args as [string, readonly string[], { timeoutMs: number; env?: Record }]), + ), })) const createMockActionContext = (options: { injector: Injector; urlParams?: Record }) => ({ @@ -58,20 +63,20 @@ describe('CheckPrerequisiteAction', () => { describe('runCheck', () => { describe('node', () => { it('should return satisfied when version meets minimum', async () => { - execFileMock.mockResolvedValue({ stdout: 'v20.11.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'v20.11.0\n', stderr: '' }) const result = await runCheck('node', { minimumVersion: '18.0.0' }) expect(result.satisfied).toBe(true) expect(result.output).toContain('20.11.0') }) it('should return not satisfied when version is below minimum', async () => { - execFileMock.mockResolvedValue({ stdout: 'v16.20.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'v16.20.0\n', stderr: '' }) const result = await runCheck('node', { minimumVersion: '18.0.0' }) expect(result.satisfied).toBe(false) }) it('should return not satisfied when version cannot be parsed', async () => { - execFileMock.mockResolvedValue({ stdout: 'unknown\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'unknown\n', stderr: '' }) const result = await runCheck('node', { minimumVersion: '18.0.0' }) expect(result.satisfied).toBe(false) expect(result.output).toContain('Could not parse') @@ -80,13 +85,13 @@ describe('CheckPrerequisiteAction', () => { describe('yarn', () => { it('should return satisfied when version meets minimum', async () => { - execFileMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) const result = await runCheck('yarn', { minimumVersion: '4.0.0' }) expect(result.satisfied).toBe(true) }) it('should return not satisfied when version is below minimum', async () => { - execFileMock.mockResolvedValue({ stdout: '1.22.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: '1.22.0\n', stderr: '' }) const result = await runCheck('yarn', { minimumVersion: '4.0.0' }) expect(result.satisfied).toBe(false) }) @@ -95,9 +100,9 @@ describe('CheckPrerequisiteAction', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { value: 'linux', writable: true }) try { - execFileMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) await runCheck('yarn', { minimumVersion: '4.0.0' }) - expect(execFileMock).toHaveBeenCalledWith('yarn', ['--version'], expect.objectContaining({ timeout: 30_000 })) + expect(runCliMock).toHaveBeenCalledWith('yarn', ['--version'], expect.objectContaining({ timeoutMs: 30_000 })) } finally { Object.defineProperty(process, 'platform', { value: originalPlatform, writable: true }) } @@ -109,12 +114,12 @@ describe('CheckPrerequisiteAction', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { value: 'win32', writable: true }) try { - execFileMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: '4.6.0\n', stderr: '' }) await runCheck('yarn', { minimumVersion: '4.0.0' }) - expect(execFileMock).toHaveBeenCalledWith( + expect(runCliMock).toHaveBeenCalledWith( 'cmd.exe', ['/c', 'yarn', '--version'], - expect.objectContaining({ timeout: 30_000 }), + expect.objectContaining({ timeoutMs: 30_000 }), ) } finally { Object.defineProperty(process, 'platform', { value: originalPlatform, writable: true }) @@ -124,7 +129,7 @@ describe('CheckPrerequisiteAction', () => { describe('git', () => { it('should return satisfied when git is available', async () => { - execFileMock.mockResolvedValue({ stdout: 'git version 2.43.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'git version 2.43.0\n', stderr: '' }) const result = await runCheck('git', {}) expect(result.satisfied).toBe(true) expect(result.output).toContain('git version') @@ -203,7 +208,7 @@ describe('CheckPrerequisiteAction', () => { describe('custom-script', () => { it('should return satisfied when script succeeds', async () => { - execFileMock.mockResolvedValue({ stdout: 'OK\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'OK\n', stderr: '' }) const result = await runCheck('custom-script', { script: 'echo OK' }) expect(result.satisfied).toBe(true) expect(result.output).toBe('OK') @@ -212,7 +217,7 @@ describe('CheckPrerequisiteAction', () => { describe('dotnet-sdk', () => { it('should return satisfied when SDK version is installed', async () => { - execFileMock.mockResolvedValue({ + runCliMock.mockResolvedValue({ stdout: '8.0.100 [/usr/share/dotnet/sdk]\n9.0.100 [/usr/share/dotnet/sdk]\n', stderr: '', }) @@ -221,7 +226,7 @@ describe('CheckPrerequisiteAction', () => { }) it('should return not satisfied when SDK version is not installed', async () => { - execFileMock.mockResolvedValue({ + runCliMock.mockResolvedValue({ stdout: '8.0.100 [/usr/share/dotnet/sdk]\n', stderr: '', }) @@ -255,7 +260,7 @@ describe('CheckPrerequisiteAction', () => { updatedAt: ts, }) - execFileMock.mockResolvedValue({ stdout: 'v20.11.0\n', stderr: '' }) + runCliMock.mockResolvedValue({ stdout: 'v20.11.0\n', stderr: '' }) const elevated = useSystemIdentityContext({ injector }) const result = await CheckPrerequisiteAction( @@ -329,7 +334,7 @@ describe('CheckPrerequisiteAction', () => { updatedAt: ts, }) - execFileMock.mockRejectedValue(new Error('Command not found')) + runCliMock.mockRejectedValue(new Error('Command not found')) const elevated = useSystemIdentityContext({ injector }) const result = await CheckPrerequisiteAction( diff --git a/service/src/app-models/prerequisites/actions/check-prerequisite-action.ts b/service/src/app-models/prerequisites/actions/check-prerequisite-action.ts index f1025ca..ad9881a 100644 --- a/service/src/app-models/prerequisites/actions/check-prerequisite-action.ts +++ b/service/src/app-models/prerequisites/actions/check-prerequisite-action.ts @@ -3,18 +3,24 @@ import { RequestError } from '@furystack/rest' import { JsonResult, type RequestAction } from '@furystack/rest-service' import type { CheckPrerequisiteEndpoint, EnvironmentVariableValue, PrerequisiteConfig, PrerequisiteType } from 'common' import { Prerequisite, PrerequisiteCheckResult, StackConfig } from 'common' -import { execFile } from 'child_process' -import { promisify } from 'util' import { CryptoService } from '../../../utils/crypto-service.js' import { legacyRepository as getRepository } from '../../../utils/legacy-repository.js' - -const execFileAsync = promisify(execFile) +import { runCli } from '../../../utils/run-cli.js' const COMMAND_TIMEOUT = 30_000 type CheckResult = { satisfied: boolean; output: string } +/** + * `gh` honors these env vars to suppress its interactive prompts and the + * "newer version available" notifier that occasionally talks to the network. + */ +const GH_ENV: Record = { + GH_PROMPT_DISABLED: '1', + GH_NO_UPDATE_NOTIFIER: '1', +} + /** * Compares two semver-like version strings (e.g. "18.2.0" >= "18.0.0"). * Returns true if `actual` >= `required`. @@ -42,7 +48,7 @@ const extractVersion = (output: string): string | null => { } const checkNode = async (config: { minimumVersion: string }): Promise => { - const { stdout } = await execFileAsync('node', ['--version'], { timeout: COMMAND_TIMEOUT }) + const { stdout } = await runCli('node', ['--version'], { timeoutMs: COMMAND_TIMEOUT }) const version = extractVersion(stdout.trim()) if (!version) { return { satisfied: false, output: `Could not parse Node.js version from: ${stdout.trim()}` } @@ -57,12 +63,13 @@ const checkNode = async (config: { minimumVersion: string }): Promise => { - // Yarn ships as `yarn.cmd` / `yarn.ps1` shims on Windows, which `execFile` cannot resolve directly. - // Route through cmd.exe so PATHEXT applies. POSIX uses execFile directly to avoid an extra fork. + // Yarn ships as `yarn.cmd` / `yarn.ps1` shims on Windows, which `spawn` cannot + // resolve directly without `shell: true`. Route through cmd.exe so PATHEXT + // applies. POSIX uses runCli directly to avoid an extra fork. const isWindows = process.platform === 'win32' const { stdout } = isWindows - ? await execFileAsync('cmd.exe', ['/c', 'yarn', '--version'], { timeout: COMMAND_TIMEOUT }) - : await execFileAsync('yarn', ['--version'], { timeout: COMMAND_TIMEOUT }) + ? await runCli('cmd.exe', ['/c', 'yarn', '--version'], { timeoutMs: COMMAND_TIMEOUT }) + : await runCli('yarn', ['--version'], { timeoutMs: COMMAND_TIMEOUT }) const version = extractVersion(stdout.trim()) if (!version) { return { satisfied: false, output: `Could not parse Yarn version from: ${stdout.trim()}` } @@ -75,7 +82,7 @@ const checkYarn = async (config: { minimumVersion: string }): Promise => { - const { stdout } = await execFileAsync('dotnet', ['--list-sdks'], { timeout: COMMAND_TIMEOUT }) + const { stdout } = await runCli('dotnet', ['--list-sdks'], { timeoutMs: COMMAND_TIMEOUT }) const lines = stdout.trim().split('\n') const isSatisfied = lines.some((line) => line.startsWith(config.version)) return { @@ -87,7 +94,7 @@ const checkDotnetSdk = async (config: { version: string }): Promise } const checkDotnetRuntime = async (config: { version: string }): Promise => { - const { stdout } = await execFileAsync('dotnet', ['--list-runtimes'], { timeout: COMMAND_TIMEOUT }) + const { stdout } = await runCli('dotnet', ['--list-runtimes'], { timeoutMs: COMMAND_TIMEOUT }) const lines = stdout.trim().split('\n') const isSatisfied = lines.some((line) => line.includes(config.version)) return { @@ -99,7 +106,7 @@ const checkDotnetRuntime = async (config: { version: string }): Promise => { - const { stdout } = await execFileAsync('dotnet', ['nuget', 'list', 'source'], { timeout: COMMAND_TIMEOUT }) + const { stdout } = await runCli('dotnet', ['nuget', 'list', 'source'], { timeoutMs: COMMAND_TIMEOUT }) const isSatisfied = stdout.includes(config.feedUrl) return { satisfied: isSatisfied, @@ -110,12 +117,15 @@ const checkNugetFeed = async (config: { feedUrl: string; feedName?: string }): P } const checkGit = async (): Promise => { - const { stdout } = await execFileAsync('git', ['--version'], { timeout: COMMAND_TIMEOUT }) + const { stdout } = await runCli('git', ['--version'], { timeoutMs: COMMAND_TIMEOUT }) return { satisfied: true, output: stdout.trim() } } const checkGithubCli = async (): Promise => { - const { stdout, stderr } = await execFileAsync('gh', ['auth', 'status'], { timeout: COMMAND_TIMEOUT }) + const { stdout, stderr } = await runCli('gh', ['auth', 'status'], { + timeoutMs: COMMAND_TIMEOUT, + env: GH_ENV, + }) const output = (stdout || stderr).trim() return { satisfied: true, output } } @@ -156,7 +166,9 @@ const checkCustomScript = async (config: { script: string }): Promise { const repository = getRepository(elevated) @@ -145,7 +142,7 @@ export const registerRepositoryTools = (mcp: McpServer, _injector: Injector, ele if (!repo) return errorResult(`Repository not found: ${repositoryId}`) try { - await execFileAsync('git', ['ls-remote', '--exit-code', repo.url], { timeout: 15000 }) + await elevated.get(GitService).lsRemote(repo.url) return textResult(`Repository ${repo.displayName} (${repo.url}) is accessible`) } catch (error) { const message = error instanceof Error ? error.message : 'Unknown error' diff --git a/service/src/services/git-service-runner.spec.ts b/service/src/services/git-service-runner.spec.ts new file mode 100644 index 0000000..e8d03f3 --- /dev/null +++ b/service/src/services/git-service-runner.spec.ts @@ -0,0 +1,230 @@ +import { Injector } from '@furystack/inject' +import { useLogging, VerboseConsoleLogger } from '@furystack/logging' +import { usingAsync } from '@furystack/utils' +import { EventEmitter } from 'events' +import { PassThrough } from 'stream' +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' + +import { GitService } from './git-service.js' + +vi.mock('child_process', () => ({ + spawn: vi.fn(), + spawnSync: vi.fn(), +})) + +const childProcess = await import('child_process') +const spawnMock = childProcess.spawn as unknown as MockInstance +const spawnSyncMock = childProcess.spawnSync as unknown as MockInstance + +type FakeChild = EventEmitter & { + pid: number + stdout: PassThrough + stderr: PassThrough +} + +const createFakeChild = (pid = 12345): FakeChild => { + const child = new EventEmitter() as FakeChild + child.pid = pid + child.stdout = new PassThrough() + child.stderr = new PassThrough() + return child +} + +/** + * GitService methods `await this.logger.information(...)` before invoking `runGit`, + * so the `spawn`-returned child has no listeners yet right after the call returns. + * Yield to the I/O queue until runGit has registered its `close` listener; only + * then is it safe to emit lifecycle events on the fake child. + */ +const awaitListenersAttached = async (child: FakeChild): Promise => { + for (let i = 0; i < 100 && child.listenerCount('close') === 0; i++) { + await new Promise((resolve) => setImmediate(resolve)) + } + if (child.listenerCount('close') === 0) { + throw new Error('runGit did not attach a close listener within 100 I/O ticks') + } +} + +const withGitService = (fn: (ctx: { git: GitService; child: FakeChild }) => Promise) => + usingAsync(new Injector(), async (injector) => { + useLogging(injector, VerboseConsoleLogger) + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + const git = injector.get(GitService) + await fn({ git, child }) + }) + +describe('GitService runner', () => { + beforeEach(() => { + spawnMock.mockReset() + spawnSyncMock.mockReset() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + describe('non-interactive env', () => { + it('forces GIT_TERMINAL_PROMPT=0 and removes inherited askpass programs', () => + withGitService(async ({ git, child }) => { + const promise = git.fetch('/repo') + await awaitListenersAttached(child) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + expect(spawnMock).toHaveBeenCalledTimes(1) + const [, , opts] = spawnMock.mock.calls[0] + const { env } = opts as { env: NodeJS.ProcessEnv } + expect(env.GIT_TERMINAL_PROMPT).toBe('0') + expect(env.GIT_ASKPASS).toBeUndefined() + expect(env.SSH_ASKPASS).toBeUndefined() + expect(env.SSH_ASKPASS_REQUIRE).toBe('never') + expect(env.GIT_SSH_COMMAND).toMatch(/BatchMode=yes/) + expect(env.GIT_SSH_COMMAND).toMatch(/ConnectTimeout=15/) + })) + + it('strips GIT_ASKPASS / SSH_ASKPASS even when the host process has them set', () => + withGitService(async ({ git, child }) => { + const original = { GIT_ASKPASS: process.env.GIT_ASKPASS, SSH_ASKPASS: process.env.SSH_ASKPASS } + process.env.GIT_ASKPASS = '/usr/bin/some-gui-helper' + process.env.SSH_ASKPASS = '/usr/bin/another-gui-helper' + try { + const promise = git.fetch('/repo') + await awaitListenersAttached(child) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + const { env } = opts as { env: NodeJS.ProcessEnv } + expect(env.GIT_ASKPASS).toBeUndefined() + expect(env.SSH_ASKPASS).toBeUndefined() + } finally { + if (original.GIT_ASKPASS === undefined) delete process.env.GIT_ASKPASS + else process.env.GIT_ASKPASS = original.GIT_ASKPASS + if (original.SSH_ASKPASS === undefined) delete process.env.SSH_ASKPASS + else process.env.SSH_ASKPASS = original.SSH_ASKPASS + } + })) + + it('spawns with stdin ignored and windowsHide enabled', () => + withGitService(async ({ git, child }) => { + const promise = git.fetch('/repo') + await awaitListenersAttached(child) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + const typed = opts as { stdio: unknown; windowsHide: boolean } + expect(typed.stdio).toEqual(['ignore', 'pipe', 'pipe']) + expect(typed.windowsHide).toBe(true) + })) + }) + + describe('timeout and process-group kill', () => { + it('kills the process group and rejects with stderr context when the call times out', async () => { + vi.useFakeTimers() + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + + try { + await usingAsync(new Injector(), async (injector) => { + useLogging(injector, VerboseConsoleLogger) + const child = createFakeChild(54321) + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const git = injector.get(GitService) + const promise = git.fetch('/repo').then( + () => ({ ok: true as const }), + (error: Error) => ({ ok: false as const, error }), + ) + + child.stderr.write('fatal: unable to access remote\n') + await vi.advanceTimersByTimeAsync(90 * 1000) + + if (process.platform === 'win32') { + expect(spawnSyncMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '54321', '/T'], + expect.objectContaining({ stdio: 'ignore' }), + ) + } else { + expect(killSpy).toHaveBeenCalledWith(-54321, 'SIGTERM') + } + + await vi.advanceTimersByTimeAsync(2000) + + if (process.platform === 'win32') { + expect(spawnSyncMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '54321', '/T', '/F'], + expect.objectContaining({ stdio: 'ignore' }), + ) + } else { + expect(killSpy).toHaveBeenCalledWith(-54321, 'SIGKILL') + } + + child.emit('close', null, 'SIGKILL') + const result = await promise + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error.message).toMatch(/timed out after 90000ms/) + expect(result.error.message).toContain('fatal: unable to access remote') + expect(result.error.message).toContain('/repo') + } + }) + } finally { + killSpy.mockRestore() + } + }) + + it('does not kill or reject before the timeout fires', async () => { + vi.useFakeTimers() + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + + try { + await usingAsync(new Injector(), async (injector) => { + useLogging(injector, VerboseConsoleLogger) + const child = createFakeChild(99) + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const git = injector.get(GitService) + const promise = git.fetch('/repo') + + await vi.advanceTimersByTimeAsync(89 * 1000) + expect(killSpy).not.toHaveBeenCalled() + expect(spawnSyncMock).not.toHaveBeenCalled() + + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await expect(promise).resolves.toBeUndefined() + }) + } finally { + killSpy.mockRestore() + } + }) + }) + + describe('error reporting', () => { + it('includes stderr, exit code, and cwd when git exits non-zero', () => + withGitService(async ({ git, child }) => { + const promise = git.checkout('/repo', 'develop').catch((error: Error) => error) + await awaitListenersAttached(child) + child.stderr.write('error: pathspec "develop" did not match\n') + child.stdout.end() + child.stderr.end() + child.emit('close', 1, null) + const error = await promise + expect(error).toBeInstanceOf(Error) + const { message } = error as Error + expect(message).toMatch(/exited with code 1/) + expect(message).toContain('/repo') + expect(message).toContain('error: pathspec "develop" did not match') + })) + }) +}) diff --git a/service/src/services/git-service.ts b/service/src/services/git-service.ts index 487a3c6..67d0837 100644 --- a/service/src/services/git-service.ts +++ b/service/src/services/git-service.ts @@ -1,9 +1,39 @@ import { defineService, type Token, type Injector } from '@furystack/inject' import { getLogger } from '@furystack/logging' -import { execFile } from 'child_process' -import { promisify } from 'util' -const execFileAsync = promisify(execFile) +import { runCli, type RunCliResult } from '../utils/run-cli.js' + +const TIMEOUTS = { + CLONE_MS: 5 * 60 * 1000, + FETCH_MS: 90 * 1000, + PULL_MS: 90 * 1000, + CHECKOUT_MS: 30 * 1000, + CHEAP_READ_MS: 10 * 1000, + REF_LOOKUP_MS: 5 * 1000, + LS_REMOTE_MS: 30 * 1000, +} as const + +/** + * Env hardening applied to every git invocation. Inherits the host env so git + * can find ssh-agent / credential helpers / PATH, then forces non-interactive + * mode so missing or expired credentials fail fast instead of popping a prompt + * (terminal or GUI) that nobody can answer from a backend service. + * + * - `GIT_TERMINAL_PROMPT=0` — no terminal prompts on missing HTTPS creds. + * - Removes inherited `GIT_ASKPASS` / `SSH_ASKPASS` so no GUI helper is invoked. + * - `SSH_ASKPASS_REQUIRE=never` — keeps ssh from spawning askpass even if `DISPLAY` is set. + * - `GIT_SSH_COMMAND` — `BatchMode=yes` blocks every ssh prompt; `ConnectTimeout` caps DNS/TCP waits. + */ +const GIT_ENV: Record = { + GIT_ASKPASS: undefined, + SSH_ASKPASS: undefined, + GIT_TERMINAL_PROMPT: '0', + SSH_ASKPASS_REQUIRE: 'never', + GIT_SSH_COMMAND: 'ssh -o BatchMode=yes -o ConnectTimeout=15 -o StrictHostKeyChecking=accept-new', +} + +const runGit = (args: readonly string[], options: { cwd?: string; timeoutMs: number }): Promise => + runCli('git', args, { ...options, env: GIT_ENV }) /** Low-level wrapper around git CLI operations (clone, fetch, pull, checkout, branch listing) */ class GitServiceImpl { @@ -15,29 +45,29 @@ class GitServiceImpl { public async clone(url: string, directory: string): Promise { await this.logger.information({ message: `Cloning ${url} into ${directory}` }) - await execFileAsync('git', ['clone', url, directory], { timeout: 300000 }) + await runGit(['clone', url, directory], { timeoutMs: TIMEOUTS.CLONE_MS }) } public async fetch(directory: string): Promise { await this.logger.verbose({ message: `Fetching in ${directory}` }) - await execFileAsync('git', ['fetch', '--all', '--prune'], { cwd: directory, timeout: 60000 }) + await runGit(['fetch', '--all', '--prune'], { cwd: directory, timeoutMs: TIMEOUTS.FETCH_MS }) } public async pull(directory: string): Promise<{ updated: boolean }> { await this.logger.information({ message: `Pulling in ${directory}` }) - const { stdout } = await execFileAsync('git', ['pull'], { cwd: directory, timeout: 60000 }) + const { stdout } = await runGit(['pull'], { cwd: directory, timeoutMs: TIMEOUTS.PULL_MS }) const updated = !stdout.includes('Already up to date') return { updated } } public async getBranches(directory: string): Promise<{ local: string[]; remote: string[] }> { - const { stdout: localOut } = await execFileAsync('git', ['branch', '--format=%(refname:short)'], { + const { stdout: localOut } = await runGit(['branch', '--format=%(refname:short)'], { cwd: directory, - timeout: 10000, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) - const { stdout: remoteOut } = await execFileAsync('git', ['branch', '-r', '--format=%(refname:short)'], { + const { stdout: remoteOut } = await runGit(['branch', '-r', '--format=%(refname:short)'], { cwd: directory, - timeout: 10000, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) const local = localOut @@ -53,18 +83,18 @@ class GitServiceImpl { } public async getCurrentBranch(directory: string): Promise { - const { stdout } = await execFileAsync('git', ['branch', '--show-current'], { + const { stdout } = await runGit(['branch', '--show-current'], { cwd: directory, - timeout: 10000, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) return stdout.trim() } public async getCommitsBehind(directory: string, branch: string): Promise { try { - const { stdout } = await execFileAsync('git', ['rev-list', '--count', `HEAD..origin/${branch}`], { + const { stdout } = await runGit(['rev-list', '--count', `HEAD..origin/${branch}`], { cwd: directory, - timeout: 10000, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) return parseInt(stdout.trim(), 10) || 0 } catch { @@ -74,21 +104,24 @@ class GitServiceImpl { public async checkout(directory: string, branch: string): Promise { await this.logger.information({ message: `Checking out ${branch} in ${directory}` }) - await execFileAsync('git', ['checkout', branch], { cwd: directory, timeout: 30000 }) + await runGit(['checkout', branch], { cwd: directory, timeoutMs: TIMEOUTS.CHECKOUT_MS }) } /** Deletes a local branch. Uses `-D` (force) when `force` is true, otherwise `-d` (safe). */ public async deleteLocalBranch(directory: string, branch: string, force = false): Promise { await this.logger.information({ message: `Deleting local branch ${branch} in ${directory}` }) - await execFileAsync('git', ['branch', force ? '-D' : '-d', branch], { cwd: directory, timeout: 10000 }) + await runGit(['branch', force ? '-D' : '-d', branch], { + cwd: directory, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, + }) } /** Returns true if `origin/` has a resolvable ref locally (i.e. the branch exists on the remote after a fetch). */ public async hasRemoteBranch(directory: string, branch: string): Promise { try { - await execFileAsync('git', ['show-ref', '--verify', '--quiet', `refs/remotes/origin/${branch}`], { + await runGit(['show-ref', '--verify', '--quiet', `refs/remotes/origin/${branch}`], { cwd: directory, - timeout: 5000, + timeoutMs: TIMEOUTS.REF_LOOKUP_MS, }) return true } catch { @@ -102,11 +135,10 @@ class GitServiceImpl { */ public async getDefaultBranch(directory: string): Promise { try { - const { stdout } = await execFileAsync( - 'git', - ['symbolic-ref', '--short', '--quiet', 'refs/remotes/origin/HEAD'], - { cwd: directory, timeout: 5000 }, - ) + const { stdout } = await runGit(['symbolic-ref', '--short', '--quiet', 'refs/remotes/origin/HEAD'], { + cwd: directory, + timeoutMs: TIMEOUTS.REF_LOOKUP_MS, + }) const trimmed = stdout.trim() return trimmed.startsWith('origin/') ? trimmed.slice('origin/'.length) : trimmed || undefined } catch { @@ -116,9 +148,9 @@ class GitServiceImpl { /** Runs `git status --porcelain` and classifies the working tree. */ public async getWorktreeStatus(directory: string): Promise<'clean' | 'dirty' | 'conflicts'> { - const { stdout } = await execFileAsync('git', ['status', '--porcelain'], { + const { stdout } = await runGit(['status', '--porcelain'], { cwd: directory, - timeout: 10000, + timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) const lines = stdout.split('\n').filter((l) => l.length > 0) if (lines.length === 0) return 'clean' @@ -132,15 +164,27 @@ class GitServiceImpl { /** Returns the SHA that a given ref points to (e.g. `HEAD`, `refs/heads/main`). Returns undefined if unresolvable. */ public async revParse(directory: string, ref: string): Promise { try { - const { stdout } = await execFileAsync('git', ['rev-parse', '--quiet', '--verify', ref], { + const { stdout } = await runGit(['rev-parse', '--quiet', '--verify', ref], { cwd: directory, - timeout: 5000, + timeoutMs: TIMEOUTS.REF_LOOKUP_MS, }) return stdout.trim() || undefined } catch { return undefined } } + + /** + * Probes whether a remote URL is reachable and authorized via `git ls-remote --exit-code`. + * Resolves on success, rejects with an error containing stderr context on failure or timeout. + * + * Use this for repository accessibility checks (validate-repo flows, pre-clone probing). Goes + * through the same env hardening (GIT_TERMINAL_PROMPT=0, BatchMode ssh) as every other git call, + * so missing credentials fail fast instead of hanging on a prompt. + */ + public async lsRemote(url: string): Promise { + await runGit(['ls-remote', '--exit-code', url], { timeoutMs: TIMEOUTS.LS_REMOTE_MS }) + } } export type GitService = GitServiceImpl diff --git a/service/src/utils/run-cli.spec.ts b/service/src/utils/run-cli.spec.ts new file mode 100644 index 0000000..15b52ac --- /dev/null +++ b/service/src/utils/run-cli.spec.ts @@ -0,0 +1,270 @@ +import { EventEmitter } from 'events' +import { PassThrough } from 'stream' +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' + +import { runCli } from './run-cli.js' + +vi.mock('child_process', () => ({ + spawn: vi.fn(), + spawnSync: vi.fn(), +})) + +const childProcess = await import('child_process') +const spawnMock = childProcess.spawn as unknown as MockInstance +const spawnSyncMock = childProcess.spawnSync as unknown as MockInstance + +type FakeChild = EventEmitter & { + pid: number + stdout: PassThrough + stderr: PassThrough +} + +const createFakeChild = (pid = 12345): FakeChild => { + const child = new EventEmitter() as FakeChild + child.pid = pid + child.stdout = new PassThrough() + child.stderr = new PassThrough() + return child +} + +describe('runCli', () => { + beforeEach(() => { + spawnMock.mockReset() + spawnSyncMock.mockReset() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + describe('env handling', () => { + it('inherits process.env and applies overrides on top', async () => { + const original = process.env.SOME_HOST_VAR + process.env.SOME_HOST_VAR = 'host-value' + try { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('git', ['--version'], { + timeoutMs: 1000, + env: { GIT_TERMINAL_PROMPT: '0' }, + }) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + const { env } = opts as { env: NodeJS.ProcessEnv } + expect(env.SOME_HOST_VAR).toBe('host-value') + expect(env.GIT_TERMINAL_PROMPT).toBe('0') + } finally { + if (original === undefined) delete process.env.SOME_HOST_VAR + else process.env.SOME_HOST_VAR = original + } + }) + + it('strips inherited env vars when override sets them to undefined', async () => { + const original = process.env.GIT_ASKPASS + process.env.GIT_ASKPASS = '/usr/bin/some-gui-helper' + try { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('git', ['--version'], { + timeoutMs: 1000, + env: { GIT_ASKPASS: undefined }, + }) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + const { env } = opts as { env: NodeJS.ProcessEnv } + expect(env.GIT_ASKPASS).toBeUndefined() + } finally { + if (original === undefined) delete process.env.GIT_ASKPASS + else process.env.GIT_ASKPASS = original + } + }) + }) + + describe('spawn options', () => { + it('spawns with stdin ignored, stdout/stderr piped, and windowsHide enabled', async () => { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('node', ['--version'], { timeoutMs: 1000 }) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + expect(spawnMock).toHaveBeenCalledWith( + 'node', + ['--version'], + expect.objectContaining({ + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true, + }), + ) + }) + + it('forwards cwd when provided', async () => { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('git', ['status'], { cwd: '/tmp/repo', timeoutMs: 1000 }) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + expect((opts as { cwd?: string }).cwd).toBe('/tmp/repo') + }) + + it('uses detached: true on POSIX so process-group kill is possible', async () => { + if (process.platform === 'win32') return + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('node', ['--version'], { timeoutMs: 1000 }) + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await promise + + const [, , opts] = spawnMock.mock.calls[0] + expect((opts as { detached: boolean }).detached).toBe(true) + }) + }) + + describe('successful invocation', () => { + it('resolves with captured stdout and stderr on exit code 0', async () => { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('git', ['--version'], { timeoutMs: 1000 }) + child.stdout.write('git version 2.43.0\n') + child.stderr.write('warning: something\n') + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + + const result = await promise + expect(result.stdout).toBe('git version 2.43.0\n') + expect(result.stderr).toBe('warning: something\n') + }) + }) + + describe('error reporting', () => { + it('rejects with stderr, exit code, and command in message on non-zero exit', async () => { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('git', ['ls-remote', 'https://example.com/missing.git'], { + timeoutMs: 1000, + }).catch((error: Error) => error) + child.stderr.write('fatal: repository not found\n') + child.stdout.end() + child.stderr.end() + child.emit('close', 128, null) + + const error = await promise + expect(error).toBeInstanceOf(Error) + const { message } = error as Error + expect(message).toContain('git ls-remote https://example.com/missing.git') + expect(message).toMatch(/exited with code 128/) + expect(message).toContain('fatal: repository not found') + }) + + it('rejects on `error` event from the child process', async () => { + const child = createFakeChild() + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('does-not-exist', [], { timeoutMs: 1000 }).catch((error: Error) => error) + child.emit('error', new Error('ENOENT')) + + const error = await promise + expect(error).toBeInstanceOf(Error) + expect((error as Error).message).toBe('ENOENT') + }) + }) + + describe('timeout and process-group kill', () => { + it('kills the process group on timeout and rejects with stderr context', async () => { + vi.useFakeTimers() + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + + try { + const child = createFakeChild(54321) + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('curl', ['https://slow.example.com'], { timeoutMs: 5_000 }).then( + () => ({ ok: true as const }), + (error: Error) => ({ ok: false as const, error }), + ) + + child.stderr.write('curl: still trying\n') + await vi.advanceTimersByTimeAsync(5_000) + + if (process.platform === 'win32') { + expect(spawnSyncMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '54321', '/T'], + expect.objectContaining({ stdio: 'ignore' }), + ) + } else { + expect(killSpy).toHaveBeenCalledWith(-54321, 'SIGTERM') + } + + await vi.advanceTimersByTimeAsync(2_000) + + if (process.platform === 'win32') { + expect(spawnSyncMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '54321', '/T', '/F'], + expect.objectContaining({ stdio: 'ignore' }), + ) + } else { + expect(killSpy).toHaveBeenCalledWith(-54321, 'SIGKILL') + } + + child.emit('close', null, 'SIGKILL') + const result = await promise + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error.message).toMatch(/timed out after 5000ms/) + expect(result.error.message).toContain('curl: still trying') + } + } finally { + killSpy.mockRestore() + } + }) + + it('does not kill or reject before the timeout fires', async () => { + vi.useFakeTimers() + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + + try { + const child = createFakeChild(99) + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + + const promise = runCli('node', ['--version'], { timeoutMs: 5_000 }) + + await vi.advanceTimersByTimeAsync(4_999) + expect(killSpy).not.toHaveBeenCalled() + expect(spawnSyncMock).not.toHaveBeenCalled() + + child.stdout.end() + child.stderr.end() + child.emit('close', 0, null) + await expect(promise).resolves.toEqual({ stdout: '', stderr: '' }) + } finally { + killSpy.mockRestore() + } + }) + }) +}) diff --git a/service/src/utils/run-cli.ts b/service/src/utils/run-cli.ts new file mode 100644 index 0000000..b8d54a5 --- /dev/null +++ b/service/src/utils/run-cli.ts @@ -0,0 +1,142 @@ +import { spawn, spawnSync } from 'child_process' + +export type RunCliOptions = { + cwd?: string + timeoutMs: number + /** + * Extra env applied on top of the inherited host env. Set a key to `undefined` + * to strip an inherited variable (useful for blocking GUI prompt helpers like + * `GIT_ASKPASS` that the host might have pre-configured). + */ + env?: Record +} + +export type RunCliResult = { + stdout: string + stderr: string +} + +const isWindows = process.platform === 'win32' + +const SIGKILL_GRACE_MS = 2000 + +const buildEnv = (overrides?: Record): NodeJS.ProcessEnv => { + const env: NodeJS.ProcessEnv = { ...process.env } + if (overrides) { + for (const [key, value] of Object.entries(overrides)) { + if (value === undefined) delete env[key] + else env[key] = value + } + } + return env +} + +/** + * Cross-platform process-group kill. POSIX uses negative pid against the group + * leader (only works because we spawn with `detached: true`). Windows has no + * POSIX-style process groups, so we use `taskkill /T` to walk the child tree, + * escalating to `/F` only when the caller asked for `SIGKILL`. + */ +const killProcessGroup = (pid: number, signal: NodeJS.Signals): void => { + try { + if (isWindows) { + const args = signal === 'SIGKILL' ? ['/pid', String(pid), '/T', '/F'] : ['/pid', String(pid), '/T'] + spawnSync('taskkill', args, { stdio: 'ignore' }) + return + } + try { + process.kill(-pid, signal) + } catch { + // Group leader may have exited while children are still alive; fall back + // to direct pid kill so we at least clear the immediate child. + process.kill(pid, signal) + } + } catch { + // Process likely exited between timeout firing and kill; nothing to do. + } +} + +/** + * Spawn-based CLI runner used as the canonical replacement for + * `promisify(execFile)` across the service layer. + * + * Why not `execFile`'s built-in `timeout`: when it fires it sends SIGTERM only + * to the parent. Grandchildren (credential helpers, ssh, git-remote-https, gh + * auth flows, user-script subshells) keep the inherited stdio pipes open, so + * the promise can stay pending well past the nominal timeout + * (nodejs/node#2098). + * + * What this helper does instead: + * - `detached: true` on POSIX so the child becomes a process-group leader + * and `process.kill(-pid, signal)` reaches the entire tree. + * - `taskkill /T` on Windows to walk the tree (escalating to `/F` on SIGKILL). + * - Two-stage kill on timeout: SIGTERM, then SIGKILL after a 2 s grace. + * - Stderr is captured and surfaced in the rejection error so callers see a + * useful diagnostic instead of the bare `"Command failed: \n"` that + * `execFile` produces. + */ +export const runCli = (command: string, args: readonly string[], options: RunCliOptions): Promise => + new Promise((resolve, reject) => { + const child = spawn(command, args, { + cwd: options.cwd, + stdio: ['ignore', 'pipe', 'pipe'], + env: buildEnv(options.env), + // `detached: true` on POSIX makes the child a process-group leader so we + // can kill the whole tree via `process.kill(-pid)`. On Windows it spawns + // the child in its own console; `windowsHide` keeps the console invisible. + detached: !isWindows, + windowsHide: true, + }) + + let stdout = '' + let stderr = '' + let timedOut = false + let killTimer: ReturnType | null = null + + child.stdout?.on('data', (chunk: Buffer) => { + stdout += chunk.toString() + }) + child.stderr?.on('data', (chunk: Buffer) => { + stderr += chunk.toString() + }) + + const timeoutTimer = setTimeout(() => { + timedOut = true + if (child.pid != null) { + killProcessGroup(child.pid, 'SIGTERM') + killTimer = setTimeout(() => { + if (child.pid != null) killProcessGroup(child.pid, 'SIGKILL') + }, SIGKILL_GRACE_MS) + } + }, options.timeoutMs) + + const cleanup = (): void => { + clearTimeout(timeoutTimer) + if (killTimer) clearTimeout(killTimer) + } + + child.once('error', (error) => { + cleanup() + reject(error) + }) + + child.once('close', (code, signal) => { + cleanup() + const cwdPart = options.cwd ? ` in ${options.cwd}` : '' + const stderrPart = stderr.trim() ? `\nstderr: ${stderr.trim()}` : '' + const commandLine = [command, ...args].join(' ') + + if (timedOut) { + reject(new Error(`${commandLine}${cwdPart} timed out after ${options.timeoutMs}ms${stderrPart}`)) + return + } + + if (code !== 0) { + const signalPart = signal ? ` (signal ${signal})` : '' + reject(new Error(`${commandLine}${cwdPart} exited with code ${code ?? 'null'}${signalPart}${stderrPart}`)) + return + } + + resolve({ stdout, stderr }) + }) + }) From 1cb9929182111c07ee95d512cf1ed71720df890c Mon Sep 17 00:00:00 2001 From: Gallay Lajos Date: Mon, 1 Jun 2026 13:18:03 +0200 Subject: [PATCH 2/3] feat: operation pools --- .env.example | 20 +++ .../src/services/git-service-runner.spec.ts | 49 +++++++- service/src/services/git-service.ts | 52 +++++--- .../services/one-shot-command-runner.spec.ts | 114 +++++++++++++++++- .../src/services/one-shot-command-runner.ts | 36 +++++- service/src/services/operation-limits.spec.ts | 54 +++++++++ service/src/services/operation-limits.ts | 54 +++++++++ service/src/utils/run-cli.ts | 42 ++++++- 8 files changed, 393 insertions(+), 28 deletions(-) create mode 100644 service/src/services/operation-limits.spec.ts create mode 100644 service/src/services/operation-limits.ts diff --git a/.env.example b/.env.example index 2cf60b1..26b9f03 100644 --- a/.env.example +++ b/.env.example @@ -43,6 +43,26 @@ DATABASE_URL=postgres://stackcraft:stackcraft@localhost:5433/stackcraft # Interval (ms) between MCP stale-session sweeps # MCP_SESSION_SWEEP_MS=60000 +# --- Operation concurrency limits --- +# Cap concurrent CLI operations across services. Each operation type has its own +# pool, so e.g. a long-running build never blocks a quick `git fetch`. +# Anything below 1 or non-numeric falls back to the default. + +# Max concurrent git CLI operations (clone, fetch, pull, ls-remote, branch reads). +# Network-bound; the cap mainly stops the periodic GitWatcher from saturating the +# system when many services share a stack. +# STACK_CRAFT_MAX_PARALLEL_GIT=10 + +# Max concurrent install operations across services. Installs are network + disk +# heavy and frequently write to shared package caches (yarn/pnpm/npm/nuget) where +# concurrent writers can race. +# STACK_CRAFT_MAX_PARALLEL_INSTALLS=3 + +# Max concurrent build operations across services. Builds are CPU-bound and each +# one already saturates multiple cores (tsc -b, webpack, dotnet build, …), so +# running more than a handful in parallel typically thrashes the machine. +# STACK_CRAFT_MAX_PARALLEL_BUILDS=1 + # --- TLS / Remote access --- # StackCraft does not terminate TLS itself. For remote (non-localhost) deployments, # place a reverse proxy (e.g. Caddy, nginx, Traefik) in front of both the main diff --git a/service/src/services/git-service-runner.spec.ts b/service/src/services/git-service-runner.spec.ts index e8d03f3..16a7d19 100644 --- a/service/src/services/git-service-runner.spec.ts +++ b/service/src/services/git-service-runner.spec.ts @@ -1,11 +1,12 @@ import { Injector } from '@furystack/inject' import { useLogging, VerboseConsoleLogger } from '@furystack/logging' -import { usingAsync } from '@furystack/utils' +import { Semaphore, usingAsync } from '@furystack/utils' import { EventEmitter } from 'events' import { PassThrough } from 'stream' import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' import { GitService } from './git-service.js' +import { GitOperationLimit } from './operation-limits.js' vi.mock('child_process', () => ({ spawn: vi.fn(), @@ -227,4 +228,50 @@ describe('GitService runner', () => { expect(message).toContain('error: pathspec "develop" did not match') })) }) + + describe('GitOperationLimit', () => { + it('serializes calls beyond the configured concurrency', async () => { + await usingAsync(new Injector(), async (injector) => { + useLogging(injector, VerboseConsoleLogger) + const limit = new Semaphore(2) + injector.bind(GitOperationLimit, () => limit) + const git = injector.get(GitService) + + const children = [createFakeChild(101), createFakeChild(102), createFakeChild(103)] + for (const child of children) { + spawnMock.mockReturnValueOnce(child as unknown as ReturnType) + } + + const promises = ['/a', '/b', '/c'].map((cwd) => git.fetch(cwd)) + + await awaitListenersAttached(children[0]) + await awaitListenersAttached(children[1]) + + // With a Semaphore(2), the third call must wait — only two children spawned. + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(limit.runningCount.getValue()).toBe(2) + expect(limit.pendingCount.getValue()).toBe(1) + + // Drain slot 1; queued task should now spawn the third child. + children[0].stdout.end() + children[0].stderr.end() + children[0].emit('close', 0, null) + await promises[0] + await awaitListenersAttached(children[2]) + + expect(spawnMock).toHaveBeenCalledTimes(3) + + children[1].stdout.end() + children[1].stderr.end() + children[1].emit('close', 0, null) + children[2].stdout.end() + children[2].stderr.end() + children[2].emit('close', 0, null) + await Promise.all(promises) + + expect(limit.runningCount.getValue()).toBe(0) + expect(limit.pendingCount.getValue()).toBe(0) + }) + }) + }) }) diff --git a/service/src/services/git-service.ts b/service/src/services/git-service.ts index 67d0837..175478b 100644 --- a/service/src/services/git-service.ts +++ b/service/src/services/git-service.ts @@ -1,7 +1,9 @@ import { defineService, type Token, type Injector } from '@furystack/inject' import { getLogger } from '@furystack/logging' +import type { Semaphore } from '@furystack/utils' import { runCli, type RunCliResult } from '../utils/run-cli.js' +import { GitOperationLimit } from './operation-limits.js' const TIMEOUTS = { CLONE_MS: 5 * 60 * 1000, @@ -32,40 +34,54 @@ const GIT_ENV: Record = { GIT_SSH_COMMAND: 'ssh -o BatchMode=yes -o ConnectTimeout=15 -o StrictHostKeyChecking=accept-new', } -const runGit = (args: readonly string[], options: { cwd?: string; timeoutMs: number }): Promise => - runCli('git', args, { ...options, env: GIT_ENV }) +const runGit = ( + args: readonly string[], + options: { cwd?: string; timeoutMs: number; signal?: AbortSignal }, +): Promise => runCli('git', args, { ...options, env: GIT_ENV }) /** Low-level wrapper around git CLI operations (clone, fetch, pull, checkout, branch listing) */ class GitServiceImpl { private logger!: ReturnType['withScope']> + private limit: Semaphore constructor(injector: Injector) { this.logger = getLogger(injector).withScope('GitService') + this.limit = injector.get(GitOperationLimit) + } + + /** + * Routes the underlying `runCli` call through {@link GitOperationLimit} so + * concurrent git invocations stay under the configured ceiling. The semaphore + * forwards an `AbortSignal` that fires on injector dispose; `runCli` honours + * it by killing the process group, so in-flight ops don't leak past shutdown. + */ + private guarded(args: readonly string[], options: { cwd?: string; timeoutMs: number }): Promise { + return this.limit.execute(({ signal }) => runGit(args, { ...options, signal })) } public async clone(url: string, directory: string): Promise { await this.logger.information({ message: `Cloning ${url} into ${directory}` }) - await runGit(['clone', url, directory], { timeoutMs: TIMEOUTS.CLONE_MS }) + await this.guarded(['clone', url, directory], { timeoutMs: TIMEOUTS.CLONE_MS }) } public async fetch(directory: string): Promise { await this.logger.verbose({ message: `Fetching in ${directory}` }) - await runGit(['fetch', '--all', '--prune'], { cwd: directory, timeoutMs: TIMEOUTS.FETCH_MS }) + await this.guarded(['fetch', '--all', '--prune'], { cwd: directory, timeoutMs: TIMEOUTS.FETCH_MS }) } public async pull(directory: string): Promise<{ updated: boolean }> { await this.logger.information({ message: `Pulling in ${directory}` }) - const { stdout } = await runGit(['pull'], { cwd: directory, timeoutMs: TIMEOUTS.PULL_MS }) + const { stdout } = await this.guarded(['pull'], { cwd: directory, timeoutMs: TIMEOUTS.PULL_MS }) const updated = !stdout.includes('Already up to date') return { updated } } public async getBranches(directory: string): Promise<{ local: string[]; remote: string[] }> { - const { stdout: localOut } = await runGit(['branch', '--format=%(refname:short)'], { + const { stdout: localOut } = await this.guarded(['branch', '--format=%(refname:short)'], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) - const { stdout: remoteOut } = await runGit(['branch', '-r', '--format=%(refname:short)'], { + const { stdout: remoteOut } = await this.guarded(['branch', '-r', '--format=%(refname:short)'], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) @@ -83,7 +99,7 @@ class GitServiceImpl { } public async getCurrentBranch(directory: string): Promise { - const { stdout } = await runGit(['branch', '--show-current'], { + const { stdout } = await this.guarded(['branch', '--show-current'], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) @@ -92,7 +108,7 @@ class GitServiceImpl { public async getCommitsBehind(directory: string, branch: string): Promise { try { - const { stdout } = await runGit(['rev-list', '--count', `HEAD..origin/${branch}`], { + const { stdout } = await this.guarded(['rev-list', '--count', `HEAD..origin/${branch}`], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) @@ -104,13 +120,13 @@ class GitServiceImpl { public async checkout(directory: string, branch: string): Promise { await this.logger.information({ message: `Checking out ${branch} in ${directory}` }) - await runGit(['checkout', branch], { cwd: directory, timeoutMs: TIMEOUTS.CHECKOUT_MS }) + await this.guarded(['checkout', branch], { cwd: directory, timeoutMs: TIMEOUTS.CHECKOUT_MS }) } /** Deletes a local branch. Uses `-D` (force) when `force` is true, otherwise `-d` (safe). */ public async deleteLocalBranch(directory: string, branch: string, force = false): Promise { await this.logger.information({ message: `Deleting local branch ${branch} in ${directory}` }) - await runGit(['branch', force ? '-D' : '-d', branch], { + await this.guarded(['branch', force ? '-D' : '-d', branch], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) @@ -119,7 +135,7 @@ class GitServiceImpl { /** Returns true if `origin/` has a resolvable ref locally (i.e. the branch exists on the remote after a fetch). */ public async hasRemoteBranch(directory: string, branch: string): Promise { try { - await runGit(['show-ref', '--verify', '--quiet', `refs/remotes/origin/${branch}`], { + await this.guarded(['show-ref', '--verify', '--quiet', `refs/remotes/origin/${branch}`], { cwd: directory, timeoutMs: TIMEOUTS.REF_LOOKUP_MS, }) @@ -135,7 +151,7 @@ class GitServiceImpl { */ public async getDefaultBranch(directory: string): Promise { try { - const { stdout } = await runGit(['symbolic-ref', '--short', '--quiet', 'refs/remotes/origin/HEAD'], { + const { stdout } = await this.guarded(['symbolic-ref', '--short', '--quiet', 'refs/remotes/origin/HEAD'], { cwd: directory, timeoutMs: TIMEOUTS.REF_LOOKUP_MS, }) @@ -148,7 +164,7 @@ class GitServiceImpl { /** Runs `git status --porcelain` and classifies the working tree. */ public async getWorktreeStatus(directory: string): Promise<'clean' | 'dirty' | 'conflicts'> { - const { stdout } = await runGit(['status', '--porcelain'], { + const { stdout } = await this.guarded(['status', '--porcelain'], { cwd: directory, timeoutMs: TIMEOUTS.CHEAP_READ_MS, }) @@ -164,7 +180,7 @@ class GitServiceImpl { /** Returns the SHA that a given ref points to (e.g. `HEAD`, `refs/heads/main`). Returns undefined if unresolvable. */ public async revParse(directory: string, ref: string): Promise { try { - const { stdout } = await runGit(['rev-parse', '--quiet', '--verify', ref], { + const { stdout } = await this.guarded(['rev-parse', '--quiet', '--verify', ref], { cwd: directory, timeoutMs: TIMEOUTS.REF_LOOKUP_MS, }) @@ -179,11 +195,11 @@ class GitServiceImpl { * Resolves on success, rejects with an error containing stderr context on failure or timeout. * * Use this for repository accessibility checks (validate-repo flows, pre-clone probing). Goes - * through the same env hardening (GIT_TERMINAL_PROMPT=0, BatchMode ssh) as every other git call, - * so missing credentials fail fast instead of hanging on a prompt. + * through the same env hardening (GIT_TERMINAL_PROMPT=0, BatchMode ssh) and shares the + * {@link GitOperationLimit} pool with the rest of the git CLI calls. */ public async lsRemote(url: string): Promise { - await runGit(['ls-remote', '--exit-code', url], { timeoutMs: TIMEOUTS.LS_REMOTE_MS }) + await this.guarded(['ls-remote', '--exit-code', url], { timeoutMs: TIMEOUTS.LS_REMOTE_MS }) } } diff --git a/service/src/services/one-shot-command-runner.spec.ts b/service/src/services/one-shot-command-runner.spec.ts index b23537b..c27c208 100644 --- a/service/src/services/one-shot-command-runner.spec.ts +++ b/service/src/services/one-shot-command-runner.spec.ts @@ -25,9 +25,13 @@ import { import { tmpdir } from 'os' import { describe, expect, it, vi } from 'vitest' +import { Semaphore } from '@furystack/utils' + import { GitHeadWatcher } from './git-head-watcher.js' import { LogStorageService } from './log-storage-service.js' import { OneShotCommandRunner } from './one-shot-command-runner.js' +import { BuildOperationLimit, InstallOperationLimit } from './operation-limits.js' +import { ProcessRunner } from './process-runner.js' import type { TriggerContext } from './trigger-context.js' import { legacyRepository as getRepository } from '../utils/legacy-repository.js' @@ -132,9 +136,11 @@ const withContext = async ( runner: OneShotCommandRunner mockLogStorage: { addEntry: ReturnType } }) => Promise, + options: { setup?: (injector: Injector) => void } = {}, ) => { const injector = new Injector() const { mockLogStorage } = await setupInjector(injector) + options.setup?.(injector) await seedService(injector) const runner = injector.get(OneShotCommandRunner) try { @@ -150,6 +156,13 @@ const withContext = async ( } } +const killAllRunningProcesses = (injector: Injector): void => { + const processRunner = injector.get(ProcessRunner) + for (const managed of processRunner.processes.values()) { + processRunner.killProcessGroup(managed.process, 'SIGKILL') + } +} + describe('OneShotCommandRunner', () => { it('should throw when installCommand is missing', () => withContext(async ({ injector, runner }) => { @@ -224,10 +237,107 @@ describe('OneShotCommandRunner', () => { await expect(runner.buildService('busy-svc', testTrigger)).rejects.toThrow('already has a') // Clean up: the install process is still running, kill it via the process runner - const { ProcessRunner: PR } = await import('./process-runner.js') - const processRunner = injector.get(PR) + const processRunner = injector.get(ProcessRunner) const managed = processRunner.processes.get('busy-svc') if (managed) processRunner.killProcessGroup(managed.process, 'SIGKILL') await installPromise.catch(() => {}) })) + + describe('semaphore-based concurrency limits', () => { + const waitFor = async (predicate: () => boolean, timeoutMs = 5_000): Promise => { + const start = Date.now() + while (!predicate()) { + if (Date.now() - start > timeoutMs) throw new Error('waitFor timed out') + await new Promise((r) => setTimeout(r, 10)) + } + } + + it('queues install calls beyond InstallOperationLimit and runs them sequentially', async () => { + const installLimit = new Semaphore(1) + await withContext( + async ({ injector, runner }) => { + await seedService(injector, { id: 'install-a', installCommand: 'sleep 0.3' }) + await seedService(injector, { id: 'install-b', installCommand: 'sleep 0.3' }) + + const promiseA = runner.installService('install-a', testTrigger) + const promiseB = runner.installService('install-b', testTrigger) + + await waitFor(() => installLimit.runningCount.getValue() === 1) + + const processRunner = injector.get(ProcessRunner) + // Only the running install is in `processes`; queued one is still + // in `pendingOperations` (it'll move to `processes` once it spawns). + expect(installLimit.runningCount.getValue()).toBe(1) + expect(installLimit.pendingCount.getValue()).toBe(1) + expect(processRunner.processes.size).toBe(1) + expect(processRunner.pendingOperations.size).toBe(1) + + await Promise.all([promiseA, promiseB]) + + expect(installLimit.runningCount.getValue()).toBe(0) + expect(installLimit.pendingCount.getValue()).toBe(0) + expect(installLimit.completedCount.getValue()).toBe(2) + }, + { setup: (injector) => injector.bind(InstallOperationLimit, () => installLimit) }, + ) + }, 10_000) + + it('queues build calls beyond BuildOperationLimit and runs them sequentially', async () => { + const buildLimit = new Semaphore(1) + await withContext( + async ({ injector, runner }) => { + await seedService(injector, { id: 'build-a', buildCommand: 'sleep 0.3' }) + await seedService(injector, { id: 'build-b', buildCommand: 'sleep 0.3' }) + + const promiseA = runner.buildService('build-a', testTrigger) + const promiseB = runner.buildService('build-b', testTrigger) + + await waitFor(() => buildLimit.runningCount.getValue() === 1) + + expect(buildLimit.runningCount.getValue()).toBe(1) + expect(buildLimit.pendingCount.getValue()).toBe(1) + expect(injector.get(ProcessRunner).processes.size).toBe(1) + + await Promise.all([promiseA, promiseB]) + + expect(buildLimit.runningCount.getValue()).toBe(0) + expect(buildLimit.completedCount.getValue()).toBe(2) + }, + { setup: (injector) => injector.bind(BuildOperationLimit, () => buildLimit) }, + ) + }, 10_000) + + it('install and build pools are independent (one install + one build run together)', async () => { + const installLimit = new Semaphore(1) + const buildLimit = new Semaphore(1) + await withContext( + async ({ injector, runner }) => { + await seedService(injector, { + id: 'mixed-a', + installCommand: 'sleep 30', + buildCommand: 'sleep 30', + }) + await seedService(injector, { + id: 'mixed-b', + installCommand: 'sleep 30', + buildCommand: 'sleep 30', + }) + + const installPromise = runner.installService('mixed-a', testTrigger) + const buildPromise = runner.buildService('mixed-b', testTrigger) + + await waitFor(() => installLimit.runningCount.getValue() === 1 && buildLimit.runningCount.getValue() === 1) + + killAllRunningProcesses(injector) + await Promise.allSettled([installPromise, buildPromise]) + }, + { + setup: (injector) => { + injector.bind(InstallOperationLimit, () => installLimit) + injector.bind(BuildOperationLimit, () => buildLimit) + }, + }, + ) + }) + }) }) diff --git a/service/src/services/one-shot-command-runner.ts b/service/src/services/one-shot-command-runner.ts index f1b4613..15efbdd 100644 --- a/service/src/services/one-shot-command-runner.ts +++ b/service/src/services/one-shot-command-runner.ts @@ -1,5 +1,6 @@ import type { ChildProcess } from 'child_process' import { type Injector, defineService, type Token } from '@furystack/inject' +import type { Semaphore } from '@furystack/utils' import type { ServiceStateEvent } from 'common' import { randomUUID } from 'crypto' @@ -7,6 +8,7 @@ import { useSystemIdentityContext } from '@furystack/core' import { ConflictError, ValidationError } from '../utils/domain-error.js' import { getServiceOrThrow } from '../utils/get-service-or-throw.js' import { resolveServiceCwd } from '../utils/resolve-service-cwd.js' +import { BuildOperationLimit, InstallOperationLimit } from './operation-limits.js' import { attachProcessIO } from './process-io-attacher.js' import { ProcessRunner } from './process-runner.js' import { ServiceEnvResolver } from './service-env-resolver.js' @@ -15,12 +17,18 @@ import type { TriggerContext } from './trigger-context.js' /** Executes one-shot commands (install, build) for services and tracks their status */ class OneShotCommandRunnerImpl { + private installLimit: Semaphore + private buildLimit: Semaphore + constructor( private readonly runner: ProcessRunner, private readonly envResolver: ServiceEnvResolver, private readonly statusManager: ServiceStatusManager, public readonly injector: Injector, - ) {} + ) { + this.installLimit = injector.get(InstallOperationLimit) + this.buildLimit = injector.get(BuildOperationLimit) + } private elevatedInjector?: Injector @@ -63,8 +71,34 @@ class OneShotCommandRunnerImpl { ) } + // Marked pending the moment the request is accepted, so concurrent triggers + // for the same service hit the conflict guard above even while we're queued + // behind the per-type semaphore. The flag flips off once the child has + // spawned (see `spawnAndAwait`) — at that point `processes` covers the + // running case. this.runner.pendingOperations.add(serviceId) + const limit = purpose === 'install' ? this.installLimit : this.buildLimit + try { + await limit.execute(() => this.spawnAndAwait(serviceId, command, cwd, purpose, trigger)) + } catch (error) { + // Catch leftover pendingOperations entries when the semaphore rejects + // before `spawnAndAwait` runs (e.g. semaphore disposed) or when + // `spawnAndAwait` throws before reaching its own cleanup. + if (this.runner.pendingOperations.has(serviceId)) { + this.runner.pendingOperations.delete(serviceId) + } + throw error + } + } + + private async spawnAndAwait( + serviceId: string, + command: string, + cwd: string, + purpose: 'install' | 'build', + trigger: TriggerContext, + ): Promise { const progressEvent: ServiceStateEvent = purpose === 'install' ? 'install-started' : 'build-started' const doneEvent: ServiceStateEvent = purpose === 'install' ? 'install-completed' : 'build-completed' const failedEvent: ServiceStateEvent = purpose === 'install' ? 'install-failed' : 'build-failed' diff --git a/service/src/services/operation-limits.spec.ts b/service/src/services/operation-limits.spec.ts new file mode 100644 index 0000000..03a6cad --- /dev/null +++ b/service/src/services/operation-limits.spec.ts @@ -0,0 +1,54 @@ +import { Injector } from '@furystack/inject' +import { usingAsync } from '@furystack/utils' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { BuildOperationLimit, GitOperationLimit, InstallOperationLimit } from './operation-limits.js' + +const ENV_KEYS = [ + 'STACK_CRAFT_MAX_PARALLEL_GIT', + 'STACK_CRAFT_MAX_PARALLEL_INSTALLS', + 'STACK_CRAFT_MAX_PARALLEL_BUILDS', +] + +describe('operation limits', () => { + const original: Record = {} + + beforeEach(() => { + for (const key of ENV_KEYS) original[key] = process.env[key] + }) + + afterEach(() => { + for (const key of ENV_KEYS) { + if (original[key] === undefined) delete process.env[key] + else process.env[key] = original[key] + } + }) + + it('uses defaults (10 / 3 / 1) when no env vars are set', () => + usingAsync(new Injector(), async (injector) => { + for (const key of ENV_KEYS) delete process.env[key] + expect(injector.get(GitOperationLimit).getMaxConcurrent()).toBe(10) + expect(injector.get(InstallOperationLimit).getMaxConcurrent()).toBe(3) + expect(injector.get(BuildOperationLimit).getMaxConcurrent()).toBe(1) + })) + + it('honours valid integer overrides from env', () => + usingAsync(new Injector(), async (injector) => { + process.env.STACK_CRAFT_MAX_PARALLEL_GIT = '5' + process.env.STACK_CRAFT_MAX_PARALLEL_INSTALLS = '7' + process.env.STACK_CRAFT_MAX_PARALLEL_BUILDS = '2' + expect(injector.get(GitOperationLimit).getMaxConcurrent()).toBe(5) + expect(injector.get(InstallOperationLimit).getMaxConcurrent()).toBe(7) + expect(injector.get(BuildOperationLimit).getMaxConcurrent()).toBe(2) + })) + + it('falls back to defaults on garbage / non-positive overrides', () => + usingAsync(new Injector(), async (injector) => { + process.env.STACK_CRAFT_MAX_PARALLEL_GIT = 'banana' + process.env.STACK_CRAFT_MAX_PARALLEL_INSTALLS = '0' + process.env.STACK_CRAFT_MAX_PARALLEL_BUILDS = '-3' + expect(injector.get(GitOperationLimit).getMaxConcurrent()).toBe(10) + expect(injector.get(InstallOperationLimit).getMaxConcurrent()).toBe(3) + expect(injector.get(BuildOperationLimit).getMaxConcurrent()).toBe(1) + })) +}) diff --git a/service/src/services/operation-limits.ts b/service/src/services/operation-limits.ts new file mode 100644 index 0000000..d08d668 --- /dev/null +++ b/service/src/services/operation-limits.ts @@ -0,0 +1,54 @@ +import { defineService, type Token } from '@furystack/inject' +import { Semaphore } from '@furystack/utils' + +const DEFAULTS = { + GIT: 10, + INSTALL: 3, + BUILD: 1, +} as const + +const parseLimit = (raw: string | undefined, fallback: number): number => { + if (!raw) return fallback + const parsed = parseInt(raw, 10) + return Number.isFinite(parsed) && parsed >= 1 ? parsed : fallback +} + +/** + * Caps concurrent git CLI operations (clone, fetch, pull, ls-remote, branch + * reads). Network-bound so a moderately high default is fine; the cap mainly + * prevents the periodic `GitWatcher` from saturating the system when many + * services share a stack. + * + * Tunable via `STACK_CRAFT_MAX_PARALLEL_GIT` (default 10). + */ +export const GitOperationLimit: Token = defineService({ + name: 'app/GitOperationLimit', + lifetime: 'singleton', + factory: () => new Semaphore(parseLimit(process.env.STACK_CRAFT_MAX_PARALLEL_GIT, DEFAULTS.GIT)), +}) + +/** + * Caps concurrent install operations across services. Installs are + * network-and-disk heavy and frequently write to shared package caches + * (yarn/pnpm/npm/nuget), where concurrent writers can race. + * + * Tunable via `STACK_CRAFT_MAX_PARALLEL_INSTALLS` (default 3). + */ +export const InstallOperationLimit: Token = defineService({ + name: 'app/InstallOperationLimit', + lifetime: 'singleton', + factory: () => new Semaphore(parseLimit(process.env.STACK_CRAFT_MAX_PARALLEL_INSTALLS, DEFAULTS.INSTALL)), +}) + +/** + * Caps concurrent build operations across services. Builds are CPU-bound and + * each one already saturates multiple cores (tsc -b, webpack, dotnet build, …), + * so running more than a handful in parallel typically thrashes the machine. + * + * Tunable via `STACK_CRAFT_MAX_PARALLEL_BUILDS` (default 1). + */ +export const BuildOperationLimit: Token = defineService({ + name: 'app/BuildOperationLimit', + lifetime: 'singleton', + factory: () => new Semaphore(parseLimit(process.env.STACK_CRAFT_MAX_PARALLEL_BUILDS, DEFAULTS.BUILD)), +}) diff --git a/service/src/utils/run-cli.ts b/service/src/utils/run-cli.ts index b8d54a5..754adb9 100644 --- a/service/src/utils/run-cli.ts +++ b/service/src/utils/run-cli.ts @@ -9,6 +9,12 @@ export type RunCliOptions = { * `GIT_ASKPASS` that the host might have pre-configured). */ env?: Record + /** + * When supplied, an `abort` event triggers the same two-stage process-group + * kill that the timeout uses. Wired to `Semaphore.execute`'s task signal so + * in-flight ops cancel cleanly when the injector disposes. + */ + signal?: AbortSignal } export type RunCliResult = { @@ -91,6 +97,7 @@ export const runCli = (command: string, args: readonly string[], options: RunCli let stdout = '' let stderr = '' let timedOut = false + let aborted = false let killTimer: ReturnType | null = null child.stdout?.on('data', (chunk: Buffer) => { @@ -100,19 +107,36 @@ export const runCli = (command: string, args: readonly string[], options: RunCli stderr += chunk.toString() }) + const startKillSequence = (): void => { + if (child.pid == null) return + killProcessGroup(child.pid, 'SIGTERM') + killTimer = setTimeout(() => { + if (child.pid != null) killProcessGroup(child.pid, 'SIGKILL') + }, SIGKILL_GRACE_MS) + } + const timeoutTimer = setTimeout(() => { timedOut = true - if (child.pid != null) { - killProcessGroup(child.pid, 'SIGTERM') - killTimer = setTimeout(() => { - if (child.pid != null) killProcessGroup(child.pid, 'SIGKILL') - }, SIGKILL_GRACE_MS) - } + startKillSequence() }, options.timeoutMs) + const onAbort = (): void => { + aborted = true + startKillSequence() + } + + if (options.signal) { + if (options.signal.aborted) { + onAbort() + } else { + options.signal.addEventListener('abort', onAbort, { once: true }) + } + } + const cleanup = (): void => { clearTimeout(timeoutTimer) if (killTimer) clearTimeout(killTimer) + options.signal?.removeEventListener('abort', onAbort) } child.once('error', (error) => { @@ -126,6 +150,12 @@ export const runCli = (command: string, args: readonly string[], options: RunCli const stderrPart = stderr.trim() ? `\nstderr: ${stderr.trim()}` : '' const commandLine = [command, ...args].join(' ') + if (aborted) { + const reason = options.signal?.reason + reject(reason instanceof Error ? reason : new Error(`${commandLine}${cwdPart} aborted${stderrPart}`)) + return + } + if (timedOut) { reject(new Error(`${commandLine}${cwdPart} timed out after ${options.timeoutMs}ms${stderrPart}`)) return From 6633a8f931681c4359dc7601d3be51814684ad62 Mon Sep 17 00:00:00 2001 From: Gallay Lajos Date: Mon, 1 Jun 2026 13:22:14 +0200 Subject: [PATCH 3/3] updated versioning and changelogs --- .yarn/changelogs/service.2f2de4cf.md | 71 ++++++++++++++++++++++++ .yarn/changelogs/stack-craft.2f2de4cf.md | 25 +++++++++ .yarn/versions/2f2de4cf.yml | 3 + 3 files changed, 99 insertions(+) create mode 100644 .yarn/changelogs/service.2f2de4cf.md create mode 100644 .yarn/changelogs/stack-craft.2f2de4cf.md create mode 100644 .yarn/versions/2f2de4cf.yml diff --git a/.yarn/changelogs/service.2f2de4cf.md b/.yarn/changelogs/service.2f2de4cf.md new file mode 100644 index 0000000..af8b64a --- /dev/null +++ b/.yarn/changelogs/service.2f2de4cf.md @@ -0,0 +1,71 @@ + +# service + +## ✨ Features + +### Per-operation concurrency pools (`GitOperationLimit`, `InstallOperationLimit`, `BuildOperationLimit`) + +Three new injector-resolved `Semaphore` singletons (from `@furystack/utils`) cap the number of CLI operations the service runs in parallel, broken down by cost profile so a long build never blocks a quick `git fetch`: + +- **`GitOperationLimit`** (default `10`) — caps every `GitService` call (`clone`, `fetch`, `pull`, `lsRemote`, branch reads, status). Network-bound; the cap mainly stops the periodic `GitWatcher` from saturating the system on stacks with many services. +- **`InstallOperationLimit`** (default `3`) — caps `OneShotCommandRunner.installService`. Installs are network + disk heavy and frequently write to shared package caches (yarn / pnpm / npm / nuget) where concurrent writers can race. +- **`BuildOperationLimit`** (default `1`) — caps `OneShotCommandRunner.buildService`. Each build already saturates multiple cores (tsc -b, webpack, dotnet build, …); running more than a handful in parallel typically thrashes the machine. + +All three are tunable via env vars (`STACK_CRAFT_MAX_PARALLEL_GIT`, `STACK_CRAFT_MAX_PARALLEL_INSTALLS`, `STACK_CRAFT_MAX_PARALLEL_BUILDS`); non-numeric or non-positive values fall back to the default. The semaphore signal is threaded through `runCli`, so disposing the injector aborts in-flight ops with a process-group kill instead of orphaning child processes. Per-service guards (`pendingOperations` / `processes` map) stack on top — the conflict check still rejects a duplicate trigger immediately rather than queueing it. + +The pre-existing `setupServices` batch flow (which previously fanned out 22 concurrent setups when no service dependencies were declared) now serializes naturally through these pools. Per-service status flips to `installing` / `building` only after the slot is acquired, so the audit log timestamps reflect actual work, not queue time. + +### `GitService.lsRemote(url)` — consolidated repository accessibility probe + +New method on `GitService` that runs `git ls-remote --exit-code ` with the standard `GitService` env hardening (`GIT_TERMINAL_PROMPT=0`, `BatchMode=yes` ssh) and shares the `GitOperationLimit` pool. Replaces the two ad-hoc `execFile('git', ['ls-remote', '--exit-code', url], { timeout: 15000 })` callers — `validate-repo-action` and the MCP `validate_repository` tool — which previously had no env hardening and no process-group kill. Default timeout is 30 s. + +## 🐛 Bug Fixes + +### Git update / pull no longer feels frozen + +`GitService` previously used `promisify(execFile)` with a `timeout` option for every git call. When the timeout fired, Node sent SIGTERM only to the parent git process, leaving grandchildren (credential helpers, ssh, `git-remote-https`, GUI askpass dialogs) holding the inherited stdio pipes. Because `execFile`'s callback fires on stream `close` (not on parent exit), the promise could stay pending well past the nominal timeout — visible in audit logs as a 60-second silence followed by a bare `Command failed: git fetch --all --prune\n` with no stderr, which is the documented Node gotcha (nodejs/node#2098). + +Every git invocation now goes through a new spawn-based `runCli` helper that: + +- Uses `detached: true` on POSIX so the child becomes a process-group leader, then kills the whole group with `process.kill(-pid, signal)` on timeout. On Windows it walks the child tree with `taskkill /T` (escalating to `/F` only on `SIGKILL`). +- Escalates SIGTERM → SIGKILL after a 2 s grace. +- Captures stderr and surfaces it in the rejection message, replacing the previous opaque `Command failed: \n`. +- Forces non-interactive mode for git: `GIT_TERMINAL_PROMPT=0`, removes inherited `GIT_ASKPASS` / `SSH_ASKPASS`, sets `SSH_ASKPASS_REQUIRE=never`, and pins `GIT_SSH_COMMAND='ssh -o BatchMode=yes -o ConnectTimeout=15 -o StrictHostKeyChecking=accept-new'`. Missing or expired credentials now fail fast instead of waiting for a prompt nobody can answer from a backend service. + +Network-side timeouts also relaxed where appropriate: `fetch` and `pull` go from 60 s to 90 s; `clone` stays at 5 min; cheap local reads stay at 5–10 s. + +### `gh auth status` prerequisite check is now non-interactive + +The `github-cli` prerequisite check ran `gh auth status` through `execFile` without any env hardening, so a stale GitHub CLI token could trigger a browser-based re-auth flow that blocked indefinitely. `GH_PROMPT_DISABLED=1` and `GH_NO_UPDATE_NOTIFIER=1` are now passed on the call, and the same process-group kill machinery applies on timeout. + +## ♻️ Refactoring + +### `runCli` helper replaces `promisify(execFile)` across the service layer + +New helper in `service/src/utils/run-cli.ts` is the canonical way to invoke any CLI from the service. Public API: + +```typescript +import { runCli } from '../utils/run-cli.js' + +const { stdout, stderr } = await runCli('node', ['--version'], { + cwd: '/path', + timeoutMs: 30_000, + env: { GH_PROMPT_DISABLED: '1' }, // overrides; set a key to `undefined` to strip an inherited var + signal: abortController.signal, // optional; aborts via process-group kill, same machinery as timeout +}) +``` + +Migrated callers — every external CLI invocation in the service now goes through `runCli` (or through `GitService`, which itself uses `runCli`): + +- `validate-repo-action` and MCP `validate_repository` tool — now use `GitService.lsRemote` (which routes through `runCli` with git env hardening + `GitOperationLimit`). +- `check-prerequisite-action` — every check (`node --version`, `yarn --version` / `cmd.exe /c yarn --version` shim, `dotnet --list-sdks` / `--list-runtimes` / `nuget list source`, `git --version`, `gh auth status`, custom-script via `cmd.exe` / `/bin/sh`) now uses `runCli`. + +The two file-level results: dropped `import { execFile } from 'child_process'` + `import { promisify } from 'util'` from three actions and the MCP repository-tools registrar; consolidated process-group kill, env hardening, and stderr-rich error reporting into one tested helper. + +## 🧪 Tests + +- Added `service/src/utils/run-cli.spec.ts` — env passthrough, env strip-on-`undefined`, `stdio` / `windowsHide` / `cwd`, POSIX `detached: true`, success path, non-zero exit reports stderr + cwd, child `error` event, two-stage timeout kill (POSIX + Windows branches), no-kill-before-deadline. +- Added `service/src/services/git-service-runner.spec.ts` — git env hardening (`GIT_TERMINAL_PROMPT=0`, askpass strip, `BatchMode=yes` ssh), spawn options, timeout-and-kill, non-zero exit error context, and `GitOperationLimit` queueing (3 concurrent fetches under `Semaphore(2)` — first two spawn, third waits, third spawns after first drains). +- Added `service/src/services/operation-limits.spec.ts` — defaults (10 / 3 / 1), valid integer overrides, and fallback on garbage / non-positive env values. +- Extended `service/src/services/one-shot-command-runner.spec.ts` — install and build calls serialize when their pool is capped to 1, and the install / build pools are independent (one install + one build run together). `withContext` now accepts a `setup(injector)` hook so tests can rebind operation limits before resolving the service. +- Updated `service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts` and `service/src/app-models/prerequisites/actions/check-prerequisite-action.spec.ts` — replaced the `vi.mock('child_process')` + `vi.mock('util')` execFile mocks with mocks of `runCli` / a stubbed `GitService.lsRemote` so the specs follow the new abstraction layer. diff --git a/.yarn/changelogs/stack-craft.2f2de4cf.md b/.yarn/changelogs/stack-craft.2f2de4cf.md new file mode 100644 index 0000000..05667b7 --- /dev/null +++ b/.yarn/changelogs/stack-craft.2f2de4cf.md @@ -0,0 +1,25 @@ + +# stack-craft + +## ✨ Features + +### Operation concurrency limits exposed in `.env.example` + +`.env.example` now documents three new tunables that cap concurrent CLI work in the service, so a long-running build never blocks a quick `git fetch` and the periodic `GitWatcher` cannot saturate the system on stacks with many services: + +```env +# STACK_CRAFT_MAX_PARALLEL_GIT=10 +# STACK_CRAFT_MAX_PARALLEL_INSTALLS=3 +# STACK_CRAFT_MAX_PARALLEL_BUILDS=1 +``` + +See the `service` changelog for the underlying `Semaphore`-backed pools and per-operation rationale (network-bound vs. shared-cache writes vs. CPU-bound). + +## ♻️ Refactoring + +- Every external CLI invocation in the service now goes through a single hardened spawn-based runner with cross-platform process-group kill — no more `promisify(execFile)` callers leaking grandchild processes past the nominal timeout. See the `service` changelog for the `runCli` helper, the `GitService` env hardening, and the migrated callers (validate-repo, MCP `validate_repository`, prerequisite checks). + +## 🐛 Bug Fixes + +- Fixed git update / pull operations sometimes appearing frozen for ~60 s before a bare `Command failed: …` error. Root cause and fix in the `service` changelog (nodejs/node#2098 — `execFile` timeout did not reach grandchild processes that held the inherited stdio pipes). +- Fixed `gh auth status` prerequisite check hanging indefinitely when GitHub CLI tried to launch an interactive browser auth flow. Details in the `service` changelog. diff --git a/.yarn/versions/2f2de4cf.yml b/.yarn/versions/2f2de4cf.yml new file mode 100644 index 0000000..01fecca --- /dev/null +++ b/.yarn/versions/2f2de4cf.yml @@ -0,0 +1,3 @@ +releases: + service: minor + stack-craft: minor