# gpur code review
Full-codebase pass over `src/` (10 modules, 8 backends) and `tests/`, at
v0.10.1. Findings are grouped by class and ordered by severity within each
group. Items already tracked in `backlog.md` are cross-referenced rather than
restated; this file covers what that document does not.
Baseline: `cargo clippy --all-targets -- -D warnings` is clean, and the suite
passes. Nothing here is a crash or a memory-safety problem.
## Status
Everything below has been fixed except **C4**, which is left open because it
changes the `--json` / `--log` record shape (`0` → `null`) and that is a call
for the maintainer, not a review finding to apply unilaterally. **D4** was
judged not worth the extraction and is recorded as considered-and-declined.
The fixes landed as `c5bf0aa`, `3581995`, `12ca248`, `fba30d1`, `a48f14f`,
`c594203`, `c080385`, `37fe867`, `0b0cb28` and `79540ed`; the suite went from
170 to 178 tests, every one of them pinning a finding above. Two fixes are worth
reading in the commit rather than here, because the final shape differs from
what this document proposed:
- **C3** is fixed structurally rather than with a guard. A new
`backend::BackendSource` carries the whole of how a session's backend is
produced (`Live` / `Mock(n)` / `Replay(path)`), and startup and the failure
re-detect run the same call — so a re-detect can only ever reproduce the kind
of backend the session began with, and there is no rule left for a future edit
to forget. A first attempt gated the re-detect on `can_signal()`, which was
also correct but cost the `GPUR_MOCK_FAIL` PTY test its only route into the
re-detect branch.
- **C5**'s eviction is a second no-field refresh over exactly the departed pids,
since sysinfo exposes no way to drop a map entry and an `All` refresh is what
this code exists to avoid. One residual is documented at the site: a pid that
left the GPU but is still alive stays cached until it exits, because nothing
short of `All` can evict a live process. That set is bounded by the machine's
process table rather than by session length, which was the actual finding.
---
## Correctness
### C1. Intel reports a confident `0%` for a GPU it cannot see into
**Severity: medium. Fixed in `c5bf0aa`.** `src/backend/intel.rs:119`, `:147`.
```rust
utilization_pct: Some(clamp_pct(sweep.util.remove(&i).unwrap_or(0.0))),
...
gtt_used_bytes: Some(system_mem.remove(&i).unwrap_or(0)),
```
Intel has no device-level busy counter, so both figures are summed over the DRM
clients the `/proc` sweep could read. `linux::drm_clients` returns an empty vec
whenever `/proc/<pid>/fd` is unreadable — which is every process belonging to
another user, unless gpur runs as root:
```rust
let Ok(entries) = fs::read_dir(&fd_dir) else {
return Vec::new(); // other users' processes without privileges
};
```
So an unprivileged gpur on a shared box renders another user's fully-loaded iGPU
as a measured, meter-drawn `0%` with an empty GTT pool. That is precisely the
fabrication `GpuSnapshot`'s doc comment, `vram_value`, `session_line` and
`draw_meter` all exist to prevent — the one backend that derives its headline
number from a privileged source is the one that hard-codes `Some(0.0)`.
The process pane already tells the truth here
(`"no GPU processes visible (need same-user or root for fdinfo)"`); the device
gauge above it does not.
**Fix:** distinguish "no client contributed" from "clients contributed zero".
`Sweep::seen` already carries per-client evidence — key the fallback off whether
any client was attributed to device `i` at all, and emit `None` when none was.
An idle device with visible clients keeps its honest `Some(0.0)`.
### C2. The kill dialog can send SIGKILL while reporting SIGTERM
**Severity: low (Windows-only today). Fixed in `fba30d1`.** `src/app.rs:972`.
```rust
let ok = p.kill_with(sig).unwrap_or_else(|| p.kill());
```
`kill_with` returns `None` when the signal is unsupported on the platform —
which is exactly the `Signal::Term`-on-Windows case the comment names. The
fallback `p.kill()` sends `Signal::Kill`. The confirmation dialog said
`send SIGTERM to 1234?`, the status line then says `sent SIGTERM to 1234`, and
the process was killed uncatchably.
The user consented to a graceful terminate and got a hard kill, silently. Every
other guard on this path (start-time pinning, pid 1, self, exe check,
`can_signal`) is careful; this one is not.
**Fix:** report what was actually sent, or refuse the fallback for the non-force
path and tell the user to use `X`.
### C3. Re-detect discards the backend's provenance
**Severity: low (currently unreachable). Fixed in `a48f14f` — see Status.**
`src/app.rs:632`.
```rust
if self.poll_failures.is_multiple_of(5)
&& let Ok(fresh) = crate::backend::detect(self.mock, None)
```
`App` keeps `mock` for re-detection but not the replay path. A replay session
that hit five consecutive poll failures would silently swap itself for the live
hardware backend — which also flips `can_signal()` from false to true, undoing
the guarantee `ReplayBackend::can_signal` exists to make.
It cannot happen today only because `ReplayBackend::poll` never returns `Err`
(EOF holds the final frame, by design, and `replay.rs`'s module doc says so).
That is an implicit invariant in one file protecting a security property in
another.
**Fix:** carry the replay path alongside `mock`, or skip re-detect entirely when
`!self.backend.can_signal()` — a fabricated backend has no driver to reload.
### C4. Unknown per-process figures are indistinguishable from zero
**Severity: low. OPEN — needs a call on breaking the record shape.**
`src/backend/mod.rs:126`, `src/backend/nvidia.rs:376`, `src/app.rs:770-771`.
`GpuProcess::gpu_mem_bytes` is a bare `u64`, so NVML's
`UsedGpuMemory::Unavailable` (MIG devices, restricted-permission queries, some
vGPU profiles) becomes `0` and prints as `0MiB`. The same pattern repeats in
`ProcRow`: `cpu_pct` and `host_mem_bytes` are `.unwrap_or(0)` when sysinfo could
not resolve the pid, so an unreadable process reads as idle and memory-free.
`gpu_util_pct` got this right — it is `Option`, and `rebuild_proc_view` sinks
unmeasured rows in both sort directions. The other three columns did not.
**Fix:** widen the three fields to `Option` and render `-`/`N/A` as the GPU%
column already does. Note this changes the `--json`/`--log` record shape (`0` →
`null`), so it wants a version bump and a changelog line — flagged for a
decision rather than applied.
### C5. sysinfo's process cache grows for the life of the session
**Severity: low. Fixed in `79540ed` — see Status.** `src/app.rs:744`.
```rust
self.sys.refresh_processes_specifics(ProcessesToUpdate::Some(&pids), true, ...)
```
With `ProcessesToUpdate::Some`, sysinfo 0.39 only evicts dead processes _within
the supplied list_ (`common/system.rs:385`). A pid that stops using the GPU is
never in a later list, so its `Process` entry is retained forever. A node
churning through short GPU jobs accumulates one struct per pid ever seen.
`confirm_kill` deliberately depends on this non-eviction (its comment says so),
so the fix must keep the single-pid refresh it performs before signalling.
**Fix:** periodically `retain` the cache against the live pid set, or accept it
and document the ceiling. `MAX_ABSENT_DEVICES` bounds the equivalent per-device
maps already; this is the one unbounded map left.
### C6. CPU% is sampled below sysinfo's documented minimum interval
**Severity: low. Fixed in `79540ed`.** `src/app.rs:14` (`MIN_TICK_MS = 50`),
`:744`.
`cpu_usage()` is only meaningful when at least `MINIMUM_CPU_UPDATE_INTERVAL`
(200 ms) elapsed between refreshes of that process. At `--tick-ms 50` — a
supported and deliberately-preserved setting, per `backlog.md` — and at the 100
ms the PTY suite uses, the CPU% column is noise rather than a measurement.
**Fix:** rate-limit the CPU half of the refresh to ≥200 ms and hold the previous
value between, or drop the column below that tick rate.
### C7. Two `u16` sums in `draw_gpus` can overflow
**Severity: very low. Fixed in `3581995`.** `src/ui.rs:185`, `:222`.
```rust
if used + h > area.height { break; }
```
`needed` overflows above ~8191 unfolded cards; `used + h` overflows when
`area.height` is within 8 of `u16::MAX`. Both are absurd inputs, but
`proc_pane_height` in the same file already computes in `u32` for exactly this
reason and carries a regression test (`proc_pane_height_never_overflows`) naming
programmatic PTY resize as the trigger. The treatment is inconsistent.
**Fix:** accumulate in `u32`, or use `saturating_add`.
### C8. Replay trusts `gpu_index` from the record
**Severity: very low. Fixed in `c594203`.** `src/backend/replay.rs:82`.
`processes()` hands back recorded rows verbatim. A recording whose `gpu_index`
exceeds the frame's GPU count — a truncated log, a hand-edited or third-party
JSONL — renders a `DEV 7` row against a two-card frame.
`CompositeBackend::processes` drops exactly this case for live children, but
replay is never composed, so nothing filters it.
**Fix:** drop rows whose `gpu_index` is outside the current frame, matching the
composite's rule.
---
## Security
The kill path was audited specifically and holds up: pid identity is pinned by
`(pid, start_time)` across the confirmation gap, pid 1 and self are refused,
`exe().is_none()` refuses kernel threads and unreadable processes,
`can_signal()` is re-checked inside `confirm_kill` (not only at `request_kill`),
and mock/replay backends refuse outright so a recording from a stranger is not a
one-keystroke signal primitive. `CompositeBackend::can_signal` is the AND of its
children. No findings there.
### S1. `--log` is created with default permissions
**Severity: low. Fixed in `c080385`.** `src/main.rs:70`.
```rust
std::fs::OpenOptions::new().create(true).append(true).open(path)?
```
The log records full command lines, usernames and container ids, one JSON line
per poll. Command lines routinely carry secrets in `argv` (`--api-key=…`,
`--token=…`). With a typical `umask 022` the file lands world-readable, and it
is meant to be left running for hours.
**Fix:** `OpenOptionsExt::mode(0o600)` on Unix. `state.json` (written by
`App::save_state`) deserves the same treatment for consistency, though its
contents are not sensitive.
### S2. Unit tests write to predictable paths in a world-writable directory
**Severity: low (test-only). Fixed in `12ca248`.** `src/backend/linux.rs:555`,
`:707`, `src/backend/amd.rs:533`, `:641`, `src/backend/intel.rs:409`.
```rust
let root = std::env::temp_dir().join(format!("gpur-drm-{name}"));
let _ = fs::remove_dir_all(&root);
```
These paths carry no pid and no counter: `/tmp/gpur-drm-partition`,
`/tmp/gpur-clock-test`, `/tmp/gpur-dpm-test`, `/tmp/gpur-intel-test-*`.
Two consequences. Concurrent `cargo test` runs — two checkouts, two users, a CI
matrix sharing a runner — race on the same directories and fail
nondeterministically. And on a shared host another user can pre-create the path
as a symlink: `remove_dir_all` fails on a symlink and the error is discarded by
`let _`, `create_dir_all` then succeeds through it, and every `fs::write` in the
test lands wherever the link points, as the test user.
`tests/smoke.rs` and `tests/tui.rs` already do this correctly, with
`std::process::id()` plus an `AtomicU32` counter and a `Drop` cleanup. The
in-crate tests should use the same pattern.
### S3. `state.json` is written non-atomically
**Severity: very low. Fixed in `37fe867`.** `src/app.rs:509`.
`fs::write` truncates before writing. A crash, a signal, or a full disk
mid-write leaves a truncated file, `load_state`'s
`serde_json::from_str(...).ok()?` returns `None`, and every persisted preference
is silently lost. The failure mode is benign but avoidable.
**Fix:** write to `state.json.tmp` and `rename` over the target.
---
## DRY
### D1. `fan_pct` is duplicated verbatim
**Fixed in `0b0cb28`.**
`src/backend/amd.rs:462` and `src/backend/nvidia.rs:306` (the nouveau module)
are the same six lines — `pwm1` over `pwm1_max`, defaulting the divisor to
hwmon's 255 and filtering a zero max. Both even carry the same explanatory
comment. `linux.rs` is where every other shared hwmon reader lives (`hwmon_u64`,
`pcie_link`, `driver_line`).
**Fix:** `linux::fan_pct(hwmon: Option<&Path>) -> Option<f64>`.
### D2. The multi-driver `driver_info` join is duplicated
**Fixed in `0b0cb28`.**
`src/backend/amd.rs:158` and `src/backend/intel.rs:160` build the identical
`BTreeSet` → `join("+")` → `linux::driver_line(..)` chain over their device
lists, for the identical reason (a box can run `amdgpu`+`radeon` or
`i915`+`xe`).
**Fix:** one helper taking an iterator of driver names.
### D3. `pci.ids` is read and parsed once per device
**Fixed in `0b0cb28`.**
`src/backend/linux.rs:483` — `card_name` reads the whole file (~1.5 MB on a
current hwdata) and scans it linearly, and it is called once per card inside
each backend's `scan`. A three-AMD-card box does three full reads and three
linear scans at probe; an AMD+Intel+nouveau box does one set per backend.
Probe-time only, so it is cost rather than a bug — but the file contents are
identical across every call in a process.
**Fix:** read the file once per `scan` (or once per process, lazily) and pass
the contents to `pci_device_name`, which already takes `&str`.
### D4. Sub-cell quantization is hand-rolled three times
**Declined.** The three sites quantize against different unit counts and
different clamp floors, so a shared helper would take both as parameters and
save nothing but the arithmetic itself.
`src/ui.rs:657` (`mini_spark`, both branches) and `:845` (`draw_waveform_cells`)
each open-code `value → filled sub-units, clamped` against different unit counts
(4, 8) and different clamp floors. Small enough that extraction may not pay, but
it is the kind of arithmetic that drifts.
---
## YAGNI
`backlog.md` §9 already tracks the three live items (`draw_meter`'s 8 arguments,
`UiTheme::temp_ok`'s asymmetric visibility, the single-variant `keys::Mode`
required by the `hjkl-keymap` API). No new dead code was found: every
`GpuSnapshot` field is rendered, every `Action` variant is bound, every helper
in `linux.rs` has at least two callers, and both `clamp_pct` and `join_throttle`
are used by more than one backend.
One near-miss worth naming:
### Y1. Braille-sized history retention is paid by all three graph styles
**Fixed in `3581995`.**
`src/ui.rs:66` — `app.history_need = area.width as usize * 2` is set
unconditionally, but only braille packs two samples per column; block and ascii
consume one. Under `--graphs ascii` on a 400-column terminal every device
retains twice the samples it can ever draw.
**Fix:** scale by the active `GraphStyle`.
---
## Verified, not findings
Recorded so the next pass does not re-open them.
- **Device identity.** `DeviceKey` / `device_keys` correctly degrade a duplicate
or absent `device_id` to a positional key, positional keys are never
persisted, and `CompositeBackend` namespaces every child's ids by child index.
The vacated-slot de-duplication (`mod.rs:261`) is correct.
- **Process index rebasing.** `CompositeBackend::processes` accumulates by
`slots` (the high-water mark), not by the current poll's device count, and
drops rows outside their child's span. The three-child and middle-child-resize
tests cover the off-by-one variants.
- **Mouse hit-testing.** `proc_visible` is bounded by both the drawn row count
and `procs.len()`, so a click can never select an off-screen row; the
`app.procs[scroll..scroll + visible]` slice is provably in range because
`proc_scroll` is clamped to `total - visible` before it.
- **Numeric guards.** `gradient` handles empty and single-stop ramps,
`parse_size` saturates, `vram_total_mb` saturates, `le_int` rejects
non-scalars, `windowed` handles short data, and `proc_pane_height` computes in
`u32`. All carry tests.
- **Modal input.** Control chords cannot reach the filter buffer, mouse events
are ignored while a modal is up, and `Ctrl-C` in filter mode still quits.
- **PDH buffer handling** (`windows.rs:403`) allocates with the item type's
alignment and sizes from the `PDH_MORE_DATA` byte count, which is the correct
contract; the `Vec` outlives the raw pointer derived from it. </content>
</invoke>