OpsDeck/module_filebrowser
Watch
2
0
Fork
You've already forked module_filebrowser
 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

perf(roots): one helper probes them, and a declared reason outranks the cancel #3

Merged
thisilike merged 3 commits from fix/roots-probe-and-cancel-reason into main 2026-08-24 19:32:44 +02:00
Conversation 1 Commits 3 Files changed 9 +379 -63
thisilike commented 2026-08-24 15:00:38 +02:00
Owner
Copy link

The two follow-ups from your approval on #2, as a branch rather than as issues.
It sits on top of #2, so until that merges the diff here shows its four commits
too; afterwards it is these three.

1. /roots and /overview asked serially — and disagreed about a root

You named /roots. /overview was the third copy of the same loop, so the fix
is a helper rather than a second Promise.all: probeRoots() in fsops.ts
answers every root at once, and the three surfaces map its result into their own
shape.

The drift was worse than the latency. /s/roots resolved each root, the web
routes mapped root.path into the container directly — so a root a deny rule
covers rendered unreadable on the schema page and perfectly fine on the web
one. Both go through resolve() now. One visible consequence: a missing root's
error is the resolver's not found rather than the OS message.

/overview shows four roots, so it passes its four in and the other twelve
cost nothing.

Against a running server (host root /, three real roots, one missing):

web  media   ok    free 29660962816  used 2784464896  total 32445427712
web  photos  ok    free 29660962816  used 2784464896  total 32445427712
web  backups ok    free 29660962816  used 2784464896  total 32445427712
web  gone    FAIL  not found
page media   ok    free 29660962816  used 2784464896  total 32445427712
page gone    unreadable
card media   free 29660962816  total 32445427712

