From 715c00bd48be04e10ec35b0792dc626c303788f2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 28 Oct 2026 12:14:10 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20split=20createUnderT?= =?UTF-8?q?askTreeLifecycleLock,=20part=205:=20request,=20parent,=20placem?= =?UTF-8?q?ent,=20agent=20and=20AI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the start of TaskService.createUnderTaskTreeLifecycleLock into named steps. Before the global mutex: readTaskCreateRequest with the module function normalizeTaskCreateBestOf. Under it, a chain: loadTaskCreateParent (readTaskCreateParent), countAndPlaceTask (refuseTaskCreateUnderParent, placeTaskWorkspace, decideSharedTaskWorkspace), readTaskCreateParentBranch, checkTaskAgentRunnable (describeRunnableTaskAgents, the getRunnableHint closure) and resolveTaskCreateAiSettings, which queues or launches the task. Values travel in the readonly TaskCreateRequest, TaskCreateParent, TaskCreatePlacement and TaskCreatePlan records, each built on the last with spread. Each step starts the next in the same tick as its own last await. The method keeps its signature, the `await using` of the mutex and one `return await this.loadTaskCreateParent`, so the mutex releases only after the create result settles. CC 122 -> 2 across the parts. --- src/node/services/taskService.ts | 423 +++++++++++++++++++++---------- 1 file changed, 295 insertions(+), 128 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 999e1e00e8c..72f61990cf8 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -899,6 +899,59 @@ interface TaskCreatePlan extends TaskCreatePlacement { readonly createdAt: string; } +/** Validates Task.create's bestOf: undefined when absent, else the normalized group slot. */ +function normalizeTaskCreateBestOf( + bestOf: TaskCreateArgs["bestOf"] +): Result { + if (!bestOf) { + return Ok(undefined); + } + const groupId = coerceNonEmptyString(bestOf.groupId); + if (!groupId) { + return Err("Task.create: bestOf.groupId is required when bestOf is provided"); + } + if (!Number.isInteger(bestOf.index) || bestOf.index < 0) { + return Err("Task.create: bestOf.index must be a non-negative integer"); + } + if (!Number.isInteger(bestOf.total) || bestOf.total < 2) { + return Err("Task.create: bestOf.total must be an integer >= 2"); + } + if (bestOf.index >= bestOf.total) { + return Err("Task.create: bestOf.index must be less than bestOf.total"); + } + + return Ok({ + groupId, + index: bestOf.index, + total: bestOf.total, + }); +} + +/** + * Whether a task shares its parent's checkout: a scratch parent, or isolation: "none" on a + * runtime that supports it for a single-project parent. A multi-project parent there is refused. + */ +function decideSharedTaskWorkspace( + isolation: TaskCreateArgs["isolation"], + taskRuntimeMode: ReturnType, + parentIsScratch: boolean, + parentIsMultiProject: boolean +): Result { + if ( + isolation === "none" && + runtimeModeSupportsSharedTaskWorkspace(taskRuntimeMode) && + parentIsMultiProject + ) { + return Err(`Task.create: ${MULTI_PROJECT_SHARED_ISOLATION_ERROR}`); + } + return Ok( + parentIsScratch || + (isolation === "none" && + runtimeModeSupportsSharedTaskWorkspace(taskRuntimeMode) && + !parentIsMultiProject) + ); +} + /** A direct launch's materialized checkout, read by the steps after its entry is persisted. */ interface TaskLaunchCheckout { readonly initLogger: InitLogger; @@ -6960,6 +7013,24 @@ export class TaskService implements AgentTaskIntegration { args: TaskCreateArgs, parentWorkspaceId: string ): Promise> { + const request = this.readTaskCreateRequest(args, parentWorkspaceId); + if (!request.success) { + return request; + } + + // Held until the create settles: every step below runs under it, and none takes it again (the + // mutex is not reentrant). The step call must be awaited here: a bare `return promise` would + // dispose the lock before the promise settles. + await using _lock = await this.mutex.acquire(); + + return await this.loadTaskCreateParent(request.data); + } + + /** Validates a Task.create request before the global mutex is taken. */ + private readTaskCreateRequest( + args: TaskCreateArgs, + parentWorkspaceId: string + ): Result { if (args.kind !== "agent") { return Err("Task.create: unsupported kind"); } @@ -6979,28 +7050,9 @@ export class TaskService implements AgentTaskIntegration { return Err(`Task.create: invalid agentId (${normalizedAgentId})`); } - let normalizedBestOf: TaskCreateArgs["bestOf"]; - const bestOf = args.bestOf; - if (bestOf) { - const groupId = coerceNonEmptyString(bestOf.groupId); - if (!groupId) { - return Err("Task.create: bestOf.groupId is required when bestOf is provided"); - } - if (!Number.isInteger(bestOf.index) || bestOf.index < 0) { - return Err("Task.create: bestOf.index must be a non-negative integer"); - } - if (!Number.isInteger(bestOf.total) || bestOf.total < 2) { - return Err("Task.create: bestOf.total must be an integer >= 2"); - } - if (bestOf.index >= bestOf.total) { - return Err("Task.create: bestOf.index must be less than bestOf.total"); - } - - normalizedBestOf = { - groupId, - index: bestOf.index, - total: bestOf.total, - }; + const normalizedBestOf = normalizeTaskCreateBestOf(args.bestOf); + if (!normalizedBestOf.success) { + return normalizedBestOf; } const agentId = parsedAgentId.data; @@ -7012,15 +7064,43 @@ export class TaskService implements AgentTaskIntegration { return Err(getErrorMessage(error)); } - await using _lock = await this.mutex.acquire(); + return Ok({ + args, + parentWorkspaceId, + prompt, + agentId, + agentType, + normalizedBestOf: normalizedBestOf.data, + taskDesktopOwnerWorkspaceId, + }); + } + /** + * Task.create under the global mutex: reads the parent, then chains to countAndPlaceTask and + * the later steps. Each step starts the next in the same tick as its own last await, so no call + * runs later than it did inline. + */ + private async loadTaskCreateParent( + request: TaskCreateRequest + ): Promise> { // Validate parent exists and fetch runtime context. - const parentMetaResult = await this.aiService.getWorkspaceMetadata(parentWorkspaceId); + const parentMetaResult = await this.aiService.getWorkspaceMetadata(request.parentWorkspaceId); if (!parentMetaResult.success) { return Err(`Task.create: parent workspace not found (${parentMetaResult.error})`); } - const parentMeta = parentMetaResult.data; + const parent = this.readTaskCreateParent(request, parentMetaResult.data); + if (!parent.success) { + return parent; + } + return this.countAndPlaceTask(parent.data); + } + /** Reads the parent's config facts once, and refuses an archived parent or untrusted project. */ + private readTaskCreateParent( + request: TaskCreateRequest, + parentMeta: WorkspaceMetadata + ): Result { + const { parentWorkspaceId } = request; // Enforce nesting depth. const cfg = this.config.loadConfigOrDefault(); // #6426: once per create, from the loaded config. @@ -7049,6 +7129,60 @@ export class TaskService implements AgentTaskIntegration { ); } + return Ok({ + ...request, + parentMeta, + cfg, + projectPluginsTrusted, + taskSettings, + parentEntry, + legacyParentRow, + parentIsScratch, + configProjectPath, + }); + } + + /** Refuses a stopped or too-deep parent, then counts the active tasks and names the new task. */ + private async countAndPlaceTask( + parent: TaskCreateParent + ): Promise> { + const { cfg, taskSettings, agentId } = parent; + const refusal = this.refuseTaskCreateUnderParent(parent); + if (refusal !== undefined) { + return refusal; + } + + // Enforce parallelism (global). + const activeCount = + this.countActiveAgentTasks(cfg) + + (await this.getWorkspaceTurnManager().countActiveWorkspaceTurns()); + const shouldQueue = activeCount >= taskSettings.maxParallelAgentTasks; + + const taskId = this.config.generateStableId(); + const workspaceName = buildAgentWorkspaceName(agentId, taskId); + + const nameValidation = validateWorkspaceName(workspaceName); + if (!nameValidation.valid) { + return Err( + `Task.create: generated workspace name invalid (${nameValidation.error ?? "unknown error"})` + ); + } + + const placement = this.placeTaskWorkspace(parent, { + activeCount, + shouldQueue, + taskId, + workspaceName, + }); + if (!placement.success) { + return placement; + } + return this.readTaskCreateParentBranch(placement.data); + } + + /** The parent-state and nesting-depth gates: the refusal, or undefined to admit the child. */ + private refuseTaskCreateUnderParent(parent: TaskCreateParent): Result | undefined { + const { cfg, parentEntry, parentWorkspaceId, taskSettings } = parent; if ( parentEntry?.workspace.taskStatus === "interrupted" && !isActiveWorkspaceTurnTaskStatus(parentEntry.workspace.taskExecutionStatus) @@ -7070,22 +7204,15 @@ export class TaskService implements AgentTaskIntegration { ); } - // Enforce parallelism (global). - const activeCount = - this.countActiveAgentTasks(cfg) + - (await this.getWorkspaceTurnManager().countActiveWorkspaceTurns()); - const shouldQueue = activeCount >= taskSettings.maxParallelAgentTasks; - - const taskId = this.config.generateStableId(); - const workspaceName = buildAgentWorkspaceName(agentId, taskId); - - const nameValidation = validateWorkspaceName(workspaceName); - if (!nameValidation.valid) { - return Err( - `Task.create: generated workspace name invalid (${nameValidation.error ?? "unknown error"})` - ); - } + return undefined; + } + /** Builds the task's runtime from the parent's and decides whether it shares the checkout. */ + private placeTaskWorkspace( + parent: TaskCreateParent, + counted: Pick + ): Result { + const { args, parentMeta, parentEntry, parentIsScratch } = parent; const parentRuntimeConfig = parentMeta.runtimeConfig; const taskRuntimeConfig: RuntimeConfig = parentRuntimeConfig; @@ -7116,18 +7243,47 @@ export class TaskService implements AgentTaskIntegration { // runtimes are refused (see MULTI_PROJECT_SHARED_ISOLATION_ERROR); otherwise fall back to forking. const taskRuntimeMode = getRuntimeType(taskRuntimeConfig); const parentIsMultiProject = (parentMeta.projects?.length ?? 0) > 1; - if ( - args.isolation === "none" && - runtimeModeSupportsSharedTaskWorkspace(taskRuntimeMode) && + const sharedWorkspace = decideSharedTaskWorkspace( + args.isolation, + taskRuntimeMode, + parentIsScratch, parentIsMultiProject - ) { - return Err(`Task.create: ${MULTI_PROJECT_SHARED_ISOLATION_ERROR}`); + ); + if (!sharedWorkspace.success) { + return sharedWorkspace; } - const useSharedWorkspace = - parentIsScratch || - (args.isolation === "none" && - runtimeModeSupportsSharedTaskWorkspace(taskRuntimeMode) && - !parentIsMultiProject); + return Ok({ + ...parent, + ...counted, + parentRuntimeConfig, + taskRuntimeConfig, + runtime, + parentWorkspacePath, + taskRuntimeMode, + parentIsMultiProject, + useSharedWorkspace: sharedWorkspace.data, + }); + } + + /** + * Task.create once the task is placed: reads the parent's checked-out branch, then chains to + * checkTaskAgentRunnable and resolveTaskCreateAiSettings, which queues or launches the task. + * Each step starts the next in the same tick as its own last await. + */ + private async readTaskCreateParentBranch( + placement: TaskCreatePlacement + ): Promise> { + const { + args, + parentEntry, + parentMeta, + runtime, + parentWorkspacePath, + useSharedWorkspace, + taskId, + taskRuntimeMode, + parentIsMultiProject, + } = placement; // The branch actually checked out in the parent's checkout (see readTaskParentBranch): an // isolation: "none" parent's name, and an auto-named worktree parent's, name no branch. // Persisting the real branch keeps dequeue fork-fallbacks (preferredTrunkBranch) on an @@ -7143,60 +7299,71 @@ export class TaskService implements AgentTaskIntegration { parentIsMultiProject, }); } + return this.checkTaskAgentRunnable(placement, parentBranchName); + } - // Helper to build error hint with all available runnable agents. - // NOTE: This resolves frontmatter inheritance so same-name overrides (e.g. project exec.md - // with base: exec) still count as runnable. - const getRunnableHint = async (): Promise => { - try { - const allAgents = await discoverAgentDefinitions(runtime, parentWorkspacePath, { - projectPluginsTrusted, - runtimeFactory: nodeRuntimeFactory, - }); + /** + * Builds the error hint with all available runnable agents. + * NOTE: This resolves frontmatter inheritance so same-name overrides (e.g. project exec.md + * with base: exec) still count as runnable. + */ + private async describeRunnableTaskAgents(placement: TaskCreatePlacement): Promise { + const { args, cfg, runtime, parentWorkspacePath, projectPluginsTrusted } = placement; + try { + const allAgents = await discoverAgentDefinitions(runtime, parentWorkspacePath, { + projectPluginsTrusted, + runtimeFactory: nodeRuntimeFactory, + }); - const runnableIds = ( - await Promise.all( - allAgents.map(async (agent) => { - try { - const frontmatter = await resolveAgentFrontmatter( - runtime, - parentWorkspacePath, - agent.id, - { - skipScopesAbove: getSkipScopesAboveForKnownScope(agent.scope), - projectPluginsTrusted, - runtimeFactory: nodeRuntimeFactory, - } - ); - if ( - !isAgentRunnableAsChild(frontmatter, { - workflowOwned: args.workflowTask != null, - }) - ) { - return null; + const runnableIds = ( + await Promise.all( + allAgents.map(async (agent) => { + try { + const frontmatter = await resolveAgentFrontmatter( + runtime, + parentWorkspacePath, + agent.id, + { + skipScopesAbove: getSkipScopesAboveForKnownScope(agent.scope), + projectPluginsTrusted, + runtimeFactory: nodeRuntimeFactory, } - - const effectivelyDisabled = isAgentEffectivelyDisabled({ - cfg, - agentId: agent.id, - resolvedFrontmatter: frontmatter, - }); - return effectivelyDisabled ? null : agent.id; - } catch { + ); + if ( + !isAgentRunnableAsChild(frontmatter, { + workflowOwned: args.workflowTask != null, + }) + ) { return null; } - }) - ) - ).filter((id): id is string => typeof id === "string"); - return runnableIds.length > 0 - ? `Runnable agentIds: ${runnableIds.join(", ")}` - : "No runnable agents available"; - } catch { - return "Could not discover available agents"; - } - }; + const effectivelyDisabled = isAgentEffectivelyDisabled({ + cfg, + agentId: agent.id, + resolvedFrontmatter: frontmatter, + }); + return effectivelyDisabled ? null : agent.id; + } catch { + return null; + } + }) + ) + ).filter((id): id is string => typeof id === "string"); + + return runnableIds.length > 0 + ? `Runnable agentIds: ${runnableIds.join(", ")}` + : "No runnable agents available"; + } catch { + return "Could not discover available agents"; + } + } + /** Refuses an agent that is unknown, disabled or not runnable as a sub-agent. */ + private async checkTaskAgentRunnable( + placement: TaskCreatePlacement, + parentBranchName: string | undefined + ): Promise> { + const { args, cfg, agentId, runtime, parentWorkspacePath, projectPluginsTrusted } = placement; let skipInitHook = false; try { const frontmatter = await resolveAgentFrontmatter(runtime, parentWorkspacePath, agentId, { @@ -7204,7 +7371,7 @@ export class TaskService implements AgentTaskIntegration { runtimeFactory: nodeRuntimeFactory, }); if (!isAgentRunnableAsChild(frontmatter, { workflowOwned: args.workflowTask != null })) { - const hint = await getRunnableHint(); + const hint = await this.describeRunnableTaskAgents(placement); return Err(`Task.create: agentId is not runnable as a sub-agent (${agentId}). ${hint}`); } @@ -7215,7 +7382,7 @@ export class TaskService implements AgentTaskIntegration { resolvedFrontmatter: frontmatter, }) ) { - const hint = await getRunnableHint(); + const hint = await this.describeRunnableTaskAgents(placement); return Err(`Task.create: agentId is disabled (${agentId}). ${hint}`); } skipInitHook = frontmatter.subagent?.skip_init_hook === true; @@ -7225,10 +7392,35 @@ export class TaskService implements AgentTaskIntegration { formatRuntimeUnreachableError(`Task.create: could not verify agentId (${agentId})`, error) ); } - const hint = await getRunnableHint(); + const hint = await this.describeRunnableTaskAgents(placement); return Err(`Task.create: unknown agentId (${agentId}). ${hint}`); } + return this.resolveTaskCreateAiSettings(placement, parentBranchName, skipInitHook); + } + /** Resolves the task's model and thinking settings, then queues or launches the task. */ + private async resolveTaskCreateAiSettings( + placement: TaskCreatePlacement, + parentBranchName: string | undefined, + skipInitHook: boolean + ): Promise> { + const { + args, + cfg, + parentWorkspaceId, + parentMeta, + agentId, + runtime, + parentWorkspacePath, + projectPluginsTrusted, + taskId, + workspaceName, + activeCount, + taskSettings, + shouldQueue, + taskRuntimeConfig, + prompt, + } = placement; let taskModelString: string; let canonicalModel: string; let effectiveThinkingLevel: ThinkingLevel; @@ -7287,32 +7479,7 @@ export class TaskService implements AgentTaskIntegration { }); const plan: TaskCreatePlan = { - args, - parentWorkspaceId, - prompt, - agentId, - agentType, - normalizedBestOf, - taskDesktopOwnerWorkspaceId, - parentMeta, - cfg, - projectPluginsTrusted, - taskSettings, - parentEntry, - legacyParentRow, - parentIsScratch, - configProjectPath, - activeCount, - shouldQueue, - taskId, - workspaceName, - parentRuntimeConfig, - taskRuntimeConfig, - runtime, - parentWorkspacePath, - taskRuntimeMode, - parentIsMultiProject, - useSharedWorkspace, + ...placement, parentBranchName, skipInitHook, taskModelString, @@ -7324,10 +7491,10 @@ export class TaskService implements AgentTaskIntegration { }; if (shouldQueue) { - return await this.createQueuedTask(plan); + return this.createQueuedTask(plan); } - return await this.launchTask(plan); + return this.launchTask(plan); } /**