Skip to content

Commit 2eb60c3

Browse files
EhabYEhabY
committed
fix: use platform-specific name for temp/cleanup in shared directories
Always download to the platform-specific binary name (coder-linux-amd64) for temp files, old backups, signatures, and lock files. After a successful download, rename the result to the user's configured file name while still holding the lock to avoid races with other windows. Scoped rmOld to only clean files prefixed with the binary basename so it does not accidentally remove unrelated files when the target directory is shared (e.g. /usr/bin).
1 parent 6714fe1 commit 2eb60c3

File tree

4 files changed

+99
-59
lines changed

4 files changed

+99
-59
lines changed

‎src/core/cliManager.ts‎

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,12 @@ export class CliManager {
172172
throw new Error("Unable to download CLI because downloads are disabled");
173173
}
174174

175-
// File destinations download to the same path. Directory destinations
176-
// always use the platform-specific binary name.
177-
const downloadBinPath =
178-
resolved.kind === "file"
179-
? resolved.binPath
180-
: path.join(path.dirname(resolved.binPath), cliUtils.name());
175+
// Always use the platform-specific name for downloads, temp files, and
176+
// cleanup so shared directories stay safe and leftovers are predictable.
177+
const downloadBinPath = path.join(
178+
path.dirname(resolved.binPath),
179+
cliUtils.name(),
180+
);
181181

182182
// Create the `bin` folder if it doesn't exist
183183
await fs.mkdir(path.dirname(downloadBinPath), { recursive: true });
@@ -203,29 +203,46 @@ export class CliManager {
203203
downloadBinPath,
204204
latestBuildInfo.version,
205205
);
206-
if (recheckAfterWait.matches) {
206+
if (!recheckAfterWait.matches) {
207+
// Parse the latest version for download
208+
const latestParsedVersion = semver.parse(latestBuildInfo.version);
209+
if (!latestParsedVersion) {
210+
throw new Error(
211+
`Got invalid version from deployment: ${latestBuildInfo.version}`,
212+
);
213+
}
214+
latestVersion = latestParsedVersion;
215+
await this.performBinaryDownload(
216+
restClient,
217+
latestVersion,
218+
downloadBinPath,
219+
progressLogPath,
220+
);
221+
} else {
207222
this.output.debug(
208223
"Using existing binary since it matches the latest server version",
209224
);
210-
return downloadBinPath;
211225
}
212-
213-
// Parse the latest version for download
214-
const latestParsedVersion = semver.parse(latestBuildInfo.version);
215-
if (!latestParsedVersion) {
216-
throw new Error(
217-
`Got invalid version from deployment: ${latestBuildInfo.version}`,
218-
);
219-
}
220-
latestVersion = latestParsedVersion;
226+
} else {
227+
await this.performBinaryDownload(
228+
restClient,
229+
latestVersion,
230+
downloadBinPath,
231+
progressLogPath,
232+
);
221233
}
222234

