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
63 changes: 63 additions & 0 deletions src/cli-workflow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,4 +310,67 @@ describe('runMainWorkflow', () => {

await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).resolves.toBe(1);
});

it('calls collectDiagnosticLogs on startContainers failure when diagnosticLogs is enabled', async () => {
const startError = new Error('Squid container is unhealthy');
const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
const configWithDiagnostics: WrapperConfig = {
...baseConfig,
diagnosticLogs: true,
};
const dependencies: WorkflowDependencies = {
ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
setupHostIptables: jest.fn().mockResolvedValue(undefined),
writeConfigs: jest.fn().mockResolvedValue(undefined),
startContainers: jest.fn().mockRejectedValue(startError),
runAgentCommand: jest.fn(),
collectDiagnosticLogs,
};
const logger = createLogger();

await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError);

expect(collectDiagnosticLogs).toHaveBeenCalledWith(configWithDiagnostics.workDir);
expect(dependencies.runAgentCommand).not.toHaveBeenCalled();
});

it('does not call collectDiagnosticLogs on startContainers failure when diagnosticLogs is disabled', async () => {
const startError = new Error('Squid container is unhealthy');
const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
const dependencies: WorkflowDependencies = {
ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
setupHostIptables: jest.fn().mockResolvedValue(undefined),
writeConfigs: jest.fn().mockResolvedValue(undefined),
startContainers: jest.fn().mockRejectedValue(startError),
runAgentCommand: jest.fn(),
collectDiagnosticLogs,
};
const logger = createLogger();

await expect(runMainWorkflow(baseConfig, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError);

expect(collectDiagnosticLogs).not.toHaveBeenCalled();
});

it('rethrows startContainers error after collecting diagnostics', async () => {
const startError = new Error('docker compose failed');
const configWithDiagnostics: WrapperConfig = {
...baseConfig,
diagnosticLogs: true,
};
const performCleanup = jest.fn().mockResolvedValue(undefined);
const dependencies: WorkflowDependencies = {
ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
setupHostIptables: jest.fn().mockResolvedValue(undefined),
writeConfigs: jest.fn().mockResolvedValue(undefined),
startContainers: jest.fn().mockRejectedValue(startError),
runAgentCommand: jest.fn(),
collectDiagnosticLogs: jest.fn().mockResolvedValue(undefined),
};
const logger = createLogger();

await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup })).rejects.toBe(startError);
// performCleanup should NOT be called — that is the caller's (cli.ts) responsibility
expect(performCleanup).not.toHaveBeenCalled();
});
});
20 changes: 19 additions & 1 deletion src/cli-workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,25 @@ export async function runMainWorkflow(
await dependencies.writeConfigs(config);

// Step 2: Start containers
await dependencies.startContainers(config.workDir, config.allowedDomains, config.proxyLogsDir, config.skipPull);
try {
await dependencies.startContainers(config.workDir, config.allowedDomains, config.proxyLogsDir, config.skipPull);
} catch (startError) {
// Signal that containers may have been partially created so the caller's
// cleanup (stopContainers / docker compose down -v) will tear them down
// instead of leaving orphaned containers and networks.
onContainersStarted?.();

// Collect diagnostics for startup failures before containers are torn down.
// Must happen before performCleanup() / stopContainers() destroys them.
Comment on lines +74 to +83
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

If startContainers throws after partially creating containers (e.g., docker compose up brings some services up but ends unhealthy), onContainersStarted is never invoked. In cli.ts cleanup, that keeps containersStarted=false so stopContainers() won’t run, potentially leaving orphaned containers/networks after a startup failure. Consider signaling the caller that containers may exist (e.g., invoke a separate callback / set the flag before rethrow, or have cleanup attempt stopContainers() when a compose file exists) so failures don’t leak resources.

Copilot uses AI. Check for mistakes.
if (config.diagnosticLogs && dependencies.collectDiagnosticLogs) {
try {
await dependencies.collectDiagnosticLogs(config.workDir);
} catch (diagError) {
logger.warn('Failed to collect diagnostic logs; continuing with cleanup.', diagError);
}
}
throw startError;
}
onContainersStarted?.();

// Step 3: Wait for agent to complete
Expand Down
4 changes: 2 additions & 2 deletions src/docker-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2521,9 +2521,9 @@ export async function collectDiagnosticLogs(workDir: string): Promise<void> {
];

for (const container of containers) {
// Collect stdout+stderr from docker logs
// Collect stdout+stderr from docker logs (last 200 lines to keep files manageable)
try {
const result = await execa('docker', ['logs', container], { reject: false });
const result = await execa('docker', ['logs', '--tail', '200', container], { reject: false });
if (result.exitCode === 0) {
const combined = [result.stdout, result.stderr].filter(Boolean).join('\n').trim();
if (combined) {
Expand Down
Loading