Host-owned task runs, and the docker module moved onto them #21
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/core-tasks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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_MODULESon the server must gainactivity, 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.ymlhere shows the expected list.The commits
080cd6bcore task system — runs, gates, schedules, expectations, the retained live protocol, per-field secrecy.d0ce373Activity is a module — core owns the mechanics and runs with nothing observing them; the view reads throughctx.tasks.observe.7a9b745docker routes become actions — six actions,JobRegistryand/jobsdeleted, scoped post-action refresh, classified compose failures.0c5e769Activity views and reusable components —StepList,ProgressList,GateListin@opsdeck/ui, typed structurally so a module can use them for its own staged work.724293bbrowser typechecking, CI, serving fixes.480abd2merge of main after #10 landed — see below.a344cdba released run keeps its lock until the work returns.a6bfb3dtests pinning the boundaries the merge moved.Decisions worth reviewing
onSelfConflictis required. A schedule firing over its own previous run has no safe default.dropstill writes askippedrow — a silently discarded click is how you get bug reports saying nothing happened.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.interruptedis terminal. One process, no workers, and a compose child dies with its parent — a system claiming a run will happen anyway is lying.startAsUserholds 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
hostPathnormalisation (".."false-positives onenv_file: ../shared/.env),makeCommitLines(ctx, hostPath),serviceon edits/patches, the stale-copy read,ALREADYcontinuing rather than breaking, andpatchDir— that last one is whygit apply --cachedused to reject, rewriting files and committing nothing.checkUpdatescarries previous verdicts forward, kept alongside this branch's scoped prune.CI: main's version won. It moved verify onto
.forgejo/deno.shand 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 runsdeno task ci, which here includes it.StacksPage.svelteneeded real reconciliation rather than a side: the merge leftprojectBusyreferenced and undeclared andansweringread above its declaration. Found bycheck: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 —
ExecOptionscarries no signal and hostexec handles none, so the compose child runs on. Reproduced against a real engine: release astop, start astarton 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.a344cdbcloses 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 reportswaiting for run #5for thirty seconds, then runs, and both containers endUp.The underlying cause is still open: no host command is abortable, so
releasecan only stop waiting, never stop work. Makinginterruptedtrue 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 thegit apply --cachedfix).Exercised against a real docker engine in
docker/dev-rig, the only place thechroot /host/rootcompose path is reachable: compose update via chroot, scoped refresh leaving other stacks intact across stop/start, pin rollback on a bad tag (git statusclean afterwards — byte-for-byte restore) and a real commit on a good one, registry-denied and compose-rejected failures classified,interruptedvia Force release from the browser, and per-field projection over HTTP with real OIDC sessions — an admin sees args, lines,reasonDataand step errors; a viewer sees the reason slug and nothing behind it.Known gaps
projectBusyreads 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.docker/dev-rigis developer tooling and ships in the repo, not the image.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".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 notDuplicate 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 DESCLIMIT ${limit} OFFSET ${offset}`,LIMIT ${limit} OFFSET ${offset}is interpolated into the SQL string. It's safe today becauseclampIntforces 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.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/OFFSETare bound as parameters now, so the driver holds the guarantee.clampIntstays, 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 asLIMIT \${.Two things I checked rather than assumed before changing working SQL: DuckDB does bind
LIMIT/OFFSET, including mixed with theWHEREparameters this query already passes; and the existing "the live view is bounded; history answers the rest" test exerciseslimit: 2/offset: 2and 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.svelteby hand. Removed..forgejo/deno.sh task cirun verbatim before pushing: 375 passed, 0 failed, 4 ignored.Re-review when you have a moment.
Re-reviewed
51129fe. Both items are addressed properly and nothing else in the branch moved.Page bounds.
query()now passes[...values, limit, offset]againstLIMIT ? OFFSET ?, andrecent()passes[sinceMs, Math.max(limit, 1)]— positional order matches clause order in both, andgit grep -E 'LIMIT \$\{|OFFSET \$\{'over the branch returns nothing, so the pattern is gone rather than moved. KeepingclampIntis 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.1bindsLIMIT/OFFSETmixed withWHEREparameters (WHERE id >= ? ... LIMIT ? OFFSET ?with[1, 2, 2]returns the correct page), and an offset ofNumber.MAX_SAFE_INTEGER— the clamp's ceiling, which no test exercises — returns an empty set instead of erroring. The existinglimit: 2/offset: 2history 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 civerbatim: 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.6to2.9.5. Since.forgejo/deno.shreads the pin out ofdocker/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 cion the merged tree under 2.9.5 gives the same 375 passed / 0 failed / 4 ignored. Becauseciis&&-chained, that also meansdeno fmt --check,deno lint,checkandcheck:svelteall 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_MODULESnote in the description still applies:activityhas to be on the server's list before this deploys, or the Activity view disappears.Approving.