Skip to content

Commit 81ed3a2

Browse files
authored
Feat/async git improvements (#20)
* feat: Async git improvements * feat: operation pools * updated versioning and changelogs
1 parent e2d8e79 commit 81ed3a2

17 files changed

Lines changed: 1253 additions & 97 deletions

.env.example

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,26 @@ DATABASE_URL=postgres://stackcraft:stackcraft@localhost:5433/stackcraft
4343
# Interval (ms) between MCP stale-session sweeps
4444
# MCP_SESSION_SWEEP_MS=60000
4545

46+
# --- Operation concurrency limits ---
47+
# Cap concurrent CLI operations across services. Each operation type has its own
48+
# pool, so e.g. a long-running build never blocks a quick `git fetch`.
49+
# Anything below 1 or non-numeric falls back to the default.
50+
51+
# Max concurrent git CLI operations (clone, fetch, pull, ls-remote, branch reads).
52+
# Network-bound; the cap mainly stops the periodic GitWatcher from saturating the
53+
# system when many services share a stack.
54+
# STACK_CRAFT_MAX_PARALLEL_GIT=10
55+
56+
# Max concurrent install operations across services. Installs are network + disk
57+
# heavy and frequently write to shared package caches (yarn/pnpm/npm/nuget) where
58+
# concurrent writers can race.
59+
# STACK_CRAFT_MAX_PARALLEL_INSTALLS=3
60+
61+
# Max concurrent build operations across services. Builds are CPU-bound and each
62+
# one already saturates multiple cores (tsc -b, webpack, dotnet build, …), so
63+
# running more than a handful in parallel typically thrashes the machine.
64+
# STACK_CRAFT_MAX_PARALLEL_BUILDS=1
65+
4666
# --- TLS / Remote access ---
4767
# StackCraft does not terminate TLS itself. For remote (non-localhost) deployments,
4868
# place a reverse proxy (e.g. Caddy, nginx, Traefik) in front of both the main
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<!-- version-type: minor -->
2+
# service
3+
4+
## ✨ Features
5+
6+
### Per-operation concurrency pools (`GitOperationLimit`, `InstallOperationLimit`, `BuildOperationLimit`)
7+
8+
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`:
9+
10+
- **`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.
11+
- **`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.
12+
- **`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.
13+
14+
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.
15+
16+
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.
17+
18+
### `GitService.lsRemote(url)` — consolidated repository accessibility probe
19+
20+
New method on `GitService` that runs `git ls-remote --exit-code <url>` 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.
21+
22+
## 🐛 Bug Fixes
23+
24+
### Git update / pull no longer feels frozen
25+
26+
`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).
27+
28+
Every git invocation now goes through a new spawn-based `runCli` helper that:
29+
30+
- 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`).
31+
- Escalates SIGTERM → SIGKILL after a 2 s grace.
32+
- Captures stderr and surfaces it in the rejection message, replacing the previous opaque `Command failed: <cmd>\n`.
33+
- 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.
34+
35+
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.
36+
37+
### `gh auth status` prerequisite check is now non-interactive
38+
39+
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.
40+
41+
## ♻️ Refactoring
42+
43+
### `runCli` helper replaces `promisify(execFile)` across the service layer
44+
45+
New helper in `service/src/utils/run-cli.ts` is the canonical way to invoke any CLI from the service. Public API:
46+
47+
```typescript
48+
import { runCli } from '../utils/run-cli.js'
49+
50+
const { stdout, stderr } = await runCli('node', ['--version'], {
51+
cwd: '/path',
52+
timeoutMs: 30_000,
53+
env: { GH_PROMPT_DISABLED: '1' }, // overrides; set a key to `undefined` to strip an inherited var
54+
signal: abortController.signal, // optional; aborts via process-group kill, same machinery as timeout
55+
})
56+
```
57+
58+
Migrated callers — every external CLI invocation in the service now goes through `runCli` (or through `GitService`, which itself uses `runCli`):
59+
60+
- `validate-repo-action` and MCP `validate_repository` tool — now use `GitService.lsRemote` (which routes through `runCli` with git env hardening + `GitOperationLimit`).
61+
- `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`.
62+
63+
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.
64+
65+
## 🧪 Tests
66+
67+
- 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.
68+
- 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).
69+
- Added `service/src/services/operation-limits.spec.ts` — defaults (10 / 3 / 1), valid integer overrides, and fallback on garbage / non-positive env values.
70+
- 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.
71+
- 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.
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!-- version-type: minor -->
2+
# stack-craft
3+
4+
## ✨ Features
5+
6+
### Operation concurrency limits exposed in `.env.example`
7+
8+
`.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:
9+
10+
```env
11+
# STACK_CRAFT_MAX_PARALLEL_GIT=10
12+
# STACK_CRAFT_MAX_PARALLEL_INSTALLS=3
13+
# STACK_CRAFT_MAX_PARALLEL_BUILDS=1
14+
```
15+
16+
See the `service` changelog for the underlying `Semaphore`-backed pools and per-operation rationale (network-bound vs. shared-cache writes vs. CPU-bound).
17+
18+
## ♻️ Refactoring
19+
20+
- 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).
21+
22+
## 🐛 Bug Fixes
23+
24+
- 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).
25+
- Fixed `gh auth status` prerequisite check hanging indefinitely when GitHub CLI tried to launch an interactive browser auth flow. Details in the `service` changelog.

.yarn/versions/2f2de4cf.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
releases:
2+
service: minor
3+
stack-craft: minor

service/src/app-models/github-repositories/actions/validate-repo-action.spec.ts

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,25 @@ import { GitHubRepositoryDataSet } from '../../data-store/tokens.js'
22
import { getDataSetFor } from '@furystack/repository'
33

44
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
import { GitService } from '../../../services/git-service.js'
57
import { createMockActionContext, withTestInjector } from '../../../test-helpers.js'
68
import { ValidateRepoAction } from './validate-repo-action.js'
7-
const execFileMock = vi.hoisted(() =>
8-
vi.fn<(cmd: string, args: string[], options: { timeout: number }) => Promise<{ stdout: string; stderr: string }>>(),
9-
)
109

11-
vi.mock('child_process', () => ({
12-
execFile: (...args: unknown[]) => execFileMock(...(args as [string, string[], { timeout: number }])),
13-
}))
10+
const lsRemoteMock = vi.fn<(url: string) => Promise<void>>()
1411

15-
vi.mock('util', () => ({
16-
promisify: () => execFileMock,
17-
}))
12+
const bindGitServiceStub = (injector: { bind: (token: typeof GitService, factory: () => GitService) => void }) => {
13+
injector.bind(GitService, () => ({ lsRemote: lsRemoteMock }) as unknown as GitService)
14+
}
1815

1916
describe('ValidateRepoAction', () => {
2017
beforeEach(() => {
21-
vi.clearAllMocks()
18+
lsRemoteMock.mockReset()
2219
})
2320

2421
it('should return accessible: true when git ls-remote succeeds', async () => {
2522
await withTestInjector(async ({ elevated }) => {
23+
bindGitServiceStub(elevated)
2624
const ts = new Date().toISOString()
2725
await getDataSetFor(elevated, GitHubRepositoryDataSet).add(elevated, {
2826
id: 'repo-1',
@@ -34,26 +32,21 @@ describe('ValidateRepoAction', () => {
3432
updatedAt: ts,
3533
})
3634

37-
execFileMock.mockResolvedValue({ stdout: 'abc123\tHEAD\n', stderr: '' })
35+
lsRemoteMock.mockResolvedValue(undefined)
3836

3937
const result = await ValidateRepoAction(
4038
createMockActionContext({ injector: elevated, urlParams: { id: 'repo-1' } }),
4139
)
4240

4341
const body = result.chunk as { accessible: boolean }
4442
expect(body.accessible).toBe(true)
45-
expect(execFileMock).toHaveBeenCalledWith(
46-
'git',
47-
['ls-remote', '--exit-code', 'https://github.com/user/repo.git'],
48-
{
49-
timeout: 15000,
50-
},
51-
)
43+
expect(lsRemoteMock).toHaveBeenCalledWith('https://github.com/user/repo.git')
5244
})
5345
})
5446

5547
it('should return accessible: false when git ls-remote fails', async () => {
5648
await withTestInjector(async ({ elevated }) => {
49+
bindGitServiceStub(elevated)
5750
const ts = new Date().toISOString()
5851
await getDataSetFor(elevated, GitHubRepositoryDataSet).add(elevated, {
5952
id: 'repo-2',
@@ -65,7 +58,7 @@ describe('ValidateRepoAction', () => {
6558
updatedAt: ts,
6659
})
6760

68-
execFileMock.mockRejectedValue(new Error('Repository not found'))
61+
lsRemoteMock.mockRejectedValue(new Error('Repository not found'))
6962

7063
const result = await ValidateRepoAction(
7164
createMockActionContext({ injector: elevated, urlParams: { id: 'repo-2' } }),
@@ -79,6 +72,7 @@ describe('ValidateRepoAction', () => {
7972

8073
it('should throw 404 when repository does not exist', async () => {
8174
await withTestInjector(async ({ elevated }) => {
75+
bindGitServiceStub(elevated)
8276
await expect(
8377
ValidateRepoAction(createMockActionContext({ injector: elevated, urlParams: { id: 'nonexistent' } })),
8478
).rejects.toThrow('Repository not found')

service/src/app-models/github-repositories/actions/validate-repo-action.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ import { RequestError } from '@furystack/rest'
55
import { JsonResult, type RequestAction } from '@furystack/rest-service'
66
import type { ValidateRepoEndpoint } from 'common'
77

8-
import { execFile } from 'child_process'
9-
import { promisify } from 'util'
10-
const execFileAsync = promisify(execFile)
8+
import { GitService } from '../../../services/git-service.js'
119

1210
export const ValidateRepoAction: RequestAction<ValidateRepoEndpoint> = async ({ injector, getUrlParams }) => {
1311
const logger = getLogger(injector).withScope('ValidateRepo')
@@ -22,7 +20,7 @@ export const ValidateRepoAction: RequestAction<ValidateRepoEndpoint> = async ({
2220
}
2321

2422
try {
25-
await execFileAsync('git', ['ls-remote', '--exit-code', repo.url], { timeout: 15000 })
23+
await injector.get(GitService).lsRemote(repo.url)
2624
await logger.information({ message: `Repository validated: ${repo.url}` })
2725
return JsonResult({ accessible: true })
2826
} catch (error) {

0 commit comments

Comments
 (0)