[Refactor] Extract managed-binary installation infrastructure - #1059
[Refactor] Extract managed-binary installation infrastructure#1059navedmerchant wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughShared managed-binary utilities now handle secure downloads, checksum verification, archive extraction, versioned atomic installation, cleanup, and concurrent-call de-duplication. The Semble downloader delegates to these utilities, and related tests and ESLint configuration were updated. ChangesManaged binary infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Semble as downloadSemble
participant Installer as ensureManagedBinaryInstalled
participant Downloader as downloadBinaryFile
participant Checksum as verifySha256Checksum
participant Extractor as extractTarGzArchive
Semble->>Installer: provide version and lifecycle callbacks
Installer->>Downloader: download trusted archive
Installer->>Checksum: verify archive checksum
Installer->>Extractor: extract into staging directory
Installer-->>Semble: return installed binary path
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/services/managed-binary/archive.ts (1)
55-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid constructing PowerShell scripts with string interpolation.
These extraction paths still interpolate user-controlled paths and archive names into PowerShell commands before shell escaping. Use PowerShell parameterized execution instead: pass values via
-ArgumentListwith a-Filescript, or build the command as script text and run it via-EncodedCommandso the values are not quoted into the command literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/managed-binary/archive.ts` around lines 55 - 87, Update extractZipArchive and extractSingleFileZipArchive to stop interpolating archive paths, destinations, expectedFile, or archiveName into PowerShell script text. Pass these values as PowerShell parameters via -ArgumentList with a -File script or an equivalent EncodedCommand approach, while preserving the existing extraction and archive-layout validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/code-index/semble/__tests__/semble-downloader.spec.ts`:
- Around line 707-723: Replace the stale fs.unlink assertion in the upgrade
cleanup test with an assertion against fs.rm, verifying the unrelated file path
is not removed while preserving the existing force-option expectations for
intended archives.
In `@src/services/managed-binary/__tests__/archive.spec.ts`:
- Around line 61-79: Update the test “validates a single-file tar.xz layout
before extraction” to assert that extraction passes the exact archive member
name returned by the listing, including the “./” prefix. Adjust the mocked
listing or expected tar arguments only as needed to verify the implementation
preserves the stored member name rather than stripping it.
In `@src/services/managed-binary/archive.ts`:
- Around line 89-105: Preserve the raw tar member name in
extractSingleFileTarXzArchive for the extraction command, while normalizing a
separate value by removing ./ for the single-entry layout comparison. Update the
archive.spec.ts test expectation so a ./binary listing results in extraction
targeting ./binary; apply the change in both specified files.
In `@src/services/managed-binary/download.ts`:
- Around line 120-144: Update the maxBytes error path in the response data
handler to explicitly destroy or close the output stream before rejecting,
alongside response.destroy() and request.destroy(). Preserve the existing
successful output.on("finish") flow and ensure the stream’s file descriptor is
released when assertSizeWithinLimit throws.
In `@src/services/managed-binary/install.ts`:
- Line 4: Add cross-process locking to ensureManagedBinaryInstalled in addition
to the in-memory installationPromises map, using an exclusive lock file under
storageDir to serialize attempts targeting the deterministic staging path.
Ensure lock acquisition, release, and stale/error cleanup are handled safely,
and keep each owner’s archive and staging writes isolated until successful
promotion.
---
Nitpick comments:
In `@src/services/managed-binary/archive.ts`:
- Around line 55-87: Update extractZipArchive and extractSingleFileZipArchive to
stop interpolating archive paths, destinations, expectedFile, or archiveName
into PowerShell script text. Pass these values as PowerShell parameters via
-ArgumentList with a -File script or an equivalent EncodedCommand approach,
while preserving the existing extraction and archive-layout validation behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dfa2c7c-2e9f-416c-ba33-4eec94aae3ba
📒 Files selected for processing (9)
src/eslint-suppressions.jsonsrc/services/code-index/semble/__tests__/semble-downloader.spec.tssrc/services/code-index/semble/semble-downloader.tssrc/services/managed-binary/__tests__/archive.spec.tssrc/services/managed-binary/__tests__/download.spec.tssrc/services/managed-binary/__tests__/install.spec.tssrc/services/managed-binary/archive.tssrc/services/managed-binary/download.tssrc/services/managed-binary/install.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
| expect(fs.rm).toHaveBeenCalledWith(versionedArchive, { force: true }) | ||
| // The prior-version archive (v0.4.0-*) is swept by cleanupStaleArchives | ||
| // after a successful install, so a version upgrade doesn't accumulate | ||
| // orphaned packages on disk. | ||
| expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz")) | ||
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz"), { | ||
| force: true, | ||
| }) | ||
| // The legacy unversioned archive (pre-v0.4.0 cache layout) is also | ||
| // swept, covering the v0.3.1 → v0.4.1 upgrade path. | ||
| expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz")) | ||
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz"), { | ||
| force: true, | ||
| }) | ||
| // Unrelated files in the storage dir must not be touched. | ||
| expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt")) | ||
| // The new version file is recorded | ||
| expect(fs.writeFile).toHaveBeenCalledWith( | ||
| path.join("/storage", "semble", ".semble-version"), | ||
| path.join("/storage", "semble.new", ".semble-version"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale fs.unlink assertion no longer verifies real behavior.
Since the migration replaced fs.unlink with fs.rm throughout install.ts, expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt")) (line 720) is vacuously true — fs.unlink is never called with anything now. This no longer protects against a regression that touches unrelated files during cleanup.
🔧 Proposed fix
- // Unrelated files in the storage dir must not be touched.
- expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt"))
+ // Unrelated files in the storage dir must not be touched.
+ expect(fs.rm).not.toHaveBeenCalledWith(
+ path.join("/storage", "unrelated-file.txt"),
+ expect.anything(),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(fs.rm).toHaveBeenCalledWith(versionedArchive, { force: true }) | |
| // The prior-version archive (v0.4.0-*) is swept by cleanupStaleArchives | |
| // after a successful install, so a version upgrade doesn't accumulate | |
| // orphaned packages on disk. | |
| expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz")) | |
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz"), { | |
| force: true, | |
| }) | |
| // The legacy unversioned archive (pre-v0.4.0 cache layout) is also | |
| // swept, covering the v0.3.1 → v0.4.1 upgrade path. | |
| expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz")) | |
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz"), { | |
| force: true, | |
| }) | |
| // Unrelated files in the storage dir must not be touched. | |
| expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt")) | |
| // The new version file is recorded | |
| expect(fs.writeFile).toHaveBeenCalledWith( | |
| path.join("/storage", "semble", ".semble-version"), | |
| path.join("/storage", "semble.new", ".semble-version"), | |
| expect(fs.rm).toHaveBeenCalledWith(versionedArchive, { force: true }) | |
| // The prior-version archive (v0.4.0-*) is swept by cleanupStaleArchives | |
| // after a successful install, so a version upgrade doesn't accumulate | |
| // orphaned packages on disk. | |
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz"), { | |
| force: true, | |
| }) | |
| // The legacy unversioned archive (pre-v0.4.0 cache layout) is also | |
| // swept, covering the v0.3.1 → v0.4.1 upgrade path. | |
| expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz"), { | |
| force: true, | |
| }) | |
| // Unrelated files in the storage dir must not be touched. | |
| expect(fs.rm).not.toHaveBeenCalledWith( | |
| path.join("/storage", "unrelated-file.txt"), | |
| expect.anything(), | |
| ) | |
| // The new version file is recorded | |
| expect(fs.writeFile).toHaveBeenCalledWith( | |
| path.join("/storage", "semble.new", ".semble-version"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/code-index/semble/__tests__/semble-downloader.spec.ts` around
lines 707 - 723, Replace the stale fs.unlink assertion in the upgrade cleanup
test with an assertion against fs.rm, verifying the unrelated file path is not
removed while preserving the existing force-option expectations for intended
archives.
| it("validates a single-file tar.xz layout before extraction", async () => { | ||
| const listing = createChild() | ||
| const extraction = createChild() | ||
| mockSpawn.mockReturnValueOnce(listing as unknown as ReturnType<typeof spawn>) | ||
| mockSpawn.mockReturnValueOnce(extraction as unknown as ReturnType<typeof spawn>) | ||
| const result = extractSingleFileTarXzArchive("/tmp/archive.tar.xz", "/tmp/output", "binary", "Tool") | ||
| listing.stdout.write("./binary\n") | ||
| listing.emit("close", 0) | ||
| await new Promise<void>((resolve) => setImmediate(resolve)) | ||
| extraction.emit("close", 0) | ||
| await result | ||
|
|
||
| expect(mockSpawn).toHaveBeenNthCalledWith( | ||
| 2, | ||
| "tar", | ||
| ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "binary"], | ||
| expect.any(Object), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test encodes the ./-stripping bug instead of catching it.
This test mocks spawn, so it can't detect that real tar requires the exact stored member name. The assertion on line 76 expects "binary" (stripped) rather than the archive's actual stored name ("./binary" per the mocked listing output at line 67), which matches the current implementation but not real tar semantics. See the companion comment on archive.ts lines 89-105.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/managed-binary/__tests__/archive.spec.ts` around lines 61 - 79,
Update the test “validates a single-file tar.xz layout before extraction” to
assert that extraction passes the exact archive member name returned by the
listing, including the “./” prefix. Adjust the mocked listing or expected tar
arguments only as needed to verify the implementation preserves the stored
member name rather than stripping it.
| export async function extractSingleFileTarXzArchive( | ||
| archivePath: string, | ||
| destination: string, | ||
| expectedFile: string, | ||
| archiveName: string, | ||
| ): Promise<void> { | ||
| const listing = await runProcess("tar", ["-tJf", archivePath]) | ||
| const entries = listing.stdout | ||
| .split(/\r?\n/) | ||
| .map((entry) => entry.trim().replace(/^\.\//, "")) | ||
| .filter(Boolean) | ||
| if (entries.length !== 1 || entries[0] !== expectedFile) { | ||
| throw new Error(`${archiveName} archive has an unexpected layout`) | ||
| } | ||
|
|
||
| await runProcess("tar", ["-xJf", archivePath, "-C", destination, expectedFile]) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)archive(\.ts|\.spec\.ts)$|src/services/managed-binary' || true
echo "== archive.ts relevant lines =="
if [ -f src/services/managed-binary/archive.ts ]; then
cat -n src/services/managed-binary/archive.ts | sed -n '1,160p'
fi
echo "== specs relevant lines =="
if [ -f src/services/managed-binary/__tests__/archive.spec.ts ]; then
cat -n src/services/managed-binary/__tests__/archive.spec.ts | sed -n '1,140p'
fi
echo "== search usages =="
rg -n "extractSingleFileTarXzArchive|tar \[" src/services/managed-binary || true
echo "== tar availability =="
tar --version 2>&1 | head -2 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 9057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
mkdir -p "$tmp/source" "$tmp/output1" "$tmp/output2"
echo "hello" > "$tmp/source/binary"
tar -cJf "$tmp/archive-xz.tar.xz" -C "$tmp/source" binary
tar -cJf "$tmp/archive-xz-dot.tar.xz" -C "$tmp/source" -T <(printf './binary\n')
tar -cvf "$tmp/archive-gz.tar.gz" -C "$tmp/source" binary
tar -cvf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/source" -T <(printf './binary\n')
echo "== listing =="
tar -tJf "$tmp/archive-xz.tar.xz"
tar -tJf "$tmp/archive-xz-dot.tar.xz"
tar -tvf "$tmp/archive-gz.tar.gz"
tar -tvf "$tmp/archive-gz-dot.tar.gz"
echo
echo "== extraction targeted tests =="
set +e
tar -xJf "$tmp/archive-xz.tar.xz" -C "$tmp/output1" binary
exit=$?
echo "targeted xz without dot exit=$exit list=$(find "$tmp/output1" -maxdepth 2 -print | sort | sed 's#'"$tmp/output1"'/##g' | tr '\n' ',')"
tar -xJf "$tmp/archive-xz-dot.tar.xz" -C "$tmp/output2" binary
echo "targeted xz dot archive without dot exit=$? list=$(find "$tmp/output2" -maxdepth 2 -print | sort | sed 's#'"$tmp/output2"'/##g' | tr '\n' ',')"
echo
echo "== help snippet =="
tar --help 2>&1 | sed -n '/POSIX Extensions/,/Exit status/p' | head -80 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
mkdir -p "$tmp/source" "$tmp/output1" "$tmp/output2"
echo "hello" > "$tmp/source/binary"
tar -czf "$tmp/archive-gz.tar.gz" -C "$tmp/source" binary
tar -czf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/source" -T <(printf './binary\n')
echo "== listing =="
tar -tzf "$tmp/archive-gz.tar.gz"
tar -tvf "$tmp/archive-gz-dot.tar.gz"
echo
echo "== extraction targeted tests =="
set +e
tar -czf "$tmp/archive-gz.tar.gz" -C "$tmp/source" binary
tar -xzf "$tmp/archive-gz.tar.gz" -C "$tmp/output1" binary
echo "gz without dot exit=$? list=$(find "$tmp/output1" -maxdepth 2 -print | sort | sed 's#'"$tmp/output1"'/##g' | tr '\n' ',')"
tar -xzf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/output2" binary
echo "gz dot archive, target without dot exit=$? list=$(find "$tmp/output2" -maxdepth 2 -print | sort | sed 's#'"$tmp/output2"'/##g' | tr '\n' ',')"
set -e
tar -xzf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/output2" "./binary"
echo "gz dot archive, target with dot exit=$? list=$(find "$tmp/output2" -maxdepth 2 -print | sort | sed 's#'"$tmp/output2"'/##' | tr '\n' ',')"
echo
tar -xzf "$tmp/archive-gz.tar.gz" -C "$tmp/output1" -P "./binary"
echo "gz plain archive, target with dot exit=$? list=$(find "$tmp/output1" -maxdepth 2 -print | sort | sed 's#'"$tmp/output1"'/##g' | tr '\n' ',')"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
mkdir -p "$tmp/source" "$tmp/output1" "$tmp/output2"
echo "hello" > "$tmp/source/binary"
printf './binary\n' > "$tmp/member-list"
tar -czf "$tmp/archive-gz.tar.gz" -C "$tmp/source" binary
tar -czf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/source" -T "$tmp/member-list"
echo "== listing =="
tar -tzf "$tmp/archive-gz.tar.gz"
tar -tvf "$tmp/archive-gz-dot.tar.gz"
echo
echo "== extraction targeted tests =="
set +e
tar -xzf "$tmp/archive-gz.tar.gz" -C "$tmp/output1" binary
echo "gz plain archive, target without dot exit=$? path=$(find "$tmp/output1" -maxdepth 3 -type f | sort | sed 's#'"$tmp/output1"'/##g' | tr '\n' ',')"
tar -xzf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/output2" binary
echo "gz dot archive, target without dot exit=$? path=$(find "$tmp/output2" -maxdepth 3 -type f | sort | sed 's#'"$tmp/output2"'/##g' | tr '\n' ',')"
tar -xzf "$tmp/archive-gz-dot.tar.gz" -C "$tmp/output2" "./binary"
echo "gz dot archive, target with dot exit=$? path=$(find "$tmp/output2" -maxdepth 3 -type f | sort | sed 's#'"$tmp/output2"'/##g' | tr '\n' ',')"
echo "gz plain archive, target with dot exit=$? path=$(find "$tmp/output1" -maxdepth 3 -type f | sort | sed 's#'"$tmp/output1"'/##g' | tr '\n' ',')"
exit_code=$?
tar -xzf "$tmp/archive-gz.tar.gz" -C "$tmp/output1" "./binary"
if [ "$exit_code" -ne 0 ]; then
echo "gz plain archive, target with dot exit=$? path=none"
fi
echo "== archive metadata =="
tar -tvzf "$tmp/archive-gz.tar.gz"
tar -tvz "$tmp/archive-gz-dot.tar.gz" -C "$tmp/source"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 850
Use the raw tar member name for target extraction. tar -t can report the member as ./binary, and targeting that archive with binary exits with “Not found in archive”; keep the raw entry only for the extraction call while stripping ./ for the layout comparison. Update the test expectation so extraction targets ./binary when the listing produces ./binary.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 2 files
src/services/managed-binary/archive.ts#L89-L105(this comment)src/services/managed-binary/__tests__/archive.spec.ts#L61-L79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/managed-binary/archive.ts` around lines 89 - 105, Preserve the
raw tar member name in extractSingleFileTarXzArchive for the extraction command,
while normalizing a separate value by removing ./ for the single-entry layout
comparison. Update the archive.spec.ts test expectation so a ./binary listing
results in extraction targeting ./binary; apply the change in both specified
files.
| let received = 0 | ||
| const output = createWriteStream( | ||
| destination, | ||
| options.exclusiveDestination ? { flags: "wx", mode: 0o600 } : undefined, | ||
| ) | ||
| response.on("data", (chunk: Buffer) => { | ||
| received += chunk.length | ||
| if (options.maxBytes !== undefined) { | ||
| try { | ||
| assertSizeWithinLimit(received, options.maxBytes, options.name) | ||
| } catch (error) { | ||
| response.destroy() | ||
| request.destroy(error as Error) | ||
| reject(error) | ||
| } | ||
| } | ||
| }) | ||
| response.on("error", reject) | ||
| response.pipe(output) | ||
| output.on("finish", () => { | ||
| output.close() | ||
| resolve() | ||
| }) | ||
| output.on("error", reject) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Output write stream is never closed when the streamed size limit is exceeded.
When received exceeds maxBytes mid-stream (line 129-134), the code destroys response and request and rejects, but never closes/destroys output. Since response.destroy() doesn't emit an 'error' event, pipe() won't auto-unpipe/close the destination, leaving the write stream (and its fd) open indefinitely. Cleanup at the install layer removes the file (fs.rm in install.ts), but the descriptor leak remains.
🔧 Proposed fix
response.on("data", (chunk: Buffer) => {
received += chunk.length
if (options.maxBytes !== undefined) {
try {
assertSizeWithinLimit(received, options.maxBytes, options.name)
} catch (error) {
response.destroy()
request.destroy(error as Error)
+ output.destroy()
reject(error)
}
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let received = 0 | |
| const output = createWriteStream( | |
| destination, | |
| options.exclusiveDestination ? { flags: "wx", mode: 0o600 } : undefined, | |
| ) | |
| response.on("data", (chunk: Buffer) => { | |
| received += chunk.length | |
| if (options.maxBytes !== undefined) { | |
| try { | |
| assertSizeWithinLimit(received, options.maxBytes, options.name) | |
| } catch (error) { | |
| response.destroy() | |
| request.destroy(error as Error) | |
| reject(error) | |
| } | |
| } | |
| }) | |
| response.on("error", reject) | |
| response.pipe(output) | |
| output.on("finish", () => { | |
| output.close() | |
| resolve() | |
| }) | |
| output.on("error", reject) | |
| }) | |
| let received = 0 | |
| const output = createWriteStream( | |
| destination, | |
| options.exclusiveDestination ? { flags: "wx", mode: 0o600 } : undefined, | |
| ) | |
| response.on("data", (chunk: Buffer) => { | |
| received += chunk.length | |
| if (options.maxBytes !== undefined) { | |
| try { | |
| assertSizeWithinLimit(received, options.maxBytes, options.name) | |
| } catch (error) { | |
| response.destroy() | |
| request.destroy(error as Error) | |
| output.destroy() | |
| reject(error) | |
| } | |
| } | |
| }) | |
| response.on("error", reject) | |
| response.pipe(output) | |
| output.on("finish", () => { | |
| output.close() | |
| resolve() | |
| }) | |
| output.on("error", reject) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/managed-binary/download.ts` around lines 120 - 144, Update the
maxBytes error path in the response data handler to explicitly destroy or close
the output stream before rejecting, alongside response.destroy() and
request.destroy(). Preserve the existing successful output.on("finish") flow and
ensure the stream’s file descriptor is released when assertSizeWithinLimit
throws.
| import * as fs from "fs/promises" | ||
| import * as path from "path" | ||
|
|
||
| const installationPromises = new Map<string, Promise<string>>() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does VS Code run a single shared local extension host process across multiple windows/workspaces open at once, or one extension host process per window?
💡 Result:
VS Code runs a separate extension host process for each individual window (workspace) [1][2]. This architecture ensures that extensions remain isolated per window, preventing an extension failure or high resource usage in one workspace from impacting others [3]. Each window manages its own lifecycle for its respective local extension host process [1][2]. When using remote development features (such as SSH, Containers, or WSL), VS Code may also spawn separate remote extension host processes on the remote machine, which are also tied to the specific remote workspace connection [4][3][5]. Consequently, you will typically observe multiple extension host processes running on your machine—or the remote host—when you have multiple VS Code windows open [1].
Citations:
- 1: https://github.com/microsoft/vscode/blob/main/src/vs/platform/extensions/electron-main/extensionHostStarter.ts
- 2: https://github.com/microsoft/vscode/blob/9d557c62/src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts
- 3: https://deepwiki.com/microsoft/vscode/5.1-extension-host-architecture
- 4: https://code.visualstudio.com/api/advanced-topics/extension-host
- 5: https://code.visualstudio.com/docs/remote/faq
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced file and relevant symbols without running repo code.
printf 'Files matching install.ts:\n'
fd -a 'install\.ts$' . || true
printf '\nRelevant imports/calls around ensureManagedBinaryInstalled:\n'
rg -n "ensureManagedBinaryInstalled|installationPromises|\\$\\{id\\}\\.new|fs\\.(rm|mkdir|rename|writeFile|createWriteStream)|lock|flock|node:fs" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 240
printf '\nReferenced file outline:\n'
ast-grep outline src/services/managed-binary/install.ts --view compact || true
printf '\nReferenced file contents:\n'
cat -n src/services/managed-binary/install.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 30941
🌐 Web query:
VS Code extension host one per window local process architecture documentation
💡 Result:
Visual Studio Code utilizes a multi-process architecture to ensure stability and performance, isolating extensions from the main workbench user interface [1]. In the standard desktop (local) environment, VS Code typically operates with a separate extension host process per window [1][2]. Key architectural aspects of the extension host include: Isolation and Performance: By running extensions in a dedicated Node.js process (the "local extension host"), VS Code ensures that computationally expensive tasks—such as language analysis or file system operations—do not block the main UI thread, maintaining a responsive editor [1]. Process Model: - Local Extension Host: In the desktop version, each window (workspace) typically manages its own dedicated extension host process [1][2]. - Multi-Environment Support: Depending on the configuration, VS Code can utilize different types of extension hosts [3][4]: - Local: A Node.js process running on the same machine as the user interface [3]. - Web: A process running in a browser Web Worker (e.g., when using VS Code for the Web) [3][1]. - Remote: A Node.js process running on a remote machine (e.g., via Remote SSH, containers, or GitHub Codespaces) [3][1]. Extension Kinds and Selection: Extensions use the "extensionKind" property in their manifest to specify where they prefer to run ("ui" or "workspace") [3][4]. The internal service responsible for orchestrating these hosts—AbstractExtensionService—manages the lifecycle, activation, and communication between these environments and the workbench [1]. When VS Code runs in a configuration that supports both local and remote environments, it selects the appropriate extension host based on these preferences and the current connection context [3][4]. Technical Implementation: The extension host process is managed and spawned by the main process (in Electron/desktop) or triggered by the workbench service layer [1][2]. Communication between the workbench and the extension host occurs over a protocol, allowing the workbench to remain decoupled from the specific implementation details of the extension host process [5][6].
Citations:
- 1: https://deepwiki.com/microsoft/vscode/5.1-extension-host-architecture
- 2: https://github.com/microsoft/vscode/blob/main/src/vs/platform/extensions/electron-main/extensionHostStarter.ts
- 3: https://code.visualstudio.com/api/advanced-topics/extension-host
- 4: https://github.com/microsoft/vscode-docs/blob/538f9c60/api/advanced-topics/extension-host.md
- 5: Move extension host out of the workbench for process reuse microsoft/vscode#123592
- 6: https://github.com/microsoft/vscode/blob/9d557c62/src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'References to managed binary install API:\n'
rg -n "ManagedBinaryInstallOptions|ensureManagedBinaryInstalled|getManagedBinaryPaths|archiver|unzip|extractArchive|managed binary|managedBinary|ManagedBinary" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 240
printf '\nTest files under managed-binary if any:\n'
fd -a 'managed-binary|install|managed' src packages . | sed -n '1,80p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 5605
Add cross-process mutual exclusion for managed-binary installs.
ensureManagedBinaryInstalled only shares an in-memory installationPromises map inside one Node.js process. Multiple VS Code extension-host processes can compute the same deterministic staging path (${id}.new) and race on download/extract/fs.rename, potentially corrupting the staging directory or promotion. Serialize installs across processes, for example with an exclusive lock file in storageDir, and keep archive/staging writes isolated to the owning attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/managed-binary/install.ts` at line 4, Add cross-process locking
to ensureManagedBinaryInstalled in addition to the in-memory
installationPromises map, using an exclusive lock file under storageDir to
serialize attempts targeting the deterministic staging path. Ensure lock
acquisition, release, and stale/error cleanup are handled safely, and keep each
owner’s archive and staging writes isolated until successful promotion.
github-actions
Bot
added
the
awaiting-review
1 participant
Related GitHub Issue
Closes #1055
Part of #1049. Split from #1050.
Description
Extracts the binary lifecycle code previously embedded in the Semble downloader into reusable managed-binary services, then migrates Semble to those services without changing its behavior.
This PR is intentionally independent of DCG. It provides:
Stack
1 / 4 — base:
mainMerge this PR first. The DCG service PR is stacked on top of this branch.
Test Procedure
pnpm --dir src exec vitest run services/managed-binary/__tests__/archive.spec.ts services/managed-binary/__tests__/download.spec.ts services/managed-binary/__tests__/install.spec.ts services/code-index/semble/__tests__/semble-downloader.spec.ts pnpm --dir src check-typesResult: 4 test files passed, 46 tests passed; type-check passed. Repository pre-push type-check also passed.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes