generated from OpsDeck/module_template
feat: hand the update runs to core #1
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/task-runs"
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?
Migrates the last module still on the deprecated
progressTopic/ async-route pattern (core#21).Why this module was the worst offender
It was a miniature task system in its own right: a
runningboolean guarding one run at a time, hand-rolledstartedMs/endedMs, anokflag, a log buffer, afollowersset, and an end-of-run signalled by the wording of a log line.That last one is the deprecation in miniature. Because a progress message starting
doneorerrorended the client's stream, dpkg's ownErrors were encountered while processing:would cut the log off mid-run — so the module had to prefix anything from apt that looked like a terminator:What changes
The three apt operations become
ctx.tasks.defineactions; the routes only start one and answer{ runId }.Answering the verdict instead — the start-and-wait pattern
module_systemdandmodule_ipmiuse on main — is not an option at this length. A dist-upgrade has a 30-minute timeout and no HTTP client will hold the connection for it.Beyond satisfying the deprecation, what actually improves:
runningwent with whatever killed it, so an upgrade interrupted by a restart came back readingidle. It is now aninterruptedrow that says so.notifyOn: ["failed"]withexposenamingreasonData, so the failure notification carries apt's exit line instead of an empty body. (Round three: no schedule declares these actions, so today every run is one a human just clicked — the notification is for the operator who navigated away before apt finished, not an unattended 03:00 run. The comment that oversold this is fixed.)exposenamesresultandreasonData, so core's projection withholds the run's line tail;GET /runwithholds itstext; therunlogprovider takesrole: "admin"; theprogresstopic is declaredctx.events.adminOnly. (The last three were added in review — the first commit closed only the projection and the description claimed the lot. See the review thread.)Design notes for review
Shared
lock: "apt". dpkg takes one lock on the host anyway; a second apt landing mid-upgrade fails on the lock file. A queue is the honest version of what therunningboolean was pretending to be. Side benefit: a security upgrade queued behind a refresh now re-simulates against lists that refresh just updated.Shared
target: "apt". Not for collision handling (identity already includes the action id) but so/summarycan callctx.tasks.isBusy("apt")synchronously — it is a decision taken mid-render, and an await there means keeping a second copy of core's state.Two ways to ask twice, two answers. The old boolean refused both; only one is worth refusing.
onSelfConflict: drop→ askippedrow → 409. Asking twice for a refresh is a double click.waitingrow → 200{ queued: true }, and it starts when the lock frees. Refusing it would only mean the operator retrying by hand what the lock already sequences.canRunis host access alone.isBusyis!isTerminal, so gating the buttons on it as well hid all three the moment anything was live or merely queued — which left the second answer above reachable only by curl (review round two). The refusal happens where it is decided, at the start; each confirmation says the run may queue.The simulation runs behind the lock — the guard is gone (round three). Rounds one and two asserted that
apt-get -sfails on the dpkg lock during a live run; round three measured it and the premise did not survive:state.tspasses-o Debug::NoLocking=true, and probing a real dist-upgrade in flight answers rc=0 with counts tracking the upgrade as it lands. The guard also had a hole its comment did not admit — with an empty cache it simulated anyway and cached that one answer for the rest of the run. So/summary,/packagesand the 5-minute collector now follow an upgrade live, andload(true)means exactly "bypass the 10-second cache".No
consoleon any action, includingrefresh. Core refusesconsoleon an action that declares atarget(tasks/service.ts:195— "console" means startable with no arguments, so it will not call a target callback), and it refuses it at registration, which disables the module at load.deno checkcannot see that; the first commit here shippedconsole: trueonrefreshand the module did not load at all. The target is what/summary's synchronousisBusy("apt")needs, soconsoleis what gives way — the Activity view offers no generic Run for the refresh. The two upgrades would not have been offered there regardless: its Run button carries no confirmation, and the schema actions deliberately do.No
onInterruptedretry — and no one-tap Retry either (round three). Everything here mutates host package state and core cannot know how far dpkg got. Re-running a dist-upgrade that died mid-unpack is the module deciding on the operator's behalf to touch a half-configured system. The row stands as the record. The same reasoning now closes Activity's Retry button:retry: falseon both upgrades, because that button is one tap with no confirmation, offered for any terminal run an admin owns (interruptedandskippedincluded), andtasks.retrycheckscapabilities.retry, notconsole.refresh— args-free, non-destructive — keepsretry: "new-run".One log entry per 500 ms, not per line. Every
h.log()is a store write plus a frame oncore:task:<id>and a dist-upgrade prints thousands of lines. Time-coalesced only — apt's burstiness is what makes the frames. The consequence is that an entry is not a line, which holds because both renderers join the tail with newlines; a client that ever renders an entry as a row breaks it. The buffer, the splitting and the flush ordering live inrunlog.tsso a fake handle can test them.The one thing to push back on if you disagree
progressTopicstays, marked transitional, alongside the{ runId }response.The shell drops the topic the moment it sees a
runId, so on web this is dead weight. It is there purely for an Android build that predates task runs: without it, such a client treats the 2xx that merely created the run as the work having succeeded, and reports nothing at all when a dist-upgrade later fails.That also means the
·prefix hack survives this PR. Both go when the deployed app understandscore:task:<runId>— mobile#4, which is the companion to this one. (Confirmed in review:SchemaViewModel.ktnever readsrunIdfrom the response, so this is the current source, not only the deployed build.)Not fixable in this repo
A structured
TaskFailure— which is what would put areasonslug and its data on a failed run, and let Activity offer a remedy instead of a message. It is a runtime export of the SDK, and an external module is imported from/data/modules/src/<slug>/, outside the workspace, where a bare specifier does not resolve (Import "@opsdeck/sdk" not a dependency); that is why every SDK import here is type-only. A locally restated class fails core'sinstanceofcheck. Core would have to duck-type the throw or hand one out onctx.tasks. Until then the failures are plainErrors andexpose: ["reasonData"]is what carries their message into the notification.Verification
deno check packages/modules/updates/backend/mod.tsclean,deno lintcleandeno test --allow-read backend/— 28 passed: 11 parser tests (untouched by this PR), 12 inrunview_test.ts(whatGET /runanswers over the run shapes that caused trouble — a run queued behind the lock, a live run the buffer does not own yet — alone and with a queue behind it, a live run buried under refused double clicks, askippedrow newer than the succeeded run it was refused for, a log lost to a restart, a non-admin reader) and 5 inrunlog_test.ts(nothing pending outlives the handle, an unterminated partial line still lands, a second run binding the buffer is reported)debian:bookwormrootfs bind-mounted at/host/rootin a privileged container, which is enough for the probe to pickchrootand for apt to be real — and, unlikedeno check, enough to prove the module loads at all. Refresh runs; a second refresh is 409/skipped; a dist-upgrade queues behind it and starts on release; twelve refused double clicks do not evict the live run from/run, which keeps agreeing with/summary; a brokensources.list.dentry fails the run withapt-get update exited 100and the admin notification carries that line; the tail arrives batched (9 entries for a run that printed dozens of lines).Not exercised: an interrupted run across a real restart, and a non-admin reader end-to-end (the rig runs
OPSDECK_AUTH=disabled, which is a static admin — the withholding has unit coverage only). Worth a look on the live server before this reaches main, since main deploys prod.🤖 Generated with Claude Code
This module was its own miniature task system: a `running` boolean guarding one run at a time, hand-rolled start/end timestamps, an ok flag, a log buffer, and an end-of-run signalled by the WORDING of a log line. Core owns all of that now (core#21), including the parts that were never right here. The three apt operations become ctx.tasks.define actions; the routes only start one and answer { runId }. Answering the verdict instead — the pattern the fast host modules use — is not an option at this length: a dist-upgrade has a 30-minute timeout and no HTTP client will hold the connection for it. What actually improves, beyond the deprecation: - A run survives the process. `running` went with whatever killed it, so an upgrade interrupted by a restart came back reading "idle"; it is now an `interrupted` row that says so. It is deliberately NOT retried — core cannot know how far dpkg got, and re-running a dist-upgrade that died mid-unpack is the module deciding on the operator's behalf to touch a half-configured system. - The outcome is a status, not a sentence. The old contract inferred the end of a run from a log line starting "done" or "error", which is why dpkg's own "Errors were encountered while processing:" had to be disguised before being published. - The runs record who asked for them. - apt output is withheld from non-admins. `expose` names `result` only: the line tail is package names, repository URLs and configuration paths, and the projection withholds what an action does not opt in. All three share `lock: "apt"`, because dpkg takes one lock on the host anyway — a second apt landing mid-upgrade fails on the lock file, so a queue is the honest version of what the `running` boolean was pretending to be. A security upgrade queued behind a refresh is strictly better than before: it re-simulates against lists that refresh just updated. They also share `target: "apt"` so /summary can answer ctx.tasks.isBusy("apt") synchronously, which is what it needs mid-render to decide whether the buttons apply. The Activity view may offer `refresh` (args-free and non-destructive) but not the two upgrades: its generic Run button carries no confirmation, and installing packages on someone's host is not a one-tap affordance. `progressTopic` stays on the schema actions, marked transitional. The routes answer { runId } and a current client follows that instead — the shell drops the topic the moment it sees one — but an Android build that predates task runs would otherwise treat the 2xx that merely CREATED the run as the work having succeeded, and report nothing when a dist-upgrade later failed. It goes when the deployed app understands core:task:<runId> (mobile#4). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Reviewed the code against core's task contracts rather than the description. Re-ran the verification:
deno check packages/modules/updates/backend/mod.tsis clean andapt_test.tsis 11 passed. Worth stating plainly that those 11 tests are the parsers, unchanged by this PR — the run lifecycle this PR is about has no test coverage at all.The design calls I would sign off on as written: the shared
lock: "apt", noonInterruptedretry,console: trueonrefreshonly, and keepingprogressTopicas a transitional field.Blocker
1. "apt output is withheld from non-admins" is not true
expose: ["result"]governs the run'slines. The same apt output still reaches any authenticated user through three doors this PR does not touch:ctx.router.get("/run", ...)— norequireAdmin, returnsrunLog.textverbatimctx.events.provide("runlog", ...)— norole: "admin", so any viewer can subscribe to the live chunksctx.events.publish("progress", { msg })— noctx.events.adminOnly("progress")The SDK says exactly this, in the doc for
adminOnly(packages/sdk/mod.ts:221):All three predate this PR. The problem is that the PR body lists the withholding as one of the things that "actually improves", which will read to the next person as done. Either close the other three doors (
requireAdminon/run,role: "admin"on the provider,adminOnlyon the topic) or drop the claim.Should fix
2. The cross-action 409 disappeared silently
On main,
begin()refused if any run was live, so a second update run of any kind got a 409.onSelfConflictis scoped to(module, action, target), so it only covers a repeat of the same action. POST/upgrade/allwhilerefreshis live now creates awaitingrun and answers 200;BUSYnever fires.Queueing may well be what you want — it is the stated point of the shared lock. But the description says "the 409 that used to come from a hand-rolled boolean now comes from the policy the action declared", and that only holds same-action.
canUpgradeAllis a 10-second-cached UI hint, not a guard, so nothing else covers the gap.3.
/runmisreports a queued runwithRunLogresetsrunLog.textwhen the handler starts, not when the run is created. While a run sitswaitingon the lock,/runtakeslabel,runningandstatusTextfrom the new row buttextfrom the previous run — so the pane reads "Install all updates waiting" over the refresh's output.4.
starterhas no failure pathctx.tasks.startthrowing gives a 500 with a stack trace, which is the thinghostRequiredwas written to avoid two functions up.refusalStatus()in the SDK exists for this mapping.5. Every apt line is now a WebSocket frame
output()callslogHandle?.log(line)per line, and eachlog()emits anevoncore:task:<id>, pushes into the 512-entry retain buffer, and setsstate.dirty. That is on top of therunlogchunks and theprogressbus publish, so adist-upgradeships its output three ways, one of them line-at-a-time. Worth chunking the run log, or dropping theprogresspublish once a client has shown it understandsrunId.Notes
collecttick callsload(true), which runsapt-get -s dist-upgradeon the host outside the"apt"lock. During a 30-minute upgrade every tick will fail on the dpkg lock and records.error. Pre-existing, but now there is a synchronousctx.tasks.isBusy("apt")sitting right there to guard it with.expose: ["result"]optsresultin, but nothing renders it — the summary sentence only reaches a client as a log line, which is the withheld field.progressTopicand the·prefix until mobile#4 is deployed is the right call. Deleting both now means an Android regression with no upside, and the shim is three lines with a delete condition written next to it.All five addressed in
d9d934a, plus one thing the review could not have seen becausedeno checkcannot either — see the last section.1. Blocker — the withholding claim
You were right that it covered one door of four. All four now agree:
ctx.events.provide("runlog", …, { role: "admin" })ctx.events.adminOnly("progress"), declared at load before anything publishesGET /runwithholdstextfor non-admins — notrequireAdmin. A schemalogTextblock whose source refuses renders as "failed to load" (SchemaPage.sveltefalls through toEmptyStateon a source error), and "not for you" is a different fact from "this module is broken". A non-admin now getsUpdate output is admin-only. <status> — <summary>, which is also what makesexpose: ["result"]reach a client for the first time (your last note).exposestill namesresultonly.2. The cross-action 409
Kept the queue — as you say, it is the point of the lock — and made the two cases say what they are instead of the description claiming one rule for both:
skipped→ 409. Asking twice for a refresh is a double click.waitingbehind the lock → 200{ queued: true, reason: "waiting for run #1" }. dpkg takes one lock anyway; the alternative to a queue is a refusal followed by the operator retrying by hand.The PR description has been rewritten to say that rather than "the 409 now comes from the policy the action declared".
3.
/runmisreporting a queued runThe buffer now records which run it holds, and the route no longer takes the newest row: while one run waits on the lock, the output being produced belongs to the run holding it, and that is what someone watching wants to see. The queue is reported as a count —
"Refresh package lists running (1 queued)".That decision is now
backend/runview.ts, a pure function, withrunview_test.tscovering it: queued-run-does-not-claim-the-live-log, stale-buffer-after-the-owner-finished, log-lost-to-a-restart, and both non-admin cases. Which is the smaller half of your preamble point — the lifecycle still has no end-to-end test, but the part of it that can lie without failing now does.4.
starterhas no failure pathWrapped;
refusalStatusrestated locally rather than imported, for the same reasonTERMINALwas (SDK is type-only here — importing the SDK's runtimerefusalStatuswould give this backend a host dependency). A start that throws logs and answers{ error, reason }, never a stack.5. Every apt line a frame
Batched: 40 lines or 500 ms, whichever comes first, flushed on close before the handle is released. The tail is joined with newlines wherever it renders (
activity/backend/ui.tsdoesrun.lines.join("\n"),RunFeedthe same), so one entry per batch reads exactly like the lines it holds — and the 512-entry retain buffer now holds 512 batches. Measured below: 7 lines arrived as 3 entries.Left the
progresspublish alone; it is the shim's whole purpose, and it goes with mobile#4.Notes
if (ctx.tasks.isBusy("apt")) return;at the top of the tick, with the reasoning you gave. Six wrong samples and a "could not get lock" in Package data was the old behaviour.expose: ["result"]renders nowhere — now it does, in the non-admin/runtext above. Activity still does not renderresultfor anyone; that is core's to fix, not this module's.progressTopicand the·prefix — agreed, they stay until mobile#4 is deployed.The thing neither of us checked
The module did not load at all:
service.ts:195refusesconsole: trueon an action that declares atarget, anddeno checkcannot see a runtime registration check. So the PR as reviewed would have disabled the updates module on the next deploy — withmaindeploying prod, that is the whole feature gone, not a regression in it.consoleis what gives way: the target is what makes/summary's synchronousisBusy("apt")work, and that decides whether the buttons apply on every client. The Activity view no longer offers a generic Run forrefresh. If you would rather keep that button, the alternative is droppingtargetand having/summaryawait ctx.tasks.list({ status: ["running", "waiting"] })— the handler is already async, so "synchronous on purpose" is a preference there, not a constraint. Say the word and I will swap it.Verification
Not just
deno checkthis time. Adebian:bookwormrootfs exported to a directory, bind-mounted at/host/rootin a privileged container running core — which is enough for the probe to pickchrootand for apt to be real:POST /refresh{"ok":true,"runId":1,"status":"running","queued":false}POST /refreshagain, live409 {"error":"an update run is already in progress","runId":2,"reason":"#1 is still live"}POST /upgrade/allduring it200 {"runId":3,"status":"waiting","queued":true,"reason":"waiting for run #1"}GET /runwith both"Refresh package lists running (1 queued)"/runfollowed itPOST /upgrade/securitydone — no security updates pendingsources.list.dfailed,error: apt-get update exited 100Plus
deno checkclean,deno lintclean, anddeno test backend/— 19 passed (11 parser, 8 new).Still not exercised: an interrupted run across a real restart, and a non-admin reader (the rig runs
OPSDECK_AUTH=disabled, which is a static admin — the withholding is covered by unit test only).Reviewed at
d9d934a, checked against core atf3426berather than against the diff alone: the branch staged into a core checkout,deno checkclean,deno test --allow-read backend/19/19 as claimed, plus five probes driven straight atrunView.The migration itself is right, and the four-door withholding claim holds up — I verified each one:
SchemaPage.svelte:170really does render a refused source as "failed to load" (so/runwithholding text rather than 403-ing is the correct call),live.ts:437answers a non-adminsub-err forbiddenon therunlogprovider, and mobile'sstartLogFollowsignores subscription errors rather than showing a broken pane. Three blockers below, one design question, and the batching contract is worth a second look.Blockers
/runloses the live run behind fiveskippedrows.ctx.tasks.list({ limit: 5 })— every refused double-click persists a row (your own verification:409 … "runId":2), so five of them push the running row to number six.runViewthen answersrunning: falseand "it ran before the last restart" while/summarysaysrunning: truein the same second.runTextfalls through to the restart message for any status that is notwaiting, includingrunning.notifyOn: ["failed"]fires an empty notification.service.ts:1233takesrun.reason, which is only ever set fromTaskFailure.reason(service.ts:991). Every throw here is a plainError, so the admin gets a title and a blank body — and this is the module's only push path for an upgrade that dies at 03:00.The queue no shipped client can reach
isBusyis!isTerminal(service.ts:621), so a merelywaitingrun also makescanRunfalse, and all three buttons carrywhen: … equals true. The moment anything is live or queued, all three vanish; the web shell additionally disables every button while one action is in flight (ActionsBlock.svelte:167). So the 200 /{ queued: true }branch — the longest design note in the PR — is reachable by curl, or by a click landing inside the 120 s/summarypoll window. Theskipped→ 409 case is genuinely reachable; the cross-action queue is not. Worth resolving one way or the other before this lands.On the push-back item
progressTopicstays — your reasoning is stronger than the PR states.mobile/…/SchemaViewModel.kt:740-780never inspects the response body forrunIdat all, so this is not only the deployed build, it is the current source. Keep the topic and the·prefix until mobile#4.Everything else is inline.
@ -30,0 +38,4 @@* module or the store being broken, which is a 500 and not the operator's* fault to fix.*/function refusalStatus(err: unknown): 403 | 404 | 409 | 500 {The 403 and 404 branches are dead.
ctx.tasks.startis the module-facing path;TaskStartRefusedis thrown bystartAsUser/ the observer (service.ts:490,504), which is what the Activity view goes through, not this. The comment already says as much — worth following it to its conclusion and dropping the mapping, since a 404 here would answer "not found" for a route that plainly exists, on what the comment itself calls "this module or the store being broken", i.e. a 500.@ -90,0 +120,4 @@// per flush: the run's tail is joined with newlines wherever it is rendered,// so a batch reads exactly like the lines it holds — and 512 retained// entries now buy 512 batches rather than 512 lines.const LOG_BATCH_LINES = 40;The batching is the right call for the store write, but what reaches
h.log()is no longer a line.SDK: "append one plain text line". The wire event is
{ type: "log", runId, line }. I checked both renderers and they do join with\n—activity/backend/ui.ts:308andRunFeed.svelte:163— so this renders correctly today, and theMAX_LINEStrim (service.ts:1112) really does become 512 batches as the description says.The cost is a coupling this module cannot enforce: the moment any client renders a line as a row, a count, or a virtualized item, a 40-line blob is one row. If you keep it, the constraint belongs in the SDK doc for
log(), not only in this comment.Cheaper alternative with most of the win: coalesce on time only (500 ms) and drop
LOG_BATCH_LINES. apt's bursty output is what generates the frames, and a 500 ms window catches nearly all of it without ever claiming a batch is a line.@ -90,0 +137,4 @@}function recordLine(line: string): void {if (logHandle === null || !line.trim()) return;recordLinedrops blank lines;outputwrites them intorunLog.textunchanged. So the pane and the run's own tail are not the same text — apt's blank-line separators survive in one and not the other. Harmless, but it means the tail cannot be used to verify what an admin saw.@ -166,0 +194,4 @@* whatever apt printed last — the same fact core records as the run's* status, written once and used for both.*/async function withRunLog<T>(h: TaskHandle, label: string, body: () => Promise<T>): Promise<T> {runLog,lineBuf,logPendingandlogHandleare module-scoped and shared by all three actions. The only thing preventing two runs from interleaving into one buffer is that all three declarelock: "apt"— an invariant nothing checks, and one a fourth action would break silently (the symptom would be one run's apt output filed under another run's id, which is exactly the bugrunview.tswas extracted to prevent).One line makes it loud:
@ -168,0 +267,4 @@// dpkg database in a state someone has to repair by handcapabilities: { cancel: false as const, retry: "new-run" as const },expose: ["result" as const],notifyOn: ["failed" as const],Blocker. This notification arrives empty.
service.ts:1233:run.reasonis populated only fromTaskFailure.reason(service.ts:991). Every failure path in this module throws a plainError, soreasonstays null and an admin gets the titleInstall all updates failedwith no body at all. The exit code lives in the run's line tail, which the notification does not carry — so the one alert that fires while nobody is looking says nothing about what happened.Add
"reasonData"here and throwTaskFailure(next comment). Safe against your own disclosure rule: notifications are admin-only (notifications/store.ts:58).@ -168,0 +350,4 @@withRunLog(h, "Install all updates", async () => {const code = await h.step("apt-get dist-upgrade", () =>apt([...APT_OPTS, "dist-upgrade"], UPGRADE_TIMEOUT_MS));if (code !== 0) throw new Error(`apt-get dist-upgrade exited ${code}`);Pairs with the
notifyOncomment above — this is the throw that produces the blank alert. All three actions have the same shape.TaskFailureis a runtime export, so this is the one place the type-only SDK import has to give — or restate the class locally the wayTERMINALandrefusalStatusalready are. The structured reason is also what lets Activity offer a matching remedy later instead of a stack trace.@ -343,1 +547,3 @@const canRun = host.available && !runState.running;// synchronous on purpose: this is a decision taken mid-render, and an// await here would mean keeping a second copy of core's own stateconst running = ctx.tasks.isBusy("apt");The collector guard below (
isBusy("apt")at the top of the tick) is right, and it closes one of three doors onto the same lock.load()is the other two:/summaryand/packagesboth call it, and both are polled every 120 s by the page and the dashboard widget.During a dist-upgrade the 10 s cache expires,
apt-get -s dist-upgradefails on the dpkg lock, andstate.ts:107turns that intoerror: "E: Could not get lock…"withpackages: []. The page then shows Pending 0 / Security 0 withPackage data: E: Could not get lock…, andOverviewWidgetreads "up to date" in the middle of an upgrade.Same fix one level down — in
load(), keep serving the last known state whileisBusy("apt")instead of re-simulating. The run already invalidates the cache when it ends.@ -344,0 +547,4 @@// synchronous on purpose: this is a decision taken mid-render, and an// await here would mean keeping a second copy of core's own stateconst running = ctx.tasks.isBusy("apt");const canRun = host.available && !running;This is where the queue becomes unreachable.
isBusyis!isTerminal(service.ts:621), so a run that is merelywaitingalso makescanRunfalse. All three buttons carrywhen: { … equals: true }, so as soon as anything is live or queued all three disappear from the page — and the web shell separately disables every button while one action is in flight (ActionsBlock.svelte:167).So the 200 /
{ queued: true }branch can only be hit by curl, or by a click that lands inside the/summarypoll window. Theskipped→ 409 case survives (a double-click beats the poll); the cross-action queue does not, which is the case the design note is actually about.Two honest options:
host.availablealone, let the buttons stay, and let the response say "queued behind the refresh" — the confirm dialogs already exist, so nothing lands by accident; orlock: "apt"as pure safety, and answer 409 for both.What is here now is a queue whose only user has a shell.
@ -406,0 +600,4 @@// Five, not one: while a run waits on the apt lock the newest row is not// the one producing output. `runView` decides which is; it is a pure// function so that decision has a test (`runview_test.ts`).const recent = await ctx.tasks.list({ limit: 5 });Blocker. Five is too few, and the rows that evict the live run are ones this module creates on purpose.
Every
onSelfConflict: droprefusal persists askippedrow — your own verification shows409 {"runId":2}. Five double-clicks and the run actually holding the lock is row six, sorunViewnever sees it:Meanwhile
/summaryanswersrunning: true, becauseisBusyreads core's live map rather than the list. The two routes on the same page disagree.store.ts:305clampslimitto 100 by default, so the 5 buys nothing. Ask for the live set by status, and only fall back to history when there is none:@ -0,0 +25,4 @@text: string;label: string;running: boolean;queued: number;queuedis a count here and a boolean in the POST responses (queued: status === "waiting"). One module, one word, two types — a client that reads both routes has to know which is which.queuedCounthere, orqueued: booleanplusqueueDepth.@ -0,0 +36,4 @@* and this backend imports the SDK type-only. Kept in step with `RunStatus`* in core's `packages/sdk/tasks.ts`.*/export const TERMINAL = [Currently in sync with core's
TERMINAL_STATUSES, so this is about drift direction rather than a bug today.Enumerating the terminal set means a status core adds later reads as live forever —
/runwould show a finished run as running until the process restarts. Enumerating the live set fails the other way: a new live status reads as terminal, which self-corrects on the next poll once it ends.Same three statuses, opposite failure mode.
@ -0,0 +107,4 @@if (run.status === "waiting") {return "Queued behind another update run — the log starts when apt does.";}return `No log for this run — it ran before the last restart. Status: ${run.status}.`;Blocker. This branch is reached by live runs too, and then it contradicts its own sentence.
The window is core flipping the row to
runningbeforewithRunLogassignsrunLog.runId— every start crosses it, and any/runpoll landing inside prints this.waitingis not the only live status that can arrive here;runningandcancellingcan too.Test
isLivefirst, and keep the restart message for terminal rows only:@ -0,0 +133,4 @@eq(done("failed"), "Install all updates — failed", "failed");eq(done("skipped"), "Install all updates — skipped", "skipped");eq(done("interrupted"), "Install all updates — interrupted", "interrupted");});The file is the right idea and covers the shapes it names. It misses exactly the two that broke under review — both fit the existing style in a few lines each:
Also untested:
recordLine/flushRunLog, whose invariant — flush before the handle is released — breaks silently if the ordering inwithRunLog'sfinallyever changes. A fakeTaskHandlecollectinglog()calls covers it without touching apt.Review round two. Every item is a claim this module made that a client could catch it out on. `/run` asked for the newest five rows, and this module manufactures rows that are newer than the run it cares about: every refused double click persists a `skipped` one. Five of them and the run holding the apt lock is row six, so `/run` answered "idle" in the same second `/summary` answered `running: true`. It asks for the LIVE runs now, and falls back to a single row of history only when nothing is live. `runView` prefers a live row for the same reason. Its text fell through to "it ran before the last restart" for any status that was not `waiting` — including `running`, which every start crosses on its way to `runLog.begin`. Live statuses are tested first, and the restart message is for terminal rows only. `notifyOn: ["failed"]` fired with an empty body: core's outcome notification is `run.reason ?? run.error` when the action exposes `reasonData` and a bare `run.reason` otherwise, and `reason` is only ever set from a structured `TaskFailure` — which an external module cannot throw. It is a runtime export, and a bare specifier does not resolve at load ("Import \"@opsdeck/sdk\" not a dependency"), while a locally restated class fails core's `instanceof`. So `expose` names `reasonData` and the alert now carries apt's exit line. The queue nothing could reach: `isBusy` is "not terminal", so a merely waiting run made `canRun` false and all three buttons vanished — the shared lock's queue was enterable only by curl. `canRun` is host access alone; the two answers a second ask gets are decided at the start, where they were already, and each confirmation says the run may queue. Also from the review: - `load()` serves the last known state while `isBusy("apt")`. Behind a live run its `apt-get -s dist-upgrade` fails on the dpkg lock, and the empty list that comes back is read as the host's package state — Pending 0 with an error in "Package data", and a dashboard card reading "up to date" in the middle of an upgrade. - Log lines are coalesced on time alone; the 40-line cap is gone. Blank lines now reach the run's tail as well as the pane, so the two are the same text. - The buffer moves to runlog.ts, where a fake handle can test the ordering its release depends on, and a second run binding to it is logged rather than interleaved in silence. - `refusalStatus` drops its dead 403/404 branches: `ctx.tasks.start` trusts its caller, so a 404 there would answer "not found" for a route that exists. - `TERMINAL` becomes `LIVE`: a status core adds later then reads as terminal, which the next poll corrects, rather than as live forever. - `/run`'s `queued` count is `queuedCount`; `queued` stays the boolean the POST routes answer. Verified on the rig (Debian rootfs bind-mounted at /host/root in a privileged container, chroot strategy): 26 tests pass, `deno check` clean, module loads, twelve refused double clicks do not evict the live run from `/run`, `/summary` keeps its counts through the run, an upgrade queues behind a failing refresh and starts when the lock frees, and the failure alert reads "apt-get update exited 100". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Pushed
dec796a. All three blockers fixed, the queue question resolved in favour of keeping it, and one item I have to push back on for a reason the review could not have known.Blockers
1.
/runloses the live run behindskippedrows. Fixed as suggested — the live set first, one row of history only when nothing is live:runViewalso prefers a live row over a newer terminal one (live.find(id === log.runId) ?? live[0] ?? recent[0]), so the shape cannot come back through a different caller. Rig: twelve refused double clicks with a refresh live,/runstill answersRefresh package lists running (1 queued)while/summaryanswersrunning: true.2. A live run the buffer does not own yet.
isLiveis tested first; the restart message is for terminal rows only. Test added — the one where the buffer still holds the finished refresh and run 9 isrunning.3. The empty notification.
exposegainsreasonData, so core'srun.reason ?? run.errorbranch is taken and the alert readsapt-get update exited 100(verified on the rig, notification body quoted in the commit).But
TaskFailurecannot be thrown from here, and neither can it be restated locally:import()ed from/data/modules/src/<slug>/, outside the workspace, and a bare specifier there does not resolve —TypeError: Import "@opsdeck/sdk" not a dependency, which I reproduced rather than inferred. That is what the type-only-imports constraint inCLAUDE.mdis about; type-only survives because it is erased before resolution.service.ts:988'se instanceof TaskFailure, soreasonwould still be null and the notification still blank.So the failures stay plain
Errors andreasonDatais what carries their message. The structured reason and the matching remedy are worth having, but they need core to reach external modules some other way — duck-typing the throw (name === "TaskFailure"plusreason/data), or handing one out onctx.tasks. Happy to open that against core if you agree it belongs there; it is not fixable in this repo.The queue no shipped client can reach
Kept, made reachable:
canRunishost.availablealone.isBusyis!isTerminal, so gating on it hid all three buttons whenever anything was live or merely waiting — which is exactly your point. The refusal happens where it is decided (the start: same action →skipped→ 409, different →waiting→ 200), and each confirmation now ends "…it queues if another update run is live", so nothing lands by accident. The web shell still disables its own buttons while it follows a run, so within one tab the queue is entered after a reload — but a second admin, a second tab, mobile and curl all reach it, and the operator's natural sequence (refresh, then install) no longer requires watching for the poll.The alternative — drop the two answers and refuse both — throws away the thing the shared lock exists for, and the re-simulation a security upgrade queued behind a refresh gets.
Everything else
LOG_BATCH_LINESgone. A batch is still not a line, so no client may render an entry as a row — but with the cap gone there is no second knob claiming otherwise, and the constraint is written where it can be read (CLAUDE.md,runlog.ts).runlog.tswith the conflict report you wrote, andrunlog_test.tscovers what a fake handle can: nothing pending outlivesrelease(), an unterminated partial line still reaches the tail, a released buffer takes no more lines, a secondbeginis reported and does not steal the first run's lines.refusalStatus. 403/404 branches dropped;TaskStartRefused→ 409, everything else 500.TERMINAL→LIVE. Same three statuses, the failure direction you argued for.queuedthe count isqueuedCount;queuedstays the boolean on the POST responses.load()behind the lock. Serves the last known state whileisBusy("apt"); only a run holding the lock passesload(true). That closes the/summaryand/packagesdoors onto the same dpkg lock the collector tick already skipped. Rig:/summarykeptstatusText: "ok"and its counts through a run that was failing on a blackholed mirror.expose: ["result"]renders nowhere. Still true;resultreaches a non-admin as the summary in/run's text, which is whatrunview_test.tsasserts. Not going further here.Verification
Same rig as before (a
debian:bookwormrootfs bind-mounted at/host/rootin a privileged container,chrootstrategy, module loaded — not justdeno checked): 26 tests pass (11 parser, 10runview, 5runlog),deno checkclean,deno lintclean. Driven end to end: refresh runs; a second refresh is 409 with askippedrow;upgrade/securityqueues at 200{queued: true}and starts when the lock frees; twelve refusals do not evict the live run; a blackholedsources.list.dentry fails the run withapt-get update exited 100and the admin notification carries that line; the run's tail arrives batched (9 entries for a run that printed dozens of lines).Still not exercised: an interrupted run across a real restart, and a non-admin reader end to end — the rig is
OPSDECK_AUTH=disabled, i.e. a static admin, so the withholding still has unit coverage only.🤖 Generated with Claude Code
Third pass at
dec796a, staged into a core checkout atmain(db57f4d) rather than thef3426beof the last round, and checked against core's source rather than against the description.deno checkclean,deno lintclean,deno test --allow-read backend/26 passed (11 parser, 10 runview, 5 runlog) — every number in the verification section holds.The three blockers from round two are genuinely fixed, and I re-derived each rather than taking the commit message for it:
service.ts:1229-1233— a plainErrorlands aserror: messagewithreasonnull (service.ts:999), andexpose.includes("reasonData")is exactly what makes the notification fall through torun.error. Soapt-get update exited 100really does reach the alert body.reasonDataitself stays null, so nothing extra is disclosed.skippedrows. Fixed, and stronger than the fix needed to be — see the note onrunview.tsabout why thelimit: 1fallback is also safe, which is not obvious and is not this repo's to keep true.The withholding claim now holds through five doors, not four — I checked each in current core rather than trusting the last round: the projection (
exposehas nolines),/run's own text, the provider role (live.ts:436-437),adminOnlyon both fan-outs (sse.ts:35andlive.ts:643), and one this PR does not mention because core closes it: thecore:task:<id>log frames are gated onexpose.includes("lines")intasks/routes.ts:199.The batching contract holds too: core stores a multi-line batch as one entry without splitting (
service.ts:1109), and both renderers join with\n(activity/backend/ui.ts:308,RunFeed.svelte:163). Andconsole: falsedoes clear the registration check that disabled the module last time (service.ts:195) — I re-ran that path. NowaitTimeoutis declared, so a queued run has no deadline and the queue cannot silently turn into askip; worth knowing, because with one it would.Three things below. One is a safety regression the PR argues against elsewhere in its own text; one is a hole in an invariant this PR just wrote into
CLAUDE.md; the third is that the invariant's premise does not survive measurement.1.
retry: "new-run"is the one-tap host mutationconsole: falseexists to refuseconsole: falseis justified twice in this diff — "its Run button carries no confirmation, and the schema action deliberately does — installing packages on someone's host is not a one-tap affordance". ThenRUN_DEFAULTSdeclarescapabilities.retry: "new-run"for all three actions, and Activity's Retry button (activity/backend/ui.ts:123-132) has noconfirm— unlike Cancel and Force release, which both have one.projection.ts:66offers it for any terminal run an admin owns. Terminal includesinterruptedandskipped. So wherever Activity is enabled:onInterruptednote refuses to make on the operator's behalf — core does not resume it, and then hands out a button that does;skippedrow that also carries Retry;tasks.retry(service.ts:567) goes straight tostartRun— it re-checkscapabilities.retry, notconsole, so nothing else stops it.retry: falseon the two upgrade actions (keep it onrefresh, which is harmless and args-free) costs one line. If you want to keep it, it needs saying out loud inCLAUDE.mdnext to theconsolereasoning, because as written the two paragraphs contradict each other.2.
load()runsapt-get -sbehind the lock exactly when the cache is emptyCLAUDE.mdnow carries "Nothing runsapt-get -swhileisBusy("apt")" as a hard constraint. The guard is insideif (!force && cache), so withcache === nullthe function skips the check and simulates anyway — and this module opens that window itself:withRunLogfinally setscache = nulland publishesupdates, so every open page and the dashboard card refetch/summary;#finish→queueMicrotask(#sweep)starts the queuedupgrade-allimmediately;cacheis null,isBusy("apt")is true, andload()runsapt-get -s dist-upgradeon the host anyway — then caches whatever it got for the rest of the run, because every later call takes theisBusybranch.That is the advertised queue sequence (refresh, then the upgrade behind it), not an exotic one. Hoisting the check out of the
cache &&guard is the fix; it needs an answer for "busy and nothing known yet", andE: Could not get lockis the one answer it must not be.3. The premise under §2 is not established — and
state.tsalready argues against itstate.ts:104passes-o Debug::NoLocking=true, with a comment saying it is there precisely so the simulation survives another apt holding the dpkg lock. So the guard, the collector skip, and theCLAUDE.mdconstraint rest on a failure mode the code below them already handles.I measured it instead of arguing about it —
debian:bookworm-20230612, a realapt-get -y dist-upgraderunning, simulating against it every two seconds:No
E: Could not get lock, with or withoutNoLocking, and the counts track the upgrade as it lands. My own round-two note asserted the opposite ("during a dist-upgrade the 10 s cache expires,apt-get -s dist-upgradefails on the dpkg lock") — that was asserted, not tested, and it is now a hard constraint in a file the next person will trust.What the guard costs, if the premise is wrong:
/summary,/packagesand the dashboard card freeze for up to 30 minutes, andupdates.pending/updates.securitylose their 5-minute samples over the only window in which they move. Either demonstrate the failure on the rig (apt-get -s -o Debug::NoLocking=true dist-upgradeduring a real one, on the host, through the chroot) and keep the guard with the evidence next to it, or drop it and let the page show the counts falling.Notes
README.md:55still saysexpose"namesresultonly".dec796aaddedreasonData;CLAUDE.mdwas updated and the README was not — and this is the paragraph that tells a reader what is withheld.notifyOn: ["failed"]is still worth having (the operator may have navigated away), but it is not the unattended-alert path the description sells.frontend/OverviewWidget.svelteis untouched, andrunningchanged meaning under it:isBusyis "not terminal", so the "upgrade running" badge now also shows while a run is merelywaiting— and for a plain refresh.runview.tsabout thelive[0]pick and about the load-bearing core detail that makeslimit: 1safe.Everything else — the shared lock, the two answers,
canRunas host access alone, keepingprogressTopicand the·prefix until mobile#4, noonInterruptedretry, therunlog/runviewextraction and its tests — I would sign off as written.@ -28,0 +52,4 @@Non-admins see that a run happened and how it ended, but never its output.apt and dpkg print package names, repository URLs and configuration paths, soall four ways out are closed together: the run's line tail (core's projection— `expose` names `result` only), `GET /run`, the `runlog` live topic and theStale as of
dec796a:exposenow namesresultandreasonData(mod.ts:247) — which is what makes the failure notification carry apt's exit line, so it is not a detail.CLAUDE.mdwas updated for it; this paragraph, which is the one telling a reader what is withheld, was not.@ -60,0 +84,4 @@// data", and the dashboard card reads "up to date" mid-upgrade. Serving// the last known state is stale by minutes; the alternative is wrong.// The run invalidates this cache when it ends.if (ctx.tasks.isBusy("apt")) return cache.state;The invariant
CLAUDE.mdnow states as a hard constraint is false whencacheis null — the check sits insideif (!force && cache), so an empty cache skips it and simulates anyway.This module opens that window itself, in the sequence the PR advertises:
withRunLogfinally setscache = nulland publishesupdates→ every page and the dashboard card refetch/summary;#finishqueues#sweep, which starts thewaitingdist-upgrade immediately;cache === nullandisBusy("apt")true, runsapt-get -s dist-upgradeon the host, and caches the result for the whole run — every later call now takes theisBusybranch and serves it.If the premise holds (see the collector tick), that is "Pending 0 / up to date" and
E: Could not get lockin Package data for the next half hour — the exact page this guard was written to prevent.busyState()has to answer "not measured while an update run is live". The one thing it must not be is a lock error, becausestatusTextrenders it verbatim.@ -168,0 +231,4 @@lock: () => "apt",// apt is not interruptible halfway through unpacking without leaving the// dpkg database in a state someone has to repair by handcapabilities: { cancel: false as const, retry: "new-run" as const },This is the affordance
console: falserefuses, through a different door.activity/backend/ui.ts:123-132renders Retry with noconfirm— Cancel and Force release both have one, this does not.projection.ts:66offers it for any terminal run an admin owns, and terminal includesinterruptedandskipped.So a dist-upgrade that died mid-unpack gets a one-tap re-run in Activity — the decision the
onInterruptedcomment forty lines below deliberately declines to make for the operator — and so does everyskippedrow a double click left behind.tasks.retry(service.ts:567) re-checkscapabilities.retryand callsstartRun; it never looks atconsole.Keep
retry: "new-run"onrefreshif you like — args-free, non-destructive, and the same reasoning that made it a fineconsolecandidate applies. On the two upgrades it wants to go, or theconsoleparagraph above needs to stop arguing the opposite.@ -476,0 +687,4 @@// samples and a "could not get lock" in the Package data field. The run// publishes "updates" when it ends and invalidates the cache with it, so// nothing is lost by waiting for it.if (ctx.tasks.isBusy("apt")) return;The failure this guard prevents does not reproduce.
state.ts:104already passes-o Debug::NoLocking=true, with a comment saying it is there so the simulation survives another apt holding the dpkg lock. I measured it rather than reasoning about it —debian:bookworm-20230612, a realapt-get -y dist-upgradein flight, probing every two seconds:Exit 0 both ways, no
E: Could not get lock, and the counts follow the upgrade as it lands. My round-two note claimed the opposite; it was asserted, not tested, and it is now a hard constraint inCLAUDE.md.What the skip costs if the premise is wrong: six missing
updates.pending/updates.securitysamples across the only window in which those series move, plus a frozen page for the same 30 minutes.The rig can settle it — run
chroot /host/root apt-get -s -o Debug::NoLocking=true dist-upgradeduring a real dist-upgrade and paste what it answers. If it fails there, keep the guard and put the output next to it; if it succeeds, this line and the one inload()should go, and the page can show the counts falling live.@ -0,0 +71,4 @@// run holding the lock — that is what somebody watching wants to see. And a// refused double click leaves a `skipped` row that is newer than the run it// was refused for, so ANY live run outranks the newest terminal one.const run = live.find((r) => r.id === log.runId) ?? live[0] ?? recent[0];Two things, neither blocking.
live[0]is the newest live row, which is the wrong one when two are live. With a running run and a queued one,liveis[waiting, running]; if the buffer does not own the running row yet (the start window this file's own test covers),live[0]picks the waiting row — so the pane reads "Queued behind another update run" whilequeuedCountcounts the run that is actually executing. It self-corrects on the first apt line, so it is a flicker rather than a lie that persists.live.find((r) => r.status !== "waiting") ?? live[0]closes it.Why the
limit: 1fallback in/runis safe is worth a comment, because it is not this repo's decision. I went looking for the terminal version of the round-two blocker — askippedrow outliving the run it was refused for — and it does not happen, but only because of core:store.ts:310orders byCOALESCE(finished_at_ms, updated_at_ms) DESC, and#insertResolvedstamps a skipped row'sfinishedAtMsat the moment of refusal (service.ts:819), i.e. before the run it was refused for finishes. So the completed run still wins the single row.Change that
ORDER BYin core tocreated_at_ms— a plausible thing for someone to do to a history query — and this pane silently starts reporting a finished dist-upgrade as "Install all updates — skipped" with "it ran before the last restart" under it. Sincerunview.tsexists to make exactly that class of mistake testable, the dependency belongs in a comment here (and, if you want it caught, in a test that handsrunViewa[skipped, succeeded]pair and asserts the succeeded one owns the pane).Round three of review: - retry: false on both upgrade actions. Activity's Retry is one tap with no confirmation, offered for any terminal run an admin owns (interrupted and skipped included), and tasks.retry checks capabilities.retry, not console — the exact affordance console: false refuses, through a different door. refresh, args-free and non-destructive, keeps retry: "new-run". - drop the isBusy("apt") guards in load() and the collector tick. The simulation runs under -o Debug::NoLocking=true (state.ts) and the reviewer measured it against a real dist-upgrade in flight: rc=0 with and without the flag, counts tracking the upgrade as it lands. The guard's premise was asserted, not tested — and it simulated anyway whenever the cache was empty, then served that one result for the rest of the run. load(true) now means "bypass the 10-second cache", nothing more; the page and both metric series follow an upgrade live. - runview: within the live set prefer the run that is not waiting — in the start window before the buffer binds, with a queued run also live, the pane captioned itself "Queued behind another update run" while queuedCount counted the run that was executing. In history prefer a non-skipped row: a skipped row is the record of a refusal, not a run. Core's history ordering already answers /run's limit: 1 with the real run (skipped rows stamp finishedAtMs at refusal); that dependency is now written down at the call site, and the runview preference is the tested defence should the ORDER BY ever change. Two new tests. - README: expose names result and reasonData, not "result only". - dashboard badge: "update run active" — running is isBusy("apt"), which is also a merely queued run, and also a plain refresh. - the "03:00 alert" comment: no schedule declares these actions, so every run is one a human just clicked; the failure notification is for the operator who navigated away, and the comment now says so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Round three addressed in
ddbda2a. What changed, per item:1. Retry.
retry: falseon both upgrades;refreshkeepsretry: "new-run"— exactly the split you proposed, for exactly your reason: Activity's Retry is one tap with no confirmation andtasks.retrycheckscapabilities.retry, notconsole. Theconsoleparagraph and the retry line now argue the same thing, inmod.tsand inCLAUDE.md.2 + 3. The guard is gone. I took your measurement over my assertion — it was asserted twice and tested never, and
state.tshad been carryingDebug::NoLocking=trueagainst the same failure mode the whole time. Both skips are deleted (load()and the collector tick), which also deletes the cache-null hole in §2 rather than patching it: there is nobusyState()because there is no guard to need one.load(true)now means "bypass the 10-second cache", nothing more. TheCLAUDE.mdconstraint is inverted — it now says do not add anisBusyguard, with your measurement as the evidence — and the comment on the NoLocking line instate.tssays the same at the point someone would delete it. Honesty note: I did not re-run the probe on the rig for this push; the claim rests on your round-three measurement, and the PR's verification section says so.Inline on
runview.ts. Took both: the pick is now buffer-owner → first non-waitinglive row →live[0]→ first non-skippedterminal row →recent[0], so the start-window flicker with a queue behind it shows the executing run. The coreORDER BY COALESCE(finished_at_ms, updated_at_ms)/finishedAtMs-at-refusal dependency is written down at the/runcall site, and the[skipped, succeeded]case is a test (plus one for the two-live-rows window) — 12 runview tests now, 28 total.README paragraph updated (
resultandreasonData, neverlines), the badge now reads "update run active" (it was "upgrade running" over a queued refresh), and the 03:00 comment now says what is true: no schedule declares these actions, the notification is for the operator who navigated away.PR description updated to match all of the above.
deno checkclean,deno lintclean, 28/28.Fourth pass, at
ddbda2a, against coremain(db57f4d). Everything from round three is fixed and I verified each on a running system rather than in the diff: core + this branch in a privileged container, a realdebian:bookworm-20230612rootfs bind-mounted at/host/root, probe pickingchroot, apt real.deno checkclean,deno lintclean, 28/28.{"id":2,"action":"upgrade-all","status":"succeeded","capabilities":{"cancel":false,"retry":false}}, same forupgrade-security, and{"id":1,"action":"refresh",…,"retry":"new-run"}. The split is exactly right./summaryanswering{"pending":42,"security":0,"statusText":"ok","running":true}and/packages42 rows while apt was unpacking, no lock error anywhere. Inverting theCLAUDE.mdconstraint and putting the reason on theNoLockingline instate.ts— where someone would delete it — is the right place for it.runViewpick order, the/runcomment recording core'sORDER BY COALESCE(finished_at_ms, updated_at_ms)dependency, README, badge text, the 03:00 comment: all as described.upgrade/all→ running,upgrade/security→{"status":"waiting","queued":true,"reason":"waiting for run #2"},/runreading"Install all updates running (1 queued)", and the queued run started by itself.Four things below. Two of them are mine from a rig session that never made it onto the PR (the forge was unreachable for the day), so this is the first you are seeing of them — one is the item I would hold the merge on. One is new, and is a consequence of removing the guard.
1. A refresh that fetched nothing reports
succeededapt-get updateexits 0 when repositories fail to download. Only a malformed sources entry gives 100 — which is the case your rig line tests. Soif (code !== 0) thrownever fires for the failure operators actually get: a mirror that went away, DNS that broke, a suite past EOL.At
ddbda2a, with every source unresolvable:"The outcome is a status, not a sentence" is the PR's second headline claim, and for this action the status says ok while nothing was fetched,
notifyOn: ["failed"]stays silent, and/summarykeeps serving counts computed from lists that were never refreshed. In a module whose whole job is to tell an operator the host is behind, that is the failure that matters most.Measured fix, one option on the command you already run:
Scanning the output for
^Err:/^W: Failed to fetchand failing the step does the same job if you prefer not to change apt's error mode. What must not stand is "ok".2. A failed simulation is recorded as
pending: 0New this round, and a direct consequence of dropping the guard — which I still think was the right call. The tick writes its three metrics before the
if (s.error !== "") returnline, and a failed simulation yieldspackages: [], i.e.pending: 0. So an unreadable package state is indistinguishable, in the series and on the dashboard, from a host with nothing pending.Rig, no upgrade run anywhere between the two samples — I broke the sources file and restarted, which fires the tick immediately:
93 → 0, and the dashboard card renders
hint="up to date"because it readspending, notstatusText. Under the old guard the tick was skipped during runs; now it runs in every situation, so this is reachable for ordinary reasons — a bad sources file, a host mount that went away, lost exec access — not only mid-upgrade.Move the three
metrics.writecalls below the error check (a gap in the series is honest; a zero is not). Worth considering an edge-triggered notification for "package data unreadable" too — it is the one state where every number on the page is silently meaningless, and the module currently alerts on unattended-upgrades failing but not on this.3. The new second line of defence in
runViewcannot fireThe
skipped-filter is a good idea, but/runasks forlimit: 1when nothing is live, sorecentholds exactly one row. In the scenario the comment names — core'sORDER BYchanging to created-at — that one row is the newest refusal,recent.find((r) => r.status !== "skipped")finds nothing, and the chain falls through torecent[0]: the skipped row, which is what the defence exists to avoid. The test passes because it handsrunViewa two-row history the route never produces.ctx.tasks.list({ limit: 3 })on the fallback makes it real for two extra rows, and the test then matches the shape the route can actually deliver.4.
/packagespublishes what the four doors withholdAlso from the unposted session. The withholding is justified — in
mod.ts,CLAUDE.mdandREADME— as: apt output "is package names, repository URLs and the occasional configuration path — none of it a non-admin's business". Measured as a plain viewer (auth-disabled dev user patched down to["viewer"]in my core copy; the module untouched):No role gate on the first two, and the code is unchanged at
ddbda2a. It predates this PR and it is clearly deliberate, but the sentence justifying four doors cannot be "package names are not a viewer's business" while a route in the same file serves 93 of them. Either narrow the rationale to what is genuinely sensitive in the log — host command lines, config paths, registry errors — or gate/packagesand/summarythe same way. As written the module contradicts itself on the only question a reader of that comment will have.Verified, for the record
The withholding claim itself holds, end to end, which nobody had shown before — admin and viewer against the same server, one refresh streaming:
Also exercised, both listed "not exercised" in your verification section: an interrupted run across a real restart (
{"status":"interrupted","reason":"process restarted"}, nothing retried,/run→ "No log for this run — it ran before the last restart. Status: interrupted.") and the failure notification ({"level":"error","title":"Refresh package lists failed","message":"apt-get update exited 100"}). Batching under real load: a 44-package dist-upgrade produced 253 lines in 11 entries.One aside that is core's, not yours: every run that declares a lock logs
WARN run is terminal but its work has not returnedand frees the lock ~5 ms later — 4 runs, 4 warnings.#finishreadsstate.handlerLive, which#executeclears only in its outerfinally, so the normal return path always trips it. Harmless, but this module declareslockon every action, so it is the one turning a wedge detector into background noise. Worth an issue against core.Fix 1 and 2 and I will approve; 3 and 4 I would take as follow-ups if you disagree, as long as the disagreement is written down.
@ -168,0 +277,4 @@run: (h) =>withRunLog(h, "Refresh package lists", async () => {const code = await h.step("apt-get update", () => apt(["update"], REFRESH_TIMEOUT_MS));if (code !== 0) throw new Error(`apt-get update exited ${code}`);This can only fail for a malformed sources file.
apt-get updateexits 0 when repositories fail to download — an unresolvable host, a dead mirror, a suite past EOL — and prints the failure asErr:plusW: Some index files failed to download. They have been ignored, or old ones used instead.Measured on the rig at this head, every source unresolvable:
So the action reports "package lists refreshed" over lists that were not refreshed,
notifyOn: ["failed"]never fires, and/summarykeeps computing counts from whatever is on disk. That is the PR's "the outcome is a status, not a sentence" failing on the most common real refresh failure.One option, measured side by side in the same rootfs:
Scanning the stream for
^Err:/^W: Failed to fetchand failing the step is equally fine. A partially-fetched refresh reported as a partial success would also be fine. "ok" is not.No role gate here, and none on
/summary— whilemod.ts,CLAUDE.mdandREADMEall justify the four-door withholding with "apt and dpkg output is package names, repository URLs, the occasional configuration path — none of it a non-admin's business".Measured as a viewer (I patched the auth-disabled dev user down to
["viewer"]in my core copy; this module untouched):So the same package names and repository origins the log is guarded for go out ungated one route above it. This predates the PR and is plainly deliberate — the table is the module's whole point — but then the reason given for the doors is wrong, and it is now written in three places. Narrow it to what is actually sensitive in a run's log (host command lines,
--env-file-style paths, registry errors, whatever apt prints about a private mirror), or put the samerequireAdminon these two routes. Either is defensible; disagreeing with itself is not.@ -476,3 +708,4 @@// falling mid-upgrade is real data over the only window it moves.if (!host.available && !await host.probe()) return;const s = await load(true);A failed simulation is written into the series as
pending: 0. These three writes sit above theif (s.error !== "") returnon line 723, and a failedapt-get -sreturnspackages: []— so "I could not read the host's package state" and "this host has nothing pending" are the same two points on the chart, and the same dashboard card.Rig, with no upgrade run at all between the samples — broken sources file, restart to fire the tick:
OverviewWidgetreadspending, notstatusText, so its hint says "up to date".This is newly reachable because the
isBusyguard went (rightly): the tick now runs in situations it used to skip, and a broken sources file or a host mount that vanished is not exotic.reboot.daysPendingcomes from a file read rather than apt, so keeping that one above the guard is defensible — the two apt-derived series are the ones that must not fabricate a zero. And since this state makes every number on the page meaningless, it is a better notification candidate than most: edge-triggered, keyedpackagedata, cleared when a collect succeeds.@ -406,0 +616,4 @@// would win it instead; `runView` prefers a non-skipped row as a second// line of defence (tested), but it can only pick from what arrives here.const live = await ctx.tasks.list({ status: [...LIVE] });const recent = live.length > 0 ? live : await ctx.tasks.list({ limit: 1 });The
skippedfilter added torunViewthis round cannot fire on this path:limit: 1meansrecentholds exactly one row, so in the scenario the comment above names — core'sORDER BYchanging to created-at — that single row is the newest refusal,recent.find((r) => r.status !== "skipped")finds nothing, and the chain falls through torecent[0], the skipped row.The new test passes because it hands
runViewa two-row history, which this call never produces.Two extra rows, and the defence becomes one. Worth adjusting the test's fixture to the shape the route can actually deliver, too.
`apt-get update` exits 0 when a repository fails to DOWNLOAD — a dead mirror, DNS that broke, a suite past EOL. Only a malformed sources entry gives 100. So the one failure this module exists to catch was recorded `succeeded`, `notifyOn: ["failed"]` stayed silent, and every count on the page went on describing a snapshot nobody refreshed. `parseRefreshFailures` scans the output for what apt says it could not fetch and the run fails on it. Scanning rather than `-o APT::Update::Error-Mode=any`, which promotes any WARNING to a failure: the rig's own healthy refresh prints a keyring warning that option would have failed the run on. Every `apt-get` is now pinned to `LC_ALL=C.UTF-8` — apt translates those lines ("Err:" is "Fehl:" under de_DE), so without it the scan finds nothing on a host with a locale set. Also, from the same review round: - A failed simulation is no longer written as `pending: 0`. It yields `packages: []`, so the two apt metrics recorded "I cannot read this host" and "this host is up to date" as the same point — on the chart and on the dashboard card, which reads `pending`, not `statusText`. Both writes move below the error guard; `reboot.daysPending` is a file read and stays above it. A gap is honest, a zero is not. The state also raises one edge-triggered alert, on the first tick too, because it is the one state nothing else on the page reports. - `/run` asks history for three rows, not one. `runView`'s skipped-row filter could not fire on a single row: that row IS the newest refusal, so the filter found nothing and the fallback returned it. The test was passing on a two-row fixture the route never produced. - The withholding rationale no longer contradicts `/packages`, which serves the whole pending table to any viewer ungated. What a run's log adds on top of that table is the admin-only part: this module's own `$ apt-get …` command lines, dpkg diagnostics, configuration paths and full repository URLs. Restated in mod.ts, runview.ts, CLAUDE.md, README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Fourth-round items at
7703979. All four are addressed — 1 and 2 fixed, 3 fixed (it was one word), 4 resolved by narrowing the rationale rather than gating the routes, with the reasoning written down where you asked for it.Rig: core
main(db57f4d) plus this branch loaded as an external module (a local clone throughOPSDECK_EXTERNAL_MODULES, so the manifest, frontend build and load path are all real), in a privileged container with adebian:bookwormrootfs bind-mounted at/host/root, probe pickingchroot.deno checkclean,deno lintclean,deno test --allow-read backend/35 passed (17 parser — 11 unchanged plus 6 new — 13 runview, 5 runlog).1. A refresh that fetched nothing reports
succeeded— fixedI measured your premise across four sources states before touching anything, same rootfs,
LC_ALL=C.UTF-8:The exit code catches the bottom two and nothing else — exactly as you had it. Your rig line was the last row.
parseRefreshFailures(pure, inapt.ts, six tests) scans forErr:/W: Failed to fetch/W: Some index files failed to download, and the run throws on it.Scanning rather than
-o APT::Update::Error-Mode=any— and the rig decided that, not taste. A healthy refresh on this rootfs prints:Error-Mode=anypromotes any warning to a failure, so it would have failed a refresh that fetched everything. That is the same lie pointing the other way, and it is the one that teaches an operator to ignore the alert. The scan ignores that line and fails only on what apt says it could not fetch.Err:andW: Failed to fetchrestate one event in two spellings of the same URL, so the second is a fallback for output carrying noErr:lines, never merged with the first — merging reports every dead source twice.Every
apt-getnow runs underLC_ALL=C.UTF-8, which is load-bearing, not tidiness: apt translates the lines being scanned, and a host with a locale set would have gone back to reporting ok.End to end, the module's own run, DNS unreachable from the chroot:
and the negative, sources restored — the keyring
W:above did not fail it:2. A failed simulation recorded as
pending: 0— fixedBoth apt-derived writes moved below the
s.error !== ""guard.reboot.daysPendingstays above it, as you suggested — it is a file read and is still true when the simulation is not.Measured the split rather than asserting it. Broke the sources file, waited one tick, no upgrade run anywhere:
A gap where the samples were, not a zero — and the tick demonstrably ran, because
reboot.daysPendingmoved.And the gap closes again on its own — sources restored, next tick:
I took your notification suggestion. It is edge-triggered and keyed
packagedata, and it fires on the first tick as well — which departs from the convention the other two checks in that tick follow, so the reason is in the comment: those describe state the page shows plainly, whereasOverviewWidgetreadspendingand notstatusText, so an unreadable host renders as "up to date". It is the one state nothing else on the page reports.3. The second line of defence in
runViewcannot fire — fixedctx.tasks.list({ limit: 3 }), and the test fixture is now a shape the route can produce (three rows: twoskippednewer than the succeeded run). I also added the case that shows the defence's reach — one skipped row and nothing else, which still shows the refusal — because the real guarantee remains core'sORDER BY, not this filter, and a test that implied otherwise would be the same mistake in a new place.Also exercised: four refused double clicks against a live refresh (409, runs 4–7
skipped),/runreading the live run throughout and"Refresh package lists — ok"after it landed.4.
/packagespublishes what the four doors withhold — rationale narrowed, routes left openI went with your first option. Gating
/packagesand/summarywould take the module's only reason to exist away from viewers; the sentence justifying the doors is what was wrong, and it was wrong in three places.It now says the log is withheld for what it carries beyond that table: this module's own
$ apt-get …command lines (which spell out how it reaches the host), dpkg maintainer-script and conffile diagnostics, configuration paths, and full repository URLs — credentials and all on a private mirror — where/packagescarries only apt'sorigindescription. Restated inmod.ts(both places),runview.ts,README.mdandCLAUDE.md, andCLAUDE.mdnow carries "the reason is NOT package names" as an explicit invariant with the route named, so the next person does not re-derive the old sentence.Not exercised
The interrupted-run path and the non-admin reader end to end — both of which you verified at
ddbda2a, and nothing in this push touches either. Thepackagedatanotification's dismissal survival across ticks is reasoned from the existing keyed-notification behaviour, not measured.Aside, confirming yours
WARN run is terminal but its work has not returned … lock freedfired on every run here too (runs 2 and 3 in the log). Your reading of#finish/state.handlerLivematches what I see. Happy to open the core issue if you have not.Fifth pass, at
7703979, against coremain(now 2844b43). Both round-four blockers are genuinely fixed and I drove each on the rig rather than reading the diff for it — this time under a German server locale, because your own fix says that is where it breaks.deno checkclean,deno lintclean, 35/35.1 — the refresh that reported ok. Unreachable source, module's own run:
And the negative, which matters more: on a rootfs that prints
W: GPG error …and twoE: gpgv, gpgv2 or gpgv1 required for verificationlines on a complete refresh, the run came backsucceeded. No false positive against noisy-but-healthy output.The locale pin is load-bearing and it works. Server env
LC_ALL=de_DE.UTF-8, and the run above still produced English and failed correctly, while the same command by hand in the same chroot:— which
parseRefreshFailureswould have found nothing in. Catching that before it shipped is the good catch of this round.2 — the fabricated zero. Broke the sources file, one tick, no run anywhere:
A gap where the samples were,
reboot.daysPendingproving the tick ran, and the alert that makes the state visible at all. Exactly the split I asked for.3 —
limit: 3. Four refusals against a live refresh (runs 4–7skipped), pane read"Refresh package lists running"throughout and"Refresh package lists — ok"with the log after it landed. The fixture now matches what the route produces, and the extra test for the defence's reach is the right instinct.4 — the rationale. Narrowing it rather than gating the routes is the call I would have made, and naming what the log adds on top of
/packages— your own$ apt-get …command lines, dpkg maintainer-script and conffile diagnostics, full repository URLs with credentials — makes the doors defensible instead of decorative.Approving. Two follow-ups, both small, neither worth holding a merge for — but the second one is a fact about apt that is now written into the code and is not true.
A. The locale pin skips the one apt run whose output feeds every number on the page
apt.tssays everyapt-getthis module runs is pinned toLC_ALL=C.UTF-8. The simulation is not:state.ts:103goes throughdeps.host.run("apt-get", ["-s", …])directly, not throughapt()in mod.ts, so it inherits whatever the OpsDeck process has. On the rig that showed up immediately — in the notification you just added:parseSimulationitself survives today only becauseInst/Confare not translated — I checked, 42Instlines under bothC.UTF-8andde_DE.UTF-8. That is luck, not the rule this PR just wrote down. Pin it the same wayapt()does and the rule holds for both parsers, plus the module's only user-facing error string stops depending on the server's environment.B.
Error-Mode=anydoes not do what the comment says it doesThe comment argues the option "promotes any WARNING to a failure, so a duplicated-source or deprecated-key warning would fail an otherwise complete refresh". Measured — including in your rootfs, the one whose warnings the argument rests on:
Error-Mode=anyW: GPG error+E: gpgv … requiredW: Target Packages … configured multiple times)Signed-By: /dev/nullkeyring warningIt promotes acquire failures, and nothing else I could find. So the premise is right — a healthy refresh on that rootfs really does print warnings — and the conclusion drawn from it is not.
I would still keep the scanner: it names the sources, which is what makes the notification worth reading, and it is testable in a way an apt option is not. But the comment should say why it was actually chosen, not assert an apt behaviour that does not reproduce — and since the scan is a text contract with apt, the option is worth adding underneath it as a belt (scan first so the good message survives, then let a non-zero exit fail the run anyway). That way the hole does not silently reopen the day apt renames
Err:.Notes
packagedatafiring on the first tick means a restart loop recreates a dismissed alert — which is precisely when a broken sources file restarts a lot. You documented the deviation and the reason is sound; just noting the cost is real.WARN run is terminal but its work has not returned/state.handlerLivething. Go ahead and open it — you have the reproduction in your own log now, and it is core's to fix.@ -166,0 +180,4 @@* refreshed" over lists that were never refreshed, which is the one failure a* module whose whole job is "the host is behind" must not swallow.** Scanning the output rather than passing `-o APT::Update::Error-Mode=any`:The choice is right; this reason for it is not, and it is now recorded as fact.
APT::Update::Error-Mode=anydoes not promote any warning — it promotes acquire failures. Measured, including in the rootfs whose warnings this argument rests on:Error-Mode=anyW: GPG error+E: gpgv … requiredW: Target Packages … configured multiple times)Signed-By: /dev/nullkeyring warningSo "a duplicated-source or deprecated-key warning would fail an otherwise complete refresh" does not reproduce.
Keep the scan — it names the sources, which is what makes the notification worth reading, and it has tests an apt option cannot have. But say that, rather than an apt behaviour that is not real: the next person reading this will believe it and rule the option out for the wrong reason.
Worth considering as a belt underneath the scan, since this function is a text contract with apt's output and apt renames things:
@ -99,1 +100,4 @@// assumed: rc=0 with and without the flag, counts tracking the upgrade as// it lands. This line is what makes it safe for load() and the collector to// simulate mid-run; do not add an `isBusy("apt")` guard upstream of it.const res = await deps.host.run(This is the
apt-getthe pin misses.apt()in mod.ts pinsLC_ALL=C.UTF-8; this one goes throughdeps.host.rundirectly and inherits the OpsDeck process environment — so the doc inapt.ts("everyapt-getthis module runs is pinned") is not true of the run whose output feedspending,security,/packagesand the dashboard card.Rig, server started with
LC_ALL=de_DE.UTF-8, sources file broken:That string is this module's only user-facing error text, and it is currently whatever locale the server happens to run under.
parseSimulationsurvives today only because apt does not translateInst/Conf— measured, 42Instlines under bothC.UTF-8andde_DE.UTF-8. Fine today; it is exactly the assumption the refresh scanner just proved dangerous, and nothing here pins it.(
uname -ron line 146 is in the same boat and does not matter — but the comment inapt.tsshould say "every apt-get whose output is parsed", and then be true of both.)