fix(controller): release completed local process records - #585
fix(controller): release completed local process records#585Yusef Syed (YusefSyed) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Timeout retries can incorrectly replace the timeout error with a generic signal-exit error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes completed local subprocess records after successful terminal status updates while retaining failed updates for retry.
Changes:
- Cleans up exited and timed-out process records.
- Adds reconciliation and retry coverage.
File summaries
| File | Description |
|---|---|
agentlightning/controller/local_reconciler.py |
Removes successfully reported terminal processes. |
tests/controller/test_local_reconciler.py |
Tests exits, timeouts, retries, and running retention. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| timeout is not None | ||
| and (now - item.spawned_at) > timeout | ||
| and await self._kill_process_group(rollout_id, item) | ||
| and await self._patch(rollout_id, RolloutState.FAILED, "local subprocess timed out") |
There was a problem hiding this comment.
Preserved the timeout outcome through the existing killed marker. The retry regression now models the completed kill (returncode=-9) and asserts that both PATCH attempts carry local subprocess timed out. A separate case verifies that a natural signal exit without the marker still reports its exit code. The controller/server/package selection passes 42 tests; Ruff and Pyright pass.
Sylvester Kaczmarek (sylvesterkaczmarek)
left a comment
Sylvester Kaczmarek (sylvesterkaczmarek)
left a comment
There was a problem hiding this comment.
The normal-exit cleanup is correct: retaining the process only until the terminal patch succeeds gives reconciliation a durable retry boundary. There is one timeout retry edge to fix, though. If the process is killed successfully but the FAILED: local subprocess timed out patch fails, the next pass goes through _finish_proc with returncode == -9 and can replace the original timeout reason with a generic signal-exit error. Please preserve the timeout terminal state/reason across a failed patch so retries report the same failure that actually caused the kill.
Completed local subprocess records remain in
_rid_to_procafter their terminal state is successfully reported, so the controller retains them and repeatedly scans them for the rest of its lifetime. This also occurs when a timed-out subprocess is killed and its failed state is reported.Remove records after successful terminal updates on both paths. Keep them when the update fails so reconciliation can retry, preserve the timeout reason across retries after a successful kill, and leave running processes tracked. Tests cover successful and failed exits, failed terminal updates followed by retry, timeouts, and running-process retention.
Validation: the controller, server, and package test selection passes all 42 cases. Scoped Ruff, formatting, Pyright, and
git diff --checkpass. No GPU or Kubernetes cluster is required for these regressions.AI assistance: Codex helped investigate, implement, and validate this change.