fix: use persistent container for execution environment commands#2721
Open
lalten wants to merge 6 commits into
Open
fix: use persistent container for execution environment commands#2721lalten wants to merge 6 commits into
lalten wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Ansible Language Server’s execution environment (EE) command execution model to reuse a single long-lived container per workspace (via docker exec / podman exec) instead of spawning an ephemeral container per command, and wires container cleanup into workspace removal and language server shutdown.
Changes:
- Add persistent container lifecycle management to
ExecutionEnvironment(deterministic naming, health checks, disposal) and introduce a simple command-result cache API. - Switch EE command execution in
CommandRunnerfromwrapContainerArgs(...)toexecInContainer(...). - Add workspace-removal and shutdown cleanup hooks, plus new unit tests for the new EE behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ansible-language-server/src/services/executionEnvironment.ts | Implements persistent container execution, health checks, dispose, and command cache; updates plugin-doc copying loop. |
| packages/ansible-language-server/src/utils/commandRunner.ts | Updates EE execution path to use execInContainer and removes the mountPaths parameter. |
| packages/ansible-language-server/src/services/workspaceManager.ts | Disposes EE containers when workspace folders are removed; adds async EE disposal method. |
| packages/ansible-language-server/src/ansibleLanguageService.ts | Disposes EE containers on language server shutdown. |
| packages/ansible-language-server/test/services/executionEnvironment.test.ts | Adds tests for persistent-container initialization, exec command generation, dispose, and cache. |
Comments suppressed due to low confidence (1)
packages/ansible-language-server/src/services/executionEnvironment.ts:786
copyPluginDocFiles()creates directories withfs.mkdirSync(destPath, { recursive: true }), butdestPathFolderis computed and used as the target folder fordocker cp. CreatingdestPath(rather thandestPathFolder) is inconsistent and can create the wrong directory structure (or fail ifdestPathis intended as a file path). Usefs.mkdirSync(destPathFolder, { recursive: true })to ensure the copy destination exists.
const destPathFolder = destPath
.split(path.sep)
.slice(0, -1)
.join(path.sep);
fs.mkdirSync(destPath, { recursive: true });
const copyCommand = `${this._container_engine} cp ${containerName}:${srcPath} ${destPathFolder}`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lalten
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Author
|
The Linux test failure seems to be #2720 |
📝 WalkthroughWalkthroughRefactors execution from ephemeral per-command containers to a deterministic persistent container per workspace, adds lifecycle and health management, command caching, a shutdown handler to dispose execution environments, and removes the Changes
Sequence DiagramsequenceDiagram
participant LS as Language Server
participant WM as Workspace Manager
participant EE as Execution Environment
participant Container as Docker/Podman Container
Note over LS,Container: Initialization
LS->>WM: create workspace context
WM->>EE: initialize / startPersistentContainer
EE->>Container: start or reuse deterministic container
Container-->>EE: container running
Note over LS,Container: Command Execution
LS->>WM: request command execution
WM->>EE: execInContainer(command)
EE->>EE: check command cache
alt cache hit
EE-->>WM: return cached result
else cache miss
EE->>Container: docker/podman exec <container> <command>
Container-->>EE: command output
EE->>EE: store result in cache
EE-->>WM: return result
end
Note over LS,Container: Shutdown
LS->>LS: onShutdown event
LS->>WM: disposeExecutionEnvironment (all contexts)
WM->>EE: dispose()
EE->>Container: stop/remove container
Container-->>EE: stopped/removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ansible-language-server/src/services/workspaceManager.ts (1)
106-114: Consider awaiting disposal before deleting context.
void context.disposeExecutionEnvironment()is fire-and-forget, meaning the context is deleted fromfolderContextsbefore container cleanup completes. If disposal encounters errors that need the context (e.g., for logging), this could cause issues.♻️ Suggested fix
- for (const removedUri of removedUris) { + for (const removedUri of removedUris) { const context = this.folderContexts.get(removedUri); /* v8 ignore next 3 */ if (context) { - void context.disposeExecutionEnvironment(); + await context.disposeExecutionEnvironment(); } this.folderContexts.delete(removedUri); }Note: This would require making
handleWorkspaceChangedasync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-language-server/src/services/workspaceManager.ts` around lines 106 - 114, The loop that disposes persistent containers currently calls void context.disposeExecutionEnvironment() which fire-and-forgets and then deletes the context from folderContexts; change handleWorkspaceChanged to be async and await context.disposeExecutionEnvironment() for each removedUri (or collect Promises and await Promise.all) before calling this.folderContexts.delete(removedUri) so disposal completes (and any errors or logging that need the context can use it); update any callers of handleWorkspaceChanged to await it if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ansible-language-server/src/services/workspaceManager.ts`:
- Around line 227-236: The test helpers enableExecutionEnvironmentSettings() and
disableExecutionEnvironmentSettings() must explicitly dispose the execution
environment since clearCachedServices() no longer does that; update
disableExecutionEnvironmentSettings() to call disposeExecutionEnvironment() (and
await it if it returns a Promise) before/after calling clearCachedServices() so
any running EE containers are properly cleaned up during teardown, ensuring you
reference the existing clearCachedServices() and disposeExecutionEnvironment()
methods when making the change.
In `@packages/ansible-language-server/src/utils/commandRunner.ts`:
- Around line 27-28: runCommand currently ignores the _mountPaths parameter so
callers like ansibleLint.ts that pass config/document paths end up with those
paths inaccessible; update runCommand to honor the _mountPaths argument by
either (A) passing its entries into startPersistentContainer so the container is
started with additional volume mounts, or (B) validating each path against the
set of already-mounted workspace paths and emitting a clear warning/error when a
path is not covered; specifically modify runCommand (and
startPersistentContainer if needed) to accept and handle mount paths and ensure
callers (e.g., ansibleLint.ts) get correct mount behavior or a visible warning.
---
Nitpick comments:
In `@packages/ansible-language-server/src/services/workspaceManager.ts`:
- Around line 106-114: The loop that disposes persistent containers currently
calls void context.disposeExecutionEnvironment() which fire-and-forgets and then
deletes the context from folderContexts; change handleWorkspaceChanged to be
async and await context.disposeExecutionEnvironment() for each removedUri (or
collect Promises and await Promise.all) before calling
this.folderContexts.delete(removedUri) so disposal completes (and any errors or
logging that need the context can use it); update any callers of
handleWorkspaceChanged to await it if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43aec0fe-d6a9-44a2-bf43-bb304ef5e70f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/ansible-language-server/package.jsonpackages/ansible-language-server/src/ansibleLanguageService.tspackages/ansible-language-server/src/services/executionEnvironment.tspackages/ansible-language-server/src/services/workspaceManager.tspackages/ansible-language-server/src/utils/commandRunner.tspackages/ansible-language-server/test/services/executionEnvironment.test.ts
💤 Files with no reviewable changes (1)
- packages/ansible-language-server/package.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ansible-language-server/src/utils/commandRunner.ts (1)
67-77:⚠️ Potential issue | 🟠 MajorDon't downgrade missing EE mounts to a warning.
mountPathsis still not honored in EE mode: callers likepackages/ansible-language-server/src/services/ansibleLint.ts:50-85andpackages/ansible-language-server/src/services/ansiblePlaybook.ts:59-81can pass config/document directories outside the workspace, butpackages/ansible-language-server/src/services/executionEnvironment.ts:265-301only mounts the workspace beforeexecInContainer()runs. Logging and continuing just turns that into a later command failure. The currentstartsWithcheck also misses sibling paths like/repo2for a workspace/repo.Minimal fail-fast alternative if mounting those paths is deferred
- if (mountPaths && this.connection) { + if (mountPaths) { const workspacePath = URI.parse(this.context.workspaceFolder.uri).path; - for (const mp of mountPaths) { - if (!mp.startsWith(workspacePath)) { - this.connection.console.warn( - `[EE] Mount path '${mp}' is outside the workspace folder and may not be accessible inside the persistent container. ` + - `Configure additional volume mounts in the Execution Environment settings.`, - ); - } + const workspacePrefix = workspacePath.endsWith("/") + ? workspacePath + : `${workspacePath}/`; + const uncoveredPaths = [...mountPaths].filter( + (mp) => mp !== workspacePath && !mp.startsWith(workspacePrefix), + ); + if (uncoveredPaths.length > 0) { + const message = + `[EE] Persistent container cannot access: ${uncoveredPaths.join(", ")}. ` + + `Configure additional volume mounts in the Execution Environment settings.`; + this.connection?.console.error(message); + throw new Error(message); } }Also applies to: 80-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-language-server/src/utils/commandRunner.ts` around lines 67 - 77, The mountPaths check silently warns when a requested mount is outside the workspace (and incorrectly uses startsWith, which treats siblings as inside); change it to fail fast by rejecting/throwing when any mp is not inside the workspace and this.connection exists. Replace the startsWith logic with a proper path containment check (use Node's path.relative or compare workspacePath + path.sep) to ensure mp is truly a descendant (i.e. path.relative(workspacePath, mp) does not start with '..' and is not equal to '' wrongly), and call the same failure behavior where the current code logs a warning (replace this.connection.console.warn with throwing an Error or returning a rejected Promise so execInContainer callers cannot proceed). Apply the same fix for the duplicate case at the other occurrence (lines around the second warn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ansible-language-server/src/utils/commandRunner.ts`:
- Around line 67-77: The mountPaths check silently warns when a requested mount
is outside the workspace (and incorrectly uses startsWith, which treats siblings
as inside); change it to fail fast by rejecting/throwing when any mp is not
inside the workspace and this.connection exists. Replace the startsWith logic
with a proper path containment check (use Node's path.relative or compare
workspacePath + path.sep) to ensure mp is truly a descendant (i.e.
path.relative(workspacePath, mp) does not start with '..' and is not equal to ''
wrongly), and call the same failure behavior where the current code logs a
warning (replace this.connection.console.warn with throwing an Error or
returning a rejected Promise so execInContainer callers cannot proceed). Apply
the same fix for the duplicate case at the other occurrence (lines around the
second warn).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1383065-831a-4729-892a-24dcb553dd56
📒 Files selected for processing (2)
packages/ansible-language-server/src/utils/commandRunner.tspackages/ansible-language-server/test/helper.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The execution environment (EE) implementation previously spawned a new ephemeral
docker run --rmcontainer for every single background diagnostic and linting command. This caused significant overhead as each invocation paid the full cost of container creation (namespace, overlayFS, veth pair setup) even though all commands ran against the same image and volume mounts.This PR replaces ephemeral containers with a single persistent background container managed via
docker exec/podman exec:sleep infinity) is started once duringinitialize()and reused for all subsequent commands viadocker exec.als_persistent_<hash12>), preventing collisions and enabling reliable cleanup.ensurePersistentContainerHealthy()performs debounced health checks (5s interval) and auto-restarts dead containers transparently.dispose()cleans up the persistent container on workspace removal, config change, or server shutdown. Wired intoWorkspaceManagerandAnsibleLanguageServiceshutdown handler.getCachedCommand/setCachedCommand/clearCommandCacheAPI for callers to avoid redundantdocker execinvocations (public, no callers yet).copyPluginDocFileshad a latent race condition —docker cpcalls viaasyncExecwere fire-and-forget insideforEach. Converted tofor...ofwithawait.Replaces ephemeral per-command containers with a single persistent background container reused via docker/podman exec to reduce startup overhead, improve reliability via debounced health checks and auto-restart, add command-result caching, and tie container lifecycle to workspace with deterministic naming and safe cleanup.
related: #2720