fix: await process.kill() in killBatchRun to prevent race condition

[pedramamini]

Summary

• Fixed race condition in killBatchRun where window.maestro.process.kill() (returns Promise<boolean> via IPC) was never awaited, causing state cleanup steps to execute before process termination completed
• Added try/catch around the awaited kill to ensure cleanup proceeds even if the kill fails

Test plan

• Start an Auto Run batch, click Kill while agent is active — verify process terminates before batch state resets
• Kill during error-paused state — verify error resolution and cleanup happen in correct order
• npm run lint passes clean

[claude]

Code Review

Summary

This PR fixes a critical race condition in batch run termination. The fix is correct and necessary - window.maestro.process.kill() returns Promise but was never awaited, allowing state cleanup to race ahead of actual process termination.

---

✅ Strengths

1. Root cause correctly identified: The IPC call returns a Promise that was being ignored
2. Proper error handling: Try/catch ensures cleanup proceeds even if kill fails
3. Good async propagation: Both killBatchRun and handleKillBatchRun are correctly marked async
4. Clear commit message: Explains the issue and consequences well

---

⚠️ Issues Found

1. Type signature mismatch (Minor)

Location: src/renderer/hooks/props/useRightPanelProps.ts:122

The type definition needs updating to reflect the async change from void to Promise

Impact: TypeScript won't catch missing await calls. Currently all callers don't await (which is fine since they're in onClick handlers), but future code might incorrectly assume synchronous execution.

2. RightPanel.tsx type definition (Minor)

Location: src/renderer/components/RightPanel.tsx:117

Same issue - prop type needs updating from void to Promise

3. handleKillBatchRun doesn't await (Question)

Location: src/renderer/App.tsx:6689

While making handleKillBatchRun async is correct for type safety, the caller in RightPanel.tsx:732 doesn't await it (fire-and-forget in onClick handler).

Question: Is this intentional? If so, consider adding a comment. If not, the modal's onConfirm should be async and await the call.

---

🔍 Performance Considerations

Positive change: The await ensures process termination completes before state cleanup, preventing:

• Stale process instances consuming resources
• Race conditions where new processes spawn before old ones die
• Corrupted batch state from premature cleanup

The added latency (~milliseconds for process.kill() IPC roundtrip) is negligible compared to the bugs it prevents.

---

🔒 Security Considerations

No security concerns. The change tightens control flow, which is good for security.

---

🧪 Test Coverage

Action needed: The test file src/tests/renderer/components/RightPanel.test.tsx has tests for onKillBatchRun (lines 617-673) that may need updating to handle async behavior.

If the function signature changes to return Promise, consider making the mock return a resolved promise and testing that the promise is actually called.

---

📋 Recommendations

High priority:

1. Update type signatures in useRightPanelProps.ts:122 and RightPanel.tsx:117
2. Run npm run lint to catch any other type mismatches
3. Clarify whether RightPanel.tsx:732 should await or is intentionally fire-and-forget

Low priority:
4. Review test coverage for async behavior
5. Consider adding a JSDoc comment to killBatchRun explaining why it's async (not obvious from callers)

---

✅ Verdict

Approve with minor fixes. The core fix is solid and addresses a real race condition. The type signature inconsistencies are minor and easily fixed.

Estimated fix time: 2-3 minutes for type updates + lint check.