feat(docker): the docker module roadmap, end to end #66
julian
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/docker-storage"
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?
85 commits, 183 files. It is large because it is the whole docker roadmap rather than one story, and it carries two independent workstreams. They touch different files almost entirely, so they can be read separately:
1. The self-update sidecar (2 commits)
4bfb14fupdate OpsDeck from OpsDeck through a sidecar container2f28e84rollback keeps the stack updatable, verify runs onceA detached host process cannot work — a
setsidchild stays in the container's cgroup, which the runtime kills once PID 1 exits. So the successor is started by a throwaway sidecar built from OpsDeck's own image, which is removed once the new process has read its verdict.docs/docker-self-update-sidecar.mdhas the reasoning.Includes one core change,
packages/server/src/tasks/service.ts:start()now skips runs already held in#runs, because a run started from a module'sregister()was being re-adopted from the store at boot and its handler launched twice. Regression test inpackages/server/tests/tasks_test.ts.2. Everything else — the docker module roadmap
Storage (measurement, prune plans, bind mounts, logs), images, networks, volumes as a managed thing, the engine page, compose verbs, version pinning, per-service scope and auto-update, and the operations surface over the task system.
Worth a reviewer's attention
The task system.
service.tsgained a second fix (dd5f4e9): every ordinary run was taking the released-early branch, becausehandlerLivewas cleared in afinallythat ran after#finishhad read it. Result was a warning on every run saying its work had not returned when it just had, plus a claim on a lock nobody held. The window closes before any awaiting caller resumes, so nothing downstream misbehaved — but it made the real signal (a released run whose work is still moving containers) indistinguishable from noise.Mobile parity is now enforced by a test. A schema client's navigation is one entry per module and nothing under it, so a page nothing links to does not exist. Four were in that state.
ui_test.tsnow walks the link graph so the next page cannot be added orphaned.One fact, one derivation. Several bugs here were the same shape: a verdict computed in two places that disagreed. A container in a restart loop was red on the stack card and green on its own row, because the row re-derived state from
statewhile the server counts restarts. Removal refusals printed a machine token (not-in-plan). A compose failure paired the reasonregistry-deniedwith a detail saying the tag did not exist. Each is now single-sourced.DOCKER_CONFIGused to lie. It named where the host keeps registry logins and governed only what OpsDeck reported — the chrooted docker CLI went on reading/root/.docker. Setting it produced a UI claiming an authentication every pull then failed.ExecOptionsgainedenvand the module binds it once onto its host facade.Verification
deno task check,deno lint,check:svelteclean; 188 server tests, 643 docker module tests, 57 UI tests.:latestshipping a broken build) diagnosed through it.Known gaps, stated rather than hidden
mod.ts(3.7k lines) andactions.ts(2.4k) are past a comfortable size. The seams exist in the comment banners; splitting them is a large no-behaviour-change diff I did not want inside this one./fileor/label.🤖 Generated with Claude Code
Where a container keeps its data, how its output is written, and where the image said the data should go. Three facts the stacks poll already had an inspect open for and threw away. `parseMounts` reads BOTH places the engine keeps them. `Mounts` holds binds and volumes; compose's `tmpfs:` shorthand never reaches it and lives in `HostConfig.Tmpfs` instead, so a table built from `Mounts` alone shows a container's disk and silently omits the RAM it also writes to. Only `--mount type=tmpfs` appears in both, which is why the merge deduplicates by target. `max-size` stays the operator's own string ("10m") rather than being parsed to bytes. It is echoed back into a compose snippet the operator copies, and a value round-tripped through a byte count comes back as 10485760. `unmountedDeclared` is the story-4 warning: a `VOLUME` the image declares with nothing mounted at it is a container writing its state into its own writable layer, where the next recreate throws it away. A mount at a PARENT of the declared path counts — a volume at /var/lib/postgresql keeps /var/lib/postgresql/data off the layer just as well — and the comparison is segment-wise, so /var/lib/postgres is not a parent of /var/lib/postgresql/data. The inspect stays enrichment: when it fails, every field it feeds keeps its empty default and the container is still listed, which is the one thing the stacks page exists to show. `stripImageDefaults` now takes `Pick<ImageConfig, "env" | "labels">`. `volumes` is on ImageConfig for the mount table, and asking three callers to state a field the function never reads is how a test comes to assert a value that decides nothing. The fake containers gain shapes the storage stories need: an unbounded json-file log on jellyfin, a rotated one elsewhere, a bind of /var/run/docker.sock on traefik, and a postgres that declares /var/lib/postgresql/data with nothing mounted at it. Done before WI-12 rather than after it, and the plan's order is updated to match: the snapshot's Logs kind, its per-stack bind mounts and its marking of OpsDeck's own volumes are all built from these fields. In the original order three WIs would each have shipped a Logs kind reading zero over an empty set, and this one would have gone back to rewire an action it did not write.Story 19: the escape hatch that makes "unused" and "unimportant" different clicks. The engine has no volume-level archive endpoint — the only way to read a volume's bytes over the API is `/containers/{id}/archive` against something that has it mounted. So the whole problem is "which container do we read this through", and the answers are ordered by how little they disturb: a stopped holder first, because tarring under a live writer archives a half-written moment; then a running holder, which at least owns the data; then a container we create. The addressed path is the mount TARGET inside the container, never the volume's host mountpoint. The archive endpoint resolves paths in the container's own filesystem, so handing it /var/lib/docker/volumes/x/_data tars whatever happens to be at that path in there — usually nothing. A throwaway reader uses an image already on the host and is never started. Pulling one would make "download my data" depend on a registry being reachable, which is often exactly why someone is taking a backup. `AutoRemove` is off so it cannot race our own delete and hide the failure if it loses, and the delete runs in a `finally` — a throwaway that outlives its request is a container the next prune offers to delete. Export is not a run: the bytes go to the caller, so there is nothing for a run row to hold. Import is, because it writes and outlives its request; the upload is staged to a file, and the staged file is removed in a `finally` whatever happened. Importing over live or non-empty data is refused rather than merged. A tar's files landing beside what is already there leaves neither the backup nor the original, which is the worst of the three possible outcomes. The cancel is honestly limited and the comment says so: aborting stops the PUT, and what already landed stays. An import is not atomic and pretending otherwise would be the lie.Eight pages each did their own header, their own back link and their own vertical rhythm, and one of them — the engine page — quietly lost its navigation entirely. `DockerPage` now owns the title, the section nav and the spacing between cards, so a page is only responsible for what is inside it, and `Facts` replaces the hand-rolled definition lists, two of which had no CSS at all and rendered as a label on one line and its value indented on the next. Content stops at a readable measure. The shell gives a module up to 1680px, and a key/value list stretched across that puts a label at one edge of the screen and its value at the other; a table or a chart opts out with `wide`. The host card on the front page was the worst of it — the composition legend's elastic column opened a gutter the size of the legend itself, and the filesystem bar printed its percentage twice. Networks became something you can manage rather than read: create (attachable and internal offered, since neither can be added afterwards), remove, prune. Members are a table, because three facts per member only mean anything read down a column. Images gained per-row remove — disabled where a container is on the image, since the engine's refusal is a worse way to learn that than a dead button — and the storage page's Logs tab gained the truncate that only the container page had, plus a download of the same file. Six prune buttons in a row became one `Prune…` with the choice behind it: each kind listed with what it would actually free and the sentence that explains it, which is also where "what is superseded?" is now answered. What it deliberately does not do is let several be ticked at once — clearing the build cache must never be one mis-click from removing a volume. Fixes found while testing this by hand: - A prune dialog dismissed by its backdrop, Escape or ✕ left the page still holding a control it thought was on screen, so opening the same prune a second time changed no state and nothing happened. - **Volume export and import were not a round trip.** The engine roots its tar at the basename of the path it was given, so an exported volume came back as `volume/…` and importing it produced `/volume/volume/note.txt` — a backup that restores to the wrong place, with both halves reporting success. The import now reads the archive's first header and extracts into the parent when the prefix is the mount's own name. Verified end to end against a real engine: a volume with a file and a subdirectory exports, imports into a fresh volume, and comes back identical. - A page header's actions mixed buttons with the odd bare line of text ("measured 2 min ago"), which sat on different baselines. - The reclaimable chart printed raw bytes on its axis — 100000000000.0 where 93.1 GiB belongs. - Card borders at 8 % over the page tint were an edge you had to look for.`docs/configuration.md` lists all 34 core variables and every module key with its default and what happens when it is unset, in the places an operator looks: the server, modules, auth, retention, host access, push, then one table per module. The README links it and says the prefix rule once, because `OPSDECK_MOD_DOCKER_STORAGE_INTERVAL_MS` is not guessable from `STORAGE_INTERVAL_MS` in the code. A test walks the core schema and every module's `ctx.config.get` and asserts each key appears in the reference. A knob nobody documents is a knob nobody finds, and that failure is silent — the code keeps working and the operator keeps not knowing — so the suite fails instead of the next person. Verified by pointing a module at an undocumented key and watching it go red. fix(docker): the self-update version came from the module's own namespace. `ctx.config.get("OPSDECK_VERSION")` reads `OPSDECK_MOD_DOCKER_OPSDECK_VERSION`, which nothing sets, so the verification compared "dev" to "dev" and could never confirm an update. It reads the process environment the build stamps, with module-scoped `VERSION`/`COMMIT` kept as an override for tests.Eighteen findings, and two of them were real bugs rather than rough edges. **A dialog nobody could reach.** `Modal` never took focus, so Escape went to the page behind it, Tab walked that page, and a screen reader was never told a dialog had opened. It now moves focus in, traps Tab at both ends, closes on Escape and puts focus back where it came from. The prune review, the prune picker, the file editor, the create-stack form and the update progress all inherit it. **A create modal that only rendered while a job was running.** The create-stack dialog had been inserted inside `{#if job.project}` — the conditional that belongs to the update-progress and scope modals — so "New stack…" opened nothing on an idle host. Lifted out, along with the new confirmation dialog that had landed in the same place. The rest: - Stopping a stack or a container, and force-recreating a stack, fire from an icon and fired with no question asked. The schema has always confirmed them; the web now does too, with a sentence naming what it touches. - Create fired once per click. `busy` reaches the dialog's buttons now, and the handlers refuse a second entry, because the disabled state lands a frame later than a fast double-click. - "Prune unused…" was enabled from the page's own orphan count while the dialog was built from the plan, so it opened on "nothing to remove". Both now read the plan, and the fake host's snapshot carries the same networks its listing invents. - Names: the empty one only failed server-side, and there was no length at all — a 300-character volume was accepted and then wrecked the table. One rule, checked in the dialog and at the route, capped at 64. - A pull with junk in the box opened a progress modal titled "Pulling not a tag" and then showed the route and the JSON shape it wanted. Validated before anything opens, and the API's own words no longer reach the screen. - An unreadable `since` was dropped by the server while the page went into its bounded state — follow off, "jump to now" showing, full tail on screen. It now says what is wrong with it and stays unbounded. - The container's log row drew its badge on top of its download link. - The storage page's open tab lives in the URL, like the container page's. - Saving a file said nothing; it names the file it wrote. - "Age" showed a calendar date; it shows an age, with the date on hover. - Disabled controls say why: an image in use, a replica count that has not changed, a prune with nothing in it. - Fake data: creates persist for the process and appear in their lists, a `.env` reads like an env file, the log tail ends with a newline so the first pushed line does not land on the end of it, and the history empty state stops being 200px tall. - The typed confirmation was case-sensitive in one dialog and not in the other. It is case-insensitive in both, and the routes agree, because a button that enables and then 400s is worse than either rule. - The phone gains network create and prune, and the two image prunes. Per-row removal stays web-only: it is `DELETE /volume/:name`, a schema action only POSTs, and pointing one at the prune route would have taken its default selection instead of the named volume.Ten places in this module pulled the server's sentence out of an ApiError, each with its own copy of the `e.body as { error?: string }` cast, and two of them additionally re-derived "is this the depends_on 409". The envelope is one shape; a change to it should not be a ten-file edit. errors.ts owns the three readings: errorBody for a body a page wants more than the message out of, conflictBody for the fields that belong to a 409 alone, errorText for the sentence. No behaviour change — every call site keeps the message it produced before, including StacksPage's "already running" special case and FileEditor's stale-sha branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>WI-43's first cut launched the compose run as a detached host process (systemd-run, else `setsid … &`). A setsid child is still in the container's cgroup, which the runtime kills once PID 1 exits, so compose died between "old container removed" and "new container created" and left the host without OpsDeck. systemd-run escapes the cgroup only with `pid: host` on a systemd host, and nothing supervised the result either way. The launch is now a sidecar: a throwaway container created through the engine API from OpsDeck's own current image, with the host-root bind read off our own inspect rather than assumed, running the same `chroot /host/root docker compose up -d` every other stack update uses. It gates the new container (HEALTHCHECK, then /api/core/health via docker exec, then 30 s without a restart) and rolls back to a tag of the previous image when the gate fails, because at that moment no OpsDeck may exist to do it. The `self-update` run does preflight (own inspect, the image the compose FILE names) and the pull in-process, writes the marker with the whole handoff before anything irreversible, hands off, and is expected to end `interrupted` — the honest record. The successor's verify run waits for the sidecar, reads its verdict and holds it against the image it is actually running before closing the story; markers from the first cut keep the version-only verification. Routes answer 409 with the reason when the sidecar cannot run and require `confirmed=1`; the stack read carries `selfUpdate: { available, reason }`; `listStacks` leaves `opsdeck.sidecar` containers out. The stack page follows the run in the usual modal with a watcher that reloads once a different process answers /api/core/system (a rollback brings the same version back), and the page that comes back picks up the verify run. The schema gets an "Update OpsDeck" action in its own block — the first is at the eight-action cap. The dev rig now runs OpsDeck as a compose project (rendered from the same argument list, `pull_policy: never`), builds `rig-next` and `rig-broken` variants, and gains two scenarios; the happy path has run green. Known gap: the verify run does not yet show the sidecar's own log lines.Reviewed at
0609043(85 commits, 183 files). Read as two workstreams plus the core changes, each area against the full files rather than only the diff.Verified locally
deno fmt --check,deno lint,deno task check,deno task check:svelte: clean.deno task test: 1007 passed, 0 failed, 1 ignored (the pre-existing windows-only case incommit_test.ts).test:builder3 passed.service.ts: both fixes are correct. The#runs.has()guard instart()stops the re-adoption double launch, and moving#finishout of thetrymeanshandlerLiveis false before it is read.tasks_test.ts:1804and:1860genuinely exercise the old behaviour.exec.tsenv: Deno.Command mergesenvover the parent environment unlessclearEnvis set, so the comment is right and PATH/HOME survive.mod.ts/actions.ts): every mutating route carriesrequireRole("admin"); ids either match/^[0-9a-f]{12,64}$/, go throughencodeURIComponent, or are resolved against the snapshot before use. No string-built shell commands. Prune re-validates the plan at execute time; self-container refusals are by identity, not by absence from the plan.only.Blocking
Digest-pinned self-update rollback leaves the stack unable to
up.self_update.sh:110-114: whenTARGET_REFis a digest reference, the rollback writes$PROJECT_DIR/.opsdeck-rollback.ymlwithimage: $ROLLBACK_TAGand brings the service up from it. The comment above explains the file must stay because the recreated container's compose labels name it. Fine so far.actions.ts:2384(removeSidecarLeftovers) then deleteshandoff.rollbackTagunconditionally, and the verify run calls it on every outcome (actions.ts:2746). The comment there says "after a rollback the file's own reference names that image and stays", which is true for the retag path but not for the digest path: the override's only reference to the old image is the tag that was just removed. The container keeps running, but the nextcompose up -don that stack (any config change, or the next update attempt) resolves the override, finds no local image for the tag, and tries to pull a name that never existed on any registry.Fix options: in the digest case write the override with the old image's
RepoDigests[0](the image is still pulled, soname@sha256:...resolves locally) instead of the rollback tag, or record inSelfUpdateHandoffthat an override was written and skip the tag delete when it was. Either way the dev-rig only drives tag references (scenarios/self-update.sh,self-update-rollback.sh), so a digest scenario would have caught this and should be added.Should fix, not blocking
mod.ts:1851volume export:await res.arrayBuffer()on the archive stream. The comment says reading fully is deliberate so the throwaway container can be released, but on a large volume this is the server's RSS. Streamingres.bodythrough and releasing in afinallyattached to the stream's close (or aTransformStreamflush) keeps the same ordering without the buffer. Same shape atmod.ts:2362for log download whentail=all.docs/docker-self-update-sidecar.md:186-191and:395still describe the override at/run/opsdeck-self-update/rollback-<runId>.ymland anrm overrideon success. The code (and the note at doc line 17) keeps it in the project directory on purpose. Reconcile the doc with the script.registry_auth.ts:218:{ DOCKER_CONFIG: dockerConfig, ...opts?.env }lets a caller'senvoverride the binding the facade exists to enforce. Reverse the spread order.actions.ts:2196compose edit writes withDeno.writeTextFiledirectly. A crash mid-write leaves a truncated file; the restore rollback covers the failure case but not the crash case. Temp file plus rename is cheap here.Nits
frontend/diff.ts:28splits on\nonly; CRLF content pasted into the editor diffs every line. Normalise\r\nfirst.FileEditor.svelte: closing the modal with unsaved edits discards them silently. A dirty check before close is enough.stats.ts:71:online_cpus === 0yields 0% rather thannull. Guardcores <= 0.stats.ts:113: memory limit0from the engine means unlimited; it currently falls out aspercent = nullby accident oflimitBytes = 0. An explicit check reads better.ui/sort.ts: missing values now sort last whereDataTableused to sort them first vialocaleCompare(""). Intentional and tested, but it is a behaviour change for the zfs pages that shareDataTable.Everything else read clean: prune planning, volume import guards, registry credential handling (never reaches the UI, sent only to the realm over HTTPS), loop detection single-sourced, compose edit path confinement, orphan-page test, Svelte cleanup on unmount.
Thanks, all of it verified against the code before touching anything. Point by point, at
33742c0:Blocking
Digest rollback override names a tag the cleanup deletes. Confirmed, and fixed in
d7abc9bwith theRepoDigestsapproach. In the digest case, the handoff now inspects the old image and recordsrollbackRef: arepo@sha256:reference on it, preferring the target's repository.ROLLBACK_REFgoes to the sidecar, and the override names${ROLLBACK_REF:-$ROLLBACK_TAG}. Your fix left one hole: an image with noRepoDigestat all (built ordocker loaded locally) has nothing to fall back to. That case still writes the tag, andremoveSidecarLeftoversnow takes the sidecar result and keeps the tag whenkeepsRollbackTag()holds (digest target, norollbackRef, notok).rollbackRefis optional inisMarker, because the successor reads a marker the previous build wrote.Tests:
self_update_test.tsruns the realself_update.shagainst achrootshim for all three paths (digest with ref, digest without, tag). I checked that the first assertion fails on the old script. I also added the rig scenario you asked for,docker/dev-rig/scenarios/self-update-rollback-digest.sh. It runsregistry:2inside the rig, pushes base/broken, pins the file to the broken digest, and checks that the override names the base digest, that the digest resolves locally and thatcompose upon the stack still works after cleanup. It has not been run yet: I haven't had a rig up for it, and it needsregistry:2to be pullable inside the rig.A related problem you didn't raise, now in the doc's risk list (
71e3ac8) rather than fixed: the restored container's labels name the override, so the override also shadows the next digest the operator writes into the file until someone deletes it. That is a design question (re-pin throughpin_applyis the real fix, already listed as a follow-up), not something for this PR.Should fix
mod.tsvolume export buffering. Confirmed, and the route's own JSDoc already claimed "the stream is handed straight through". Fixed in5b77172:releasingStream()(volume_archive.ts) releases the throwaway reader once, when the stream is drained, cancelled by the client, or errors. The route releases directly only if it never handed the stream over. Tested for all three.tail=all. Confirmed, same commit. It now streams through the existinglogStreamDemuxer(the one the live tail uses), and there's a test with a frame split across chunks. While in there, I found thatstdcopyDemuxerdidbuf.slice()per frame, copying the rest of the chunk for every log line. That's quadratic per chunk and would have hurt on exactly this path, so it's nowsubarray.71e3ac8.registry_auth.tsspread order. Fixed in85480f8, but I'd tone down "the binding the facade exists to enforce": no caller passesDOCKER_CONFIGtoday, and the callers are this module's own code, so it was a default rather than an enforced binding. The reversed order is still the better contract, and the test now pins it.d40baf4, though "cheap" undersells it. The write ishostFs().write, which the pin and label writers share, so this changes every write to an operator's compose file. A plain temp+rename would have (a) turned a symlinked compose file into a regular file and (b) handed the operator's file to root with the umask mode.replaceFile()resolves the symlink target, carries mode and owner over, and falls back to an in-place write if chown is refused. It's tested for symlink, mode, no temp left behind, and a fresh file.Nits
diff.tsCRLF: I disagree with the diagnosis and the fix. Pasted CRLF never reachestext, because a textarea's value normalises CRLF to LF. The real case is a CRLF file on disk: after one keystroketextis all LF. The preview then showed every line changed, and that was true, because the save really did rewrite every line ending. Normalising only indiffLineswould have made the preview hide a whole-file rewrite. Soa23c0b3fixes the cause instead: the editor works on LF (splitEol), and the save restores the file's own endings (joinEol). The preview is then both small and accurate. There's a test.a23c0b3.Modalhas a newonbeforeclosehook, and X/Escape/backdrop with a dirty buffer now keeps the dialog open with an inline "Discard them / Keep editing". I used an inline prompt rather than a nested ConfirmDialog, because two focus traps fight each other.stats.ts:71zero cores. Fixed in33742c0.stats.ts:113limit 0. Fixed in33742c0. You called it cosmetic, but it wasn't quite: limit 0 producedunlimited: falsewithlimitBytes: 0, a limit of nothing. It now reads as unlimited, and there's a test.ui/sort.tsmissing-last. No change. It's intentional, the comment incompareValuessays why ("a reversed sort that opens with a screen of dashes has buried the rows the operator reversed it to see"), and it's tested. For the zfs pages it's the same improvement.Checks at
33742c0deno fmt --check,deno lint,deno task checkandcheck:svelteare clean (svelte warning counts unchanged from before).deno task test: 1015 passed, 0 failed, 1 ignored (+8).test:builder: 3 passed.Two commits (
85480f8,a23c0b3) are unsigned, because the YubiKey touch timed out.33742c0aa61c76917b80Force-pushed to sign two commits that went out unsigned. The tree is identical to
33742c0. New SHAs for the ones in my reply above:85480f8→1a6043e,d40baf4→3a69570,a23c0b3→2f01d12,33742c0→1c76917.Re-review at
1c76917, covering the seven commits since0609043(d7abc9b..1c76917).Verified locally
deno fmt --check,deno lint,deno task check: clean.deno task test: 1015 passed, 0 failed, 1 ignored.Previous blocker: resolved
The digest rollback is fixed.
digestRollbackRefprefers the target's repository and falls back to any localRepoDigest, both of which resolve without a pull. The override names${ROLLBACK_REF:-$ROLLBACK_TAG}and quotes it properly.keepsRollbackTagcovers the no-RepoDigest hole you found. The calls that pass noresult(the create/start failures) run before any override exists, so deleting the tag there is correct.self_update_test.tsruns the real script for all three paths.The rig scenario
self-update-rollback-digest.shhas not been run. Please run it before or shortly after the merge. It is the only end-to-end proof of the digest path.Should fix (follow-up is fine)
replaceFileresolves absolute symlinks in the container's namespace, not the host's.pin_apply.tsreplaceFilecallsDeno.realPath(hostPath(file)). When the strategy is notdirect,hostPathgives/host/opt/stack/compose.yaml. If that path is a symlink to/srv/stacks/compose.yaml, the kernel resolves the target inside OpsDeck's container rather than under/host. Two outcomes:realPathgets NotFound, the code falls back topath, and the rename replaces the operator's symlink with a regular file. That is the exact case the commit says it prevents, and/srv/stacks/compose.yamlgoes stale.The test (
pin_apply_test.ts:458) uses an absolute symlink with no host-root prefix, so it cannot catch this. The same resolution was already in the oldwriteTextFileand in everyreadTextFile(hostPath(p)), so this is a class issue and not a regression. Still,replaceFilenow claims to preserve symlinks. A resolver that follows links one hop at a time withDeno.readLink, re-roots absolute targets under the host root and re-checks for escape would fix reads and writes together.Nits
volume_archive.tsreleasingStream: ifrelease()throws, its error replaces the read error in thecatchbranch. The stream still errors, becausepullrejects, but the operator sees the cleanup failure instead of the cause.await once().catch(() => {})beforectl.error(e)would keep the cause.diff.tssplitEol: a file with mixed endings (any CRLF present) is saved as all-CRLF, and the preview does not show that change, because both sides are normalised. This is the whole-file rewrite your CRLF reasoning was meant to surface, now limited to mixed files. Rare. Either say so in the editor or keep LF-only lines as they are.Approving: the blocker is fixed and the rest can land as follow-ups.