Skip to content

Improve checkpoint reliability and cleanup of temp files #1367

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

Merged
nicktrn merged 20 commits into from
Sep 30, 2024
Merged

Improve checkpoint reliability and cleanup of temp files #1367

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2694e9a
improve cleanup reliability
nicktrn Sep 26, 2024
9fb9c6f
Merge branch 'main' into checkpoint-cleanup
nicktrn Sep 26, 2024
dcd1743
improve logging
nicktrn Sep 26, 2024
25276f0
bye-bye execa
nicktrn Sep 26, 2024
fd95693
fix for trailing newlines
nicktrn Sep 26, 2024
adb3d44
prettier errors
nicktrn Sep 26, 2024
0cc518f
trim args and log output by default
nicktrn Sep 26, 2024
e9415f5
fix archive cleanup
nicktrn Sep 26, 2024
43430b3
prevent potential memleak
nicktrn Sep 26, 2024
34e4a64
more cleanup debug logs
nicktrn Sep 26, 2024
3967f96
ignore abort during cleanup
nicktrn Sep 26, 2024
3f6c52f
rename checkpoint dir env var and move to helper
nicktrn Sep 27, 2024
69a5e5b
add global never throw override
nicktrn Sep 27, 2024
e124d3d
add tmp cleaner
nicktrn Sep 27, 2024
2221283
Merge remote-tracking branch 'origin/main' into checkpoint-cleanup
nicktrn Sep 27, 2024
6ecf4ac
also clean up checkpoint dir by default
nicktrn Sep 27, 2024
9437c7e
Merge remote-tracking branch 'origin/main' into checkpoint-cleanup
nicktrn Sep 27, 2024
6b75ba8
split by any whitespace, not just tabs
nicktrn Sep 30, 2024
c900253
only create tmp cleaner if paths to clean
nicktrn Sep 30, 2024
57c1f5f
Merge remote-tracking branch 'origin/main' into checkpoint-cleanup
nicktrn Sep 30, 2024
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
Prev Previous commit
Next Next commit
improve logging
  • Loading branch information
@nicktrn
nicktrn committed Sep 26, 2024
commit dcd1743d5abfd7de01daf339696a6251d063bcb6
11 changes: 6 additions & 5 deletions apps/coordinator/src/exec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ export class Exec {
}

async x(command: string, args?: string[], opts?: { neverThrow?: boolean }) {
this.logger.debug(`exec: ${command}${args?.length ? ` ${args[0]}` : ""}`, { command, args });
const commandWithFirstArg = `${command}${args?.length ? ` ${args[0]}` : ""}`;
this.logger.debug(`exec: ${commandWithFirstArg}}`, { command, args });

const result = x(command, args, {
signal: this.abortSignal,
Expand All @@ -63,25 +64,25 @@ export class Exec {
const output = await result;

if (this.logOutput) {
this.logger.debug("output", { command, args, output });
this.logger.debug(`output: ${commandWithFirstArg}}`, { command, args, output });
}

if (opts?.neverThrow) {
return output;
}

if (result.aborted) {
this.logger.error("aborted", { command, args, output });
this.logger.error(`aborted: ${commandWithFirstArg}`, { command, args, output });
throw new TinyResult(result);
}

if (result.killed) {
this.logger.error("killed", { command, args, output });
this.logger.error(`killed: ${commandWithFirstArg}`, { command, args, output });
throw new TinyResult(result);
}

if (result.exitCode !== 0) {
this.logger.error("non-zero exit", { command, args, output });
this.logger.error(`non-zero exit: ${commandWithFirstArg}`, { command, args, output });
throw new TinyResult(result);
}
Comment on lines +78 to +91
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect variable usage in error handling in Exec.x method.

In the Exec.x method, you are checking result.aborted, result.killed, and result.exitCode, but result is the promise returned by x(). You should instead reference output, which is the resolved result of the promise.

Apply this diff to correct the variable usage:

78     if (result.aborted) {
79       this.logger.error(`aborted: ${commandWithFirstArg}`, metadata);
80       throw new TinyResult(result);
81     }
82 
83     if (result.killed) {
84       this.logger.error(`killed: ${commandWithFirstArg}`, metadata);
85       throw new TinyResult(result);
86     }
87 
88     if (result.exitCode !== 0) {
89       this.logger.error(`non-zero exit: ${commandWithFirstArg}`, metadata);
90       throw new TinyResult(result);
91     }

Should be:

78     if (output.aborted) {
79       this.logger.error(`aborted: ${commandWithFirstArg}`, metadata);
80       throw new TinyResult(output);
81     }
82 
83     if (output.killed) {
84       this.logger.error(`killed: ${commandWithFirstArg}`, metadata);
85       throw new TinyResult(output);
86     }
87 
88     if (output.exitCode !== 0) {
89       this.logger.error(`non-zero exit: ${commandWithFirstArg}`, metadata);
90       throw new TinyResult(output);
91     }
📝 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.

Suggested change
if (result.aborted) {
this.logger.error(`aborted: ${commandWithFirstArg}`, metadata);
throw new TinyResult(result);
}
if (result.killed) {
this.logger.error(`killed: ${commandWithFirstArg}`, metadata);
throw new TinyResult(result);
}
if (result.exitCode !== 0) {
this.logger.error(`non-zero exit: ${commandWithFirstArg}`, metadata);
throw new TinyResult(result);
}
if (output.aborted) {
this.logger.error(`aborted: ${commandWithFirstArg}`, metadata);
throw new TinyResult(output);
}
if (output.killed) {
this.logger.error(`killed: ${commandWithFirstArg}`, metadata);
throw new TinyResult(output);
}
if (output.exitCode !== 0) {
this.logger.error(`non-zero exit: ${commandWithFirstArg}`, metadata);
throw new TinyResult(output);
}


Expand Down