mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-12 07:01:09 -07:00
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.
This commit is contained in:
@@ -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.');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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)) {
|
||||
|
||||
87
pr-description.md
Normal file
87
pr-description.md
Normal file
@@ -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
|
||||
|
||||
<!-- Use keywords to auto-close issues (Closes #123, Fixes #456). If this PR is
|
||||
only related to an issue or is a partial fix, simply reference the issue number
|
||||
without a keyword (Related to #123). -->
|
||||
|
||||
## 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
|
||||
Reference in New Issue
Block a user