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

fix(portable-ui): free space that is real, and progress a phone can see #2

Merged
thisilike merged 4 commits from fix/portable-ui-gaps into main 2026-08-24 16:55:59 +02:00
Conversation 5 Commits 4 Files changed 14 +689 -211
thisilike commented 2026-08-12 09:38:17 +02:00
Owner
Copy link

Closes #1.

Apologies for the detour: this was pushed straight to main, which is why it
never reached your review queue and looked like nothing had happened. Main has
been rolled back and the commit moved here, unchanged.

1. Free-space fields

Wired rather than dropped, as you suggested. The df helper moved out of
routes.ts into fsops.ts as a shared UsageCache, so /s/roots and the web
routes go through the same 15-second cache instead of each starting one — df
is a subprocess and both surfaces ask about the same roots. Absent df still
yields null, so the documented degradation holds: no bar rather than a wrong
one.

Against a running server:

media   free 27934715904  used 4510720000  total 32445435904
photos  free 27934715904  used 4510720000  total 32445435904
backups free 27934715904  used 4510720000  total 32445435904

2. Progress topics

scan-progress and delete-progress, named on Delete, Scan this directory and
Start scan. A real scan watched over SSE:

event: filebrowser:scan-progress
data: {"msg":"13 entries · /tmp/srv/media"}
data: {"msg":"done · 13 entries"}

The done prefix is what ends the stream for a progressTopic client, so it
is the first word rather than merely present.

The role decision

Core's gate is binary — ctx.events.adminOnly(topic) and nothing finer — so "a
per-topic role matching the action's role" is not expressible directly. The
streams follow the read role instead: admin-only exactly when reads are,
which is the default deployment. Pinning them admin unconditionally would mean
an operator allowed to start a scan cannot watch the one they started.

What makes that safe is the payload rather than the gate, so that is where the
rule lives: a line carries counts and a name the subscriber could already list
— never file content, never a path outside the roots. Both your options
collapse into the same discipline once the gate cannot be finer than binary.
README updated to say this instead of the old flat "live topics are
admin-gated".

Not in here

