Conversation
Confidence Score: 1/5
Important Files Changed
Last reviewed commit: 39ef683 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,344 @@ | |||
| use std::collections::{HashMap, HashSet}; | |||
| use std::io::Write; | |||
| use std::os::unix::fs::MetadataExt; | |||
There was a problem hiding this comment.
Unix-only import used unconditionally
std::os::unix::fs::MetadataExt is a Unix-only trait imported unconditionally. This will cause a compile error on Windows. The import and all code that calls .ino() (lines ~45–47) must be gated behind #[cfg(unix)], with the inode-watching block either disabled or replaced with a no-op on non-Unix platforms.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm/src/daemon/coordinator.rs
Line: 3
Comment:
Unix-only import used unconditionally
`std::os::unix::fs::MetadataExt` is a Unix-only trait imported unconditionally. This will cause a compile error on Windows. The import and all code that calls `.ino()` (lines ~45–47) must be gated behind `#[cfg(unix)]`, with the inode-watching block either disabled or replaced with a no-op on non-Unix platforms.
How can I resolve this? If you propose a fix, please make it concise.| let output_buffer: OutputBuffer | ||
| = Arc::new(RwLock::new(HashMap::new())); | ||
|
|
||
| let subscription_registry | ||
| = Arc::new(SubscriptionRegistry::new()); | ||
|
|
||
| let long_lived_registry | ||
| = Arc::new(LongLivedRegistry::new()); | ||
|
|
||
| let scheduler_for_loop | ||
| = scheduler.clone(); | ||
|
|
||
| let (loop_event_tx, mut loop_event_rx) | ||
| = mpsc::unbounded_channel::<ExecutorEvent>(); | ||
|
|
||
| let subscription_registry_for_loop | ||
| = subscription_registry.clone(); | ||
|
|
||
| let subscription_registry_for_events | ||
| = subscription_registry.clone(); | ||
|
|
||
| let output_buffer_for_events | ||
| = output_buffer.clone(); | ||
|
|
||
| let long_lived_registry_for_events | ||
| = long_lived_registry.clone(); | ||
|
|
||
| let scheduler_for_events | ||
| = scheduler.clone(); | ||
|
|
||
| tokio::spawn(async move { | ||
| while let Some(event) = loop_event_rx.recv().await { | ||
| if let ExecutorEvent::Output { task_id, line, stream } = &event { | ||
| if let Ok(mut buffer) = output_buffer_for_events.write() { | ||
| let lines: &mut Vec<BufferedOutputLine> | ||
| = buffer | ||
| .entry(task_id.to_string()) | ||
| .or_insert_with(Vec::new); | ||
|
|
||
| lines.push(BufferedOutputLine { | ||
| line: line.to_string(), | ||
| stream: stream.as_str().to_string(), | ||
| }); | ||
|
|
||
| if lines.len() > OUTPUT_BUFFER_MAX_LINES { | ||
| let excess | ||
| = lines.len() - OUTPUT_BUFFER_MAX_LINES; | ||
|
|
||
| lines.drain(0..excess); | ||
| } |
There was a problem hiding this comment.
Unbounded memory growth in output buffer
The output_buffer HashMap (created at line 58–59) accumulates an entry for every task ID that ever runs in this daemon session. While the per-task line count is capped at OUTPUT_BUFFER_MAX_LINES (1000 lines), the number of task entries in the HashMap is never pruned. For a long-running daemon that processes thousands of short-lived tasks, this will steadily grow the resident memory of the daemon process. Entries for completed tasks (particularly non-long-lived ones whose output has already been retrieved by the client) should be removed once they are no longer needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm/src/daemon/coordinator.rs
Line: 58-107
Comment:
Unbounded memory growth in output buffer
The `output_buffer` HashMap (created at line 58–59) accumulates an entry for every task ID that ever runs in this daemon session. While the per-task line count is capped at `OUTPUT_BUFFER_MAX_LINES` (1000 lines), the number of task entries in the HashMap is never pruned. For a long-running daemon that processes thousands of short-lived tasks, this will steadily grow the resident memory of the daemon process. Entries for completed tasks (particularly non-long-lived ones whose output has already been retrieved by the client) should be removed once they are no longer needed.
How can I resolve this? If you propose a fix, please make it concise.| pub fn kill(&self) { | ||
| #[cfg(unix)] | ||
| { | ||
| let _ = std::process::Command::new("kill") | ||
| .arg("-9") | ||
| .arg(format!("-{}", self.pid)) | ||
| .status(); | ||
| } | ||
| } |
There was a problem hiding this comment.
SIGKILL on entire process group prevents graceful cleanup
kill -9 -{pid} sends SIGKILL to every process in the daemon's process group. Because SIGKILL cannot be caught or ignored, neither the daemon nor any of its running task children will have a chance to flush buffers, clean up temporary files, or release resources. In addition, if any task subprocess moves itself to a different process group, it will survive this kill.
For the standalone case it may be acceptable to be forceful, but using SIGTERM first (with a timeout and SIGKILL as a fallback) would be safer and more consistent with the SIGTERM used elsewhere in the codebase.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm/src/daemon/client.rs
Line: 41-49
Comment:
SIGKILL on entire process group prevents graceful cleanup
`kill -9 -{pid}` sends SIGKILL to every process in the daemon's process group. Because SIGKILL cannot be caught or ignored, neither the daemon nor any of its running task children will have a chance to flush buffers, clean up temporary files, or release resources. In addition, if any task subprocess moves itself to a different process group, it will survive this kill.
For the standalone case it may be acceptable to be forceful, but using SIGTERM first (with a timeout and SIGKILL as a fallback) would be safer and more consistent with the SIGTERM used elsewhere in the codebase.
How can I resolve this? If you propose a fix, please make it concise.| #[cfg(windows)] | ||
| { | ||
| use std::ptr::null_mut; | ||
| unsafe { | ||
| let handle = winapi::um::processthreadsapi::OpenProcess( | ||
| winapi::um::winnt::PROCESS_QUERY_LIMITED_INFORMATION, | ||
| 0, | ||
| pid, | ||
| ); | ||
| if handle.is_null() { | ||
| false | ||
| } else { | ||
| winapi::um::handleapi::CloseHandle(handle); | ||
| true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(any(unix, windows)))] | ||
| { | ||
| true | ||
| } | ||
| } | ||
|
|
||
| pub fn kill_process(pid: u32) -> bool { | ||
| #[cfg(unix)] | ||
| { | ||
| unsafe { libc::kill(pid as i32, libc::SIGTERM) == 0 } | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| use std::ptr::null_mut; | ||
| unsafe { | ||
| let handle = winapi::um::processthreadsapi::OpenProcess( | ||
| winapi::um::winnt::PROCESS_TERMINATE, | ||
| 0, | ||
| pid, | ||
| ); | ||
| if handle.is_null() { | ||
| false | ||
| } else { | ||
| let result = winapi::um::processthreadsapi::TerminateProcess(handle, 1) != 0; | ||
| winapi::um::handleapi::CloseHandle(handle); | ||
| result | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(any(unix, windows)))] | ||
| { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
winapi crate referenced but not declared as a dependency
Both is_process_alive and kill_process have #[cfg(windows)] branches that reference winapi::um::processthreadsapi, winapi::um::winnt, and winapi::um::handleapi. However, winapi does not appear in the Cargo.toml for zpm-switch (nor in the workspace Cargo.toml changes in this PR). This will produce a compile error on Windows. You need to add:
[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3", features = ["processthreadsapi", "winnt", "handleapi"] }Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-switch/src/daemons.rs
Line: 100-153
Comment:
`winapi` crate referenced but not declared as a dependency
Both `is_process_alive` and `kill_process` have `#[cfg(windows)]` branches that reference `winapi::um::processthreadsapi`, `winapi::um::winnt`, and `winapi::um::handleapi`. However, `winapi` does not appear in the `Cargo.toml` for `zpm-switch` (nor in the workspace `Cargo.toml` changes in this PR). This will produce a compile error on Windows. You need to add:
```toml
[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3", features = ["processthreadsapi", "winnt", "handleapi"] }
```
How can I resolve this? If you propose a fix, please make it concise.| client.push_tasks(task_subscriptions, parent_task_id, None, None).await?; | ||
|
|
||
| Ok(ExitStatus::from_raw(0)) | ||
| Ok(ExitStatus::from_raw(0 << 8)) |
There was a problem hiding this comment.
0 << 8 is always 0 — confusing no-op
ExitStatus::from_raw(0 << 8) evaluates identically to ExitStatus::from_raw(0). The << 8 shift pattern is used elsewhere to encode non-zero exit codes in the wait-status format, but shifting zero is a no-op and reads as if the author forgot to put a real value in.
| Ok(ExitStatus::from_raw(0 << 8)) | |
| Ok(ExitStatus::from_raw(0)) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm/src/commands/tasks/push.rs
Line: 66
Comment:
`0 << 8` is always `0` — confusing no-op
`ExitStatus::from_raw(0 << 8)` evaluates identically to `ExitStatus::from_raw(0)`. The `<< 8` shift pattern is used elsewhere to encode non-zero exit codes in the wait-status format, but shifting zero is a no-op and reads as if the author forgot to put a real value in.
```suggestion
Ok(ExitStatus::from_raw(0))
```
How can I resolve this? If you propose a fix, please make it concise.| bar: | ||
| sleep 5 | ||
|
|
||
| bar2: | ||
| sleep 10 | ||
|
|
||
| x: | ||
| python3 -c "import time; print(f'ts:{int(time.time()*1000)}:line1')" | ||
| sleep 1 | ||
| python3 -c "import time; print(f'ts:{int(time.time()*1000)}:line2')" | ||
| sleep 1 | ||
| python3 -c "import time; print(f'ts:{int(time.time()*1000)}:line3')" | ||
|
|
||
| producer: | ||
| for x in {1..10}; do | ||
| echo "producer: $x" | ||
| sleep 1 | ||
| done | ||
|
|
||
| foo: bar& bar2& | ||
| echo "foo" | ||
|
|
There was a problem hiding this comment.
Debug/test tasks left in the repository root taskfile
The tasks bar, bar2, x, producer, and foo appear to be development scratch entries added to test the new daemon functionality. They don't appear to serve any project-level purpose and should be removed before merging, or moved to a test fixture if they are needed for acceptance tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: taskfile
Line: 1-22
Comment:
Debug/test tasks left in the repository root `taskfile`
The tasks `bar`, `bar2`, `x`, `producer`, and `foo` appear to be development scratch entries added to test the new daemon functionality. They don't appear to serve any project-level purpose and should be removed before merging, or moved to a test fixture if they are needed for acceptance tests.
How can I resolve this? If you propose a fix, please make it concise.| pub async fn execute(&self) -> Result<(), Error> { | ||
| let project_cwd = get_final_cwd()?; | ||
|
|
||
| let find_result = find_closest_package_manager(&project_cwd)?; | ||
|
|
||
| let detected_root = find_result | ||
| .detected_root_path | ||
| .ok_or(Error::NoProjectFound)?; | ||
|
|
||
| let Some(daemon) = daemons::get_daemon(&detected_root)? else { | ||
| println!( | ||
| "{} No daemon registered for this project", | ||
| DataType::Info.colorize("ℹ") | ||
| ); | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| if !daemons::is_process_alive(daemon.pid) { | ||
| daemons::unregister_daemon(&detected_root)?; | ||
| println!( | ||
| "{} Daemon was not running (cleaned up stale entry)", | ||
| DataType::Info.colorize("ℹ") | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if daemons::kill_process(daemon.pid) { | ||
| daemons::unregister_daemon(&detected_root)?; | ||
| println!( | ||
| "{} Stopped daemon for {} (PID: {})", | ||
| DataType::Success.colorize("✓"), | ||
| detected_root.to_print_string(), | ||
| daemon.pid | ||
| ); |
There was a problem hiding this comment.
Killing the daemon does not terminate its running task children
daemons::kill_process sends SIGTERM only to the daemon process itself (the yarn debug daemon binary). All task subprocesses that the daemon has spawned are in the same session but may be in their own process groups. When the daemon receives SIGTERM it will exit — but because nothing in the daemon's signal handling path terminates the child processes, those tasks continue running as orphans.
This means switch daemon --kill can leave long-running tasks (e.g. @long-lived dev servers) silently running in the background after the user believes they have been stopped. The daemon should either propagate the signal to its children on shutdown, or the kill command should enumerate and terminate task children before sending SIGTERM to the daemon.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-switch/src/commands/switch/daemon_kill.rs
Line: 21-54
Comment:
Killing the daemon does not terminate its running task children
`daemons::kill_process` sends `SIGTERM` only to the daemon process itself (the `yarn debug daemon` binary). All task subprocesses that the daemon has spawned are in the same session but may be in their own process groups. When the daemon receives SIGTERM it will exit — but because nothing in the daemon's signal handling path terminates the child processes, those tasks continue running as orphans.
This means `switch daemon --kill` can leave long-running tasks (e.g. `@long-lived` dev servers) silently running in the background after the user believes they have been stopped. The daemon should either propagate the signal to its children on shutdown, or the kill command should enumerate and terminate task children before sending SIGTERM to the daemon.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
This PR adds support for long-running tasks, currently annotated with a
@long-runningattribute.To support that, task execution has been moved inside a daemon process managed by Yarn Switch. The core logic still lives inside Yarn (not Yarn Switch), with Yarn Switch being merely responsible to keep records about which daemons are in use in which projects.
The daemons are currently accessible through unauthenticated websockets listening on localhost. It's slightly insecure in a multi-user context, auth should be implemented in a follow-up.