Skip to content

fix(controller): release completed subprocesses while retaining launch history - #618

Open
Bluu (Bluuok) wants to merge 1 commit into
microsoft:mainfrom
Bluuok:fix/local-process-history
Open

Bluu (Bluuok) wants to merge 1 commit into
microsoft:mainfrom
Bluuok:fix/local-process-history

Conversation

@Bluuok

@Bluuok Bluu (Bluuok) commented Oct 3, 2026 •

Copy link
Copy Markdown

Local reconciliation currently retains each completed asyncio.subprocess.Process for the controller's lifetime. Once its terminal status has been reported successfully, this change replaces the process record with compact launch history, allowing the process object to be released while preserving the already-launched check.

Follow-up to #585: this deliberately retains history for every rollout ID. It does not evict history or bound the total number of IDs. Failed terminal patches and processes whose kill/wait has not completed keep their full record for retries; stale QUEUING/RUNNING responses cannot launch a completed rollout again.

Validation:

  • Full CPU suite on Ubuntu 24.04 / Python 3.12 using the locked dev/docs/verl-cpu dependencies: 143 passed, including the native POSIX controller tests.
  • Real Linux subprocess integration: normal completion and timed-out process-group termination both report terminal status, reap the child, release the Process object (weak reference cleared), and retain compact launch history.
  • Weak-reference regression: the same behavior check fails against unchanged main (the process remains referenced) and passes with this patch.
  • Ruff, formatting, header check, Pyright for the core package, pre-commit, and standard sdist/wheel build passed.
  • Native Windows baseline has existing POSIX-only test failures; the Linux results above exercise the relevant platform directly. No GPU training run was needed or performed.

The Microsoft CLA check has passed. The fork CI workflow is awaiting maintainer approval and has not run. The existing local validation and its scope are documented above; this PR is being submitted for maintainer review with those limits disclosed.

AI assistance: Codex helped implement the change and tests. The diff and validation results were reviewed before submission.

@Bluuok

Bluu (Bluuok) commented Oct 3, 2026 via email

Copy link
Copy Markdown
Author

@Bluuok
Bluu (Bluuok) marked this pull request as ready for review October 3, 2026 14:39
Copilot AI balanced review requested due to automatic review settings October 3, 2026 14:39

Copilot AI left a comment

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.

Copilot review overview

🟢 Approval recommended

The lifecycle changes are narrowly scoped, preserve retry records, and have comprehensive regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Releases completed subprocess objects after successful terminal reporting while preserving compact rollout launch history.

Changes:

  • Adds compact completed-process records and archival logic.
  • Prevents stale rollout states from relaunching completed rollouts.
  • Expands retry, shutdown, timeout, and garbage-collection coverage.
File Description
agentlightning/​controller/​local_reconciler.py Archives completed processes after successful terminal patches.
tests/​controller/​test_local_reconciler.py Verifies archival, retry, stale-state, shutdown, and object-release behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants