Skip to content

Commit 2478d1f

Browse files
committed
fix: harden managed binary install and download against review findings
- Adopt a concurrently completed installation instead of removing it when another VS Code window finishes first, preventing transient ENOENT for consumers mid-swap. - Destroy the piped output stream when aborting an oversized download so the file descriptor is not leaked until GC. - Make the semble downloader test mock path-aware so the added mid-install re-check is modeled correctly.
1 parent 6868f54 commit 2478d1f

5 files changed

Lines changed: 65 additions & 5 deletions

File tree

src/services/code-index/semble/__tests__/semble-downloader.spec.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -766,11 +766,9 @@ describe("semble-downloader", () => {
766766
// Version matches
767767
;(fs.readFile as any).mockResolvedValue("v0.4.1")
768768
// But binary is missing
769-
let accessCallCount = 0
770-
;(fs.access as any).mockImplementation(() => {
771-
accessCallCount++
772-
// First call: binary path check (miss), subsequent: staged binary verify (pass)
773-
if (accessCallCount === 1) {
769+
;(fs.access as any).mockImplementation((target: string) => {
770+
// The installed binary path misses; the staged binary verifies fine.
771+
if (target === path.join("/storage", "semble", "semble")) {
774772
return Promise.reject(new Error("ENOENT"))
775773
}
776774
return Promise.resolve(undefined)

src/services/managed-binary/__tests__/download.spec.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,4 +158,36 @@ describe("managed binary downloads", () => {
158158
).rejects.toThrow("Example archive exceeds the download size limit")
159159
expect(mockCreateWriteStream).not.toHaveBeenCalled()
160160
})
161+
162+
it("destroys the output stream when streamed data exceeds the size limit", async () => {
163+
const request = createRequest()
164+
const response = createResponse(200, { "content-length": "5" })
165+
const output = Object.assign(new EventEmitter(), { close: vi.fn(), destroy: vi.fn() })
166+
mockCreateWriteStream.mockReturnValue(output as unknown as ReturnType<typeof createWriteStream>)
167+
mockGet.mockImplementation(
168+
(
169+
_url: string | URL,
170+
optionsOrCallback: RequestOptions | ((response: IncomingMessage) => void),
171+
optionalCallback?: (response: IncomingMessage) => void,
172+
) => {
173+
const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback
174+
setImmediate(() => callback?.(response as unknown as IncomingMessage))
175+
return request as unknown as ReturnType<typeof get>
176+
},
177+
)
178+
179+
const download = downloadBinaryFile("https://github.com/release", "/tmp/archive", {
180+
name: "Example",
181+
trustedDomains,
182+
timeoutMs: 1_000,
183+
maxBytes: 10,
184+
})
185+
await new Promise<void>((resolve) => setImmediate(resolve))
186+
response.emit("data", Buffer.alloc(11))
187+
188+
await expect(download).rejects.toThrow("Example archive exceeds the download size limit")
189+
expect(response.destroy).toHaveBeenCalled()
190+
expect(output.destroy).toHaveBeenCalled()
191+
expect(request.destroy).toHaveBeenCalledWith(expect.any(Error))
192+
})
161193
})

src/services/managed-binary/__tests__/install.spec.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,24 @@ describe("managed binary installation", () => {
8585
await expect(access(paths.archivePath)).rejects.toThrow()
8686
await expect(access(paths.stagingDir)).rejects.toThrow()
8787
})
88+
89+
it("adopts a valid installation completed by a concurrent process instead of replacing it", async () => {
90+
const options = createOptions({
91+
download: async (archivePath) => {
92+
await writeFile(archivePath, "archive")
93+
},
94+
extractArchive: async (_archivePath, stagingDir) => {
95+
await writeFile(path.join(stagingDir, "example"), "binary")
96+
const paths = getManagedBinaryPaths(options)
97+
await mkdir(paths.installRoot, { recursive: true })
98+
await writeFile(paths.binaryPath, "concurrent binary")
99+
await writeFile(paths.versionPath, options.version)
100+
},
101+
})
102+
const paths = getManagedBinaryPaths(options)
103+
104+
await expect(ensureManagedBinaryInstalled(options)).resolves.toBe(paths.binaryPath)
105+
expect(await readFile(paths.binaryPath, "utf8")).toBe("concurrent binary")
106+
await expect(access(paths.stagingDir)).rejects.toThrow()
107+
})
88108
})

src/services/managed-binary/download.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ function downloadBinaryFileWithRedirects(
129129
assertSizeWithinLimit(received, options.maxBytes, options.name)
130130
} catch (error) {
131131
response.destroy()
132+
output.destroy()
132133
request.destroy(error as Error)
133134
reject(error)
134135
}

src/services/managed-binary/install.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,15 @@ async function installManagedBinary(options: ManagedBinaryInstallOptions): Promi
102102
await makeExecutable(paths.stagedBinaryPath)
103103
await options.validateBinary?.(paths.stagedBinaryPath)
104104
await fs.writeFile(path.join(paths.stagingDir, options.versionFile), options.version, "utf-8")
105+
const currentVersion = await readInstalledVersion(paths.versionPath)
106+
if (currentVersion === options.version) {
107+
try {
108+
await makeExecutable(paths.binaryPath)
109+
return paths.binaryPath
110+
} catch {
111+
// A concurrent installer left a partial install, so replace it below.
112+
}
113+
}
105114
await fs.rm(paths.installRoot, { recursive: true, force: true })
106115
await fs.rename(paths.stagingDir, paths.installRoot)
107116
await cleanupStaleArchives(options, paths.archivePath)

0 commit comments

Comments
 (0)