refactor: streamline team process termination and enhance team name conflict handling
- Introduced a new `killTeamProcess` function to consistently use SIGKILL for terminating team processes, preventing unintended cleanup of team files. - Updated multiple instances in `TeamProvisioningService` to utilize `killTeamProcess` for improved clarity and maintainability. - Enhanced `CreateTeamDialog` and `TeamListView` components to manage team name conflicts more effectively by incorporating active provisioning states.
This commit is contained in:
iliya
parent
b886b6bf83
commit
e60229622d
3 changed files with 65 additions and 33 deletions
|
|
@ -37,7 +37,7 @@ import { parseAllTeammateMessages } from '@shared/utils/teammateMessageParser';
|
|||
import { createCliAutoSuffixNameGuard } from '@shared/utils/teamMemberName';
|
||||
import { extractToolPreview, formatToolSummaryFromCalls } from '@shared/utils/toolSummary';
|
||||
import * as agentTeamsControllerModule from 'agent-teams-controller';
|
||||
import { spawn } from 'child_process';
|
||||
import { spawn, type ChildProcess } from 'child_process';
|
||||
import { randomUUID } from 'crypto';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
|
|
@ -56,6 +56,19 @@ import { TeamSentMessagesStore } from './TeamSentMessagesStore';
|
|||
import { isTaskCommentForwardingLive } from './TeamTaskCommentForwarding';
|
||||
import { TeamTaskReader } from './TeamTaskReader';
|
||||
|
||||
/**
|
||||
* Kill a team CLI process using SIGKILL (uncatchable).
|
||||
*
|
||||
* Newer Claude CLI versions (≥2.1.x) handle SIGTERM gracefully and run cleanup
|
||||
* that deletes team files (config.json, inboxes/, tasks/). SIGKILL prevents this.
|
||||
*
|
||||
* ALWAYS use this instead of killProcessTree() for team processes.
|
||||
* stdin.end() is also forbidden — EOF triggers the same cleanup.
|
||||
*/
|
||||
function killTeamProcess(child: ChildProcess | null | undefined): void {
|
||||
killProcessTree(child, 'SIGKILL');
|
||||
}
|
||||
|
||||
import type {
|
||||
CrossTeamSendResult,
|
||||
InboxMessage,
|
||||
|
|
@ -2376,10 +2389,9 @@ export class TeamProvisioningService {
|
|||
|
||||
run.processKilled = true;
|
||||
run.cancelRequested = true;
|
||||
// Note: do NOT call stdin.end() before kill — EOF triggers CLI's graceful
|
||||
// shutdown which deletes team files (config.json, inboxes/, tasks/).
|
||||
// SIGTERM alone kills the process before cleanup runs, preserving files.
|
||||
killProcessTree(run.child);
|
||||
// SIGKILL: newer Claude CLI versions handle SIGTERM gracefully and delete
|
||||
// team files during cleanup. SIGKILL is uncatchable — files are preserved.
|
||||
killTeamProcess(run.child);
|
||||
this.cleanupRun(run);
|
||||
}
|
||||
|
||||
|
|
@ -2406,7 +2418,7 @@ export class TeamProvisioningService {
|
|||
} else {
|
||||
logger.error(`[${run.teamName}] Auth failure detected in ${source} after retry — giving up`);
|
||||
run.processKilled = true;
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
const progress = updateProgress(run, 'failed', 'Authentication failed — CLI requires login', {
|
||||
error:
|
||||
'Claude CLI is not authenticated. Run `claude auth login` (or start `claude` and run `/login`) ' +
|
||||
|
|
@ -2441,7 +2453,7 @@ export class TeamProvisioningService {
|
|||
run.child.stderr?.removeAllListeners('data');
|
||||
run.child.removeAllListeners('error');
|
||||
run.child.removeAllListeners('exit');
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
run.child = null;
|
||||
}
|
||||
|
||||
|
|
@ -2527,7 +2539,7 @@ export class TeamProvisioningService {
|
|||
run.finalizingByTimeout = true;
|
||||
void (async () => {
|
||||
const readyOnTimeout = await this.tryCompleteAfterTimeout(run);
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
if (readyOnTimeout) return;
|
||||
|
||||
const hint = run.isLaunch ? ' (launch)' : '';
|
||||
|
|
@ -2828,7 +2840,7 @@ export class TeamProvisioningService {
|
|||
run.finalizingByTimeout = true;
|
||||
void (async () => {
|
||||
const readyOnTimeout = await this.tryCompleteAfterTimeout(run);
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
if (readyOnTimeout) {
|
||||
return; // cleanupRun already called inside tryCompleteAfterTimeout
|
||||
}
|
||||
|
|
@ -3231,7 +3243,7 @@ export class TeamProvisioningService {
|
|||
run.finalizingByTimeout = true;
|
||||
void (async () => {
|
||||
const readyOnTimeout = await this.tryCompleteAfterTimeout(run);
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
if (readyOnTimeout) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -3288,10 +3300,9 @@ export class TeamProvisioningService {
|
|||
|
||||
run.cancelRequested = true;
|
||||
run.processKilled = true;
|
||||
// Note: do NOT call stdin.end() before kill — EOF triggers CLI's graceful
|
||||
// shutdown which deletes team files (config.json, inboxes/, tasks/).
|
||||
// SIGTERM alone kills the process before cleanup runs, preserving files.
|
||||
killProcessTree(run.child);
|
||||
// SIGKILL: newer Claude CLI versions handle SIGTERM gracefully and delete
|
||||
// team files during cleanup. SIGKILL is uncatchable — files are preserved.
|
||||
killTeamProcess(run.child);
|
||||
const progress = updateProgress(run, 'cancelled', 'Provisioning cancelled by user');
|
||||
run.onProgress(progress);
|
||||
this.cleanupRun(run);
|
||||
|
|
@ -4347,8 +4358,9 @@ export class TeamProvisioningService {
|
|||
|
||||
/**
|
||||
* Stop the running process for a team. No-op if team is not running.
|
||||
* Always uses SIGKILL via killTeamProcess() to prevent CLI cleanup.
|
||||
*/
|
||||
stopTeam(teamName: string, signal?: NodeJS.Signals): void {
|
||||
stopTeam(teamName: string): void {
|
||||
const runId = this.getTrackedRunId(teamName);
|
||||
if (!runId) {
|
||||
return;
|
||||
|
|
@ -4364,27 +4376,24 @@ export class TeamProvisioningService {
|
|||
}
|
||||
run.processKilled = true;
|
||||
run.cancelRequested = true;
|
||||
// Note: do NOT call stdin.end() before kill — EOF triggers CLI's graceful
|
||||
// shutdown which deletes team files (config.json, inboxes/, tasks/).
|
||||
// SIGTERM/SIGKILL kills the process before cleanup runs, preserving files.
|
||||
killProcessTree(run.child, signal);
|
||||
killTeamProcess(run.child);
|
||||
const progress = updateProgress(run, 'disconnected', 'Team stopped by user');
|
||||
run.onProgress(progress);
|
||||
this.cleanupRun(run);
|
||||
logger.info(`[${teamName}] Process stopped (signal=${signal ?? 'SIGTERM'})`);
|
||||
logger.info(`[${teamName}] Process stopped (SIGKILL)`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Stop all running team processes. Called during app shutdown.
|
||||
* Uses SIGKILL (uncatchable) to guarantee the process dies instantly
|
||||
* without any cleanup — prevents CLI from deleting team files on exit.
|
||||
* Uses killTeamProcess() (SIGKILL) to guarantee instant death
|
||||
* without CLI cleanup that would delete team files.
|
||||
*/
|
||||
stopAllTeams(): void {
|
||||
const alive = this.getAliveTeams();
|
||||
if (alive.length === 0) return;
|
||||
logger.info(`Killing all team processes on shutdown (SIGKILL): ${alive.join(', ')}`);
|
||||
for (const teamName of alive) {
|
||||
this.stopTeam(teamName, 'SIGKILL');
|
||||
this.stopTeam(teamName);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -4724,7 +4733,7 @@ export class TeamProvisioningService {
|
|||
run.onProgress(progress);
|
||||
// Kill the process on provisioning error
|
||||
run.processKilled = true;
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
this.cleanupRun(run);
|
||||
} else if (run.provisioningComplete) {
|
||||
// Post-provisioning error: process alive, waiting for input.
|
||||
|
|
@ -5389,7 +5398,7 @@ export class TeamProvisioningService {
|
|||
});
|
||||
run.onProgress(progress);
|
||||
run.processKilled = true;
|
||||
killProcessTree(run.child);
|
||||
killTeamProcess(run.child);
|
||||
this.cleanupRun(run);
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -15,7 +15,10 @@ import { getTeamColorSet, getThemedBadge } from '@renderer/constants/teamColors'
|
|||
import { useBranchSync } from '@renderer/hooks/useBranchSync';
|
||||
import { useTheme } from '@renderer/hooks/useTheme';
|
||||
import { useStore } from '@renderer/store';
|
||||
import { getCurrentProvisioningProgressForTeam } from '@renderer/store/slices/teamSlice';
|
||||
import {
|
||||
getCurrentProvisioningProgressForTeam,
|
||||
isTeamProvisioningActive,
|
||||
} from '@renderer/store/slices/teamSlice';
|
||||
import { buildMemberColorMap } from '@renderer/utils/memberHelpers';
|
||||
import { buildTaskCountsByTeam, normalizePath } from '@renderer/utils/pathNormalize';
|
||||
import { getBaseName } from '@renderer/utils/pathUtils';
|
||||
|
|
@ -241,6 +244,13 @@ export const TeamListView = (): React.JSX.Element => {
|
|||
[currentProvisioningRunIdByTeam, provisioningRuns]
|
||||
);
|
||||
|
||||
/** Team names currently in active provisioning — prevents name conflicts in create dialog. */
|
||||
const provisioningTeamNames = useMemo(() => {
|
||||
return Object.keys(currentProvisioningRunIdByTeam).filter((teamName) =>
|
||||
isTeamProvisioningActive(provisioningState, teamName)
|
||||
);
|
||||
}, [currentProvisioningRunIdByTeam, provisioningState]);
|
||||
|
||||
// Fetch alive teams on mount and when teams list changes
|
||||
useEffect(() => {
|
||||
if (!electronMode) return;
|
||||
|
|
@ -536,6 +546,7 @@ export const TeamListView = (): React.JSX.Element => {
|
|||
provisioningErrorsByTeam={provisioningErrorByTeam}
|
||||
clearProvisioningError={clearProvisioningError}
|
||||
existingTeamNames={teams.map((t) => t.teamName)}
|
||||
provisioningTeamNames={provisioningTeamNames}
|
||||
activeTeams={activeTeams}
|
||||
initialData={copyData ?? undefined}
|
||||
defaultProjectPath={currentProjectPath}
|
||||
|
|
|
|||
|
|
@ -83,6 +83,8 @@ interface CreateTeamDialogProps {
|
|||
provisioningErrorsByTeam: Record<string, string | null>;
|
||||
clearProvisioningError?: (teamName?: string) => void;
|
||||
existingTeamNames: string[];
|
||||
/** Team names currently in active provisioning (launching) — used to prevent name conflicts. */
|
||||
provisioningTeamNames?: string[];
|
||||
activeTeams?: ActiveTeamRef[];
|
||||
initialData?: TeamCopyData;
|
||||
defaultProjectPath?: string | null;
|
||||
|
|
@ -207,6 +209,7 @@ export const CreateTeamDialog = ({
|
|||
provisioningErrorsByTeam,
|
||||
clearProvisioningError,
|
||||
existingTeamNames,
|
||||
provisioningTeamNames = [],
|
||||
activeTeams,
|
||||
initialData,
|
||||
defaultProjectPath,
|
||||
|
|
@ -339,7 +342,12 @@ export const CreateTeamDialog = ({
|
|||
|
||||
const effectiveCwd = cwdMode === 'project' ? selectedProjectPath.trim() : customCwd.trim();
|
||||
const dialogTeamNameKey = sanitizeTeamName(teamName.trim());
|
||||
const suggestedTeamName = getNextSuggestedTeamName(existingTeamNames);
|
||||
/** All taken names: existing teams + teams currently being provisioned. */
|
||||
const allTakenTeamNames = useMemo(
|
||||
() => [...new Set([...existingTeamNames, ...provisioningTeamNames])],
|
||||
[existingTeamNames, provisioningTeamNames]
|
||||
);
|
||||
const suggestedTeamName = getNextSuggestedTeamName(allTakenTeamNames);
|
||||
|
||||
// Clear stale provisioning error when dialog opens
|
||||
useEffect(() => {
|
||||
|
|
@ -552,6 +560,9 @@ export const CreateTeamDialog = ({
|
|||
);
|
||||
|
||||
const sanitizedTeamName = sanitizeTeamName(teamName.trim());
|
||||
const isNameProvisioning =
|
||||
provisioningTeamNames.includes(sanitizedTeamName) &&
|
||||
!existingTeamNames.includes(sanitizedTeamName);
|
||||
|
||||
const request = useMemo<TeamCreateRequest>(
|
||||
() => ({
|
||||
|
|
@ -640,9 +651,10 @@ export const CreateTeamDialog = ({
|
|||
}, [conflictingTeam?.teamName, effectiveCwd]);
|
||||
|
||||
const handleSubmit = (): void => {
|
||||
if (existingTeamNames.includes(sanitizedTeamName)) {
|
||||
setFieldErrors({ teamName: 'Team name already exists' });
|
||||
setLocalError('Team name already exists');
|
||||
if (allTakenTeamNames.includes(sanitizedTeamName)) {
|
||||
const msg = isNameProvisioning ? 'Team is currently launching' : 'Team name already exists';
|
||||
setFieldErrors({ teamName: msg });
|
||||
setLocalError(msg);
|
||||
return;
|
||||
}
|
||||
const validation = validateRequest(request, { requireCwd: launchTeam });
|
||||
|
|
@ -818,16 +830,16 @@ export const CreateTeamDialog = ({
|
|||
id="team-name"
|
||||
className={cn(
|
||||
'h-8 text-xs',
|
||||
(fieldErrors.teamName || existingTeamNames.includes(sanitizedTeamName)) &&
|
||||
(fieldErrors.teamName || allTakenTeamNames.includes(sanitizedTeamName)) &&
|
||||
'border-[var(--field-error-border)] bg-[var(--field-error-bg)] focus-visible:ring-[var(--field-error-border)]'
|
||||
)}
|
||||
value={teamName}
|
||||
onChange={(event) => handleTeamNameChange(event.target.value)}
|
||||
placeholder={suggestedTeamName}
|
||||
/>
|
||||
{existingTeamNames.includes(sanitizedTeamName) ? (
|
||||
{allTakenTeamNames.includes(sanitizedTeamName) ? (
|
||||
<p className="text-[11px]" style={{ color: 'var(--field-error-text)' }}>
|
||||
Team name already exists
|
||||
{isNameProvisioning ? 'Team is currently launching' : 'Team name already exists'}
|
||||
</p>
|
||||
) : validateTeamNameInline(teamName) ? (
|
||||
<p className="text-[11px]" style={{ color: 'var(--field-error-text)' }}>
|
||||
|
|
|
|||
Loading…
Reference in a new issue