Conversation
…voking `agentflare work` Flattens the daemon's 3-layer dispatch (daemon -> agentflare-jobs::Supervisor -wrapped `agentflare work` OS subprocess -> agent CLI subprocess) to 2 layers: the daemon now runs a work item's claim/resolve-agent/build-prompt/done logic in-process, spawning a real subprocess only for the one genuinely open-ended part -- the agent CLI itself (still via agent_launch::run_captured, with its own hard-cap/idle-timeout kill from item #20). Extracted `run_work`'s body into `execute_work` (src/cli/work.rs), which now takes a `&mut dyn Write` for progress output instead of println!, so the same logic works identically for a human running `agentflare work` and for `WorkItemExecutor` running in-process. Progress lands in that job's own `{id}.stdout` log file -- the same path/name Supervisor already used for subprocess jobs -- so the dashboard's existing live log tail (`/api/jobs/:id /stream`) keeps working unchanged. agentflare-jobs gained an `InProcessExecutor` trait (defined there so the crate never depends upward on the binary that implements it) and an `AgentJob::in_process` flag; `WorkerPool` branches on it per job, keeping the existing subprocess path (used by ad-hoc `POST /api/jobs` submissions) fully intact and unchanged. An in-process job's executor call runs on a short-lived thread with `job.timeout_secs` as a watchdog, so a stuck claim/worktree/done step -- which loses the OS-level SIGKILL backstop a subprocess had -- can't wedge a pooled worker thread forever; the stuck thread itself is abandoned (no safe way to force-kill a thread in Rust) rather than truly killed, a real, deliberate trade-off called out where it's implemented. Two correctness issues surfaced by moving multiple work items into worker threads sharing one process, both fixed narrowly rather than serializing dispatch back down to one job at a time: - claim/comment/done authorship (`claims::owner_id()`) read the process-global `AGENTFLARE_AGENT` env var, which two worker threads dispatching different agents would race on. Added a thread-local override (`claims::with_owner_override`) that `owner_id()` checks first -- each worker thread's identity stays independent, no shared mutable state. `cli/handoff.rs`'s sender resolution duplicated the same env read separately; unified it through `claims::owner_id()` so it picks up the override too instead of needing its own fix. - the agent CLI subprocess itself relied on inheriting `AGENTFLARE_AGENT` from its parent's ambient env for `flare-git-shim`'s bypass classification when the agent shells out to git -- correct for a subprocess-per-dispatch caller, wrong for a daemon whose own ambient env can't be right for every concurrently-running job. `agent_launch::run_headless` now sets it explicitly on the spawned Command instead of relying on inheritance. `dispatch_item` (src/supervisor.rs) now enqueues an in-process job with `[item_id, agent]` args instead of `agentflare work <item> --agent <agent>`; the outer job timeout (aligned to work's hard cap by the previous commit) still applies, now as the in-process watchdog rather than a subprocess kill.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
Comment |
…rap() Unrelated to this branch's actual change -- fixing it here because it's been intermittently failing build (windows-latest), a required status check, blocking this PR (and #402, based on it) from reaching green. Confirmed pre-existing on master's own HEAD, not something either PR introduced: a different one of these tests failed on each of three consecutive Windows CI reruns, always at the same remove_dir_all(&dir) cleanup line, after the test's real assertions had already passed. Classic Windows CI flake: a file just written/renamed can stay transiently locked for a few ms (real-time AV scanning) after the code under test is done with it, so an immediate remove_dir_all can hit PermissionDenied through no fault of the test's own logic. temp_dir_for_test's own pre-cleanup already treats this as best-effort (`let _ = ...`); these end-of-test cleanups asserted success instead, so the flake surfaced as a spurious test failure with nothing left to actually assert by that point in the test.
# Conflicts: # src/supervisor.rs
1 participant
Summary
Depends on #401 — targeted at that branch so this diff only shows item #19's incremental work; retarget to
masteronce #401 merges.agentflare-jobs::Supervisor-wrappedagentflare workOS subprocess → agent CLI subprocess) to 2: the daemon now runs claim/resolve-agent/build-prompt/done logic in-process, spawning a real subprocess only for the one genuinely open-ended part — the agent CLI itself (unchanged: stillagent_launch::run_captured, with feat(work): idle-timeout instead of fixed wall-clock timeout for dispatched work #401's hard-cap/idle-timeout kill).run_work's body intoexecute_work(src/cli/work.rs), now taking a&mut dyn Writeinstead ofprintln!, so the identical logic runs for a human'sagentflare workinvocation and for the daemon's in-processWorkItemExecutor. Progress lands in that job's{id}.stdoutlog file — same pathSupervisoralready used — so the dashboard's live log tail keeps working unchanged.agentflare-jobsgained anInProcessExecutortrait (defined there so the crate never depends upward on the binary implementing it) andAgentJob::in_process;WorkerPoolbranches per job, and the existing subprocess path (used by ad-hocPOST /api/jobssubmissions) is fully unchanged. An in-process job's executor call runs on a short-lived thread withjob.timeout_secsas a watchdog, so a stuck claim/worktree/done step — which loses the OS-level SIGKILL backstop a subprocess had — can't wedge a pooled worker thread forever (the stuck thread itself leaks rather than being force-killed; there's no safe way to kill a thread in Rust — a real, deliberate, documented trade-off).Correctness issues found and fixed while moving multiple work items into worker threads sharing one process
Rather than serializing dispatch back down to one job at a time to sidestep these:
AGENTFLARE_AGENT(a process-global env var two worker threads dispatching different agents would clobber). Addedclaims::with_owner_override— a thread-localowner_id()checks first, so each worker thread's identity stays independent.cli/handoff.rs's sender resolution duplicated the same env read separately; unified it throughclaims::owner_id()so it picks up the override for free.AGENTFLARE_AGENTfrom ambient parent env forflare-git-shim's bypass classification when the agent shells out to git — correct for a subprocess-per-dispatch caller, wrong once the daemon's own ambient env can't be right for every concurrently-running job.agent_launch::run_headlessnow sets it explicitly on the spawnedCommand.Test plan
cargo build --workspace— cleancargo test -p agentflare -p agentflare-jobs --lib --bins— 1136+1 passedcargo test -p agentflare-jobs --tests— all existing subprocess-path tests (queue/supervisor/worker) unchanged and passing, proving that path is untouchedcrates/agentflare-jobs/tests/in_process_test.rs(4 tests): executor dispatch + log capture, failure-message propagation, fail-fast with no executor registered, stuck-job watchdogclaims::tests(4 tests) proving the owner-override: correct value, scope doesn't leak,has_owner_overridereflects state, and 8 concurrent threads never observe each other's overridecargo test -p agentflare --bin agentflare -- handoff:: agent_launch:: cli::work:: supervisor::— 53 passedcargo clippy -p agentflare -p agentflare-jobs --all-targets— no new warnings