The up/:p redirect page stays until the Android renderer can render parent
(OpsDeck/core#35 adds the field, OpsDeck/mobile#2 renders it). Removing it now
would take the up-arrow off the phone.

38 backend tests pass; deno check clean.

Closes #1. Apologies for the detour: this was pushed straight to main, which is why it never reached your review queue and looked like nothing had happened. Main has been rolled back and the commit moved here, unchanged. ## 1. Free-space fields Wired rather than dropped, as you suggested. The df helper moved out of `routes.ts` into `fsops.ts` as a shared `UsageCache`, so `/s/roots` and the web routes go through the *same* 15-second cache instead of each starting one — df is a subprocess and both surfaces ask about the same roots. Absent df still yields null, so the documented degradation holds: no bar rather than a wrong one. Against a running server: ``` media free 27934715904 used 4510720000 total 32445435904 photos free 27934715904 used 4510720000 total 32445435904 backups free 27934715904 used 4510720000 total 32445435904 ``` ## 2. Progress topics `scan-progress` and `delete-progress`, named on Delete, Scan this directory and Start scan. A real scan watched over SSE: ``` event: filebrowser:scan-progress data: {"msg":"13 entries · /tmp/srv/media"} data: {"msg":"done · 13 entries"} ``` The `done` prefix is what ends the stream for a `progressTopic` client, so it is the first word rather than merely present. ## The role decision Core's gate is binary — `ctx.events.adminOnly(topic)` and nothing finer — so "a per-topic role matching the action's role" is not expressible directly. The streams follow the **read role** instead: admin-only exactly when reads are, which is the default deployment. Pinning them admin unconditionally would mean an operator allowed to start a scan cannot watch the one they started. What makes that safe is the payload rather than the gate, so that is where the rule lives: a line carries counts and a name the subscriber could already list — never file content, never a path outside the roots. Both your options collapse into the same discipline once the gate cannot be finer than binary. README updated to say this instead of the old flat "live topics are admin-gated". ## Not in here The `up/:p` redirect page stays until the Android renderer can render `parent` (OpsDeck/core#35 adds the field, OpsDeck/mobile#2 renders it). Removing it now would take the up-arrow off the phone. 38 backend tests pass; `deno check` clean. 
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"}`.
julian requested changes 2026-08-12 09:44:38 +02:00
Dismissed
julian left a comment
Copy link

Both gaps are addressed and the reasoning on the role gate is sound — a binary adminOnly really cannot express "the action's role", and following the read role is the right fallback. The shared UsageCache is also the correct shape: one df per path per interval for both surfaces.

Verified locally at 1bedb0c: deno test --allow-read --allow-write --allow-run=python3 backend/ → 38 passed. (deno check needs a core checkout for @opsdeck/sdk, so I could not reproduce that claim standalone.)

Requesting changes on two things, one of them the same class of bug this PR exists to fix:

1. No terminal line on failure or cancel — the phone hangs exactly like before (actions.ts).
A progressTopic client ends its stream only on a line starting with done or error; the route already answered 200 with a runId, so the HTTP error path in the Android renderer never fires. Concretely: Deno.remove fails on entry 2 of 3 → TaskFailure → the run is failed, the module publishes nothing more, and SchemaViewModel keeps runningAction set with the log frozen on removing … until the user leaves the page. Same for delete's resolve/assertWritable throws, and for scan's unknown-root, unreachable-path, a store.save failure, and CancelledScan. Both handlers need a try/catch that publishes error · <reason> before rethrowing.

Related, in the other direction: scan publishes done · N entries before if (h.signal.aborted) throw new Error("scan cancelled"), so a cancelled scan tells every subscriber it finished and triggers refetchAll() while the run itself records failure.

2. deno lint fails on this branch.
backend/routes.ts:48 — FsUsage is now unused after the class moved out. no-unused-vars, one problem, and fa22a93 lints clean.

Non-blocking, but worth deciding on:

3. One topic, many runs. scan-progress and delete-progress are module-wide, and the end of a stream is a done prefix from any run. With SCAN=periodic there is one schedule per root, so a scheduled scan of backups finishing terminates the stream of the phone watching the scan of media it just started. Deletes queue on onSelfConflict: wait, so the same happens to the second of two deletes into one directory. Core has no per-run topic (progressTopic is a static string, no interpolation), so this may just be a documented limit of the deprecated mechanism — but the README currently reads as if the stream belongs to the action you pressed.

4. delete-progress is gated on the read role, not the write role. With ROLE=viewer, WRITE_ROLE=admin, a viewer subscribes and watches what an admin is deleting. The "watch the one you started" argument does not need the read role here: starting a delete already requires the write role, so gating that one topic on policy.writeRole === "admin" keeps the argument intact and closes the gap. scan-progress following the read role is correct as written — scan is a read-role action.

5. freeBytes is total - used, which is not what df calls free. df -Pk reports Available in column 4; usageOf drops it. On ext4 with the default 5% root reserve, a 32 GB filesystem reports ~1.6 GB more free than a non-root process can actually write. The web UI computes it the same way, so this is pre-existing and consistent — but the PR title is "free space that is real", and the honest version is one field: keep availBytes from cols[3] in FsUsage and use it in both surfaces.

6. UsageCache never evicts. /list calls usage.get(target.container) with whatever directory is being listed, not just roots, so the map grows one entry per directory ever browsed for the lifetime of the process. Pre-existing, and now the shared one. A cap or keying by the root's mount would bound it. Two nits in the same area: /s/roots awaits df serially per root, and there is no in-flight coalescing, so N concurrent cold requests for one path spawn N subprocesses.

7. No tests. contract_test.ts is the natural home for both behaviours — /s/roots returning non-null usage with a stubbed exec, and the terminal line starting with done/error for each exit path of the two actions. The second one would have caught #1.

Fix 1 and 2 and I will merge. 4 is a small change I would like in the same pass; 3, 5, 6, 7 can be follow-ups if you would rather keep this focused.

Both gaps are addressed and the reasoning on the role gate is sound — a binary `adminOnly` really cannot express "the action's role", and following the read role is the right fallback. The shared `UsageCache` is also the correct shape: one df per path per interval for both surfaces. Verified locally at 1bedb0c: `deno test --allow-read --allow-write --allow-run=python3 backend/` → 38 passed. (`deno check` needs a core checkout for `@opsdeck/sdk`, so I could not reproduce that claim standalone.) Requesting changes on two things, one of them the same class of bug this PR exists to fix: **1. No terminal line on failure or cancel — the phone hangs exactly like before (actions.ts).** A `progressTopic` client ends its stream only on a line starting with `done` or `error`; the route already answered 200 with a `runId`, so the HTTP error path in the Android renderer never fires. Concretely: `Deno.remove` fails on entry 2 of 3 → `TaskFailure` → the run is failed, the module publishes nothing more, and `SchemaViewModel` keeps `runningAction` set with the log frozen on `removing …` until the user leaves the page. Same for delete's `resolve`/`assertWritable` throws, and for scan's `unknown-root`, `unreachable-path`, a `store.save` failure, and `CancelledScan`. Both handlers need a `try/catch` that publishes `error · <reason>` before rethrowing. Related, in the other direction: scan publishes `done · N entries` *before* `if (h.signal.aborted) throw new Error("scan cancelled")`, so a cancelled scan tells every subscriber it finished and triggers `refetchAll()` while the run itself records failure. **2. `deno lint` fails on this branch.** `backend/routes.ts:48` — `FsUsage` is now unused after the class moved out. `no-unused-vars`, one problem, and `fa22a93` lints clean. Non-blocking, but worth deciding on: **3. One topic, many runs.** `scan-progress` and `delete-progress` are module-wide, and the end of a stream is a `done` prefix from *any* run. With `SCAN=periodic` there is one schedule per root, so a scheduled scan of `backups` finishing terminates the stream of the phone watching the scan of `media` it just started. Deletes queue on `onSelfConflict: wait`, so the same happens to the second of two deletes into one directory. Core has no per-run topic (`progressTopic` is a static string, no interpolation), so this may just be a documented limit of the deprecated mechanism — but the README currently reads as if the stream belongs to the action you pressed. **4. `delete-progress` is gated on the read role, not the write role.** With `ROLE=viewer, WRITE_ROLE=admin`, a viewer subscribes and watches what an admin is deleting. The "watch the one you started" argument does not need the read role here: starting a delete already requires the write role, so gating that one topic on `policy.writeRole === "admin"` keeps the argument intact and closes the gap. `scan-progress` following the read role is correct as written — scan is a read-role action. **5. `freeBytes` is `total - used`, which is not what df calls free.** `df -Pk` reports Available in column 4; `usageOf` drops it. On ext4 with the default 5% root reserve, a 32 GB filesystem reports ~1.6 GB more free than a non-root process can actually write. The web UI computes it the same way, so this is pre-existing and consistent — but the PR title is "free space that is real", and the honest version is one field: keep `availBytes` from `cols[3]` in `FsUsage` and use it in both surfaces. **6. `UsageCache` never evicts.** `/list` calls `usage.get(target.container)` with whatever directory is being listed, not just roots, so the map grows one entry per directory ever browsed for the lifetime of the process. Pre-existing, and now the shared one. A cap or keying by the root's mount would bound it. Two nits in the same area: `/s/roots` awaits df serially per root, and there is no in-flight coalescing, so N concurrent cold requests for one path spawn N subprocesses. **7. No tests.** `contract_test.ts` is the natural home for both behaviours — `/s/roots` returning non-null usage with a stubbed exec, and the terminal line starting with `done`/`error` for each exit path of the two actions. The second one would have caught #1. Fix 1 and 2 and I will merge. 4 is a small change I would like in the same pass; 3, 5, 6, 7 can be follow-ups if you would rather keep this focused.
backend/actions.ts Outdated
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

This throw leaves the topic without a terminal line. The route already answered {runId} with 200, so a progressTopic client's HTTP error path never fires — the log stays frozen on removing … and runningAction is never cleared. Wrap the run body so a failure publishes error · <reason> before rethrowing; same for the resolve/assertWritable throws above.

This throw leaves the topic without a terminal line. The route already answered `{runId}` with 200, so a `progressTopic` client's HTTP error path never fires — the log stays frozen on `removing …` and `runningAction` is never cleared. Wrap the run body so a failure publishes `error · <reason>` before rethrowing; same for the `resolve`/`assertWritable` throws above.
backend/actions.ts Outdated
@ -374,0 +420,4 @@
SCAN_PROGRESS_TOPIC,
// "done" is what ends the stream for a progressTopic client, so it
// has to be the first word rather than merely somewhere in the line
`done · ${result.entries.toLocaleString()} entries`,
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

Two problems on this line. It is published before if (h.signal.aborted) throw new Error("scan cancelled") below, so a cancelled scan announces done and triggers the client's refetch while the run records a failure. And every earlier throw (unknown-root, unreachable-path, store.save) publishes nothing terminal at all. The failure paths need error · …, and this one belongs after the cancel check.

Two problems on this line. It is published before `if (h.signal.aborted) throw new Error("scan cancelled")` below, so a cancelled scan announces `done` and triggers the client's refetch while the run records a failure. And every earlier throw (`unknown-root`, `unreachable-path`, `store.save`) publishes nothing terminal at all. The failure paths need `error · …`, and this one belongs after the cancel check.
backend/fsops.ts
@ -347,0 +349,4 @@
* and the schema routes must not pay for a second one alongside the web
* routes. One cache, handed to both.
*/
export class UsageCache {
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

No eviction: /list calls get() with the directory being listed, not just roots, so this map grows one entry per directory ever browsed and never shrinks. Also no in-flight coalescing — N concurrent cold requests for one path each spawn a df.

No eviction: `/list` calls `get()` with the directory being listed, not just roots, so this map grows one entry per directory ever browsed and never shrinks. Also no in-flight coalescing — N concurrent cold requests for one path each spawn a df.
backend/mod.ts Outdated
@ -44,0 +55,4 @@
* because a subscriber who can read the module can read those paths. When
* reads are admin-only, so are the streams.
*/
if (policy.readRole === "admin") {
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

delete-progress follows the read role here, but starting a delete requires the write role — so with ROLE=viewer, WRITE_ROLE=admin a viewer watches admin deletions without ever being able to trigger one. Gating this topic on policy.writeRole === "admin" keeps your "watch the run you started" argument fully intact. scan-progress on the read role is right, since scan is a read-role action.

`delete-progress` follows the read role here, but starting a delete requires the write role — so with `ROLE=viewer, WRITE_ROLE=admin` a viewer watches admin deletions without ever being able to trigger one. Gating this topic on `policy.writeRole === "admin"` keeps your "watch the run you started" argument fully intact. `scan-progress` on the read role is right, since scan is a read-role action.
backend/routes.ts Outdated
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

FsUsage is unused now that the class moved to fsops.ts — deno lint fails on this branch with no-unused-vars, and the base commit is clean.

`FsUsage` is unused now that the class moved to fsops.ts — `deno lint` fails on this branch with `no-unused-vars`, and the base commit is clean.
backend/ui.ts Outdated
@ -363,1 +380,3 @@
freeBytes: null,
usedBytes: fs?.usedBytes ?? null,
totalBytes: fs?.totalBytes ?? null,
freeBytes: fs ? fs.totalBytes - fs.usedBytes : null,
julian commented 2026-08-12 09:44:38 +02:00
Owner
Copy link

total - used is not free space: df -Pk reports Available separately in column 4, and usageOf throws it away. With ext4's default 5% reserve this overstates what a non-root process can write by ~1.6 GB on a 32 GB filesystem — which is why the numbers in the PR description sum exactly. The web UI does the same arithmetic, so it is pre-existing, but keeping availBytes in FsUsage would fix both surfaces at once.

`total - used` is not free space: `df -Pk` reports Available separately in column 4, and `usageOf` throws it away. With ext4's default 5% reserve this overstates what a non-root process can write by ~1.6 GB on a 32 GB filesystem — which is why the numbers in the PR description sum exactly. The web UI does the same arithmetic, so it is pre-existing, but keeping `availBytes` in `FsUsage` would fix both surfaces at once.
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.
thisilike commented 2026-08-15 21:03:30 +02:00
Author
Owner
Copy link

All seven addressed, in dd04785 + 5a95471. You were right that 1 is the same bug this branch exists to fix — publishing the terminal line at the one call site that already worked is exactly the shape of the original gap, so it is now owned by a wrapper rather than by remembering.

1. Terminal line on every exit. backend/progress.ts is new: watched(publish, signal, body, done) runs a run body and publishes done · … or error · <reason> whichever way it leaves. Both handlers go through it, so resolve, assertWritable and Deno.remove on delete, and unknown-root, unreachable-path and a failing store.save on scan, all end the stream.

The cancel ordering falls out of the same change: the done line is built from the value the body returns, so it is published after if (h.signal.aborted) throw, not before it. A cancelled scan no longer announces completion.

One decision inside the wrapper worth your eye. The line carries the declared reason — error · unknown-root, error · delete-failed, error · cancelled — and never the error message; anything unnamed is error · failed. Run fields are secret per field and default to withheld, while a topic is gated only by a role and has no projection, so Deno.remove's message would put a host path in front of every subscriber that expose exists to keep it from. A cancel is decided by the signal rather than by the exception type, the way core decides it (cancelRequested).

2. Lint. Unused FsUsage import gone; deno lint is clean.

4. delete-progress follows the write role. readRole === "admin" still gates scan-progress; writeRole === "admin" gates delete-progress. viewer + WRITE_ROLE=admin no longer watches deletions it cannot start, and since the write role never resolves below the read role, that gate is never looser.

5. availBytes. FsUsage carries df's Available (column 4), and usageOf falls back to total - used only when the column is unparseable. The schema page's freeBytes, the listing bar and the overview widget all read it, so the ~1.6 GB the ext4 reserve was being counted as writable is gone from all three at once.

6. UsageCache is bounded and coalesces. 64 entries, dropping the least recently refreshed (a re-insert moves a path back to the young end, so browsing a deep tree cannot push the roots out for long), and an in-flight map so N concurrent cold reads of one path spawn one df. 5a95471 takes the other nit: /s/roots asks its roots concurrently instead of paying the sum of every mount, order preserved.

3. Documented rather than fixed. progressTopic is a fixed string in the schema with nowhere to put a run id, so the topic belongs to the action and not to the run — a scheduled scan of backups really does end the stream a phone was watching for media, and the second of two queued deletes does it to the first. README now says that under platform limits, including that the run itself is always exact (Activity, the web UI and the runId the route answers with all follow one run), so the fix is core growing a per-run topic rather than anything here.

7. Tests — 51 pass (deno test --allow-read --allow-write --allow-run=python3 backend/), up from 38.

  • progress_test.ts: every exit of watched — done after the body, error-and-rethrow, an OS message not reaching the topic, a cancel saying cancelled whatever it threw, and the reason mapping.
  • usage_test.ts: Available vs total - used (asserting the reserve as a number), the fallback, the three ways df yields nothing, one df for three concurrent cold reads, ttl expiry, and the eviction bound.
  • contract_test.ts gains two: every topic a schema button follows has its run wrapped in watched() and nothing else writes a terminal line; and no file in backend/ or frontend/ recomputes free space as total - used.

Not what you asked for on the first one: /s/roots with a stubbed exec would need to import ui.ts, which pulls in @opsdeck/sdk and hono and therefore cannot run in a bare clone — the property this repo's test setup buys. The seam I tested instead is UsageCache/usageOf, which both surfaces go through, plus the source assertion that neither surface does the arithmetic itself. If you would rather have the route tested for real, that is a test harness with a fake ModuleContext and it should land as its own change.

deno check clean in a core checkout. deno fmt --check I left alone — it reports 23 of 40 files, including ones neither of us touched, so a fmt pass belongs in its own commit if you want the repo clean.

All seven addressed, in `dd04785` + `5a95471`. You were right that 1 is the same bug this branch exists to fix — publishing the terminal line at the one call site that already worked is exactly the shape of the original gap, so it is now owned by a wrapper rather than by remembering. **1. Terminal line on every exit.** `backend/progress.ts` is new: `watched(publish, signal, body, done)` runs a run body and publishes `done · …` or `error · <reason>` whichever way it leaves. Both handlers go through it, so `resolve`, `assertWritable` and `Deno.remove` on delete, and `unknown-root`, `unreachable-path` and a failing `store.save` on scan, all end the stream. The cancel ordering falls out of the same change: the `done` line is built from the value the body *returns*, so it is published after `if (h.signal.aborted) throw`, not before it. A cancelled scan no longer announces completion. One decision inside the wrapper worth your eye. The line carries the **declared reason** — `error · unknown-root`, `error · delete-failed`, `error · cancelled` — and never the error message; anything unnamed is `error · failed`. Run fields are secret per field and default to withheld, while a topic is gated only by a role and has no projection, so `Deno.remove`'s message would put a host path in front of every subscriber that `expose` exists to keep it from. A cancel is decided by the signal rather than by the exception type, the way core decides it (`cancelRequested`). **2. Lint.** Unused `FsUsage` import gone; `deno lint` is clean. **4. `delete-progress` follows the write role.** `readRole === "admin"` still gates `scan-progress`; `writeRole === "admin"` gates `delete-progress`. `viewer` + `WRITE_ROLE=admin` no longer watches deletions it cannot start, and since the write role never resolves below the read role, that gate is never looser. **5. `availBytes`.** `FsUsage` carries df's Available (column 4), and `usageOf` falls back to `total - used` only when the column is unparseable. The schema page's `freeBytes`, the listing bar and the overview widget all read it, so the ~1.6 GB the ext4 reserve was being counted as writable is gone from all three at once. **6. `UsageCache` is bounded and coalesces.** 64 entries, dropping the least recently refreshed (a re-insert moves a path back to the young end, so browsing a deep tree cannot push the roots out for long), and an in-flight map so N concurrent cold reads of one path spawn one df. `5a95471` takes the other nit: `/s/roots` asks its roots concurrently instead of paying the sum of every mount, order preserved. **3. Documented rather than fixed.** `progressTopic` is a fixed string in the schema with nowhere to put a run id, so the topic belongs to the action and not to the run — a scheduled scan of `backups` really does end the stream a phone was watching for `media`, and the second of two queued deletes does it to the first. README now says that under platform limits, including that the run itself is always exact (Activity, the web UI and the `runId` the route answers with all follow one run), so the fix is core growing a per-run topic rather than anything here. **7. Tests — 51 pass** (`deno test --allow-read --allow-write --allow-run=python3 backend/`), up from 38. - `progress_test.ts`: every exit of `watched` — done after the body, error-and-rethrow, an OS message *not* reaching the topic, a cancel saying `cancelled` whatever it threw, and the reason mapping. - `usage_test.ts`: Available vs `total - used` (asserting the reserve as a number), the fallback, the three ways df yields nothing, one df for three concurrent cold reads, ttl expiry, and the eviction bound. - `contract_test.ts` gains two: every topic a schema button follows has its run wrapped in `watched()` and nothing else writes a terminal line; and no file in `backend/` or `frontend/` recomputes free space as `total - used`. Not what you asked for on the first one: `/s/roots` with a stubbed exec would need to import `ui.ts`, which pulls in `@opsdeck/sdk` and hono and therefore cannot run in a bare clone — the property this repo's test setup buys. The seam I tested instead is `UsageCache`/`usageOf`, which both surfaces go through, plus the source assertion that neither surface does the arithmetic itself. If you would rather have the route tested for real, that is a test harness with a fake `ModuleContext` and it should land as its own change. `deno check` clean in a core checkout. `deno fmt --check` I left alone — it reports 23 of 40 files, including ones neither of us touched, so a fmt pass belongs in its own commit if you want the repo clean.
julian requested changes 2026-08-15 21:45:54 +02:00
Dismissed
julian left a comment
Copy link

All seven from the first pass verified fixed at 5a95471, and verified locally: deno lint clean, the suite passes (44 here on Windows, with the 7 failures being the Unix-only symlink/python tests — consistent with your 51 on Linux), deno check clean in a core checkout. The details that mattered all hold up under a close read: watched() covers every exit including the cancel ordering, terminalReason keeps host paths off the topic by construction, the write-role gate is correct — and since Role is binary, the narrowing in config.ts really does guarantee the write role never resolves below the read role — availBytes is on every surface with the contract test to keep it there, and the cache is bounded, coalescing, and cleans up its in-flight entry on rejection.

Requesting changes on one small thing, because it is the last one:

1. backend/progress_test.ts is not formatted. It is one of the 23 deno fmt --check failures, so "including ones neither of us touched" quietly counts a file this PR adds. Two reflows, lines 57 and 82. deno fmt backend/progress_test.ts and this is done — and since you already reflowed README, the repo-wide pass you suggested can stay a separate commit; I'll take it.

Noted, not yours to fix here: svelte-check against core main fails with 28 errors, all pre-existing — the frontend imports @opsdeck/ui components (AccessDenied, ActionProgress, Breadcrumbs, DropZone, LinkButton, SplitButton, Select, CompositionBar) that core main does not export, while CLAUDE.md says they already moved. Your two frontend lines typecheck fine (availBytes is in the listing type). That is a core release gap; I'll track it on the core side.

Fix the formatting and I merge.

All seven from the first pass verified fixed at 5a95471, and verified locally: `deno lint` clean, the suite passes (44 here on Windows, with the 7 failures being the Unix-only symlink/python tests — consistent with your 51 on Linux), `deno check` clean in a core checkout. The details that mattered all hold up under a close read: `watched()` covers every exit including the cancel ordering, `terminalReason` keeps host paths off the topic by construction, the write-role gate is correct — and since `Role` is binary, the narrowing in config.ts really does guarantee the write role never resolves below the read role — `availBytes` is on every surface with the contract test to keep it there, and the cache is bounded, coalescing, and cleans up its in-flight entry on rejection. Requesting changes on one small thing, because it is the last one: **1. `backend/progress_test.ts` is not formatted.** It is one of the 23 `deno fmt --check` failures, so "including ones neither of us touched" quietly counts a file this PR adds. Two reflows, lines 57 and 82. `deno fmt backend/progress_test.ts` and this is done — and since you already reflowed README, the repo-wide pass you suggested can stay a separate commit; I'll take it. Noted, not yours to fix here: svelte-check against core main fails with 28 errors, all pre-existing — the frontend imports `@opsdeck/ui` components (`AccessDenied`, `ActionProgress`, `Breadcrumbs`, `DropZone`, `LinkButton`, `SplitButton`, `Select`, `CompositionBar`) that core main does not export, while CLAUDE.md says they already moved. Your two frontend lines typecheck fine (`availBytes` is in the listing type). That is a core release gap; I'll track it on the core side. Fix the formatting and I merge.
backend/progress_test.ts Outdated
@ -0,0 +54,4 @@
running,
() =>
Promise.reject(
new Error("Permission denied (os error 13): remove '/host/root/srv/x'"),
julian commented 2026-08-15 21:45:54 +02:00
Owner
Copy link

deno fmt --check reflows this line (and line 82): a new file should land formatted. deno fmt backend/progress_test.ts.

`deno fmt --check` reflows this line (and line 82): a new file should land formatted. `deno fmt backend/progress_test.ts`.
`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.
thisilike commented 2026-08-23 20:50:20 +02:00
Author
Owner
Copy link

Formatted, in f3fad8b — deno fmt backend/progress_test.ts, the two reflows on lines 57 and 82 and nothing else. deno fmt --check backend/progress_test.ts is clean.

You are right that the repo-wide backlog was not an excuse for a file this PR adds; that is the whole point of the check. The other 22 are yours if you want them, and the branch does not touch them.

Re-verified at f3fad8b: deno lint clean (32 files), 51 tests pass (deno test --allow-read --allow-write --allow-run=python3 backend/).

Thanks for the note on svelte-check — agreed that is a core release gap rather than anything this branch can fix, and the eight components it names are the ones CLAUDE.md already claims moved. I'll leave it on your core-side tracking.

Formatted, in `f3fad8b` — `deno fmt backend/progress_test.ts`, the two reflows on lines 57 and 82 and nothing else. `deno fmt --check backend/progress_test.ts` is clean. You are right that the repo-wide backlog was not an excuse for a file this PR adds; that is the whole point of the check. The other 22 are yours if you want them, and the branch does not touch them. Re-verified at `f3fad8b`: `deno lint` clean (32 files), 51 tests pass (`deno test --allow-read --allow-write --allow-run=python3 backend/`). Thanks for the note on svelte-check — agreed that is a core release gap rather than anything this branch can fix, and the eight components it names are the ones CLAUDE.md already claims moved. I'll leave it on your core-side tracking.
julian approved these changes 2026-08-23 22:03:52 +02:00
julian left a comment
Copy link

Verified at f3fad8b: the commit touches only backend/progress_test.ts (the two reflows, lines 57 and 82, nothing else), deno fmt --check is clean on all three files this PR adds, deno lint clean (32 files), and the suite gives 44 passed / 7 failed here on Windows — the same seven Unix-only symlink/python tests as at 5a95471. The repo-wide fmt backlog is down to 22, all pre-existing; I'll take that pass as offered.

I also re-checked the terminal-line contract against the actual consumers: both the shell's ActionsBlock and the Android SchemaViewModel end the stream on a case-insensitive startsWith("done")/startsWith("error"), and none of the intermediate lines (removing …, removed …, N entries · path) can collide with those prefixes.

Approving. Two small things surfaced in this pass — neither blocks, but I'd like both addressed in a future change rather than forgotten:

  1. Web /roots still asks its roots serially (backend/routes.ts:106-120) — stat plus a possible df per root, one after the other. 5a95471 parallelised /s/roots for exactly this reason; the web route deserves the same Promise.all shape, or one shared helper so the two cannot drift again.

  2. The cancel comment in progress.ts overstates the symmetry with core. Core checks instanceof TaskFailure before cancelRequested (packages/server/src/tasks/service.ts), so a TaskFailure thrown while a cancel is pending records failed/<reason> in the run while the topic says error · cancelled. Harmless for a phone — the stream ends either way — but either reorder terminalReason to prefer the declared reason, or trim the "core judges it the same way" claim to what is actually true.

File them as follow-ups or fold them into the next branch, whichever you prefer. Thanks for the quick turnaround on the formatting.

Verified at f3fad8b: the commit touches only `backend/progress_test.ts` (the two reflows, lines 57 and 82, nothing else), `deno fmt --check` is clean on all three files this PR adds, `deno lint` clean (32 files), and the suite gives 44 passed / 7 failed here on Windows — the same seven Unix-only symlink/python tests as at 5a95471. The repo-wide fmt backlog is down to 22, all pre-existing; I'll take that pass as offered. I also re-checked the terminal-line contract against the actual consumers: both the shell's `ActionsBlock` and the Android `SchemaViewModel` end the stream on a case-insensitive `startsWith("done")`/`startsWith("error")`, and none of the intermediate lines (`removing …`, `removed …`, `N entries · path`) can collide with those prefixes. Approving. Two small things surfaced in this pass — neither blocks, but I'd like both addressed in a future change rather than forgotten: 1. **Web `/roots` still asks its roots serially** (`backend/routes.ts:106-120`) — stat plus a possible df per root, one after the other. 5a95471 parallelised `/s/roots` for exactly this reason; the web route deserves the same `Promise.all` shape, or one shared helper so the two cannot drift again. 2. **The cancel comment in `progress.ts` overstates the symmetry with core.** Core checks `instanceof TaskFailure` *before* `cancelRequested` (`packages/server/src/tasks/service.ts`), so a TaskFailure thrown while a cancel is pending records `failed/<reason>` in the run while the topic says `error · cancelled`. Harmless for a phone — the stream ends either way — but either reorder `terminalReason` to prefer the declared reason, or trim the "core judges it the same way" claim to what is actually true. File them as follow-ups or fold them into the next branch, whichever you prefer. Thanks for the quick turnaround on the formatting.
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!2
No description provided.