Host-owned task runs, and the docker module moved onto them #21

Merged
julian merged 12 commits from feat/core-tasks into main 2026-08-11 10:01:43 +02:00
Owner

Anything that takes long enough to watch — an update, a pin, a rebuild — becomes an action rather than an async function behind a route. A module declares the handler; core owns the row, the ordering and the clock. That split is what lets the scheduler fire an action from a process that restarted twice since the schedule was written: the row holds (module, action, args), never a closure.

⚠ Before merging

OPSDECK_MODULES on the server must gain activity, or the Activity view disappears on the next deploy. Activity is an ordinary built-in now, enabled and disabled like any other module — docker/docker-compose.yml here shows the expected list.

The commits

  • 080cd6b core task system — runs, gates, schedules, expectations, the retained live protocol, per-field secrecy.
  • d0ce373 Activity is a module — core owns the mechanics and runs with nothing observing them; the view reads through ctx.tasks.observe.
  • 7a9b745 docker routes become actions — six actions, JobRegistry and /jobs deleted, scoped post-action refresh, classified compose failures.
  • 0c5e769 Activity views and reusable componentsStepList, ProgressList, GateList in @opsdeck/ui, typed structurally so a module can use them for its own staged work.
  • 724293b browser typechecking, CI, serving fixes.
  • 480abd2 merge of main after #10 landed — see below.
  • a344cdb a released run keeps its lock until the work returns.
  • a6bfb3d tests pinning the boundaries the merge moved.

Decisions worth reviewing

  • onSelfConflict is required. A schedule firing over its own previous run has no safe default. drop still writes a skipped row — a silently discarded click is how you get bug reports saying nothing happened.
  • Gates are evaluated against the table, anchored by a persisted gate_anchor_ms. The event-driven version is the one that comes naturally and it is wrong: a completion that happened while the process was down has to count exactly once.
  • interrupted is terminal. One process, no workers, and a compose child dies with its parent — a system claiming a run will happen anyway is lying.
  • Secrecy is per field and defaults to withheld. A whole-run boolean had already leaked once, when step errors and checkpoints were added and nobody updated the single place that decided what was secret.
  • One authorised way in. startAsUser holds the console opt-in and the role check. Stated three times, the copies had drifted.

Merging #10 was not a formality

Main's ten new commits included seven fixing code this branch had moved elsewhere, so git could not map the hunks — a textual resolution would have kept the new structure and silently dropped the fixes. Each was located and ported by hand: the hostPath normalisation (".." false-positives on env_file: ../shared/.env), makeCommitLines(ctx, hostPath), service on edits/patches, the stale-copy read, ALREADY continuing rather than breaking, and patchDir — that last one is why git apply --cached used to reject, rewriting files and committing nothing.

checkUpdates carries previous verdicts forward, kept alongside this branch's scoped prune.

CI: main's version won. It moved verify onto .forgejo/deno.sh and gated it same-repository — the opposite of the trade this branch had made, and main's call. Its job is taken whole; the Svelte check survives because that single step runs deno task ci, which here includes it.

StacksPage.svelte needed real reconciliation rather than a side: the merge left projectBusy referenced and undeclared and answering read above its declaration. Found by check:svelte, which main does not run and this branch does.

One regression found and fixed

Force release freed the target the instant it marked a run interrupted, while nothing had actually stopped — ExecOptions carries no signal and hostexec handles none, so the compose child runs on. Reproduced against a real engine: release a stop, start a start on the same project, and the start reported succeeded while the released stop was still running, then killed the containers it had just brought up. Both runs claimed to have worked.

Main closed the same hole for the old bridge in 665bea2. a344cdb closes it here: the run still goes terminal immediately, but the claim outlives it until the module's handler actually returns. Verified after the fix — the queued run reports waiting for run #5 for thirty seconds, then runs, and both containers end Up.

The underlying cause is still open: no host command is abortable, so release can only stop waiting, never stop work. Making interrupted true means threading a signal through the SDK's exec contract — a larger, separate change.

Verification

