feat(iii-worker): replace dev subcommand with integrated local-path worker support#1443
Merged
andersonleal merged 11 commits intomainfrom Apr 10, 2026
Merged
feat(iii-worker): replace dev subcommand with integrated local-path worker support#1443andersonleal merged 11 commits intomainfrom
andersonleal merged 11 commits intomainfrom
Conversation
local-path worker support via `iii worker add ./path`. Local directory
workers are now first-class managed workers with full lifecycle
(add/start/stop/remove/list).
Key changes:
- Add `local_worker` module with handle_local_add, start_local_worker,
and workspace management (lazy setup/install on first start)
- Add `ResolvedWorkerType` enum and `resolve_worker_type()` to
centralize worker type resolution across start, stop, and list
- Support `worker_path:` field in config.yaml for local workers
- Move binary worker PID files to ~/.iii/pids/{name}.pid
- Make kill_stale_worker async; preserve installed deps across restarts
- Delete `dev.rs`, `Commands::Dev`, and associated integration tests
- Add TYPE column to `iii worker list` output
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRemoved the CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config as "Config File"
participant Local as "Local Worker"
participant LibKrun as "LibKrun Runtime"
User->>CLI: iii worker add ./my-project [--force]
CLI->>Local: handle_local_add(path, force, ...)
Local->>Local: validate path, load manifest
Local->>Config: append_worker_with_path(name, path)
Config-->>Local: updated entry
Local-->>CLI: registered / status
CLI-->>User: Worker registered
User->>CLI: iii worker start my-project
CLI->>Config: resolve_worker_type(name)
Config-->>CLI: ResolvedWorkerType::Local
CLI->>Local: start_local_worker(name, path, port)
Local->>Local: prepare sandbox, copy workspace
Local->>LibKrun: run_dev(rootfs, env, script, background, worker_name)
LibKrun->>LibKrun: spawn VM, write vm.pid, create logs
LibKrun-->>Local: started (pid) / exit code
Local-->>CLI: status
CLI-->>User: Worker active
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/iii-worker/src/cli/local_worker.rs (2)
63-85: Minor inconsistency:TCSANOWvsTCSAFLUSHfor terminal restoration.This function uses
TCSANOW(apply immediately), whilelibkrun.rsusesTCSAFLUSH(apply after draining output). Both are called fromrestore_terminal_cooked_mode, but the behavior differs slightly.For consistency with
libkrun.rs, consider usingTCSAFLUSH:💡 Suggested fix
- let _ = nix::sys::termios::tcsetattr(&stderr, nix::sys::termios::SetArg::TCSANOW, &termios); + let _ = nix::sys::termios::tcsetattr(&stderr, nix::sys::termios::SetArg::TCSAFLUSH, &termios);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 63 - 85, The restore_terminal_cooked_mode function currently calls nix::sys::termios::tcsetattr with SetArg::TCSANOW which applies changes immediately; change it to use SetArg::TCSAFLUSH to match libkrun.rs and ensure consistency (apply after draining output). Locate the call to tcsetattr in restore_terminal_cooked_mode and replace the SetArg::TCSANOW argument with SetArg::TCSAFLUSH so both modules use the same terminal restoration semantics.
21-47:detect_lan_ipis unused and macOS-specific without platform gating.This function is never called anywhere in the codebase. Additionally, it uses macOS-specific commands (
route -n get defaultandifconfig) without#[cfg]guards. If this is intended for future use or cross-platform support, it should either be removed, gated with#[cfg(target_os = "macos")], or made cross-platform (usingip routeon Linux).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 21 - 47, The function detect_lan_ip is unused and calls macOS-specific shell commands; either remove it if not needed, or gate it to macOS with #[cfg(target_os = "macos")] on the detect_lan_ip function (and add #[cfg_attr(..., allow(dead_code))] if you keep it but unused), or implement a cross-platform alternative (e.g., detect_lan_ip_linux/detect_lan_ip_macos or runtime branching using "ip route" on Linux vs "route -n get" + "ifconfig" on macOS) and expose a single cross-platform wrapper used by callers; update/remove any dead imports (tokio::process::Command) accordingly and ensure tests/usage reference the chosen variant.crates/iii-worker/src/cli/managed.rs (1)
762-777: Verify double-counting prevention logic in artifact deletion.The condition
if freed == 0prevents countingmanaged_dirsize when OCI artifacts were already freed. However, this assumes OCI and local workers are mutually exclusive for the same name. If a worker was converted from OCI to local (or vice versa), both directories might exist.Consider explicitly tracking which directories were deleted rather than relying on
freed == 0:💡 Suggested improvement
// Local-path worker: ~/.iii/managed/{name}/ (same as OCI) let managed_dir = home.join(".iii/managed").join(worker_name); if managed_dir.is_dir() { - // Only count if we haven't already freed anything (avoid double-counting with OCI) - if freed == 0 { - freed += dir_size(&managed_dir); - if let Err(e) = std::fs::remove_dir_all(&managed_dir) { + // OCI workers also use managed_dir, so only add size if we haven't + // already counted an image dir (which is separate from managed_dir) + let managed_size = dir_size(&managed_dir); + freed += managed_size; + if let Err(e) = std::fs::remove_dir_all(&managed_dir) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/managed.rs` around lines 762 - 777, The current deletion logic uses the global counter `freed == 0` to decide whether to include `managed_dir` in size counting, which can miss double-counting when both OCI and local directories exist (e.g., a worker converted between types); instead, explicitly track which directories have been removed: introduce a set or booleans (e.g., `removed_oci`, `removed_managed`, or a HashSet of removed paths) and use that to decide whether to add `dir_size(&managed_dir)` and call `remove_dir_all`; update the code paths that handle OCI artifacts (the earlier branch that modifies `freed` and removes OCI dirs) to mark the OCI path as removed so `managed_dir` handling (variables `managed_dir`, `freed`, `dir_size`) can check the explicit marker rather than `freed == 0`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/iii-worker/src/cli/local_worker.rs`:
- Around line 63-85: The restore_terminal_cooked_mode function currently calls
nix::sys::termios::tcsetattr with SetArg::TCSANOW which applies changes
immediately; change it to use SetArg::TCSAFLUSH to match libkrun.rs and ensure
consistency (apply after draining output). Locate the call to tcsetattr in
restore_terminal_cooked_mode and replace the SetArg::TCSANOW argument with
SetArg::TCSAFLUSH so both modules use the same terminal restoration semantics.
- Around line 21-47: The function detect_lan_ip is unused and calls
macOS-specific shell commands; either remove it if not needed, or gate it to
macOS with #[cfg(target_os = "macos")] on the detect_lan_ip function (and add
#[cfg_attr(..., allow(dead_code))] if you keep it but unused), or implement a
cross-platform alternative (e.g., detect_lan_ip_linux/detect_lan_ip_macos or
runtime branching using "ip route" on Linux vs "route -n get" + "ifconfig" on
macOS) and expose a single cross-platform wrapper used by callers; update/remove
any dead imports (tokio::process::Command) accordingly and ensure tests/usage
reference the chosen variant.
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 762-777: The current deletion logic uses the global counter `freed
== 0` to decide whether to include `managed_dir` in size counting, which can
miss double-counting when both OCI and local directories exist (e.g., a worker
converted between types); instead, explicitly track which directories have been
removed: introduce a set or booleans (e.g., `removed_oci`, `removed_managed`, or
a HashSet of removed paths) and use that to decide whether to add
`dir_size(&managed_dir)` and call `remove_dir_all`; update the code paths that
handle OCI artifacts (the earlier branch that modifies `freed` and removes OCI
dirs) to mark the OCI path as removed so `managed_dir` handling (variables
`managed_dir`, `freed`, `dir_size`) can check the explicit marker rather than
`freed == 0`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5ba2663-9a91-404a-b785-18a0d5f40f4c
📒 Files selected for processing (10)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/config_file.rscrates/iii-worker/src/cli/dev.rscrates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/managed.rscrates/iii-worker/src/cli/mod.rscrates/iii-worker/src/cli/worker_manager/libkrun.rscrates/iii-worker/src/main.rscrates/iii-worker/tests/config_file_integration.rscrates/iii-worker/tests/worker_integration.rs
💤 Files with no reviewable changes (3)
- crates/iii-worker/src/main.rs
- crates/iii-worker/src/cli/app.rs
- crates/iii-worker/src/cli/dev.rs
Make iii worker start for local-path workers run in the background instead of blocking the terminal. Adds background mode to run_dev() that redirects stdout/stderr to log files and returns immediately after spawning, matching binary worker behavior.
Display PID as a plain number instead of debug Option format.
Capture child PID into a variable instead of calling child.id() twice. Align home_dir fallback with rest of file (use /tmp, not empty path).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/iii-worker/src/cli/local_worker.rs`:
- Around line 169-176: The current logic in local_worker.rs pushes
project.setup_cmd and project.install_cmd as separate newline-separated commands
into parts, then always pushes "mkdir -p /var && touch /var/.iii-prepared",
which marks the workspace prepared even if setup/install fail; change the
emission so the prepared marker is only added when prior commands succeed — e.g.
build a single shell-safe command string (using "set -e;" at the start or
joining commands with " && ") when composing parts so that project.setup_cmd and
project.install_cmd are concatenated with " && " before appending "mkdir -p /var
&& touch /var/.iii-prepared"; update the code that manipulates parts,
project.setup_cmd, project.install_cmd, and the marker '/var/.iii-prepared'
accordingly.
- Around line 149-156: The directory copy currently follows symlinks and copies
targets (using src_path.is_dir() and std::fs::copy), allowing escapes from
worker_path; update the copy logic in the copy_dir_contents function to call
std::fs::symlink_metadata(&src_path) for each entry, skip the entry if
metadata.file_type().is_symlink(), and otherwise use
metadata.file_type().is_dir() to decide between creating a directory and
recursing versus copying a regular file; ensure errors are propagated the same
way (map_err to string) when metadata calls fail.
- Around line 189-193: The current env-key validation using k.bytes().all(|b|
b.is_ascii_alphanumeric() || b == b'_') || k.is_empty() allows invalid POSIX
names like "1FOO"; change the check so keys must match [A-Za-z_][A-Za-z0-9_]*:
ensure k is non-empty, the first byte/char is ASCII alphabetic or '_' and all
remaining bytes/chars are ASCII alphanumeric or '_', and skip (continue) if this
fails before pushing parts with parts.push(format!("export {}='{}'", k,
shell_escape(v))).
In `@crates/iii-worker/src/cli/worker_manager/libkrun.rs`:
- Around line 125-130: The PID file write currently ignores errors (the let _ =
std::fs::write(&pid_file, pid.to_string()) line), so a failed write leaves a
running VM that the manager cannot find; update the block in run_dev (where
pid_file, pid = child.id(), and std::fs::write are used) to check the Result
from std::fs::write and propagate or return an Err on failure (or panic) so
startup fails instead of succeeding silently—e.g., replace the ignored write
with proper error handling that returns a failure from run_dev (including a
clear error message that mentions pid_file and the underlying io::Error).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41ea2d6a-db10-4ef0-9fa1-21f36e9b7f5a
📒 Files selected for processing (2)
crates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/worker_manager/libkrun.rs
Move the inline TcpStream port-probe from handle_worker_list() into a reusable public function in config_file.rs so future add handlers can share it without duplicating the probe logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/iii-worker/src/cli/local_worker.rs (2)
188-204:⚠️ Potential issue | 🟡 MinorEnv keys starting with digits produce invalid shell syntax.
The validation at line 194 accepts names like
1FOO, butexport 1FOO='x'is invalid POSIX shell syntax. Shell variable names must match[A-Za-z_][A-Za-z0-9_]*.Suggested fix: Validate first character separately
- if !k.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'_') || k.is_empty() { + let mut bytes = k.bytes(); + let Some(first) = bytes.next() else { + continue; + }; + if !(first.is_ascii_alphabetic() || first == b'_') + || !bytes.all(|b| b.is_ascii_alphanumeric() || b == b'_') + { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 188 - 204, The env key validation in build_env_exports currently allows keys starting with digits (e.g., "1FOO") which produce invalid shell exports; update the check in build_env_exports to enforce POSIX variable names by first ensuring the key is non-empty and its first character is an ASCII letter or underscore (A-Za-z_) and then verifying the remaining characters are ASCII alphanumeric or underscore; keep the existing exclusions for "III_ENGINE_URL" and "III_URL" and continue to use shell_escape for values.
166-186:⚠️ Potential issue | 🟠 MajorSetup/install failures still mark workspace as prepared.
The script commands are joined with newlines (line 185:
parts.join("\n")), so a failedsetup_cmdorinstall_cmdstill executes subsequent commands includingtouch /var/.iii-prepared. After one transient failure, later starts will permanently skip setup/install and run with a broken environment.Suggested fix: Add `set -e` to fail fast on errors
pub fn build_libkrun_local_script(project: &ProjectInfo, prepared: bool) -> String { let env_exports = build_env_exports(&project.env); let mut parts: Vec<String> = Vec::new(); + parts.push("set -e".to_string()); parts.push("export PATH=/usr/local/bin:/usr/bin:/bin:$PATH".to_string()); parts.push("export LANG=${LANG:-C.UTF-8}".to_string());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 166 - 186, The generated script from build_libkrun_local_script currently concatenates commands so a failing setup_cmd or install_cmd still allows the final "touch /var/.iii-prepared" to run; modify build_libkrun_local_script to prepend a fail-fast shell option (e.g., add "set -e" or "set -euo pipefail") to the script (before env_exports and before the conditional prepared block) so any error in project.setup_cmd or project.install_cmd aborts the script and prevents creating /var/.iii-prepared; ensure the fail-fast option is included in the parts vector alongside the existing PATH/LANG exports and that the final joined string still uses parts.join("\n").
🧹 Nitpick comments (2)
crates/iii-worker/src/cli/local_worker.rs (2)
513-523: Redundantprepare_rootfscall.
prepare_rootfsis called twice: first at line 450 (for cloning the base rootfs) and again here at line 513 (to read OCI env). The second call should be redundant sinceprepare_rootfsreturns the same cached path. Consider reusingbase_rootfsfrom line 450 if it's still in scope, or restructuring to avoid the duplicate call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 513 - 523, The code calls worker_manager::oci::prepare_rootfs(language) twice causing redundant work; reuse the previously obtained base_rootfs instead of calling prepare_rootfs again and pass that path into worker_manager::oci::read_oci_env(&base_rootfs) (or keep the earlier variable in scope) so you only call prepare_rootfs once; update the block that populates env (env.entry...) to iterate the map returned by read_oci_env(base_rootfs) using the existing base_rootfs variable and remove the second prepare_rootfs call.
21-47:detect_lan_ipis macOS-specific.The
route -n get defaultandifconfigcommands are macOS-specific. On Linux, these would needip routeandip addrinstead. Currently this is low-impact sinceengine_url_for_runtimeignores thelan_ipparameter anyway, but worth noting for future cross-platform support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/local_worker.rs` around lines 21 - 47, detect_lan_ip is macOS-specific (uses "route -n get default" and "ifconfig"); replace it with OS-aware logic so Linux and macOS are supported: inside detect_lan_ip detect target OS (e.g., cfg!(target_os = "macos") vs cfg!(target_os = "linux")) and run macOS commands as currently implemented for macOS, but for Linux use "ip route" or "ip route get 8.8.8.8" to find the interface/default route and "ip -o -4 addr show dev <iface>" (or "ip addr show <iface>") to extract the IPv4 address; ensure all external command outputs are parsed robustly and keep returning Option<String> (None on failure), and note that engine_url_for_runtime currently ignores lan_ip so preserve behavior if detection fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 824-840: The managed_dir removal is currently gated by freed == 0
which prevents deleting the local-path sandbox when other artifacts were
removed; keep the dir_size counting guard but always attempt removal. Inside the
managed_dir.is_dir() block, call dir_size(&managed_dir) and add to freed only if
freed == 0 (as now) but move the std::fs::remove_dir_all(&managed_dir) call out
of that freed==0 branch so the directory is removed regardless; preserve the
same std::fs::remove_dir_all error handling (eprintln with "warning:".yellow(),
managed_dir.display(), and the error) so failures are still logged.
---
Duplicate comments:
In `@crates/iii-worker/src/cli/local_worker.rs`:
- Around line 188-204: The env key validation in build_env_exports currently
allows keys starting with digits (e.g., "1FOO") which produce invalid shell
exports; update the check in build_env_exports to enforce POSIX variable names
by first ensuring the key is non-empty and its first character is an ASCII
letter or underscore (A-Za-z_) and then verifying the remaining characters are
ASCII alphanumeric or underscore; keep the existing exclusions for
"III_ENGINE_URL" and "III_URL" and continue to use shell_escape for values.
- Around line 166-186: The generated script from build_libkrun_local_script
currently concatenates commands so a failing setup_cmd or install_cmd still
allows the final "touch /var/.iii-prepared" to run; modify
build_libkrun_local_script to prepend a fail-fast shell option (e.g., add "set
-e" or "set -euo pipefail") to the script (before env_exports and before the
conditional prepared block) so any error in project.setup_cmd or
project.install_cmd aborts the script and prevents creating /var/.iii-prepared;
ensure the fail-fast option is included in the parts vector alongside the
existing PATH/LANG exports and that the final joined string still uses
parts.join("\n").
---
Nitpick comments:
In `@crates/iii-worker/src/cli/local_worker.rs`:
- Around line 513-523: The code calls
worker_manager::oci::prepare_rootfs(language) twice causing redundant work;
reuse the previously obtained base_rootfs instead of calling prepare_rootfs
again and pass that path into worker_manager::oci::read_oci_env(&base_rootfs)
(or keep the earlier variable in scope) so you only call prepare_rootfs once;
update the block that populates env (env.entry...) to iterate the map returned
by read_oci_env(base_rootfs) using the existing base_rootfs variable and remove
the second prepare_rootfs call.
- Around line 21-47: detect_lan_ip is macOS-specific (uses "route -n get
default" and "ifconfig"); replace it with OS-aware logic so Linux and macOS are
supported: inside detect_lan_ip detect target OS (e.g., cfg!(target_os =
"macos") vs cfg!(target_os = "linux")) and run macOS commands as currently
implemented for macOS, but for Linux use "ip route" or "ip route get 8.8.8.8" to
find the interface/default route and "ip -o -4 addr show dev <iface>" (or "ip
addr show <iface>") to extract the IPv4 address; ensure all external command
outputs are parsed robustly and keep returning Option<String> (None on failure),
and note that engine_url_for_runtime currently ignores lan_ip so preserve
behavior if detection fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e4300ea6-4395-4c6c-8020-5316e526d28c
📒 Files selected for processing (3)
crates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/managed.rscrates/iii-worker/src/cli/worker_manager/libkrun.rs
… running Updated the auto-start functionality for local, binary, and managed workers to include a check for whether the worker is already running before attempting to start it. This prevents unnecessary start attempts and provides clearer feedback to the user.
sergiofilhowz
approved these changes
Apr 9, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/iii-worker/src/cli/managed.rs (2)
727-762: Short delay between SIGTERM and SIGKILL may not allow graceful shutdown.The 500ms delay (line 750) between SIGTERM and SIGKILL is quite aggressive. Workers performing cleanup (e.g., flushing logs, closing connections) may need more time. Consider increasing to 2-3 seconds for consistency with
handle_managed_stopwhich uses 3 seconds for binary workers (line 957).Suggested fix
let _ = kill(p, Signal::SIGTERM); // Brief wait then force-kill - tokio::time::sleep(std::time::Duration::from_millis(500)).await; + tokio::time::sleep(std::time::Duration::from_secs(2)).await; let _ = kill(p, Signal::SIGKILL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/managed.rs` around lines 727 - 762, The SIGTERM→SIGKILL wait in kill_stale_worker is too short (currently 500ms); update the delay in the function kill_stale_worker to wait ~3 seconds (or 2–3s) after sending SIGTERM before sending SIGKILL so managed workers have time for graceful shutdown, matching the timeout used by handle_managed_stop for binary workers.
1858-1872: Test writes to real user home directory, which may conflict with actual data.The test
kill_stale_worker_handles_invalid_pid_contentcreates a PID file at~/.iii/pids/__iii_test_garbage_pid__.pid(line 1866), which is the real user home directory. While the test name is unlikely to collide, this could leave test artifacts behind if the test fails before cleanup.Consider using a similar pattern to
kill_stale_worker_removes_pid_files(lines 1815-1850) which usestempfile::tempdir()and tests the logic directly without touching real paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/managed.rs` around lines 1858 - 1872, The test currently writes a PID file into the real user home (using dirs::home_dir())—instead create a temporary home with tempfile::tempdir(), set the HOME environment variable to that temp dir for the duration of the test, create the .iii/pids directory and the __iii_test_garbage_pid__.pid file there, call kill_stale_worker("__iii_test_garbage_pid__").await, then restore the original HOME (or unset it) so no real user files are touched; update the test function kill_stale_worker_handles_invalid_pid_content to follow the same tempfile pattern used in kill_stale_worker_removes_pid_files and ensure the tempdir is dropped at test end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 727-762: The SIGTERM→SIGKILL wait in kill_stale_worker is too
short (currently 500ms); update the delay in the function kill_stale_worker to
wait ~3 seconds (or 2–3s) after sending SIGTERM before sending SIGKILL so
managed workers have time for graceful shutdown, matching the timeout used by
handle_managed_stop for binary workers.
- Around line 1858-1872: The test currently writes a PID file into the real user
home (using dirs::home_dir())—instead create a temporary home with
tempfile::tempdir(), set the HOME environment variable to that temp dir for the
duration of the test, create the .iii/pids directory and the
__iii_test_garbage_pid__.pid file there, call
kill_stale_worker("__iii_test_garbage_pid__").await, then restore the original
HOME (or unset it) so no real user files are touched; update the test function
kill_stale_worker_handles_invalid_pid_content to follow the same tempfile
pattern used in kill_stale_worker_removes_pid_files and ensure the tempdir is
dropped at test end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebb982be-d135-4cb5-8c3b-dfe80805df54
📒 Files selected for processing (2)
crates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/managed.rs
✅ Files skipped from review due to trivial changes (1)
- crates/iii-worker/src/cli/local_worker.rs
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
iii worker devsubcommand with integrated local-path worker support viaiii worker add ./pathlocal_workermodule with workspace management (lazy setup/install on first start)ResolvedWorkerTypeenum to centralize worker type resolution across start, stop, and listworker_path:field in config.yaml for local workersdev.rs,Commands::Dev, and associated integration testsTest plan
iii worker add ./local-pathadds a local worker to configiii worker start <name>starts local workers with lazy setupiii worker stop <name>stops local workers cleanlyiii worker remove <name>removes local workersiii worker listshows TYPE column distinguishing local vs binary workerscrates/iii-worker/tests/Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests