Follow-ups from PR #1 review: pin the simulation's locale, and correct the Error-Mode=any rationale #2

New issue

Open

opened 2026-08-20 10:51:19 +02:00 by julian · 0 comments

julian commented

2026-08-20 10:51:19 +02:00

Owner

Both items are from the approving review on #1 (review), explicitly not merge blockers. Filed so they are not lost.

1. The locale pin skips the simulation — the one apt run whose output feeds every number on the page

backend/apt.ts:195-196 states that "every apt-get this module runs is pinned to LC_ALL=C.UTF-8 (see apt() in mod.ts)". That is not true of loadPackages in backend/state.ts:103, which calls deps.host.run("apt-get", ["-s", ...]) directly and so inherits whatever locale the OpsDeck process runs under.

Measured on the rig with the server started under LC_ALL=de_DE.UTF-8 and a broken sources file:

GET /summary → {"pending":0,"statusText":"E: Paketdatei /etc/apt/sources.list.d/debian.sources konnte nicht verarbeitet werden (1)."}
notification → "Package data unreadable" | same German string

Two consequences:

statusText is the module's only user-facing error string, and it currently depends on the server's environment.
parseSimulation survives today only because apt does not translate Inst/Conf — verified, 42 Inst lines under both C.UTF-8 and de_DE.UTF-8. That is luck, and it is exactly the assumption the refresh scanner (parseRefreshFailures) was just written to stop relying on.

Fix:

const res = await deps.host.run(
  "env",
  ["LC_ALL=C.UTF-8", "apt-get", "-s", "-o", "Debug::NoLocking=true", "dist-upgrade"],
  { timeoutMs: 120_000 },
);

and widen the apt.ts comment to "every apt-get whose output is parsed", so it is true of both parsers. (uname -r at backend/state.ts:146 is in the same position but its output is not locale-dependent in any way that matters.)

2. `APT::Update::Error-Mode=any` does not promote warnings — the comment says it does

The comment at backend/apt.ts:183 justifies scanning apt's output instead of using the option on the grounds that it "promotes any WARNING to a failure, so a duplicated-source or deprecated-key warning would fail an otherwise complete refresh". Measured, including in the rootfs whose warnings the argument rests on:

case	plain	`Error-Mode=any`
healthy refresh, rootfs printing `W: GPG error` + `E: gpgv … required`	0	0 (2 Packages lists fetched)
duplicated source (`W: Target Packages … configured multiple times`)	0	0
`Signed-By: /dev/null` keyring warning	0	0
unresolvable source	0	100

It promotes acquire failures and nothing else that reproduces. The premise is right — a healthy refresh on that rootfs really does print warnings — but the conclusion drawn from it does not hold, and it is now recorded in the code as fact.

The scanner stays either way: it names the failing sources, which is what makes the notification worth reading, and it is testable in a way an apt option is not. Two changes:

Rewrite the comment to give the real reason (named sources, testability) instead of an apt behaviour that does not reproduce.
Consider adding the option underneath the scan as a belt — the scan is a text contract with apt's output, and apt renames things. Scan first so the specific message wins, then let a non-zero exit fail the run anyway:

const code = await h.step(
  "apt-get update",
  () => apt(["-o", "APT::Update::Error-Mode=any", "update"], REFRESH_TIMEOUT_MS, (t) => text += t),
);
const failed = parseRefreshFailures(text);   // scan FIRST, so the good message wins
if (failed.sources.length > 0) throw new Error(`apt-get update could not fetch …`);
if (failed.staleLists) throw new Error("apt-get update kept the old package lists — some sources failed");
if (code !== 0) throw new Error(`apt-get update exited ${code}`);

