From 013cfafbf96e1589e0a7c31c486ca729204797a1 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 9 Mar 2026 00:43:41 -0400 Subject: [PATCH] refactor: rename registerExecution to attachExecution for clarity The "attach" verb makes it clear that the caller brings an existing process/handle, while "create" (createExecution) means the lifecycle service allocates and owns the execution from scratch. --- .../executionLifecycleService.test.ts | 12 +-- .../src/services/executionLifecycleService.ts | 4 +- .../src/services/shellExecutionService.ts | 4 +- pr-description.md | 87 +++++++++++++++++++ 4 files changed, 97 insertions(+), 10 deletions(-) create mode 100644 pr-description.md diff --git a/packages/core/src/services/executionLifecycleService.test.ts b/packages/core/src/services/executionLifecycleService.test.ts index 60a29a7f5c..e8c08206ad 100644 --- a/packages/core/src/services/executionLifecycleService.test.ts +++ b/packages/core/src/services/executionLifecycleService.test.ts @@ -155,7 +155,7 @@ describe('ExecutionLifecycleService', () => { const chunks: string[] = []; let output = 'seed'; - const handle: ExecutionHandle = ExecutionLifecycleService.registerExecution( + const handle: ExecutionHandle = ExecutionLifecycleService.attachExecution( 4321, { executionMethod: 'child_process', @@ -211,7 +211,7 @@ describe('ExecutionLifecycleService', () => { it('supports late subscription catch-up after backgrounding an external execution', async () => { let output = 'seed'; const onExit = vi.fn(); - const handle = ExecutionLifecycleService.registerExecution(4322, { + const handle = ExecutionLifecycleService.attachExecution(4322, { executionMethod: 'child_process', getBackgroundOutput: () => output, getSubscriptionSnapshot: () => output, @@ -258,7 +258,7 @@ describe('ExecutionLifecycleService', () => { it('kills external executions and settles pending promises', async () => { const terminate = vi.fn(); const onExit = vi.fn(); - const handle = ExecutionLifecycleService.registerExecution(4323, { + const handle = ExecutionLifecycleService.attachExecution(4323, { executionMethod: 'child_process', initialOutput: 'running', kill: terminate, @@ -276,14 +276,14 @@ describe('ExecutionLifecycleService', () => { }); it('rejects duplicate execution registration for active execution IDs', () => { - ExecutionLifecycleService.registerExecution(4324, { + ExecutionLifecycleService.attachExecution(4324, { executionMethod: 'child_process', }); expect(() => { - ExecutionLifecycleService.registerExecution(4324, { + ExecutionLifecycleService.attachExecution(4324, { executionMethod: 'child_process', }); - }).toThrow('Execution 4324 is already registered.'); + }).toThrow('Execution 4324 is already attached.'); }); }); diff --git a/packages/core/src/services/executionLifecycleService.ts b/packages/core/src/services/executionLifecycleService.ts index 7d1384e3c5..cf3939af7a 100644 --- a/packages/core/src/services/executionLifecycleService.ts +++ b/packages/core/src/services/executionLifecycleService.ts @@ -167,12 +167,12 @@ export class ExecutionLifecycleService { this.nextExecutionId = NON_PROCESS_EXECUTION_ID_START; } - static registerExecution( + static attachExecution( executionId: number, registration: ExternalExecutionRegistration, ): ExecutionHandle { if (this.activeExecutions.has(executionId) || this.activeResolvers.has(executionId)) { - throw new Error(`Execution ${executionId} is already registered.`); + throw new Error(`Execution ${executionId} is already attached.`); } this.exitedExecutionInfo.delete(executionId); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index c5b52dca2b..713b2eac8e 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -258,7 +258,7 @@ export class ShellExecutionService { }; const lifecycleHandle = child.pid - ? ExecutionLifecycleService.registerExecution(child.pid, { + ? ExecutionLifecycleService.attachExecution(child.pid, { executionMethod: 'child_process', getBackgroundOutput: () => state.output, getSubscriptionSnapshot: () => state.output || undefined, @@ -554,7 +554,7 @@ export class ShellExecutionService { maxSerializedLines: shellExecutionConfig.maxSerializedLines, }); - const result = ExecutionLifecycleService.registerExecution(ptyPid, { + const result = ExecutionLifecycleService.attachExecution(ptyPid, { executionMethod: ptyInfo?.name ?? 'node-pty', writeInput: (input) => { if (!ExecutionLifecycleService.isActive(ptyPid)) { diff --git a/pr-description.md b/pr-description.md new file mode 100644 index 0000000000..7b32b2dd21 --- /dev/null +++ b/pr-description.md @@ -0,0 +1,87 @@ +## Summary + +Extract a generic `ExecutionLifecycleService` that owns background-execution state (create, stream, background, complete, kill) so that **any tool** — not just shell — can be backgrounded via Ctrl+B. This is a pure abstraction/renaming refactor with no new features; the follow-up PR (stacked on this branch) wires remote agents into the same lifecycle. + +## Details + +### Why this change + +Background execution was deeply coupled to `ShellExecutionService`: resolver maps, listener maps, exit-info TTL, and the `background()` / `subscribe()` / `onExit()` API all lived inside the shell service. Adding Ctrl+B support for remote agents (or MCP tools, local agents, etc.) would have required either duplicating that machinery or reaching into shell internals. This PR pulls the lifecycle out into a standalone service that shell delegates to. + +### Recommended reading order + +This is a large diff (13 files, ~1500 insertions) but the bulk is mechanical delegation. Read in this order: + +1. **`executionLifecycleService.ts`** — the new abstraction. Two execution kinds: + - *virtual*: tool calls `createExecution()`, gets an ID + result promise, streams via `appendOutput()`, completes via `completeExecution()`. Used by the upcoming remote-agent wiring. + - *external*: shell calls `attachExecution(pid, {...hooks})` to attach lifecycle tracking to a real OS process. Same lifecycle, but with write/kill/isActive hooks delegated back to the PTY/child_process owner. + - Shared: `background()` resolves the result promise early with `backgrounded: true` but keeps the execution active for continued streaming. `subscribe()` replays a snapshot for late joiners. `onExit()` fires on completion or replays from a 5-minute TTL cache. + +2. **`shellExecutionService.ts`** — the main deletion site. All background-state maps (`activeResolvers`, `activeListeners`, `exitedPidInfo`) are removed. Shell now calls `attachExecution()` at spawn time and `completeWithResult()` at exit. The public `background()`, `subscribe()`, `onExit()`, `kill()`, `isActive()`, `writeInput()` methods become one-line delegates. + +3. **`tool-executor.ts` + `coreToolHookTriggers.ts`** — the `setPidCallback` → `setExecutionIdCallback` rename chain. `ToolExecutor` no longer checks `instanceof ShellToolInvocation`; every tool gets the callback. `executeToolWithHooks` passes it through to `invocation.execute()`. + +4. **`tools.ts` + `shell.ts`** — `ToolInvocation.execute()` signature gains optional `setExecutionIdCallback`. `BackgroundExecutionData` interface + `isBackgroundExecutionData` / `getBackgroundExecutionId` helpers added. Shell tool populates `data.executionId` alongside the existing `data.pid`. + +5. **`useGeminiStream.ts` + `shellCommandProcessor.ts`** — UI generalization. The `activePtyId` computation no longer filters on `request.name === 'run_shell_command'`; any executing tool with a numeric `pid` (execution ID) qualifies for Ctrl+B. Background registration uses the core `isBackgroundExecutionData` / `getBackgroundExecutionId` helpers. Parameter renamed `activeToolPtyId` → `activeBackgroundExecutionId`. + +### Key design decisions + +- **Static class, not singleton instance**: `ExecutionLifecycleService` uses static methods and maps, matching the existing `ShellExecutionService` pattern. This avoids DI plumbing changes across the codebase. +- **`pid` field as backward-compat alias**: `ExecutionHandle.pid` and `BackgroundExecutionData.pid` remain for existing shell consumers. New code should use `executionId` / `getBackgroundExecutionId()`. +- **`NON_PROCESS_EXECUTION_ID_START = 2_000_000_000`**: Virtual execution IDs start above any realistic OS PID. `isActive()` short-circuits for IDs in this range to avoid spurious `process.kill()` syscalls. +- **No `finalizeExecution`**: The deprecated alias was removed (zero callers). + +## Related Issues + + + +## How to Validate + +1. **Read the new service first**: `packages/core/src/services/executionLifecycleService.ts` — verify the create → stream → background → complete lifecycle makes sense in isolation. + +2. **Verify shell delegation**: `shellExecutionService.ts` should have no background-state maps of its own. Every public lifecycle method should be a one-liner delegating to `ExecutionLifecycleService`. + +3. **Run the targeted test suites**: + ```bash + npm run test --workspace @google/gemini-cli-core -- --run \ + src/services/executionLifecycleService.test.ts \ + src/services/shellExecutionService.test.ts \ + src/core/coreToolHookTriggers.test.ts \ + src/scheduler/tool-executor.test.ts + + npm run test --workspace @google/gemini-cli -- --run \ + src/ui/hooks/useGeminiStream.test.tsx + ``` + +4. **Typecheck and lint**: + ```bash + npm run typecheck --workspace @google/gemini-cli-core + npm run lint --workspace @google/gemini-cli-core + npm run lint --workspace @google/gemini-cli + ``` + +5. **Verify no behavioral change**: Existing shell backgrounding (Ctrl+B during a `run_shell_command`) should work identically. The UI background panel, `onExit` notifications, and kill behavior are unchanged. + +## Pre-Merge Checklist + +- [ ] Updated relevant documentation and README (if needed) +- [x] Added/updated tests (if needed) +- [ ] Noted breaking changes (if any) +- [ ] Validated on required platforms/methods: + - [ ] MacOS + - [ ] npm run + - [ ] npx + - [ ] Docker + - [ ] Podman + - [ ] Seatbelt + - [ ] Windows + - [ ] npm run + - [ ] npx + - [ ] Docker + - [ ] Linux + - [ ] npm run + - [ ] npx + - [ ] Docker