379 tests. The CI job run verbatim via .forgejo/deno.sh task ci: 375 passed, 4 ignored (main's git-dependent tests, which the pinned Deno image cannot run — worth knowing, since those are the ones covering the git apply --cached fix).

Exercised against a real docker engine in docker/dev-rig, the only place the chroot /host/root compose path is reachable: compose update via chroot, scoped refresh leaving other stacks intact across stop/start, pin rollback on a bad tag (git status clean afterwards — byte-for-byte restore) and a real commit on a good one, registry-denied and compose-rejected failures classified, interrupted via Force release from the browser, and per-field projection over HTTP with real OIDC sessions — an admin sees args, lines, reasonData and step errors; a viewer sees the reason slug and nothing behind it.

Known gaps

  • After a force release the UI's projectBusy reads free while the server still holds the claim. Not dangerous — a new run queues behind it rather than colliding — but the button disagrees with the server until claims are surfaced to clients.
  • The engine-API updater's recreate branch is covered by a fake engine rather than live: the pull it performs first reconciles any locally moved tag back to what the registry serves, so a real daemon never reaches it.
  • docker/dev-rig is developer tooling and ships in the repo, not the image.
Anything that takes long enough to watch — an update, a pin, a rebuild — becomes an **action** rather than an async function behind a route. A module declares the handler; core owns the row, the ordering and the clock. That split is what lets the scheduler fire an action from a process that restarted twice since the schedule was written: the row holds `(module, action, args)`, never a closure. ## ⚠ Before merging `OPSDECK_MODULES` on the server must gain `activity`, or the Activity view disappears on the next deploy. Activity is an ordinary built-in now, enabled and disabled like any other module — `docker/docker-compose.yml` here shows the expected list. ## The commits - **`080cd6b` core task system** — runs, gates, schedules, expectations, the retained live protocol, per-field secrecy. - **`d0ce373` Activity is a module** — core owns the mechanics and runs with nothing observing them; the view reads through `ctx.tasks.observe`. - **`7a9b745` docker routes become actions** — six actions, `JobRegistry` and `/jobs` deleted, scoped post-action refresh, classified compose failures. - **`0c5e769` Activity views and reusable components** — `StepList`, `ProgressList`, `GateList` in `@opsdeck/ui`, typed structurally so a module can use them for its own staged work. - **`724293b` browser typechecking, CI, serving fixes.** - **`480abd2` merge of main** after #10 landed — see below. - **`a344cdb` a released run keeps its lock** until the work returns. - **`a6bfb3d`** tests pinning the boundaries the merge moved. ## Decisions worth reviewing - **`onSelfConflict` is required.** A schedule firing over its own previous run has no safe default. `drop` still writes a `skipped` row — a silently discarded click is how you get bug reports saying nothing happened. - **Gates are evaluated against the table**, anchored by a persisted `gate_anchor_ms`. The event-driven version is the one that comes naturally and it is wrong: a completion that happened while the process was down has to count exactly once. - **`interrupted` is terminal.** One process, no workers, and a compose child dies with its parent — a system claiming a run will happen anyway is lying. - **Secrecy is per field and defaults to withheld.** A whole-run boolean had already leaked once, when step errors and checkpoints were added and nobody updated the single place that decided what was secret. - **One authorised way in.** `startAsUser` holds the console opt-in and the role check. Stated three times, the copies had drifted. ## Merging #10 was not a formality Main's ten new commits included **seven fixing code this branch had moved elsewhere**, so git could not map the hunks — a textual resolution would have kept the new structure and silently dropped the fixes. Each was located and ported by hand: the `hostPath` normalisation (`".."` false-positives on `env_file: ../shared/.env`), `makeCommitLines(ctx, hostPath)`, `service` on edits/patches, the stale-copy read, `ALREADY` continuing rather than breaking, and `patchDir` — that last one is why `git apply --cached` used to reject, rewriting files and committing nothing. `checkUpdates` carries previous verdicts forward, kept alongside this branch's scoped prune. **CI: main's version won.** It moved verify onto `.forgejo/deno.sh` and gated it same-repository — the opposite of the trade this branch had made, and main's call. Its job is taken whole; the Svelte check survives because that single step runs `deno task ci`, which here includes it. `StacksPage.svelte` needed real reconciliation rather than a side: the merge left `projectBusy` referenced and undeclared and `answering` read above its declaration. Found by `check:svelte`, which main does not run and this branch does. ## One regression found and fixed Force release freed the target the instant it marked a run interrupted, while nothing had actually stopped — `ExecOptions` carries no signal and hostexec handles none, so the compose child runs on. Reproduced against a real engine: release a `stop`, start a `start` on the same project, and the start reported **succeeded** while the released stop was still running, then killed the containers it had just brought up. Both runs claimed to have worked. Main closed the same hole for the old bridge in `665bea2`. `a344cdb` closes it here: the run still goes terminal immediately, but the **claim** outlives it until the module's handler actually returns. Verified after the fix — the queued run reports `waiting for run #5` for thirty seconds, then runs, and both containers end `Up`. The underlying cause is still open: no host command is abortable, so `release` can only stop waiting, never stop work. Making `interrupted` true means threading a signal through the SDK's exec contract — a larger, separate change. ## Verification **379 tests.** The CI job run verbatim via `.forgejo/deno.sh task ci`: 375 passed, 4 ignored (main's git-dependent tests, which the pinned Deno image cannot run — worth knowing, since those are the ones covering the `git apply --cached` fix). Exercised against a real docker engine in `docker/dev-rig`, the only place the `chroot /host/root` compose path is reachable: compose update via chroot, scoped refresh leaving other stacks intact across stop/start, pin rollback on a bad tag (`git status` clean afterwards — byte-for-byte restore) and a real commit on a good one, registry-denied and compose-rejected failures classified, `interrupted` via Force release from the browser, and per-field projection over HTTP with real OIDC sessions — an admin sees args, lines, `reasonData` and step errors; a viewer sees the reason slug and nothing behind it. ## Known gaps - After a force release the UI's `projectBusy` reads free while the server still holds the claim. Not dangerous — a new run queues behind it rather than colliding — but the button disagrees with the server until claims are surfaced to clients. - The engine-API updater's recreate branch is covered by a fake engine rather than live: the pull it performs first reconciles any locally moved tag back to what the registry serves, so a real daemon never reaches it. - `docker/dev-rig` is developer tooling and ships in the repo, not the image.
Anything that takes long enough to watch — an update, a pin, a rebuild —
becomes an **action** rather than an async function behind a route. A
module declares the handler; core owns the row, the ordering and the
clock. That split is what lets the scheduler fire an action from a
process that restarted twice since the schedule was written: the row
holds `(module, action, args)`, never a closure.

The decisions that shape it, and why they are not defaults:

- Identity is `module + action + target`, at most one live run per
  triple, and `onSelfConflict` is REQUIRED. A schedule firing over its
  own previous run has no safe default, and `drop` still writes a
  `skipped` row — a silently discarded click is how you get bug reports
  saying nothing happened.
- Ordering and exclusion are one primitive: gates. They are evaluated
  against the TABLE, anchored by a persisted `gate_anchor_ms`. The
  event-driven version is the one that comes naturally and it is wrong,
  because a completion that happened while the process was down has to
  count exactly once.
- `interrupted` is terminal. There is one process and no workers, and a
  `docker compose` child dies with its parent, so a system claiming a run
  will eventually happen anyway is lying. Core records where the run was
  left and stops; whether re-running is safe is the module's judgement,
  expressed as a new run linked `retry_of`.
- Secrecy is per field and defaults to withheld. An action lists what a
  non-admin may see (`expose`), and a run reports what was stripped
  (`withheld`). A whole-run boolean had already leaked once — step errors
  and checkpoints were added and nobody updated the single place that
  decided what was secret — so the cost of forgetting is now an empty
  pane, not a host command line in somebody's browser.
- One authorised way in. `startRun` is the mechanism and trusts its
  caller; everything originating from a browser goes through
  `startAsUser`, so the console opt-in and the role check exist once.
  Stated three times, the copies drifted: the HTTP route accepted an
  `args` body for actions whose entire contract is that a generic client
  cannot compose one.
- A schedule's key is built by `scheduleKey(module, name)` and nowhere
  else. The module-facing API takes the module-local name, so the
  "already prefixed?" guess that produced `docker:docker:updates` cannot
  be written.

Gate evaluation is serialized through one chain: two interleaved
evaluations would both see a free lock, which is exactly the "checked the
mutex and never took it" bug the design exists to make unwriteable.

Live topics `core:tasks` and `core:task:<id>` are retained — a subscriber
gets the current state plus everything after it from one source, so there
is no REST baseline to reconcile and no timer to notice it drifted.
Resumption carries an `epoch`, because a `tseq` only means anything
within the generation that issued it and a restart must not splice new
deltas onto a stale snapshot.
The operations view was welded into core: a UI schema built in
`tasks/ui.ts`, served by the task routes, so core could not run without
the thing that renders it.

Activity is now an ordinary built-in, listed in `OPSDECK_MODULES` like
any other, reading through the `ctx.tasks.observe` capability. Leave it
out of the list and every mechanic keeps working — runs, gates,
schedules, expectations, the retained topics and the REST surface over
them all. That is the test that the split is real, so keep it true.

The capability grants nothing the core HTTP API does not already give the
same user: every read is projected for the requester, and every write
re-checks the capability the projection computed. Reads of schedules and
the enable/disable write are admin-gated inside the capability rather
than by whoever mounts it — a schedule row carries the args it fires
with, and relying on each consumer to remember its own `requireRole` is
the assumption that put an unprojected run on `/jobs`.
Docker declares six actions (stack-update / -recreate / -lifecycle /
-pin, container-update / -pin), all targeted at the compose project so
the identity IS the per-project mutex. `JobRegistry`, the `/jobs` routes
and the `docker:update` topic are gone: routes answer `{ runId }` and the
frontend follows `core:task:<id>`. Structured failures carry the data the
pin routes map back to HTTP.

The actions live in `actions.ts` behind an explicit `ActionDeps` rather
than inline in `register()`. Closing over module state is a dependency,
not a reason to inline; `mod.ts` drops from 1863 lines to ~1285, and the
rules for touching the operator's files (`hostPath`, `BAD_TAG`) become a
module of their own instead of a local of whichever function needed them
first.

Post-action refresh is scoped to the project the action touched.
`listStacks` filters engine-side and the scoped read merges in place —
in place because `/stacks` serves that array in order and the change
signature hashes it in order, so appending would reshuffle the UI and
make every action look like a change. Update verdicts are pruned to
images something still runs, which a full sweep got for free by replacing
the map: merging kept the pre-pin tag around, still flagged outdated, and
the header renders that count.

`docker compose exited 1` was the whole of a compose failure, which is
the least useful true statement available — and since a compose action
exposes only `entities`, a non-admin's log is withheld and the exit code
was all they got. Failures are classified from compose's own
`{"error":true,"message":…}` frames into registry-denied,
image-not-found, registry-unreachable and disk-full, because the fix
differs for each. Every pattern was captured from a real daemon in the
dev rig rather than written from memory.
The shell gets the Activity console, the archive, the run detail and a
live run feed, all on the retained `core:tasks` subscription — the
subscription delivers current state and then only what changed, so there
is no baseline to keep in sync.

`StepList`, `ProgressList` and `GateList` live in `@opsdeck/ui`, where
modules already import their components from, and are typed
STRUCTURALLY rather than against the task contract: a module doing its
own staged work gets the same vocabulary without its data having to be a
core task run. That independence would drift unnoticed, so
`shell/src/tasks/contract.ts` asserts the SDK's `RunStep`,
`TaskProgress` and `Gate` stay assignable. It compiles to nothing and
caught a missing `pending` state the first time it ran.

Gates were `JSON.stringify(run.gates)`, which is not a rendering — the
reader of a queued run wants to know what it is waiting FOR, and
`[{"lock":"project:web"}]` makes them parse a data structure to find out.
They now read as sentences, past tense once the run is over, with what
they name turned into links: another run, another module's action. A
gate may legitimately name an action nothing registers — that is why
`whenMissing` must be declared — so that case is struck through and
labelled rather than offered as a link into nothing. A stored gate keeps
the raw form its module wrote, so `$same` and an omitted module are
resolved against the holding run; `undefined` (not stated) and `null` (no
target) stay distinct, because collapsing them renders a scoped gate as
an unscoped one.

Status carries its tone in the lists rather than rendering as grey text
next to a coloured dot, derived from one table — a second one is how
"interrupted" ended up amber in one list and orange in another.

`coreFetch` stamps the CSRF header on any unsafe method. Leaving it to
each call site meant the Activity page's start, cancel, retry and release
buttons all answered "missing CSRF header".
Nothing type-checked browser-side TypeScript. `deno check` stops at the
first `.svelte` import — a component is an opaque wildcard module to it —
so `check:frontend` lists files individually instead of following entry
points, and `check:svelte` is added as the only thing that reads a
template. That distinction is not cosmetic: an identifier the markup
calls and nobody declares type-checks, bundles, and then fails on click.
Turning it on found 120 errors and two real bugs.

A file nothing imports is not checked either, which is how a
compile-time assertion becomes decoration — `contract.ts` is named
explicitly for that reason. CI gains a Svelte step, because the workflow
runs `deno lint`, not `deno task lint`, so the template check ran
nowhere.

`?url` imports need an explicit `@ts-types`: Deno 2.5.6, which CI and the
Dockerfile pin, resolves the literal subpath against the package's
exports map and dies, while 2.9.x lets it through. That class of failure
only ever appears in the image.

Two serving bugs, both found by loading a page rather than reasoning
about one:

`.wasm` was never in the MIME table, so tree-sitter was served as
octet-stream. We send nosniff, so the browser refused
`WebAssembly.compileStreaming` and logged two errors per page before
silently retrying the slow path — highlighting still worked, which is why
it survived since the initial implementation.

`/assets/*` is content-hashed and immutable; the shared singleton bundles
are not, and were served with a stable URL and max-age. A deploy hands a
returning browser a fresh index.html pointing at a NEW shell bundle,
which imports the singletons from a URL its cache still answers with the
old build — and the first missing export fails the whole module graph, so
it is a blank page rather than a degraded one. The import map now carries
a `?v=` token derived from the bundles' own bytes.

Activity must be listed in OPSDECK_MODULES; the shipped compose file
says so.
main gained the dependency-hardening work (#11) while this branch was
out: actions and base images pinned by digest, a weekly checker that
watches those pins, and `tools/dep-check` behind it.

Two resolutions:

deno.json — main added `tools/dep-check/main.ts` to `check`, this branch
split `check` into backend and frontend. Keeping only one side would
either stop checking the new tool or lose the browser-side check
entirely, so the tool joins `check:backend`. main's `start` task was the
older form without the host-mount write permissions; this branch's is the
evolution of it.

README.md — main deliberately dropped a `deno fmt` reflow of unrelated
docs (4533047) and this branch carries a repo-wide format, so the textual
merge landed on something `deno fmt --check` rejects. Reformatted.

Verified on the merged tree rather than assumed: `deno install --frozen`
is satisfied, and fmt, lint, check, check:svelte and 354 tests pass — 305
from this branch plus main's 49 for the new tool.
The verify job was written before main pinned anything and has never
run — this branch was never pushed. Merging it as-is would have put an
unpinned `actions/checkout@v6` and an undigested toolchain image into the
workflow that ends by SSHing to production with DEPLOY_SSH_KEY in scope,
which is the exact hole 06cd02f closed.

Both are pinned now, the image to the same digest docker/Dockerfile's
build stage uses.

I did NOT move the job onto .forgejo/deno.sh, which is how deps.yml
solves the same problem. deno.sh drives the HOST docker daemon — host
root, on the runner that holds PACKAGE_TOKEN and DEPLOY_SSH_KEY — which
is why deps.yml restricts itself to same-repository pull requests.
Verification is the one job that has to run on a FORK's pull request,
this repository being public with open registration, and a container job
touches no daemon. So it keeps its container and pays for it with a
second declaration of the toolchain.

A pin nothing watches goes stale in silence, which is the objection
deno.sh exists to answer. tools/ci_pins_test.ts answers it differently:
the Dockerfile stays the one declaration dep-check watches, and the test
fails the moment the workflow disagrees with it — at the moment someone
is already looking at exactly this. It also refuses any action referenced
by a movable tag, in either workflow.

Confirmed by mutation: bumping the workflow's tag or unpinning checkout
fails the tests rather than passing quietly. The job itself was run
end to end in the digest-pinned image — fmt, lint, check, check:svelte
and 357 tests.
main took the docker overhaul (#10) and, with it, ten commits that had
not reached this branch — seven of them fixing the very code this branch
had moved somewhere else. A textual resolution would have kept my
structure and silently dropped their fixes, so each was located in the
new arrangement and ported by hand.

Into hostpath.ts: the ".." substring test false-positives on
`env_file: ../shared/.env`, which resolveEnvPath turns into a perfectly
legal absolute path. Normalise, then refuse only what escapes the root.

Into actions.ts, all five from the pin actions:
  - makeCommitLines takes hostPath, and edits/patches carry the service
    the change belongs to
  - a file we have already written must be re-readable; swallowing that
    error means reasoning from a stale copy
  - ALREADY continues instead of breaking, because compose merges left to
    right and a base that already reads correctly says nothing about an
    override still pinning the old tag
  - the commit is probed from a file we actually PATCHED, not from
    workingDir — a wrong root made `git apply --cached` reject paths it
    could not resolve, so files were rewritten and nothing was committed

Into mod.ts: checkUpdates carries the previous verdicts forward, or an
image the budget ran out on downgrades from "update available" to
unknown and drops out of the count. Kept alongside this branch's scoped
prune.

CI: main moved verify onto .forgejo/deno.sh and gated it to
same-repository pull requests. That is the opposite of the trade this
branch made, and it is main's call — its version is taken whole. The
Svelte check survives because main's single step runs `deno task ci`,
and this branch's `ci` task includes it. Two of the three pin tests in
tools/ci_pins_test.ts guarded a second toolchain declaration that no
longer exists; they are gone, the action-pinning one stays.

.mcp.json goes with main's removal: per-user, and it hardcodes a local
token path.

StacksPage.svelte needed real reconciliation rather than a side: the
merge left `projectBusy` referenced and undeclared, and `answering` read
above its declaration. Both are now fixed — found by check:svelte, which
main does not run and this branch does.

up.sh gains RIG_PORT. 8080 is the most contested port on a dev machine
and the rig refusing to start beside whatever holds it is a papercut.

Verified: 368 tests, including main's commit_test.ts driving a real
repository. In the rig against a real engine — a pin that rewrites,
validates and actually COMMITS (the patchDir fix), and a compose-rejected
pin whose rollback leaves `git status` empty.
Force release freed the target the instant it marked a run interrupted,
and nothing had actually stopped. Two facts made that inevitable:
`lockHolder` counts a run only while its status is running or cancelling,
and `state.abort()` reaches the handler's own await but not the command
underneath — `ExecOptions` carries no signal and hostexec handles none, so
the `docker compose` child runs to completion regardless.

Reproduced in the rig against a real engine, on a stack whose containers
ignore SIGTERM: release a `stop`, start a `start` on the same project, and
the start reported SUCCEEDED while the released stop was still running —
then killed the containers it had just brought up. Both runs claimed to
have worked; the stack ended `Exited (137)`.

main closed the same hole for the old bridge in 665bea2, by splitting
release in two: the default stops clients following while the project
stays claimed. This branch replaced that bridge with the task system and
brought the hole back with it.

The run still goes terminal immediately — that is honest about what core
knows — but the CLAIM outlives it. A run finished while its handler has
not returned moves into #claims, which lockHolder and isBusy read exactly
as they read a live run; #execute's finally drops the claim when the
module's own code returns and sweeps, so whoever was queued starts then.
No new user-visible state, and "interrupted is terminal" still holds.

Verified after the fix, same scenario: the queued run reports
`waiting for run #5` for thirty seconds, then runs, and both containers
end Up. The test fails without the claim handoff and passes with it.

The confirm dialog said the work may still be running but not that the
target stays claimed, which is the half an operator plans around.
test: pin the boundaries the merge moved
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m13s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
a6bfb3dd18
Reviewing the merge-fitting changes rather than trusting them, and
testing what was not obvious.

hostpath.ts had no test on either side. It decides what a pin may write,
it arrived here by hand out of main's inline version, and the rule it
enforces changed while this branch was away — a substring ".." test
became real normalisation. Six cases now cover it: relative refused,
".." inside an absolute path normalised (the `env_file: ../shared/.env`
that forced the change), escapes still refused however they are spelled,
and a bare-metal "/" host root not producing "//srv/...". Writing them
caught my own arithmetic, not a defect: `web/../shared` pops `web`.

The claim added in a344cdb is new core state, so its edges are pinned
rather than argued: a claim drops when the orphaned work THROWS as well
as when it returns — the release lives in a finally, and a project locked
for the life of the process because its compose run errored would be a
worse bug than the one being fixed. Releasing a run that never started
takes no claim, since there is no handler to outlive it. And a claim
keeps the target busy, which is what docker reads to suppress "container
stopped unexpectedly" while something deliberate is still moving it.

Checked and found sound, no change needed: all eleven hunks of main's
mod.ts diff are accounted for; `previous` in checkUpdates only ever
carries images that are in `inUse`, so it cannot collide with the scoped
prune; and StacksPage's `loadJobs` single-flight fix is moot here because
this branch subscribes rather than polls — no leftover polling remains.
julian requested review from julian 2026-08-10 23:13:06 +02:00
julian requested changes 2026-08-10 23:23:06 +02:00
Dismissed
julian left a comment

Reviewed across the server task system, SDK + docker backend, frontend, and tests/infra. Every claim in the PR description was checked against the code and held up — secrecy projection, force-release claim lifetime, gate anchoring, host-exec escaping, CI wiring, the conditional test ignores. Solid work.

Two small items before merge; neither is behavioral.

Reviewed across the server task system, SDK + docker backend, frontend, and tests/infra. Every claim in the PR description was checked against the code and held up — secrecy projection, force-release claim lifetime, gate anchoring, host-exec escaping, CI wiring, the conditional test ignores. Solid work. Two small items before merge; neither is behavioral.
@ -389,3 +340,1 @@
const job = new JobFollow(feed, {
wants: () => true,
onFinish: () => {
// this modal follows whichever run it was pointed at, stack-wide or not
Owner

Duplicate comment line — merge artifact. Delete one of the two identical lines.

Duplicate comment line — merge artifact. Delete one of the two identical lines.
@ -0,0 +306,4 @@
`SELECT ${RUN_COLUMNS} FROM task_runs
${sql}
ORDER BY COALESCE(finished_at_ms, updated_at_ms) DESC, id DESC
LIMIT ${limit} OFFSET ${offset}`,
Owner

LIMIT ${limit} OFFSET ${offset} is interpolated into the SQL string. It's safe today because clampInt forces both to bounded integers first (and the comment above documents that), but the safety lives two lines away from the interpolation — anyone who later adds a caller or loosens the clamp breaks it silently. Consider binding them as parameters (LIMIT ? OFFSET ?) so the invariant is enforced by the driver rather than by proximity.

`LIMIT ${limit} OFFSET ${offset}` is interpolated into the SQL string. It's safe today because `clampInt` forces both to bounded integers first (and the comment above documents that), but the safety lives two lines away from the interpolation — anyone who later adds a caller or loosens the clamp breaks it silently. Consider binding them as parameters (`LIMIT ? OFFSET ?`) so the invariant is enforced by the driver rather than by proximity.
fix(tasks): bind the page bounds instead of interpolating them
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m10s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m12s
51129fea4a
Both from Julian's review of #21; neither is behavioural.

`LIMIT ${limit} OFFSET ${offset}` was safe only because clampInt had
forced both to bounded integers two lines earlier. The invariant lived in
the proximity of the two statements rather than in anything that enforces
it, so a later caller — or a loosened clamp — would break it silently.
They are bound as parameters now, which moves the guarantee to the
driver. The clamp stays: it bounds the page size, which is a different
job from keeping the query safe.

`recent()` had the same shape with `LIMIT ${Math.max(limit, 1)}` and gets
the same treatment; nothing interpolates a limit any more.

Checked rather than assumed: DuckDB does bind LIMIT and OFFSET, including
mixed with the WHERE parameters this query already passes. The existing
"the live view is bounded; history answers the rest" test exercises
limit 2 / offset 2 and asserts the pages do not overlap, so the change is
covered by a test that was already there.

Also drops a duplicated comment line in StacksPage.svelte, a merge
artifact from resolving that file by hand.
Author
Owner

Both addressed in 51129fe.

Page bounds — agreed, and the framing was the useful part: the invariant lived in the proximity of two statements rather than in anything enforcing it. LIMIT/OFFSET are bound as parameters now, so the driver holds the guarantee. clampInt stays, because bounding the page size is a different job from keeping the query safe.

recent() had the same shape (LIMIT ${Math.max(limit, 1)}) and got the same treatment — nothing interpolates a limit any more, which is greppable as LIMIT \${.

Two things I checked rather than assumed before changing working SQL: DuckDB does bind LIMIT/OFFSET, including mixed with the WHERE parameters this query already passes; and the existing "the live view is bounded; history answers the rest" test exercises limit: 2 / offset: 2 and asserts the pages do not overlap, so the change is covered by a test that was already there.

Duplicate comment line — merge artifact from resolving StacksPage.svelte by hand. Removed.

.forgejo/deno.sh task ci run verbatim before pushing: 375 passed, 0 failed, 4 ignored.

Re-review when you have a moment.

Both addressed in 51129fe. **Page bounds** — agreed, and the framing was the useful part: the invariant lived in the proximity of two statements rather than in anything enforcing it. `LIMIT`/`OFFSET` are bound as parameters now, so the driver holds the guarantee. `clampInt` stays, because bounding the page size is a different job from keeping the query safe. `recent()` had the same shape (`LIMIT ${Math.max(limit, 1)}`) and got the same treatment — nothing interpolates a limit any more, which is greppable as `LIMIT \${`. Two things I checked rather than assumed before changing working SQL: DuckDB does bind `LIMIT`/`OFFSET`, including mixed with the `WHERE` parameters this query already passes; and the existing *"the live view is bounded; history answers the rest"* test exercises `limit: 2` / `offset: 2` and asserts the pages do not overlap, so the change is covered by a test that was already there. **Duplicate comment line** — merge artifact from resolving `StacksPage.svelte` by hand. Removed. `.forgejo/deno.sh task ci` run verbatim before pushing: 375 passed, 0 failed, 4 ignored. Re-review when you have a moment.
julian approved these changes 2026-08-11 09:57:44 +02:00
julian left a comment

Re-reviewed 51129fe. Both items are addressed properly and nothing else in the branch moved.

Page bounds. query() now passes [...values, limit, offset] against LIMIT ? OFFSET ?, and recent() passes [sinceMs, Math.max(limit, 1)] — positional order matches clause order in both, and git grep -E 'LIMIT \$\{|OFFSET \$\{' over the branch returns nothing, so the pattern is gone rather than moved. Keeping clampInt is the right call and the new comment says why. I checked the binding claim against the real driver rather than taking it on the test's word: @duckdb/node-api@1.5.4-r.1 binds LIMIT/OFFSET mixed with WHERE parameters (WHERE id >= ? ... LIMIT ? OFFSET ? with [1, 2, 2] returns the correct page), and an offset of Number.MAX_SAFE_INTEGER — the clamp's ceiling, which no test exercises — returns an empty set instead of erroring. The existing limit: 2 / offset: 2 history test runs against a real DuckDB file, not a fake, so the coverage claim holds too.

Duplicate comment line. Gone.

CI. .forgejo/deno.sh task ci verbatim: 375 passed, 0 failed, 4 ignored — matches what you reported.

One thing worth knowing before you merge, which is not a change request. Main has moved four commits ahead since #10 landed here (#23), and one of them bumps the build stage from denoland/deno:2.5.6 to 2.9.5. Since .forgejo/deno.sh reads the pin out of docker/Dockerfile, the run above exercised 2.5.6 — the merged result would run on 2.9.5, which nothing here had tested. Given this branch's history with main's changes, I did the trial merge locally: it is clean, and .forgejo/deno.sh task ci on the merged tree under 2.9.5 gives the same 375 passed / 0 failed / 4 ignored. Because ci is &&-chained, that also means deno fmt --check, deno lint, check and check:svelte all pass on the merged tree under the new toolchain — including the HTML/CSS/SVG fmt exclusion that main's comment rewrite says is still load-bearing. So the toolchain bump is a non-event for this branch; no action needed beyond merging main in whichever direction you prefer.

The OPSDECK_MODULES note in the description still applies: activity has to be on the server's list before this deploys, or the Activity view disappears.

Approving.

Re-reviewed `51129fe`. Both items are addressed properly and nothing else in the branch moved. **Page bounds.** `query()` now passes `[...values, limit, offset]` against `LIMIT ? OFFSET ?`, and `recent()` passes `[sinceMs, Math.max(limit, 1)]` — positional order matches clause order in both, and `git grep -E 'LIMIT \$\{|OFFSET \$\{'` over the branch returns nothing, so the pattern is gone rather than moved. Keeping `clampInt` is the right call and the new comment says why. I checked the binding claim against the real driver rather than taking it on the test's word: `@duckdb/node-api@1.5.4-r.1` binds `LIMIT`/`OFFSET` mixed with `WHERE` parameters (`WHERE id >= ? ... LIMIT ? OFFSET ?` with `[1, 2, 2]` returns the correct page), and an offset of `Number.MAX_SAFE_INTEGER` — the clamp's ceiling, which no test exercises — returns an empty set instead of erroring. The existing `limit: 2` / `offset: 2` history test runs against a real DuckDB file, not a fake, so the coverage claim holds too. **Duplicate comment line.** Gone. **CI.** `.forgejo/deno.sh task ci` verbatim: 375 passed, 0 failed, 4 ignored — matches what you reported. One thing worth knowing before you merge, which is not a change request. Main has moved four commits ahead since #10 landed here (#23), and one of them bumps the build stage from `denoland/deno:2.5.6` to `2.9.5`. Since `.forgejo/deno.sh` reads the pin out of `docker/Dockerfile`, the run above exercised 2.5.6 — the merged result would run on 2.9.5, which nothing here had tested. Given this branch's history with main's changes, I did the trial merge locally: it is clean, and `.forgejo/deno.sh task ci` on the merged tree under 2.9.5 gives the same 375 passed / 0 failed / 4 ignored. Because `ci` is `&&`-chained, that also means `deno fmt --check`, `deno lint`, `check` and `check:svelte` all pass on the merged tree under the new toolchain — including the HTML/CSS/SVG fmt exclusion that main's comment rewrite says is still load-bearing. So the toolchain bump is a non-event for this branch; no action needed beyond merging main in whichever direction you prefer. The `OPSDECK_MODULES` note in the description still applies: `activity` has to be on the server's list before this deploys, or the Activity view disappears. Approving.
Merge branch 'main' into feat/core-tasks
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m9s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
92ba51dc4f
julian merged commit 34ba658574 into main 2026-08-11 10:01:43 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
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/core!21
No description provided.