fix: watchdog supervision, bounded index growth, and full-filter aggregation - #591
Closed
githubrobbi wants to merge 16 commits into
Closed
fix: watchdog supervision, bounded index growth, and full-filter aggregation#591githubrobbi wants to merge 16 commits into
githubrobbi wants to merge 16 commits into
Conversation
Four separate causes, all of which made a column ragged:
1. `statusfmt::field` printed the value verbatim, but several producers
right-align internally — `format_duration` emits `{minutes:>3} m`,
i.e. ` 12 m 35 s`. That leading pad pushed those rows one or two
columns right of their plain-text neighbours:
Version: 0.6.31
Uptime: 12 m 35 s <- shifted
The value is now trimmed, so every value in a block starts in one
column. Internal spacing is untouched, so the durations keep their
own digit alignment.
2. The per-drive memory line printed heap MB unpadded, so `2 MB` and
`1669 MB` started at different columns and the sizes could not be
compared down the list. Now `{:>6}`.
3. Physical-drive rows pad every numeric but not the drive letter, and
the boot marker makes `C:*` one column wider than `D:` — shifting
every field on that one row. The letter is now padded to 3, applied
to the RAW string before colouring: a width specifier on an
already-coloured string counts the ANSI escapes and silently breaks
the alignment it was meant to fix.
4. `status_drives`' RESIDENT column left-aligned whole cells, so
`2 MiB` and `1.07 GiB` shared a start column. It is now a fixed
10-wide cell — a 6-wide right-aligned magnitude plus a 3-wide unit —
so rows line up on the decimal point:
1.069 GiB
509 MiB
2 MiB
GiB precision goes to three decimals to fill that column.
Regression test pins the property that matters (values sharing one
column) rather than just the literal strings.
…rvice
`just use-local` failed the whole recipe on
uffs-broker.exe copy failed: The process cannot access the file
because it is being used by another process. (os error 32)
even though 22 of 23 binaries installed fine — and, more to the point,
even though the broker had not changed at all. Its sources are
byte-identical across v0.6.30..v0.6.31; the copy was pure churn against
a LocalSystem service that legitimately holds its own image open.
Two changes:
* Skip binaries byte-identical to what is already installed (length
check first, then contents). The broker is the motivating case, but
every unchanged binary now avoids a needless rewrite.
* When the broker HAS changed and is running, stop the service, copy,
and restart it. The broker already exposes native SCM control for
exactly this — `--stop` waits for STOPPED, `--start` waits for
RUNNING and for the pipe to actually serve — which is the same
quiesce/restore sequence `uffs --update` performs. No sc.exe, no
reboot.
The restart is best-effort and never fails the install: a new binary in
place with the service down is recoverable via `uffs-broker --start`,
and that instruction is printed. Stopping is a no-op when the service
is not installed or not running, so boxes without the broker are
unaffected.
Note the compatibility background that makes skipping safe: the broker
speaks a tiny fixed wire protocol (1-byte request, 9-byte response) and
is deliberately decoupled from the daemon's version, so an unchanged
broker serves a newer daemon. That protocol has no explicit version
handshake yet ("currently implicit, future work"), so the guarantee is
by convention — which is another reason to replace the binary only when
it genuinely differs.
`use-local` deliberately kills the daemon + MCP so their images can be
replaced, but never brought them back — so a routine dev install left
the machine with no daemon at all:
✅ Installed 22 binaries (1 skipped)
error: Recipe `use-local` failed on line 141 with exit code 1
> uffs.exe --daemon status
○ Daemon not running
That directly breaks the promise `uffs --daemon resident` makes: the
user asked for a permanently-resident daemon that even starts at login,
and then a build silently took it away.
The install now notes whether a daemon was serving BEFORE the teardown
and restarts it afterwards with the freshly installed binary, restoring
the invariant "use-local leaves the machine as it found it".
The restart goes through the normal `--daemon start` path, so the
client's auto-spawn merges the resident marker (`resident.args`): a
daemon that was resident comes back resident with `--no-retire`, not as
a plain ephemeral one.
Ordering detail: the restart runs BEFORE the non-zero exit for skipped
binaries. A partially-failed install is precisely the case where being
left daemon-less hurts most, so the recovery must not be skipped by the
early exit.
Both halves stay best-effort and never mask a failure: a restart that
does not take prints the exact command to run by hand, and the probe
failing (no `uffs` on PATH on a first install) reads as "was not
running", so nothing is started that was not there before.
`status_drives` shows a TIER column with four values but the manual never explained them, and the one that actually surprises people was undocumented: every drive reads `warm` forever and `Hot` never appears. That is not a bug — it is by construction. There is exactly one code path that creates a Hot shard (`preload`); a freshly loaded drive starts Warm, and a query that promotes a Parked/Cold drive promotes it back to Warm, never past it. So on a daemon where preload has never run, `UFFS_HOT_TO_WARM_IDLE_SECS` is inert config: nothing is ever Hot to demote, and the effective ladder is Warm -> Parked -> Cold. Also records what preload actually buys, since "Hot" oversells it: for dispatch, Warm and Hot are one set and a Hot drive is not searched faster. The real win is the PrefetchVirtualMemory hint moving first-touch paging off the next query's critical path — the difference between a multi-second first query on a multi-GB index (much of it on HDDs) and a memory-speed one — plus the pin blocking demotion. Finally states the distinction that catches people out: residency and Hot are different promises. `--no-retire` keeps the PROCESS alive while the ladder still parks the drives underneath it, so a resident daemon left idle overnight still pays the page-in unless it was preloaded.
Completes the symmetry started for the daemon. Two problems remained on
the MCP side of `use-local`:
* The teardown only ever `taskkill /IM uffsmcp.exe /F`-ed. A force-kill
by image name never lets the gateway remove its PID file, so the next
`uffs --mcp status` reported
MCP server: not running (stale PID file, PID 64184)
which reads like a crash rather than the install doing it. The
teardown now asks `uffs --mcp stop` first and keeps the force-kill as
the backstop for a wedged process.
* Nothing restarted it. Like the daemon, the gateway is now noted as
running BEFORE the teardown and brought back with the freshly
installed binary afterwards.
Worth recording why the MCP supervisor did not save this: it fronts
STDIO sessions only (they deliberately write no PID file — the stale
one proves this was the HTTP gateway), and in any case `taskkill /F` by
image name kills the supervisor itself. A supervisor can hot-swap its
worker child; it cannot survive its own kill. Real crash protection has
to live outside the process tree the install tears down.
Residency promised a daemon that is always there; the login item delivers that at boot and the auto-spawn marker revives one on the next search, but nothing noticed a service vanishing mid-session while no one was searching. launchd and systemd close that gap on macOS/Linux; the Windows Run key fires once at login and never again. This is the missing supervisor, on every platform. Design decisions worth recording: * SEPARATE BINARY, not a `uffs` subcommand. A supervisor cannot supervise its own death, so it must outlive the teardowns that kill what it watches — `install-bins.rs` force-kills `uffsd`/`uffsmcp` by image name, which is exactly how the MCP stdio supervisor died. A different image name survives that. A long-running `uffs.exe` would also lock the most frequently replaced binary in the tree, reintroducing the `os error 32` the installer just learned to avoid; the watchdog's own code changes rarely, so skip-if-identical means it almost never blocks an install. * NOT ELEVATED, and the broker is deliberately NOT supervised. The broker is a LocalSystem service registered `start= auto`, so the SCM already restarts it at boot, and a non-elevated process cannot StartService it at all. Supervising it here would demand elevation and destroy the zero-UAC property residency exists to protect. The right mechanism there is SCM failure actions at `--install` time. * DELIBERATE STOPS WIN. A clean stop records intent and the watchdog honours it until the next explicit start (launchd's KeepAlive.SuccessfulExit=false semantics). Without it the watchdog would fight the operator every time they stop something on purpose. * Respawns are rate-limited to 3 per 60 s per service, then it gives up loudly — a service that dies instantly on every start is broken in a way respawning cannot fix, and an unbounded retry is a fork bomb that buries the real error. * It never INTRODUCES a service: a gateway the user has never started is not started by the watchdog, only restarted once seen running. The policy is pure and unit-tested (decide/RespawnLedger); process spawning is kept at the edge. `dirs-next` rather than `uffs-client` for the one directory path it needs — a supervisor that only shells out to `uffs` should not pull the whole client in. Still to wire (follow-up): the stop-intent markers are read but not yet written by the `stop`/`start` paths, and nothing launches the watchdog yet — `resident on` should, and `install-bins` should cycle it.
Makes the watchdog crate actually run and actually correct. STOP INTENT (uffs-client::daemon_ctl) — the piece without which the watchdog would fight the operator. `--daemon stop` and `--mcp stop` record intent; `--daemon start` and `--mcp start` clear it. The watchdog already reads these to tell "this crashed, put it back" from "the operator wanted it down" (launchd's KeepAlive.SuccessfulExit=false semantics). Shared helpers live in `daemon_ctl` beside the PID file because both writers (uffs-cli, uffs-mcp) already depend on it. ARMING (`resident on`) — starts the watchdog, and refuses to start a second one if one is already supervising, since two supervisors would double every respawn decision. Windows only, deliberately: launchd (`KeepAlive`) and systemd (`Restart=on-failure`) already supervise the daemon on macOS and Linux, so a second supervisor there would be redundant machinery racing the OS. The Windows Run key fires once at login and never again — exactly the gap this fills. INSTALL COOPERATION (install-bins.rs) — the watchdog is stopped FIRST in the teardown and restarted LAST. Order matters both ways: left running, it would dutifully restart the daemon mid-install (supervisor versus installer), and restarted too early it would race the daemon and MCP restarts happening just above it. Everything stays best-effort: a watchdog that fails to arm leaves residency installed and working, and every failure prints the command to run by hand. Workspace clippy clean; 2430 tests pass.
The `[rec=… names=… tri=… ch=… ext=…]` block was unpadded, so a
one-digit `rec=1` and a three-digit `rec=608` started at the same
column and shoved every later field out of line — the one part of the
row you actually want to compare vertically:
[rec=1 names=0 tri=0 ch=0 ext=0]
[rec=608 names=439 tri=518 ch=55 ext=27]
Each numeric is now width-padded: rec/names/tri hold four digits (a
~10 GB component on a very large drive), ch/ext three.
The source label is padded too, since `(live)` and `(cache)` differ in
width and would otherwise shift the `·` and everything right of it.
The padding goes on the whole `(source)` token rather than the text
inside it — `(live )` reads as a typo.
Everything after `… free` ragged, because the volume label is variable width and nothing reserved a column for it: a short "DATA" and a long "NTFS_16_GB" pushed the `· indexed (…)` note to different places, and a drive with no label at all pulled its note further left still. The label now occupies a fixed 12-column field (plus quotes), and an unlabelled drive holds that column with spaces rather than collapsing it. The record count in the index note is right-aligned to 11, so the counts line up on their commas the way the `── Drives ──` block does. 12 fits every label seen in practice; NTFS permits 32, and a longer one pushes its own row instead of being truncated — losing information to preserve a column would be the wrong trade.
Live-box test: `uffs --daemon stop` reported success, the daemon went away — and roughly ten seconds later the watchdog brought it back. A deliberate stop did not stick, which is the one behaviour that makes a supervisor unusable. Not a wiring mistake: the ordering was wrong. `shutdown()` blocks until the daemon is actually gone, and tearing down a 24.9 M-record index with seven journal loops takes seconds. Writing the marker after the RPC returned left a multi-second window in which the daemon was already dead and the marker did not exist yet. A watchdog tick landing in that window sees an unexplained death and does exactly what it is built to do — respawn. Worse, the respawn runs `uffs --daemon start`, which clears stop intent, so the evidence erased itself. The marker is now written before the RPC. If the shutdown then fails, the intent is cleared again: an intent that was never carried out must not stop the watchdog reviving a later genuine crash. `--daemon kill` gets the same treatment — a kill is as deliberate as a stop, and it was not recording intent at all. The MCP stop path already wrote before signalling; its comment now states why, so the ordering is not "tidied" back later. Found only by running it on real hardware: the unit tests cover the decision (`decide`), and the decision was right — the input was late.
Live test: a deliberate `--daemon stop` still bounced back, and it only stuck on the SECOND stop in a row. That "second one sticks" is the tell — it is the 3-per-60s respawn limiter running out, which means intent was being ignored every time rather than intermittently. Two defects, both visible by inspection once the symptom pointed here: 1. The watchdog respawns by invoking `uffs --daemon start`, and that command cleared the stop-intent marker. So the supervisor erased the very marker it is meant to obey: the intent survived at most one tick and the service came back regardless. The clear is now skipped when `UFFS_SUPERVISED_RESTART` is set, which the watchdog sets on the restart it drives — an operator start still revokes intent, a supervisor restart never does. 2. The watchdog was unobservable. `resident on` spawns it with stdio discarded, so every decision it made vanished; the earlier fix was a guess because nothing could be inspected. It now appends each decision to `<lifecycle-dir>/watchdog.log` WITH THE INPUTS — service, stop_intent, the exact marker path it consulted, recent respawn count and the resulting action. If this is still wrong, that file says why instead of requiring another guess. Ordering from the previous commit (marker before the RPC) stays; it was necessary but not sufficient.
…opped A deliberate `uffs --daemon stop` bounced straight back, and the watchdog log exonerated itself: every daemon line read `HonourStopIntent`. It never touched the daemon — it defeated the stop through the MCP. The liveness probe was `uffs --<service> status` scanned for the substring `running` minus `not running`. But `--mcp status` reports the daemon too, so a stopped daemon put `Daemon: not running` into the *MCP* report and the healthy gateway read as dead. The watchdog then ran `uffs --mcp start`, whose preflight sees "gateway up, daemon down" and helpfully restarts the daemon. Three rounds of that exhausted the respawn ledger, the watchdog gave up on the MCP, and only then did the stop finally stick — which is exactly why it appeared to need two stops in a row. The same substring read had a second defect: `◐ loading (3/7 drives)` contains neither string, so a daemon still reading the MFT counted as dead and was liable to be respawned on top of itself. Liveness now comes from `uffs --status --json`, which reports every service under its own key, so one service's state can no longer be mistaken for another's. It is one subprocess per tick instead of two, and `connect_raw` never auto-spawns, so probing stays side-effect free. An unreadable probe now means *unknown* and is left alone, where it previously read as "down" and triggered a respawn. Closing the same hole from the other side: `--mcp start` no longer revives an unreachable daemon when `UFFS_SUPERVISED_RESTART` is set and a daemon stop-intent marker exists. Interactively the old behaviour is still right — you asked for a gateway, a gateway needs a daemon — but the watchdog is not the operator and must not drag a deliberately stopped daemon back up with the gateway. `install-bins.rs` carried both bugs in its own probes: `use-local` would have failed to restore an MCP gateway whenever the daemon was down, and failed to restore a daemon caught mid-load.
The residency work landed across several commits without the user manual catching up, and one earlier feature never reached it at all. Documents the watchdog: what it supervises, what it deliberately does not (the Access Broker, and why supervising a LocalSystem service from a non-elevated process would break the zero-UAC promise residency exists to protect), the crash budget, that a deliberate stop always wins, that it never introduces a service you never ran, and how to read `watchdog.log` when a service comes back and you want to know who did it. Refreshes the `--daemon status -v` sample, which had drifted: it predated the physical-drive inventory (shipped in the status/physical drive view work and never documented), the mimalloc line, and this branch's column alignment. Adds the `── Physical drives ──` section — the one that answers "why did my search miss that drive" — and the `status_drives` tier table, until now mentioned only in passing, with a column-by-column key. Documents `uffs --status --json` as the multi-service contract, with the reason to prefer it: each service carries its own `running` flag, and the human views mention other services by design, so a substring scan attributes one service's state to another. That is precisely the bug fixed in the previous commit. Explains what `just use-local` now does to running services, and adds a troubleshooting entry for "the daemon restarts after I stop it". Fixes an asymmetry found while writing this: `resident on` armed the watchdog but `resident off` never disarmed it, so switching residency off left a supervisor running that would second-guess the next stop. `resident off` now disarms it, and `resident status` reports whether supervision is active — without that line the watchdog is invisible to the command whose job is to describe residency.
An anchor sweep over all 277 markdown files found twelve links whose targets exist but whose fragments never matched, so every one of them landed the reader at the top of the page instead of the section they asked for. Most were off by a hyphen: GitHub does not collapse whitespace when it slugifies, so a numbered heading (`## 3 Bulkiness`) or one containing a dash (`— Daemon Runs`) yields a *double* hyphen the hand-written links did not have. Two were pointing at the wrong section entirely, and their visible labels were wrong with them: Descendants is Concepts §4, not §5, and Tree Size is §2, not §4. The FAQ's Administrator answer pointed at Installation §5 (Build from Source) rather than §3 (Platform Requirements). While fixing that last one: the FAQ still answered "On Windows, yes — reading the MFT requires elevated access", which predates the Access Broker and contradicts the installation guide two clicks away. It now says what is actually true — elevation once at `uffs-broker --install`, then no UAC on any later search, daemon start/stop, or update.
A single file created on a live drive permanently doubled two of the largest structures in the index. On C: the record column went 276 MB → 552 MB and the name arena 95 MB → 190 MB — 371 MB, for one file — and stayed there for the life of the shard. Seen in the field as a daemon reporting `[rec=552 names=190]` where an identical daemon on the same drive reported `[rec=276 names=95]`, with the trigram, child-map and extension shards byte-identical between the two. The exact-2x ratio, confined to precisely the two columns that get appended to, is the tell. `build_compact_index` ends with `shrink_compact_vecs`, which reclaims ~500 MB across seven drives by shrinking the columns to an exact fit — leaving `capacity == len`. The cache-load path lands there too (`aligned_vec_from_bytes`, `to_vec`). The first USN create then calls `Vec::push` / `extend_from_slice` on a full vector, and `Vec`'s amortised growth reallocates to *twice* the capacity. Shrink reclaimed 500 MB; the first created file handed back more than it saved. Doubling is the right default for a `Vec` that knows nothing about its contents. These columns are hundreds of megabytes and grow by a handful of records per USN batch, so they want a different policy: reserve an eighth of the current length. Growth stays geometric — appends remain amortised O(1) — while the waste is capped at 12.5% instead of 100%. For C: that is ~34 MB of slack instead of ~276 MB. `ColumnStorage::as_mut_vec` is replaced by `vec_for_append(additional)`, which does the reserve before handing out the `&mut Vec`. The old accessor is gone rather than deprecated: leaving a doubling-growth escape hatch beside the bounded one is how this reached production in the first place. `frs_to_compact` grows through the same policy — its `resize` past the FRS high-water mark had the identical defect at smaller scale (~13 MB per drive). Three tests pin it, including the exact-fit case: shrink to `capacity == len`, append one element, assert the capacity did not double.
The aggregation manual promises "All --agg commands accept every filter from the filters page". The scan honoured four: extensions, files/dirs, size bounds, and drive scope. Everything else was silently dropped — found live when `--in-path` naming a directory that cannot exist still counted 3,835,372 files, and the path-aware glob `'**\GitHub\**\*' --count` returned 0 while the same scope as a literal counted 400. Two distinct gaps, one shared root: Record-level filters (dates, attributes, excludes, months, name/path lengths, tree metrics, bulkiness) simply never reached the scan — the daemon built a full `SearchFilters` for the row search and handed the aggregation a 4-field `AggregateFilter`. `--newer 7d --count` counted every file ever written. The scan now also runs `matches_record` — the SAME predicate the row search's record scans run — with extension IDs resolved per drive, so a count and a row listing can no longer disagree. The aggregate cache key hashes the filter set (via its Debug rendering, so a future filter field cannot be silently omitted); the in-process cache can never serve a date-scoped count computed without the dates. Path-dependent scoping (--in-path, --exclude-path, --type, path-aware globs, --match-path, regex patterns) cannot be honoured by a record scan at all: it needs resolved paths, and the scan matched bare names — which is exactly why the path glob counted 0. Rather than re-implement path semantics in the aggregation engine (guaranteed drift), such queries now aggregate over the row search's matched set: the search — already unbounded for these shapes, and now also when an aggregation rides on one — applies the full path semantics exactly once, and `run_aggregate_over_records` folds the surviving (drive, record) pairs into the same accumulators. The matched set is snapshotted before the display truncation, so `--limit` bounds what the user sees, never what a count reports. Regression tests pin all three behaviours: a date bound splits the fixture and the count matches the split; an explicit two-record set counts exactly 2; the empty set — the impossible `--in-path` shape — counts 0, never the drive total.
Collaborator
Author
|
Superseded: shipping via the ship-fresh release PR, which carries this branch's commits directly — one CI flow instead of two. |
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
1 participant
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.
Sixteen commits of daemon-lifecycle, correctness, and documentation work, dogfooded live on the Windows production box throughout.
Watchdog / residency
uffs-watchdog: user-level supervisor for the resident daemon + MCP gateway (crash budget 3/60s, deliberate stops honoured via stop-intent markers, decision log at<lifecycle>/watchdog.log). Deliberately excludes the Access Broker (SCM owns it; zero-UAC preserved).uffs --status --json— the prose-scan probe misread a stopped daemon as a dead MCP gateway and resurrected the daemon through--mcp start's preflight; a loading daemon (◐ 3/7 drives) read as dead. Unknown probe = leave alone.--mcp startunderUFFS_SUPERVISED_RESTARTno longer revives a deliberately stopped daemon.resident offnow disarms the watchdog;resident statusreports supervision state.Installer (
just use-local)Memory
capacity == len, and the first USN append reallocated to 2× (C: rec 276→552 MB, names 95→190 MB for a single file). Columns now grow by bounded 12.5% slack viavec_for_append(the doubling escape hatchas_mut_vecis removed);frs_to_compactuses the same policy. Verified live: ~950 creates cost 1 MB where one create used to cost 371 MB.Aggregation correctness
--count,--agg,--facet, …) honours every search filter, as the manual has always promised. Record-level filters (dates, attrs, excludes, months, tree metrics) now run the samematches_recordpredicate as the row search; path-dependent scoping (--in-path,--exclude-path,--type, path-aware globs,--match-path, regex) aggregates over the row search's matched set so path semantics apply exactly once. Found live:--in-pathnaming an impossible directory still counted 3,835,372 files; a path glob counted 0. Verified live post-fix: impossible dir → 0, real scope → 52,300.Status output
--daemon status -vdrive/physical tables andstatus_drivestier table fully column-aligned.Docs
status_driveskey,--status --jsonas the multi-service contract,use-locallifecycle, "daemon restarts after I stop it" troubleshooting; 12 broken intra-doc anchors repaired; FAQ elevation answer updated for the Access Broker.Tests