Add fabro-static::EnvVars as the shared registry for fixed environment variable names and migrate env reads, clap env bindings, and subprocess/test allowlists to use it.
Add clippy bans for raw std::env lookup APIs so future dynamic env facades must be documented explicitly.
Workspace clippy.toml bans std::fs::read_dir / read_to_string without
an explicit expect annotation. The new Linux zombie-group probe uses
both and only compiles on Linux, so the lint wasn't hit locally on
macOS. Annotate the helper with the reason it needs sync I/O.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Linux's kill(-pgid, 0) succeeds even when every group member is a zombie
waiting to be reaped; macOS returns ESRCH in the same situation. Callers
polling on process_group_alive (fabro-server's SIGTERM grace loop, plus
the zombie-only regression test in fabro-proc) therefore saw divergent
behavior: CI on Linux had been failing for days on the asserting test.
After the cheap kill(2) probe, walk /proc and confirm at least one
non-zombie process still reports the given pgid. Non-Linux unix targets
keep the fast path. Falls back to "alive" on /proc read failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `foreground_start_writes_tracing_to_storage_server_log` test
consistently took ~10.4 s. 10.3 s of that was spent inside `fabro
server stop`, which polls `process_running(pid)` every 100 ms until
the server exits. The test's server is spawned as a child of the test
process (`child.spawn()`), and the test only reaps it via
`child.wait_with_output()` after `fabro server stop` returns. After
Step A's revert, `process_running` is a plain `kill(pid, 0)`, which
returns true for a zombie — so the poll saw the dead-but-unreaped
server as alive and burned the full 10 s timeout.
Add `fabro_proc::process_running_strict(pid)` — the same
ps-shelling zombie-aware predicate commit 1ed8e6cbd introduced — and
use it only in `fabro-cli`'s server stop poll. The hot paths that
motivated Step A (test-harness marker scans, daemon-liveness probes)
continue to use the cheap `process_running`.
The ps cost (~2 ms per call) is paid at most once per 100 ms poll
interval and only while the server process still exists. In a normal
clean shutdown that's zero calls (process exits before the first
poll). In the zombie scenario the loop exits after ~1 poll instead
of running out the full timeout.
Verified on this branch:
cargo nextest run -p fabro-cli -E 'test(foreground_start_writes_tracing)'
before: 10.48s, 10.45s, 10.42s
after: 0.35s, 0.32s, 0.25s (30x faster)
The zombie regression test removed in commit da87f978c returns as
`process_running_strict_returns_false_for_unreaped_zombie_child`,
and also asserts that the cheap `process_running` keeps its
"zombie == alive" semantics so the harness hot paths stay honest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace PID-based liveness probing in `live_marker_count` with flock
advisory-lock presence detection. Each test process opens
`<session_root>/clients/<pid>` once, holds LOCK_SH for the lifetime
of any live TestContext in the process, and releases it explicitly
when `cleanup_session_root` fires at refcount zero. Reapers probe with
LOCK_EX | LOCK_NB: success means the previous owner is gone (normal
exit, panic, SIGKILL, or zombie — the kernel releases advisory locks
at process exit in every case) and the stale marker is removed.
Compared to the PID check this was replacing:
- Handles PID recycling correctly (the new holder does not inherit
the previous owner's advisory lock).
- Handles zombies correctly without shelling out to `ps`.
- Costs one open + one flock per peer, ~50 us on macOS.
The marker handle is stored in a process-scoped
`Mutex<Option<(PathBuf, File)>>` so it can be released and
reacquired across the drop-to-zero / rise-from-zero cycles that
`session_refs` already implements. Storing the path alongside the
handle enables a debug assertion that the process never drifts
between session roots.
`ClientMarker` and its serde plumbing are removed; the marker file is
now empty, its existence and lock state carrying the signal.
Full workspace wall-clock after A+B+C: 13.3–13.6 s, down from 20–25 s
on HEAD before the fix and comparable to the 14 s Friday baseline
despite the intervening +85 tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 1ed8e6cbd changed process_running(pid) to shell out to `ps` on
every call to distinguish running processes from zombies. That cost
~2 ms per invocation on macOS (fork + exec + wait), and the test
harness calls process_running O(tests × markers) times under session
flock contention. Across a `cargo nextest run -p fabro-cli` that added
up to ~90 s of suite time, and the zombie-aware semantics turned out
to have no production caller on Unix (the server's worker-termination
loop uses process_group_alive; the CLI stop/status paths don't need
zombie detection for a daemon that reparents to init).
Restore the pre-1ed8e6cbd body: process_running is now a straight
kill(pid, 0) via process_exists on Unix, true on non-unix. Delete
unix_process_state (the `ps` helper) and its zombie regression test,
since they describe behavior we're rolling back. process_group_alive
and its tests are unchanged.
Measured on this branch against baseline db953c838:
reap_nextest p50: 172 ms -> 0.3 ms
TestContext:🆕 316 ms mean -> 15 ms mean
If a future caller genuinely needs zombie-aware semantics, add it back
alongside that caller with a benchmark in context.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds expect(disallowed_types) at the tests module for the intentional
sync BufReader usage in the zombie-process-group helper, and drops the
absolute-path call site by bringing pre_exec_setpgid into scope.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Split raw PID existence from actual process liveness in fabro-proc and
switch the server shutdown paths to the running-process predicate. This
avoids waiting out stop timeouts for unreaped zombie children while
keeping process-group behavior covered by measured regression tests.
Enable clippy::allow_attributes_without_reason at the workspace level.
Add concise, callsite-specific reasons to existing allow attributes, including generated code paths.
Phase 2/3 of the std::fs lint initiative (Phase 1 refactors landed in
commit 9d1c0d98c).
clippy.toml additions (appended to disallowed-methods):
std::fs::read, read_to_string, write, read_dir, copy, canonicalize
std::fs::File::open, File::create, File::create_new
std::fs::OpenOptions::open
File::options was deliberately excluded — it returns an OpenOptions
builder with no syscall. OpenOptions::open is where the block happens.
Non-blocking std::fs items (metadata, exists, create_dir_all, remove_*,
rename, and all std::fs types) remain legal.
Annotation policy (per updated plan):
- Mixed async/sync production source: function- or statement-scoped
#[expect(...)] so future accidental Tokio-path regressions in the
same file still fire.
- Fully-sync production source, test modules, integration tests,
build.rs: file-level #![expect(...)].
- Every #[expect] has a specific reason identifying the sync context.
Annotations added in ~90 files across the workspace. Notable narrow
placements: fabro-server server.rs current_server_target,
build_disk_usage_response, create_test_app_state_with_session_key;
fabro-server install.rs read_to_string rollback snapshot;
fabro-sandbox local.rs list_recursive; fabro-agent cli.rs FOLLOW-UP on
the JSON-stdout writer; fabro-llm providers/common.rs FOLLOW-UP for
load_file_as_base64 (7 translator call sites; revisit if file:// URL
usage grows).
build.rs blanket allows: fabro-api/build.rs, fabro-util/build.rs.
Pre-existing unrelated nightly-clippy warnings fixed under scope:
fabro-sandbox sandbox_spec.rs (unused_imports, unused_async),
reconnect.rs (unused_variables, unused_async).
Verified: cargo +nightly-2026-04-14 clippy --workspace --all-targets
-- -D warnings passes; fmt clean; 4129/4131 tests pass (two known
flakes under parallel nextest load, both pass individually and are
unrelated to this change).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No production deployments exist, so there's no need for migration shims.
Remove all six backwards-compat type aliases (AgentError, SdkError,
CoreError, GraphvizError, StoreError, FabroError) and migrate ~880
callsites to use the canonical Error name directly within each crate,
or qualified imports (e.g., `use fabro_llm::Error as LlmError`) for
cross-crate references. Also fix a pre-existing absolute-path clippy
lint in fabro-server error.rs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move subprocess workers fully behind the server-owned run store by
switching worker/server coordination to HTTP-backed run events and
control state. Reconcile stale in-flight runs on boot, terminate live
workers during shutdown, and update process titles to reflect server and
worker lifecycle phases.
Rename fabro-proctitle to fabro-proc and add safe wrappers for all
process management primitives (signals, pre-exec hooks). This contains
all unsafe proc code behind a safe API so downstream crates no longer
need #[allow(unsafe_code)] or direct libc dependencies.
New modules: signal (process_alive, sigterm, sigkill, sigterm_process_group),
pre_exec (pre_exec_setsid, pre_exec_setpgid, pre_exec_pdeathsig),
title (existing proctitle code). Eliminates three duplicate process_alive
definitions and removes libc as a direct dep of fabro-cli and fabro-sandbox.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