223-
return await this.performBinaryDownload(
224-
restClient,
225-
latestVersion,
226-
downloadBinPath,
227-
progressLogPath,
228-
);
235+
// If the user configured a specific file path, rename while we hold
236+
// the lock to avoid races with other windows.
237+
if (resolved.kind === "file" && downloadBinPath !== resolved.binPath) {
238+
this.output.info(
239+
"Renaming downloaded binary to",
240+
path.basename(resolved.binPath),
241+
);
242+
await fs.rename(downloadBinPath, resolved.binPath);
243+
return resolved.binPath;
244+
}
245+
return downloadBinPath;
229246
} catch (error) {
230247
// Unified error handling - check for fallback binaries and prompt user
231248
return await this.handleAnyBinaryFailure(

‎src/core/cliUtils.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,18 @@ export interface RemovalResult {
4141
*/
4242
export async function rmOld(binPath: string): Promise<RemovalResult[]> {
4343
const binDir = path.dirname(binPath);
44+
const binBase = path.basename(binPath);
4445
try {
4546
const files = await fs.readdir(binDir);
4647
const results: RemovalResult[] = [];
4748
for (const file of files) {
4849
const fileName = path.basename(file);
4950
if (
50-
fileName.includes(".old-") ||
51-
fileName.includes(".temp-") ||
52-
fileName.endsWith(".asc") ||
53-
fileName.endsWith(".progress.log")
51+
fileName.startsWith(binBase) &&
52+
(fileName.includes(".old-") ||
53+
fileName.includes(".temp-") ||
54+
fileName.endsWith(".asc") ||
55+
fileName.endsWith(".progress.log"))
5456
) {
5557
try {
5658
await fs.rm(path.join(binDir, file), { force: true });

‎test/unit/core/cliManager.test.ts‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -474,25 +474,40 @@ describe("CliManager", () => {
474474
});
475475

476476
it("cleans up old files before download", async () => {
477-
// Create old temporary files and signature files
477+
// Create old temporary files and signature files matching the binary name
478478
vol.mkdirSync(BINARY_DIR, { recursive: true });
479-
memfs.writeFileSync(path.join(BINARY_DIR, "coder.old-xyz"), "old");
480-
memfs.writeFileSync(path.join(BINARY_DIR, "coder.temp-abc"), "temp");
481-
memfs.writeFileSync(path.join(BINARY_DIR, "coder.asc"), "signature");
479+
memfs.writeFileSync(
480+
path.join(BINARY_DIR, `${BINARY_NAME}.old-xyz`),
481+
"old",
482+
);
483+
memfs.writeFileSync(
484+
path.join(BINARY_DIR, `${BINARY_NAME}.temp-abc`),
485+
"temp",
486+
);
487+
memfs.writeFileSync(
488+
path.join(BINARY_DIR, `${BINARY_NAME}.asc`),
489+
"signature",
490+
);
491+
// Unrelated files should not be removed
482492
memfs.writeFileSync(path.join(BINARY_DIR, "keeper.txt"), "keep");
493+
memfs.writeFileSync(path.join(BINARY_DIR, "other.old-xyz"), "keep");
483494

484495
withSuccessfulDownload();
485496
await manager.fetchBinary(mockApi);
486497

487-
// Verify old files were actually removed but other files kept
488-
expect(memfs.existsSync(path.join(BINARY_DIR, "coder.old-xyz"))).toBe(
489-
false,
490-
);
491-
expect(memfs.existsSync(path.join(BINARY_DIR, "coder.temp-abc"))).toBe(
492-
false,
493-
);
494-
expect(memfs.existsSync(path.join(BINARY_DIR, "coder.asc"))).toBe(false);
498+
expect(
499+
memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.old-xyz`)),
500+
).toBe(false);
501+
expect(
502+
memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.temp-abc`)),
503+
).toBe(false);
504+
expect(
505+
memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.asc`)),
506+
).toBe(false);
495507
expect(memfs.existsSync(path.join(BINARY_DIR, "keeper.txt"))).toBe(true);
508+
expect(memfs.existsSync(path.join(BINARY_DIR, "other.old-xyz"))).toBe(
509+
true,
510+
);
496511
});
497512

498513
it("moves existing binary to backup file before writing new version", async () => {

‎test/unit/core/cliUtils.test.ts‎

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,50 +40,56 @@ describe("CliUtils", () => {
4040
expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([]);
4141

4242
await fs.mkdir(binDir, { recursive: true });
43-
await fs.writeFile(path.join(binDir, "bin.old-1"), "echo hello");
44-
await fs.writeFile(path.join(binDir, "bin.old-2"), "echo hello");
45-
await fs.writeFile(path.join(binDir, "bin.temp-1"), "echo hello");
46-
await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello");
47-
await fs.writeFile(path.join(binDir, "bin1"), "echo hello");
48-
await fs.writeFile(path.join(binDir, "bin2"), "echo hello");
49-
await fs.writeFile(path.join(binDir, "bin.asc"), "echo hello");
50-
await fs.writeFile(path.join(binDir, "bin.old-1.asc"), "echo hello");
51-
await fs.writeFile(path.join(binDir, "bin.temp-2.asc"), "echo hello");
43+
await fs.writeFile(path.join(binDir, "coder.old-1"), "echo hello");
44+
await fs.writeFile(path.join(binDir, "coder.old-2"), "echo hello");
45+
await fs.writeFile(path.join(binDir, "coder.temp-1"), "echo hello");
46+
await fs.writeFile(path.join(binDir, "coder.temp-2"), "echo hello");
47+
await fs.writeFile(path.join(binDir, "coder"), "echo hello");
48+
await fs.writeFile(path.join(binDir, "other-bin"), "echo hello");
49+
await fs.writeFile(path.join(binDir, "coder.asc"), "echo hello");
50+
await fs.writeFile(path.join(binDir, "coder.old-1.asc"), "echo hello");
51+
await fs.writeFile(path.join(binDir, "coder.temp-2.asc"), "echo hello");
52+
// Unrelated files with matching patterns should not be removed.
53+
await fs.writeFile(path.join(binDir, "unrelated.old-1"), "echo hello");
54+
await fs.writeFile(path.join(binDir, "unrelated.temp-1"), "echo hello");
5255

53-
expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([
56+
expect(await cliUtils.rmOld(path.join(binDir, "coder"))).toStrictEqual([
5457
{
55-
fileName: "bin.asc",
58+
fileName: "coder.asc",
5659
error: undefined,
5760
},
5861
{
59-
fileName: "bin.old-1",
62+
fileName: "coder.old-1",
6063
error: undefined,
6164
},
6265
{
63-
fileName: "bin.old-1.asc",
66+
fileName: "coder.old-1.asc",
6467
error: undefined,
6568
},
6669
{
67-
fileName: "bin.old-2",
70+
fileName: "coder.old-2",
6871
error: undefined,
6972
},
7073
{
71-
fileName: "bin.temp-1",
74+
fileName: "coder.temp-1",
7275
error: undefined,
7376
},
7477
{
75-
fileName: "bin.temp-2",
78+
fileName: "coder.temp-2",
7679
error: undefined,
7780
},
7881
{
79-
fileName: "bin.temp-2.asc",
82+
fileName: "coder.temp-2.asc",
8083
error: undefined,
8184
},
8285
]);
8386

84-
expect(await fs.readdir(path.join(tmp, "bins"))).toStrictEqual([
85-
"bin1",
86-
"bin2",
87+
// Only the binary and unrelated files should remain.
88+
expect((await fs.readdir(binDir)).sort()).toStrictEqual([
89+
"coder",
90+
"other-bin",
91+
"unrelated.old-1",
92+
"unrelated.temp-1",
8793
]);
8894
});
8995

0 commit comments

Comments
 (0)