Both items are from the approving review on #1 ([review](https://git.imhof.cloud/OpsDeck/module_updates/pulls/1#issuecomment-849)), explicitly not merge blockers. Filed so they are not lost. ## 1. The locale pin skips the simulation — the one apt run whose output feeds every number on the page `backend/apt.ts:195-196` states that "every `apt-get` this module runs is pinned to `LC_ALL=C.UTF-8` (see `apt()` in mod.ts)". That is not true of `loadPackages` in [backend/state.ts:103](backend/state.ts#L103), which calls `deps.host.run("apt-get", ["-s", ...])` directly and so inherits whatever locale the OpsDeck process runs under. Measured on the rig with the server started under `LC_ALL=de_DE.UTF-8` and a broken sources file: ``` GET /summary → {"pending":0,"statusText":"E: Paketdatei /etc/apt/sources.list.d/debian.sources konnte nicht verarbeitet werden (1)."} notification → "Package data unreadable" | same German string ``` Two consequences: - `statusText` is the module's only user-facing error string, and it currently depends on the server's environment. - `parseSimulation` survives today only because apt does not translate `Inst`/`Conf` — verified, 42 `Inst ` lines under both `C.UTF-8` and `de_DE.UTF-8`. That is luck, and it is exactly the assumption the refresh scanner (`parseRefreshFailures`) was just written to stop relying on. Fix: ```ts const res = await deps.host.run( "env", ["LC_ALL=C.UTF-8", "apt-get", "-s", "-o", "Debug::NoLocking=true", "dist-upgrade"], { timeoutMs: 120_000 }, ); ``` and widen the `apt.ts` comment to "every `apt-get` whose output is parsed", so it is true of both parsers. (`uname -r` at [backend/state.ts:146](backend/state.ts#L146) is in the same position but its output is not locale-dependent in any way that matters.) ## 2. `APT::Update::Error-Mode=any` does not promote warnings — the comment says it does The comment at [backend/apt.ts:183](backend/apt.ts#L183) justifies scanning apt's output instead of using the option on the grounds that it "promotes any WARNING to a failure, so a duplicated-source or deprecated-key warning would fail an otherwise complete refresh". Measured, including in the rootfs whose warnings the argument rests on: | case | plain | `Error-Mode=any` | |---|---|---| | healthy refresh, rootfs printing `W: GPG error` + `E: gpgv … required` | 0 | **0** (2 Packages lists fetched) | | duplicated source (`W: Target Packages … configured multiple times`) | 0 | **0** | | `Signed-By: /dev/null` keyring warning | 0 | **0** | | unresolvable source | 0 | **100** | It promotes *acquire failures* and nothing else that reproduces. The premise is right — a healthy refresh on that rootfs really does print warnings — but the conclusion drawn from it does not hold, and it is now recorded in the code as fact. The scanner stays either way: it names the failing sources, which is what makes the notification worth reading, and it is testable in a way an apt option is not. Two changes: - Rewrite the comment to give the real reason (named sources, testability) instead of an apt behaviour that does not reproduce. - Consider adding the option underneath the scan as a belt — the scan is a text contract with apt's output, and apt renames things. Scan first so the specific message wins, then let a non-zero exit fail the run anyway: ```ts const code = await h.step( "apt-get update", () => apt(["-o", "APT::Update::Error-Mode=any", "update"], REFRESH_TIMEOUT_MS, (t) => text += t), ); const failed = parseRefreshFailures(text); // scan FIRST, so the good message wins if (failed.sources.length > 0) throw new Error(`apt-get update could not fetch …`); if (failed.staleLists) throw new Error("apt-get update kept the old package lists — some sources failed"); if (code !== 0) throw new Error(`apt-get update exited ${code}`); ```

julian referenced this issue from a commit

2026-08-20 10:51:33 +02:00

feat: hand the update runs to core (#1)

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

OpsDeck/module_updates#2

No description provided.

Rows
Columns

Follow-ups from PR #1 review: pin the simulation's locale, and correct the Error-Mode=any rationale #2

1. The locale pin skips the simulation — the one apt run whose output feeds every number on the page

2. APT::Update::Error-Mode=any does not promote warnings — the comment says it does

2. `APT::Update::Error-Mode=any` does not promote warnings — the comment says it does