and with DENY=<root>/photos/**, which is the case the two surfaces used to
answer differently:

web  photos  FAIL  refused by deny rule /…/srv/photos/**
page photos  unreadable

2. terminalReason preferred the cancel over the declared reason

Reordered rather than trimmed, since you were right about which way core does
it: service.ts classifies a TaskFailure before it consults
cancelRequested, so a delete that fails on Deno.remove mid-cancel records
failed/delete-failed and the topic now says error · delete-failed instead of
error · cancelled. Everything undeclared while aborted is still the cancel —
scan's cancel throw is a plain Error, so a cancelled scan still ends
error · cancelled. The comment that claimed the old order is gone.

Tests — 58 pass

deno test --allow-read --allow-write --allow-run=python3 backend/, up from 51.

  • roots_test.ts: a df that answers nothing until three of them are waiting at
    once (a serial caller deadlocks it, which is the assertion), the configured
    order surviving Promise.all, a missing root and a denied root each costing
    no df at all, the subset argument, and two probes sharing one df through the
    cache.
  • contract_test.ts gains "no route file walks policy.roots itself" and drops
    the toContainer(c, root.path) exception those loops needed — a route file
    may not name toContainer at all now.
  • progress_test.ts: the declared reason wins mid-cancel; a reasonless
    TaskFailure while aborted is still cancelled.

deno lint clean (33 files). deno check and the module-builder import contract
both clean in a core checkout. deno fmt --check clean on every file this branch
adds, and on the lines it adds to README.md; the repo-wide backlog is
untouched.

The two follow-ups from your approval on #2, as a branch rather than as issues. It sits on top of #2, so until that merges the diff here shows its four commits too; afterwards it is these three. ## 1. `/roots` and `/overview` asked serially — and disagreed about a root You named `/roots`. `/overview` was the third copy of the same loop, so the fix is a helper rather than a second `Promise.all`: `probeRoots()` in `fsops.ts` answers every root at once, and the three surfaces map its result into their own shape. The drift was worse than the latency. `/s/roots` resolved each root, the web routes mapped `root.path` into the container directly — so a root a deny rule covers rendered `unreadable` on the schema page and perfectly fine on the web one. Both go through `resolve()` now. One visible consequence: a missing root's `error` is the resolver's `not found` rather than the OS message. `/overview` shows four roots, so it passes its four in and the other twelve cost nothing. Against a running server (host root `/`, three real roots, one missing): ``` web media ok free 29660962816 used 2784464896 total 32445427712 web photos ok free 29660962816 used 2784464896 total 32445427712 web backups ok free 29660962816 used 2784464896 total 32445427712 web gone FAIL not found page media ok free 29660962816 used 2784464896 total 32445427712 page gone unreadable card media free 29660962816 total 32445427712 ``` and with `DENY=<root>/photos/**`, which is the case the two surfaces used to answer differently: ``` web photos FAIL refused by deny rule /…/srv/photos/** page photos unreadable ``` ## 2. `terminalReason` preferred the cancel over the declared reason Reordered rather than trimmed, since you were right about which way core does it: `service.ts` classifies a `TaskFailure` before it consults `cancelRequested`, so a delete that fails on `Deno.remove` mid-cancel records `failed/delete-failed` and the topic now says `error · delete-failed` instead of `error · cancelled`. Everything undeclared while aborted is still the cancel — scan's cancel throw is a plain `Error`, so a cancelled scan still ends `error · cancelled`. The comment that claimed the old order is gone. ## Tests — 58 pass `deno test --allow-read --allow-write --allow-run=python3 backend/`, up from 51. - `roots_test.ts`: a df that answers nothing until three of them are waiting at once (a serial caller deadlocks it, which is the assertion), the configured order surviving `Promise.all`, a missing root and a denied root each costing no df at all, the subset argument, and two probes sharing one df through the cache. - `contract_test.ts` gains "no route file walks `policy.roots` itself" and drops the `toContainer(c, root.path)` exception those loops needed — a route file may not name `toContainer` at all now. - `progress_test.ts`: the declared reason wins mid-cancel; a reasonless `TaskFailure` while aborted is still `cancelled`. `deno lint` clean (33 files). `deno check` and the module-builder import contract both clean in a core checkout. `deno fmt --check` clean on every file this branch adds, and on the lines it adds to `README.md`; the repo-wide backlog is untouched. 
Both halves of issue #1, both module-side.

**The roots page promised free space and never had any.** It declared a "Free"
field and a used/total bar while the route answered null for all three, so
every schema client — the Android app, and the web schema renderer for any
schema-only module — rendered the promise and none of the numbers. The df
helper existed and the web routes were already using it behind a 15-second
cache; the schema route now shares that same cache rather than starting a
second one, because df is a subprocess and both surfaces ask about the same
roots. Absent df still means null: a deployment without coreutils shows no bar
rather than a wrong one.

**Scan and delete streamed nothing.** They start a task run and answer with a
runId, which is the better protocol and which the phone does not speak — so
until its next refresh tick, up to 15 seconds, a scan looked like nothing
happening. Both now publish a line per meaningful step to a topic named on the
action, ending with one that starts "done", which is how a progressTopic client
knows the work finished.

The role question that came with it: core's gate is binary, a topic is
admin-only or it is not. Pinning these admin would mean an operator allowed to
start a scan cannot watch the one they started, so they follow the READ role
instead — admin-only exactly when reads are, which is the default deployment.
The invariant that makes this safe is in the payload, not the gate: a line
carries counts and a name the subscriber could already list, never file
content, never a path outside the roots.

Verified against a running server: the roots route reports real used/free/total
from df, the schema carries progressTopic on Delete, Scan this directory and
Start scan, and an SSE subscriber watching a real scan received
`{"msg":"13 entries · /tmp/srv/media"}` followed by `{"msg":"done · 13 entries"}`.
The review found the same class of bug this branch exists to fix: a run
that threw published nothing terminal, so the route's 200 had already
happened, the Android renderer's HTTP error path never fired, and the log
stayed frozen on the last thing that worked. Delete's `resolve`,
`assertWritable` and `Deno.remove` failures did it, and so did scan's
`unknown-root`, `unreachable-path` and a failing `store.save`.

The terminal line now has exactly one owner. `progress.ts` wraps a run
body and publishes either `done · …` or `error · <reason>` whatever
happens, so a new exit path cannot forget: the failure paths are covered
by the wrapper rather than by remembering to add a line to each. Two
consequences worth naming:

- The `done` line is built from the value the body returns, so it lands
  after the cancel throw instead of before it. A cancelled scan used to
  announce completion and trigger `refetchAll()` while the run recorded
  a failure.
- The line carries the reason the action declared, never the error
  message. Run fields are secret per field and default to withheld; a
  topic is gated only by a role and has no projection to withhold with,
  so an OS error string on it would hand every subscriber exactly what
  `expose` exists to keep back. Anything unnamed is `error · failed`.

`delete-progress` now follows the WRITE role rather than the read role.
Scanning is a read-role action and its stream still follows reads, but a
delete needs the write role to start, so under `ROLE=viewer,
WRITE_ROLE=admin` a viewer could watch admin deletions. Gating on
`writeRole` keeps "watch the run you started" intact and closes that.

`freeBytes` is df's Available (column 4) rather than `total - used`,
which counts the filesystem's reserve as writable space — ~1.6 GB of it
on a 32 GB ext4. `FsUsage` carries `availBytes`, so the schema page, the
listing bar and the overview widget are all fixed at once.

`UsageCache` no longer grows without bound: `/list` keys it by whatever
directory is being browsed, so it now caps at 64 entries, dropping the
least recently refreshed, and coalesces in-flight calls so N concurrent
cold requests for one path spawn one df instead of N.

Tests: the two behaviours above, plus two contract assertions — every
topic a schema button follows has its run wrapped, and no surface
recomputes free space as `total - used`. 51 pass, `deno lint` is clean
(the unused `FsUsage` import in routes.ts is gone).
`/s/roots` awaited a stat and a df per root in sequence, so the landing
page cost the sum of every mount rather than the slowest one. The roots
are independent; ask them concurrently and keep the configured order.
`deno fmt --check` reflows two lines in `backend/progress_test.ts` — a file
this PR adds, so the repo-wide formatting backlog is no excuse for it.
/roots and /overview walked the configured roots in sequence, paying a stat
and possibly a df per root, so a landing page cost the sum of its mounts and
one spun-down disk decided how long that was. 5a95471 fixed that for /s/roots
and left the copy in the web routes, which is the third copy of a per-root
loop nobody was keeping in step.

They were already out of step about a more interesting thing: /s/roots
resolved each root, the web routes mapped root.path into the container
directly, so a root a deny rule covers rendered "unreadable" on the schema
page and perfectly fine on the web one.

probeRoots() in fsops.ts owns both halves now — Promise.all over the roots,
resolve() before the stat — and the three surfaces map its answer into their
own shape. An optional subset argument keeps /overview's four-root card from
probing the twelve it does not show.

Verified against a running server (host root /, four roots, one missing, one
covered by a deny rule): /roots, /s/roots and /overview report the same
numbers and agree that the missing root is unreadable, and with the deny rule
in place the web route now refuses the root it used to call fine.

roots_test.ts asserts the shape rather than the wording: a df that answers
nothing until three are waiting at once (a serial caller deadlocks it), the
configured order surviving Promise.all, a missing root and a denied root
costing no df at all, the subset argument, and two probes sharing one df
through the cache. contract_test.ts gains the rule — no route file walks
policy.roots itself — and drops the toContainer exception those loops needed.
terminalReason() read the abort signal first, so a delete that fails on
Deno.remove while a cancel is pending published "error · cancelled" while the
run itself recorded failed/delete-failed: core's service.ts classifies a
TaskFailure BEFORE it consults cancelRequested. Harmless for a phone — the
stream ends either way — but it is the one place a subscriber and the run
disagree about the same run, and the comment above the function claimed the
opposite.

A declared reason now wins, and everything else while aborted is still read as
the cancel, which is how a cooperative one arrives: whatever the module threw
when it noticed the signal. Scan's cancel throw is a plain Error, so a
cancelled scan still says "error · cancelled".
README says the three surfaces cannot disagree about a readable root, since
that is the part an operator sees; CLAUDE.md gains the rule that produced it
(no surface walks policy.roots itself) and the reason ordering on the
progress.ts row.
julian approved these changes 2026-08-24 17:12:50 +02:00
julian left a comment
Copy link

Approving. I re-ran everything the description claims and checked the core-side premise behind the reorder; both hold.

Verified

  • deno test --allow-read --allow-write --allow-run=python3 backend/ — 58 passed, 0 failed.
  • deno lint — clean, 33 files.
  • deno check backend/mod.ts — clean, with @opsdeck/sdk and hono mapped in from a core checkout.
  • deno fmt --check — roots_test.ts, progress.ts and progress_test.ts clean; the complaints in fsops.ts, routes.ts, ui.ts and contract_test.ts all land on lines this branch does not touch, so the claim about the added lines holds.
  • The premise for the terminalReason reorder is real: core catches at packages/server/src/tasks/service.ts:986, takes e instanceof TaskFailure first and only then consults state.cancelRequested, so failed/delete-failed is what the run records for a delete that fails mid-cancel. The new order matches it. And the undeclared throws stay undeclared — h.signal.throwIfAborted() (AbortError), CancelledScan, new Error("scan cancelled") — so a cancelled scan still ends error · cancelled on both sides.
  • No fourth surface probes the roots: actions.ts:385 and mod.ts:91 iterate policy.roots but neither stats nor df's them, and ui.ts still needs resolve for its client-path routes, so nothing is left unused by the rewrite.

Notes, none blocking

  1. backend/routes.ts:655 — the overview card is the one surface that still cannot say "unreadable". probeRoots hands it ok and error for free, but /overview drops both, so a denied or missing root renders as — free with a 0%-full bar (frontend/OverviewWidget.svelte:63-66), which reads like a healthy empty mount. README now says the three surfaces "cannot disagree about whether a root is readable" — the card does not disagree because it does not answer. Passing ok through and rendering it would close that.

  2. backend/contract_test.ts, "no surface probes the roots itself" — it is a single-line, single-spelling tripwire: policy.roots.forEach(, a const rs = policy.roots aliased before the loop, or a for ( that fmt has split over lines all slip past it. Fine as a tripwire, but it catches the shape that was there rather than the shape that is possible, and the comment above it promises more than the regex delivers.

  3. backend/roots_test.ts, barrierDf — df.done() runs after the assertions, so a failing assertion leaks the 2s timer and Deno's sanitizer reports a leaked timer instead of the assertion that actually failed. try { … } finally { df.done() }, the way the temp dirs are already handled.

  4. backend/fsops.ts:436 — as flagged in the description, a missing root's error is now the resolver's not found. Worth knowing that paths.ts spells three cases that way (:156, :161, :204), so "the mount is gone" and "this root's path is not inside any root" become the same message on /roots. The deny case keeps its rule and an unreadable-but-present root keeps the OS message from the stat, so the loss is narrow.

  5. probeRoots fans out with no cap: N roots with a cold cache is N concurrent df subprocesses on one request. That is the point of the change and UsageCache collapses repeats per path — just noting the burst is now bounded only by how many roots the operator configured.

Approving. I re-ran everything the description claims and checked the core-side premise behind the reorder; both hold. **Verified** - `deno test --allow-read --allow-write --allow-run=python3 backend/` — 58 passed, 0 failed. - `deno lint` — clean, 33 files. - `deno check backend/mod.ts` — clean, with `@opsdeck/sdk` and `hono` mapped in from a core checkout. - `deno fmt --check` — `roots_test.ts`, `progress.ts` and `progress_test.ts` clean; the complaints in `fsops.ts`, `routes.ts`, `ui.ts` and `contract_test.ts` all land on lines this branch does not touch, so the claim about the added lines holds. - The premise for the `terminalReason` reorder is real: core catches at `packages/server/src/tasks/service.ts:986`, takes `e instanceof TaskFailure` first and only then consults `state.cancelRequested`, so `failed/delete-failed` is what the run records for a delete that fails mid-cancel. The new order matches it. And the undeclared throws stay undeclared — `h.signal.throwIfAborted()` (AbortError), `CancelledScan`, `new Error("scan cancelled")` — so a cancelled scan still ends `error · cancelled` on both sides. - No fourth surface probes the roots: `actions.ts:385` and `mod.ts:91` iterate `policy.roots` but neither stats nor df's them, and `ui.ts` still needs `resolve` for its client-path routes, so nothing is left unused by the rewrite. **Notes, none blocking** 1. `backend/routes.ts:655` — the overview card is the one surface that still cannot say "unreadable". `probeRoots` hands it `ok` and `error` for free, but `/overview` drops both, so a denied or missing root renders as `—` free with a 0%-full bar (`frontend/OverviewWidget.svelte:63-66`), which reads like a healthy empty mount. README now says the three surfaces "cannot disagree about whether a root is readable" — the card does not disagree because it does not answer. Passing `ok` through and rendering it would close that. 2. `backend/contract_test.ts`, "no surface probes the roots itself" — it is a single-line, single-spelling tripwire: `policy.roots.forEach(`, a `const rs = policy.roots` aliased before the loop, or a `for (` that fmt has split over lines all slip past it. Fine as a tripwire, but it catches the shape that was there rather than the shape that is possible, and the comment above it promises more than the regex delivers. 3. `backend/roots_test.ts`, `barrierDf` — `df.done()` runs after the assertions, so a failing assertion leaks the 2s timer and Deno's sanitizer reports a leaked timer instead of the assertion that actually failed. `try { … } finally { df.done() }`, the way the temp dirs are already handled. 4. `backend/fsops.ts:436` — as flagged in the description, a missing root's error is now the resolver's `not found`. Worth knowing that `paths.ts` spells three cases that way (:156, :161, :204), so "the mount is gone" and "this root's path is not inside any root" become the same message on `/roots`. The deny case keeps its rule and an unreadable-but-present root keeps the OS message from the stat, so the loss is narrow. 5. `probeRoots` fans out with no cap: N roots with a cold cache is N concurrent `df` subprocesses on one request. That is the point of the change and `UsageCache` collapses repeats per path — just noting the burst is now bounded only by how many roots the operator configured.
thisilike deleted branch fix/roots-probe-and-cancel-reason 2026-08-24 19:32:44 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
julian
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
julian
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/module_filebrowser!3
No description provided.