feat(doctor): detect and repair stale runtime run lock#785
Open
mmTheBest wants to merge 2 commits intopaperclipai:masterfrom
Open
feat(doctor): detect and repair stale runtime run lock#785mmTheBest wants to merge 2 commits intopaperclipai:masterfrom
mmTheBest wants to merge 2 commits intopaperclipai:masterfrom
Conversation
Contributor
Greptile SummaryThis PR introduces a new Key findings:
Confidence Score: 2/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: cli/src/checks/runtime-lock-check.ts
Line: 14-19
Comment:
**`EPERM` mis-classified as dead process**
`process.kill(pid, 0)` on POSIX systems throws `EPERM` when the target **process is alive but owned by a different user** (you lack permission to signal it). The current catch-all means a live lock held by another OS user will be reported as stale and offered for deletion, which can corrupt a legitimately running instance.
The fix is to treat `EPERM` as "alive":
```suggestion
try {
process.kill(pid, 0);
return true;
} catch (err) {
if ((err as NodeJS.ErrnoException).code === "EPERM") return true;
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/src/__tests__/runtime-lock-check.test.ts
Line: 36
Comment:
**Fragile PID assumption in stale lock test**
The test writes PID `999999` into the lock file, betting that no process with that PID is running. On Linux systems where `/proc/sys/kernel/pid_max` is raised above the default (e.g. to `4194304`), PID 999999 is a valid, potentially live process, and the check would return `"warn"` with `canRepair: false` (live lock path) instead of `"warn"` with a repair function — silently breaking the assertion on line 40.
A more reliable approach is to spawn a child process, record its PID, let it exit, and then write that PID into the lock file — guaranteeing the PID is dead at assertion time. Alternatively, capture the ESRCH error from a probing call to verify the PID is dead before writing the lock file.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/src/__tests__/runtime-lock-check.test.ts
Line: 33-53
Comment:
**Missing test for corrupt/unreadable lock file**
The PR description lists "warn + repairable: unreadable/corrupt lock file" as a distinct case, but there is no corresponding test. The code path (lines 39-50 of `runtime-lock-check.ts`) is completely untested. Consider adding a test that writes a non-JSON payload (e.g. `"not-json"`) to the lock file, asserts `status === "warn"` and `canRepair === true`, calls `repair()`, and confirms the file is removed.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: a1840ef |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+14
to
+19
| try { | ||
| process.kill(pid, 0); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
EPERM mis-classified as dead process
process.kill(pid, 0) on POSIX systems throws EPERM when the target process is alive but owned by a different user (you lack permission to signal it). The current catch-all means a live lock held by another OS user will be reported as stale and offered for deletion, which can corrupt a legitimately running instance.
The fix is to treat EPERM as "alive":
Suggested change
| try { | |
| process.kill(pid, 0); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| try { | |
| process.kill(pid, 0); | |
| return true; | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === "EPERM") return true; | |
| return false; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/checks/runtime-lock-check.ts
Line: 14-19
Comment:
**`EPERM` mis-classified as dead process**
`process.kill(pid, 0)` on POSIX systems throws `EPERM` when the target **process is alive but owned by a different user** (you lack permission to signal it). The current catch-all means a live lock held by another OS user will be reported as stale and offered for deletion, which can corrupt a legitimately running instance.
The fix is to treat `EPERM` as "alive":
```suggestion
try {
process.kill(pid, 0);
return true;
} catch (err) {
if ((err as NodeJS.ErrnoException).code === "EPERM") return true;
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.| it("warns and repair removes stale lock", async () => { | ||
| const home = setupHome(); | ||
| const lock = path.join(home, "instances", "default", "run.lock.json"); | ||
| fs.writeFileSync(lock, JSON.stringify({ pid: 999999, startedAt: new Date().toISOString() }), "utf8"); |
Contributor
There was a problem hiding this comment.
Fragile PID assumption in stale lock test
The test writes PID 999999 into the lock file, betting that no process with that PID is running. On Linux systems where /proc/sys/kernel/pid_max is raised above the default (e.g. to 4194304), PID 999999 is a valid, potentially live process, and the check would return "warn" with canRepair: false (live lock path) instead of "warn" with a repair function — silently breaking the assertion on line 40.
A more reliable approach is to spawn a child process, record its PID, let it exit, and then write that PID into the lock file — guaranteeing the PID is dead at assertion time. Alternatively, capture the ESRCH error from a probing call to verify the PID is dead before writing the lock file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/__tests__/runtime-lock-check.test.ts
Line: 36
Comment:
**Fragile PID assumption in stale lock test**
The test writes PID `999999` into the lock file, betting that no process with that PID is running. On Linux systems where `/proc/sys/kernel/pid_max` is raised above the default (e.g. to `4194304`), PID 999999 is a valid, potentially live process, and the check would return `"warn"` with `canRepair: false` (live lock path) instead of `"warn"` with a repair function — silently breaking the assertion on line 40.
A more reliable approach is to spawn a child process, record its PID, let it exit, and then write that PID into the lock file — guaranteeing the PID is dead at assertion time. Alternatively, capture the ESRCH error from a probing call to verify the PID is dead before writing the lock file.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+33
to
+53
| it("warns and repair removes stale lock", async () => { | ||
| const home = setupHome(); | ||
| const lock = path.join(home, "instances", "default", "run.lock.json"); | ||
| fs.writeFileSync(lock, JSON.stringify({ pid: 999999, startedAt: new Date().toISOString() }), "utf8"); | ||
|
|
||
| const r = runtimeLockCheck(); | ||
| expect(r.status).toBe("warn"); | ||
| expect(typeof r.repair).toBe("function"); | ||
| await r.repair?.(); | ||
| expect(fs.existsSync(lock)).toBe(false); | ||
| }); | ||
|
|
||
| it("warns on live lock", () => { | ||
| const home = setupHome(); | ||
| const lock = path.join(home, "instances", "default", "run.lock.json"); | ||
| fs.writeFileSync(lock, JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString() }), "utf8"); | ||
|
|
||
| const r = runtimeLockCheck(); | ||
| expect(r.status).toBe("warn"); | ||
| expect(r.message).toContain("active"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing test for corrupt/unreadable lock file
The PR description lists "warn + repairable: unreadable/corrupt lock file" as a distinct case, but there is no corresponding test. The code path (lines 39-50 of runtime-lock-check.ts) is completely untested. Consider adding a test that writes a non-JSON payload (e.g. "not-json") to the lock file, asserts status === "warn" and canRepair === true, calls repair(), and confirms the file is removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/__tests__/runtime-lock-check.test.ts
Line: 33-53
Comment:
**Missing test for corrupt/unreadable lock file**
The PR description lists "warn + repairable: unreadable/corrupt lock file" as a distinct case, but there is no corresponding test. The code path (lines 39-50 of `runtime-lock-check.ts`) is completely untested. Consider adding a test that writes a non-JSON payload (e.g. `"not-json"`) to the lock file, asserts `status === "warn"` and `canRepair === true`, calls `repair()`, and confirms the file is removed.
How can I resolve this? If you propose a fix, please make it concise.
Author
|
Addressed review feedback in e7edb72:\n- Treat EPERM as alive in runtime lock PID probing\n- Harden stale-lock test (no hardcoded 999999 PID assumption)\n- Add corrupt/unreadable lock file repair test\n\nValidated with: |
Author
|
Follow-up (escaped): validated with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a doctor check that inspects runtime run-lock state and helps recover from stale locks.
Why
A recurring reliability issue is stale or conflicting
run.lock.jsonstate, which causes confusing startup behavior on reruns.What changed
runtimeLockCheck<instanceRoot>/run.lock.jsonpaperclipai doctorflow before port check.Validation
cd cli && pnpm exec vitest run src/__tests__/runtime-lock-check.test.ts✅