Skip to content

fix(desktop): a failed t3 launcher install no longer leaves temp files in its bin folder #31910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
spoukyii wants to merge 2 commits into from
Closed

fix(desktop): a failed t3 launcher install no longer leaves temp files in its bin folder #31910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d17cabc
test(desktop): a failed t3 launcher install keeps its bin folder clean
spoukyii Nov 6, 2026
377913b
fix(desktop): a failed t3 launcher install no longer leaves temp file…
spoukyii Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions apps/desktop/src/app/DesktopCliCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,20 @@ it.layer(NodeServices.layer)("DesktopCliCommand", (it) => {
}).pipe(Effect.scoped),
);

it.effect("removes its staging file when the launcher cannot be replaced", () =>

@macroscopeapp macroscopeapp Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests staging cleanup through the command wrapper, although DesktopCliShim.install is the lower layer that fails on main. Could you move this case to the shim tests and exercise install directly?

Posted via Macroscope — Test Signal

@spoukyii spoukyii Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it in DesktopCliCommand.test.ts. DesktopCliShim.install needs the desktop environment, filesystem and path services. DesktopCliShim.test.ts has no Effect harness for those; it only renders the launcher and runs it in a shell. commandIn in this file already builds those layers, and this file's existing install-failure case lives here too. command.install reaches DesktopCliShim.install with nothing in between that touches the staging file, so this case fails on main for the shim's reason. Moving it would mean copying that harness into the shim tests for one case.

@macroscopeapp macroscopeapp Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Effect.gen(function* () {
const fs = yield* FileSystem.FileSystem;
const path = yield* Path.Path;
const home = yield* fs.makeTempDirectoryScoped();
const command = yield* commandIn({ home });
const launcher = path.join(home, ".t3", "bin", "t3");
yield* fs.makeDirectory(path.join(launcher, "occupied"), { recursive: true });

expect((yield* Effect.flip(command.install)).message).toContain("Could not set up");
expect(yield* fs.readDirectory(path.dirname(launcher))).toEqual(["t3"]);
}).pipe(Effect.scoped),
);

it.effect("finds and removes a link left by a previous T3 home", () =>
Effect.gen(function* () {
const fs = yield* FileSystem.FileSystem;
Expand Down
8 changes: 6 additions & 2 deletions apps/desktop/src/app/DesktopCliShim.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,12 @@ export const install = Effect.gen(function* () {
if (Option.getOrUndefined(existing) !== content) {
yield* fs.makeDirectory(path.dirname(shimPath), { recursive: true });
const staging = `${shimPath}.${process.pid}.tmp`;
yield* fs.writeFileString(staging, content, { mode: 0o755 });
yield* fs.rename(staging, shimPath);
yield* fs
.writeFileString(staging, content, { mode: 0o755 })
.pipe(
Effect.andThen(fs.rename(staging, shimPath)),
Effect.ensuring(fs.remove(staging, { force: true }).pipe(Effect.ignore)),
Comment thread
spoukyii marked this conversation as resolved.
Resolved

@spoukyii spoukyii Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I'm keeping this one out of scope, for the same reason as the CodeRabbit thread above. Two installs in one process already share <launcher>.<pid>.tmp on main. When they interleave, one rename moves the shared file and the other fails with ENOENT. With the cleanup, the losing attempt fails at a different step, but the result is the same: the launcher is installed and the other attempt reports it couldn't set it up. So this PR doesn't introduce the race or change its outcome. It only stops a failed install from leaving its staging file behind. Per-attempt staging names (like the settings writers' suffix) and a two-call regression would fix that older race, and I'd rather send that as its own PR than widen this one.

);
yield* logInfo("installed t3 launcher", { shimPath });
}
return Option.some(shimPath);
Expand Down
Loading