Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/cli/src/commands/lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -601,9 +601,14 @@ describe("lifecycle command integration", () => {
.spyOn(process.stdout, "write")
.mockImplementation(() => true);

await recoverModule.default(["--dry-run"], baseOptions(configDir));
const isProcessRunning = vi.fn().mockReturnValue(false);

await recoverModule.default(["--dry-run"], baseOptions(configDir), {
isProcessRunning,
});

expect(orchestratorRunCli).not.toHaveBeenCalled();
expect(isProcessRunning).toHaveBeenCalledWith(999_999);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seam 이 실제로 평가됐다는 단언은 정확히 이슈가 요구한 가드입니다 👍

한 가지 남는 공백: 프로덕션 기본값이 여전히 진짜 프로브인지 를 지키는 자동 테스트가 없습니다. 누군가 recover.ts:63?? isProcessRunning 폴백을 지워도 이 스위트는 그대로 green 이고(주입 경로만 타므로), 실제 repo recover --dry-run 은 그때 깨집니다. 커버리지 리포트에서도 해당 파일 branch 가 28.57% 로 잡힌 이유입니다.

dependencies 를 넘기지 않고 dry-run 을 호출해 process.kill spy 가 불렸는지만 확인하는 케이스 하나면 이 방향도 닫힙니다. 이번 PR 범위 밖이라 봐도 무방하고, 저는 수동 블랙박스로 기본 경로가 살아있는 것을 확인했습니다(리뷰 본문 참고).


Generated by Claude Code

expect(
stdout.mock.calls.some((call) =>
String(call[0]).includes("acme/platform#7")
Expand Down
42 changes: 27 additions & 15 deletions packages/cli/src/commands/recover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ import { readFile, readdir } from "node:fs/promises";
import { join } from "node:path";
import type { GlobalOptions } from "../index.js";
import { runCli as orchestratorRunCli } from "@gh-symphony/orchestrator";
import {
resolveRuntimeRoot,
} from "../orchestrator-runtime.js";
import { resolveRuntimeRoot } from "../orchestrator-runtime.js";
import {
handleMissingManagedProjectConfig,
resolveManagedProjectConfig,
Expand All @@ -17,6 +15,10 @@ type RecoverCandidate = {
reason: string;
};

type RecoverDependencies = {
isProcessRunning?: (pid: number) => boolean;
};

function parseRecoverArgs(args: string[]): {
dryRun: boolean;
projectId?: string;
Expand All @@ -37,7 +39,8 @@ function parseRecoverArgs(args: string[]): {

const handler = async (
args: string[],
options: GlobalOptions
options: GlobalOptions,
dependencies: RecoverDependencies = {}
): Promise<void> => {
const parsed = parseRecoverArgs(args);

Expand All @@ -54,7 +57,11 @@ const handler = async (
const projectId = projectConfig.projectId;
if (parsed.dryRun) {
process.stdout.write("Dry run — scanning for stalled runs...\n");
const candidates = await listRecoverCandidates(runtimeRoot, projectId);
const candidates = await listRecoverCandidates(
runtimeRoot,
projectId,
dependencies.isProcessRunning ?? isProcessRunning

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 이 seam 은 --dry-run 분기에만 연결되어 있고, 실제 복구 경로(L82 orchestratorRunCli)에서는 dependencies 가 조용히 무시됩니다.

실제 복구는 orchestrator CLI 로 위임되고 그쪽에 이미 자체 seam(OrchestratorService.dependencies.isProcessRunning, #766)이 있으므로 동작상 문제는 없습니다. 다만 코드만 봐서는 "주입했는데 안 먹는" 경우가 있다는 걸 알 수 없으니, RecoverDependencies 선언부(L18)에 한 줄 주석으로 dry-run 분류 전용 이라는 점을 남겨두면 다음 사람이 헷갈리지 않을 것 같습니다. 블로커는 아닙니다.


Generated by Claude Code

);
if (options.json) {
process.stdout.write(JSON.stringify(candidates, null, 2) + "\n");
return;
Expand Down Expand Up @@ -85,7 +92,8 @@ export default handler;

async function listRecoverCandidates(
runtimeRoot: string,
projectId: string
projectId: string,
isRunning: (pid: number) => boolean
): Promise<RecoverCandidate[]> {
const runRoots = [
join(runtimeRoot, "runs"),
Expand Down Expand Up @@ -119,7 +127,7 @@ async function listRecoverCandidates(
continue;
}

const reason = detectRecoveryReason(run);
const reason = detectRecoveryReason(run, isRunning);
if (!reason) {
continue;
}
Expand All @@ -141,19 +149,23 @@ async function listRecoverCandidates(
return candidates;
}

function detectRecoveryReason(run: {
status: string;
processId: number | null;
startedAt: string | null;
nextRetryAt: string | null;
}): string | null {
function detectRecoveryReason(
run: {
status: string;
processId: number | null;
startedAt: string | null;
nextRetryAt: string | null;
},
isRunning: (pid: number) => boolean
): string | null {
if (run.processId) {
const startedAt = run.startedAt ? new Date(run.startedAt).getTime() : 0;
const runningForMs = Date.now() - startedAt;
if (isProcessRunning(run.processId) && runningForMs > 30 * 60 * 1000) {
const processRunning = isRunning(run.processId);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (의도 확인): 여기서 liveness 를 두 번 조회하던 것을 스냅샷 1회로 바꾼 것은 이슈 #769 가 요구한 범위(주입 가능한 프로브 + seam 단언)를 살짝 넘어서는 동작 변경입니다.

기존 코드에서는 두 process.kill 호출 사이에 프로세스가 죽으면 running && !stuck!running 으로 흘러 "worker process is no longer running" 으로 분류됐는데, 지금은 단일 스냅샷이라 그 TOCTOU 경로가 사라집니다.

미반영 요청은 아닙니다 — 하나의 분류가 "stuck" 과 "dead" 를 동시에 근거로 삼을 수 없게 되므로 오히려 일관적이고, syscall 도 절반이 됩니다. 다만 테스트 단언이 toHaveBeenCalledWith (호출 횟수 아님) 라서 이 변경 없이도 이슈는 닫혔을 것이므로, 의식적인 결정이었는지만 확인 부탁드립니다. 아래 세 분기 모두 base 와 출력이 바이트 단위로 동일한 것은 확인했습니다(리뷰 본문 Smoke Test 참고).


Generated by Claude Code

if (processRunning && runningForMs > 30 * 60 * 1000) {
return "worker appears stuck";
}
if (!isProcessRunning(run.processId)) {
if (!processRunning) {
return "worker process is no longer running";
}
}
Expand Down
Loading