Skip to content

fix(spurd): remove dead code and wire cgroup cleanup#157

Merged
shiv-tyagi merged 1 commit intoROCm:mainfrom
shiv-tyagi:fix/spurd-dead-code-cleanup
May 7, 2026
Merged

fix(spurd): remove dead code and wire cgroup cleanup#157
shiv-tyagi merged 1 commit intoROCm:mainfrom
shiv-tyagi:fix/spurd-dead-code-cleanup

Conversation

@shiv-tyagi
Copy link
Copy Markdown
Member

Remove superseded functions from executor.rs (job_execution_loop, report_completion, signal_job, run_epilog) and the unused create_server helper from agent_server.rs. Drop the never-read job_id field from RunningJob variants and the test-only which() utility from container.rs.

Wire cleanup_cgroup into the agent server's monitor loop so cgroup directories are properly removed when jobs finish. Clean up all unused imports and prefix intentionally-unused parameters with underscore.

Remove superseded functions from executor.rs (job_execution_loop,
report_completion, signal_job, run_epilog) and the unused create_server
helper from agent_server.rs. Drop the never-read job_id field from
RunningJob variants and the test-only which() utility from container.rs.

Wire cleanup_cgroup into the agent server's monitor loop so cgroup
directories are properly removed when jobs finish. Clean up all unused
imports and prefix intentionally-unused parameters with underscore.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes superseded/dead code in the spurd agent implementation and wires job cgroup cleanup into the agent’s monitoring loop so cgroup directories are removed when jobs finish.

Changes:

  • Removed unused helpers/fields (executor loop stubs, RunningJob.job_id, agent_server::create_server, container::which() + tests) and cleaned up unused imports/params.
  • Exposed and invoked cgroup cleanup from the agent server’s job monitor loop on job completion.
  • Minor housekeeping in GPU/Landlock modules (unused imports, intentionally-unused parameter naming).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/spurd/src/reporter.rs Removes unused imports after resource reporting refactors.
crates/spurd/src/landlock.rs Removes unused warn import.
crates/spurd/src/gpu.rs Removes unused warn import and marks an unused parameter.
crates/spurd/src/executor.rs Deletes dead executor-loop code, removes RunningJob.job_id, adds take_cgroup, and makes cleanup_cgroup callable from the agent server.
crates/spurd/src/container.rs Removes the test-only which() helper and its tests.
crates/spurd/src/agent_server.rs Drops unused server helper and wires cgroup cleanup into the completion/monitor loop.
Comments suppressed due to low confidence (1)

crates/spurd/src/executor.rs:542

  • cleanup_cgroup only reads/kills PIDs from cgroup.procs in the current cgroup. In cgroup v2, cgroup.procs does not include processes in descendant cgroups, so jobs that create nested cgroups can leave processes running and prevent successful removal. Consider using cgroup.kill when available, or recursively walking child cgroups to kill their cgroup.procs before attempting removal.
    // Kill any remaining processes
    if let Ok(pids) = std::fs::read_to_string(cgroup_path.join("cgroup.procs")) {
        for pid_str in pids.lines() {
            if let Ok(pid) = pid_str.trim().parse::<i32>() {
                let _ = signal::kill(Pid::from_raw(pid), Signal::SIGKILL);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/spurd/src/executor.rs
Comment thread crates/spurd/src/agent_server.rs
@shiv-tyagi shiv-tyagi force-pushed the fix/spurd-dead-code-cleanup branch from 8564376 to 178046e Compare May 7, 2026 10:54
@shiv-tyagi
Copy link
Copy Markdown
Member Author

Addressed the copilot review comments. Should be okay to merge now. The other things raised by copilot should be done in a separate PR.

@shiv-tyagi shiv-tyagi merged commit b96af21 into ROCm:main May 7, 2026
11 of 12 checks passed
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