fix: stale update badge, chart crosshair alignment, scheduled image prune, and unbounded Docker calls - #32
Merged
Conversation
The hover crosshair on the time-series charts snapped to a sample by array-index fraction, but points are positioned on the x-axis by timestamp. Since the deduped union of timestamps across series is generally not evenly spaced, the crosshair drifted away from the mouse cursor. Convert the cursor fraction back to a target time and binary-search the sorted timestamp array for the nearest sample instead.
A stack updated on the host by hand (docker compose pull && up -d) kept advertising "update available" until an operator pressed Check: nothing observed Docker changes Watchtower did not make itself. The check now records the remote digest behind each outdated image, which turns clearing the flag into a local question — once that digest is among the image's local repo digests, the update has landed. Read handlers announce stacks with a pending image update to a new singleton that runs that comparison in the background, debounced to once per stack per 30s and never awaited, so the UI corrects itself on its next refetch without a single registry request. Commit-based state is left strictly alone; rows written before the digests existed are skipped and self-correct on the next full check. Also aligns the check with revalidation by comparing the remote digest against all local repo digests rather than only the first.
Adds a third automation row to Settings: an enable toggle plus an interval in minutes that periodically removes dangling (untagged) images, reclaiming the disk each pull-and-redeploy cycle leaves behind (GitHub issue #26). The prune goes through the Docker Engine API (POST /images/prune?filters={"dangling":["true"]}) rather than the CLI, and never the -a / "all unused" variant: that would delete tagged images a stack manager may still want, including the previous version of every stack. The toggle and interval are settings-backed Watchtower:* keys, so they are runtime-editable through system.updateAutomation without a restart, exactly like the existing self-update and stack-check toggles.
The hover crosshair's client-to-viewBox conversion assumed the SVG stretches to its container's full width. In fact the viewBox has no preserveAspectRatio override, so the default xMidYMid meet applies: on any container wider than the 720-unit viewBox (e.g. the max-w-[1200px] /metrics/history layout) the content is letterboxed and centered, not stretched. The naive `clientX / rect.width` ratio ignored that offset, so the crosshair was off by tens to over a hundred pixels near the plot edges and only correct near the center. Convert client coordinates to viewBox coordinates through the SVG's screen CTM instead, which accounts for any letterboxing/scale correctly. Also bail out of the hover handler for degenerate layouts (zero width/height, or no invertible CTM) so the crosshair stays hidden rather than pinning to the first sample.
Review follow-up on 62b7ca5. The write-back was a lost update: it re-read the row in a fresh scope and then assigned a list derived from the pre-inspect snapshot, so a full check that landed while the Docker inspects ran had its findings erased until the next check. Revalidation now reports only what it positively confirmed as applied, and the write subtracts those from the stored row — and only while that row still names the digest this pass verified, so a newer update published in the meantime survives too. Clearing an entry now also requires a running container to be using the new image, not merely for it to be on disk: a bare `docker compose pull` left the old container serving while the badge went away, which is precisely the state the badge exists to report. Container image ids come from the containers list Docker already returns. Scheduling gets the two limits the read path needs. Work is serialized onto a single chain, so a cold start listing twenty stacks queues their inspects instead of firing them at a small host at once; and a stack that is still in flight is not queued again when the debounce window lapses under it. The test-only completion seam is internal rather than public API. Also folds the three Docker calls into named virtual seams, which is what lets the tests assert that the local path never asks a registry anything.
…down DockerEngineClient (and the registry clients) leave HttpClient.Timeout at its 100-second default, and a timeout surfaces as a TaskCanceledException — an OperationCanceledException. Every background loop caught that unconditionally as "normal shutdown" and logged nothing, so a pass that ran into the ceiling left a toggle that looks enabled with no evidence of anything happening. Guarding each of those catches on the token (`when (ct.IsCancellationRequested)`) keeps real shutdown silent and lets a timeout fall through to the warning that was already there. Swept the whole codebase: the image-prune, stack-update, self-update and auto-deploy loops plus the reverse-proxy reconcile needed it; MetricsSampler and CiRunnerOrchestrator already guard theirs. The outer catches around each loop's Task.Delay stay unguarded on purpose — with the inner ones fixed they only ever see shutdown, and rethrowing there would stop the host (BackgroundServiceExceptionBehavior.StopHost). Also drops the aspirational "self-update rollback" justification from the prune's doc comment: no such rollback-by-previous-image path exists. The sound half — a `compose up` without a pull reuses tagged images — stays.
… body
BuildImagePruneUrl becomes internal, matching ComposeCliService.BuildComposeArgs;
the test project already sees internals via InternalsVisibleTo.
PruneImagesAsync now goes through EnsureSuccessWithBodyAsync instead of
EnsureSuccessStatusCode: the prune runs unattended with no UI surface, so the
daemon's {"message":...} body is the only diagnostic there will ever be — a
read-only socket mount otherwise reports a bare 403.
Drops the two substring assertions over the whole URL from the prune test; the
exact-equality assertion above them already pins the request, and the decoded
filter JSON is asserted on its own.
Re-review follow-up on f299dd9. Nothing exercised the real /containers/json wire format — every test overrides the listing seam — and that binding fails closed: if "ImageID" stopped mapping onto ImageId, no container would ever match a local image, the badge would never clear again, and the suite would stay green. A literal fragment now goes through DockerJsonContext and asserts the id binds, plus that an omitted or null one reads as unset rather than as a matchable value; the property's comment said "empty" where null is also reachable. Also: skip announcing pre-migration rows from the read handlers, which cost a database read per debounce window to conclude nothing; make the digest map's hash order- and case-independent, matching its equality; and take the overstatement out of three comments — the write-back's remaining read-then-write window, the per-stack cost serialization does not remove, and the stopped-stack consequence of the running-container gate.
…them The daemon returns an untagged reference and a deleted layer as separate entries, so one image can account for two — say entries, not images. And a client-side timeout abandons the request while the daemon carries the prune through, so 'gave up on' is the honest wording, not 'failed'.
The Docker client built one HttpClient and never set Timeout, so every call inherited the 100-second default. Buffered calls are the ones that feel it: the container wait blocks until the container exits, and a dangling-image prune on a host with a long layer backlog can take longer than that — both were being abandoned mid-flight with nothing wrong on the host. (Streamed responses read with ResponseHeadersRead are unaffected: the per-request timeout stops applying once the headers arrive.) Add a second HttpClient over the same SocketsHttpHandler — one connection pool, two timeout policies — with Timeout.InfiniteTimeSpan, and route only the wait and the prune through it. The default client keeps the 100-second ceiling so list/inspect/stats still fail fast on a wedged socket. Neither client owns the handler; Dispose disposes both clients and then the handler exactly once. With no client timeout, cancellation is the only bound. The wait is inherently open-ended and stays bounded by its caller's token. The prune runs from a background loop, so it links the caller's token with a 30-minute cap and surfaces a cap hit as TimeoutException — ImagePruneBackgroundService swallows OperationCanceledException as shutdown, and a prune that ran into the ceiling has to be logged rather than vanish.
Moving the container wait onto the untimed client left the startup reconcile with no bound at all. IHost.StartAsync is handed CancellationToken.None here (the host is run with no token, and StartupTimeout is left infinite), so a coordinator that inspects as running but never exits — paused, wedged, host thrashing — would hold startup open forever: the app never reaches Started, and SIGTERM cannot help because the shutdown signal never reaches the startup path. That path is routinely entered, since the coordinator starts the replacement before removing the old container. Before the routing change the client's 100-second default ended the wait and startup carried on. Link the startup token with _cts and a 60-second ceiling, so the reconcile gives up and startup continues exactly as it did, leaving the apply stage for a later reconcile — now with a warning saying so. The live call site keeps threading _cts.Token and is untouched. Also from review: - Make the prune cap race-free: the cap gets its own CancellationTokenSource and the catch filter reads it, instead of inferring "the cap fired" from the caller's token. A shutdown landing between an expired cap and the filter no longer turns the TimeoutException back into a swallowed cancellation. - Cover the owning half of the disposal invariant via an internal constructor that takes the handler to own — the production shape. - Test ImagePruneBackgroundService.RunPruneAsync directly (now internal): a capped prune is logged as a warning, a shutdown is swallowed silently. - Format the cap in the timeout message as a TimeSpan, so a short injected cap no longer renders as "0-minute".
The ceiling added for the startup reconcile was scoped too widely and to only one of the two waits. Bound the wait, not the bookkeeping. The ceiling was threaded through the whole reconcile, so a coordinator exiting near the ceiling could have it fire between clearing the stage and clearing the CoordinatorId — leaving ApplyStage="idle" with a stale id, the container never removed, and nothing to fix it later: StartAsync only reconciles stages of "pulling"/"restarting". The ceiling now wraps WaitContainerAsync alone (TryWaitForExitAsync, with its own source so the "gave up" warning cannot be misattributed to a racing shutdown), and everything after the wait runs on the caller's token as before. Bound the live apply watch too. The hazard is the untimed wait, not the call site: PullAndSpawnAsync watches on _cts.Token, which only host shutdown fires, so a wedged coordinator left _applyTask running and every retry rejected with "already in progress" until a restart. It now gets ApplyWatchTimeout — ten minutes rather than the startup ceiling, because this coordinator is doing the recreate rather than picking up someone else's leftovers. Also: describe Dispose's actual client/handler ownership; give the startup tests a 1s ceiling instead of 200ms so the margin is not a cliff; cancel the losing watchdog delay.
swimmesberger
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three issues Siedler6 reported.
Closes #25
Closes #26
Closes #30
#30 — Metrics history crosshair does not follow the cursor
Two stacked bugs; the visible one was the second.
Math.round(frac * (ts.length - 1))) while points and the crosshair are positioned by timestamp.tsis the deduped union of timestamps across series, so it is only evenly spaced when every series covers the same range — otherwise the two disagree. Now snaps to the nearest timestamp via binary search.viewBox="0 0 720 h"withwidth="100%"and nopreserveAspectRatio, so the defaultxMidYMid meetletterboxes the content on containers wider than 720px. The pointer conversion assumed the viewBox stretched to full width, so on the 1112px-wide chart in the issue screenshot the crosshair was off by up to ~120px near the edges and correct only at the centre. Conversion now goes throughsvg.getScreenCTM().inverse(), which is correct under anypreserveAspectRatio, container size, page zoom, or ancestor transform.The conversion is now the exact algebraic inverse of the render-side
x(t), so an un-snapped crosshair lands precisely under the cursor. Verified in a browser against a 1112px container: probes at the chart edges rendered exactly at the computed positions, diverging 104–165px from the old behaviour.Also guards the degenerate zero-size case, which would otherwise pin a crosshair to the first sample.
#25 — Stale "update available" badge
The compose-file half of the issue was already fixed by
77c1ac6f(self-update now recreates the container through the Docker Engine API — no compose file read, no mount, no host-path configuration). The reporter's instance predates that commit; one manual update is enough and UI self-updates work from then on. Only a stale doc comment onApplySelfUpdatestill described the old compose-validating behaviour; corrected here.The badge itself was a real bug.
HasUpdatesis cached instack_update_checksand cleared only by a deploy Watchtower performs itself, or by a fresh check. An out-of-banddocker compose pull && up -dnever touches it, the periodic checker is off by default, and the UI does not refetch outside active deploys — so the badge was stale by construction.Fix: record the remote digest behind each outdated image at check time, then revalidate against local Docker state only — no registry traffic on this path, so no Docker Hub rate-limit exposure. Triggered fire-and-forget from
stacks.list/stacks.get, debounced per stack (30s) and serialized through a single queue, so the read path never blocks and a large stack list cannot fan out into hundreds of concurrent Docker calls.Beyond the issue: an image is cleared only when a running container actually uses it, so a bare
pullwithoutup -dno longer clears the badge while the old container is still serving.Deliberately not done: no Docker
/eventssubscription (new long-lived-connection infrastructure this bug does not justify) and no change to default polling intervals.#26 — Scheduled
docker image pruneNew "Prune dangling images" row on
/settings, alongside the existing automation toggles: enable flag plus an interval, persisted live through the settings store and picked up without a restart.Two departures from the request:
POST /images/prunewith adangling=truefilter, matching how the rest of the codebase talks to Docker. Explicitly never the-a/all-unused variant, which also deletes tagged images that acompose upwithout a pull would reuse.Defaults to off, once per day. Pruning cannot interfere with a self-update rollback: the coordinator keeps the previous container, and Docker will not prune an image any container still references.
Unrelated bug found along the way
Sweeping for the swallowed-cancellation pattern turned up a pre-existing bug in
AutoDeployBackgroundService: a timed-out registry check (HttpClient's 100s default surfaces asTaskCanceledException, which derives fromOperationCanceledException) was rethrown as if it were shutdown, unwound to the outer handler, and permanently ended the auto-deploy loop for every stack — with no log output, until a restart. Guarded onct.IsCancellationRequestedhere and in four other services so only genuine shutdown is treated as shutdown.Long-running Docker calls are no longer capped at 100 seconds
DockerEngineClientused oneHttpClientwith noTimeoutset, so the .NET 100s default applied. Streamed calls (PullImageAsync,StreamLogsAsync) are unaffected once headers arrive — measured on .NET 10, not assumed, becauseHttpClient's per-request timeout CTS is disposed whenSendAsyncreturns and so does not cover aResponseHeadersReadbody. Buffered calls are capped, and two of them legitimately exceed it:WaitContainerAsync(blocks until a container exits; used by the self-update coordinator watch) andPruneImagesAsync.Now: a second
HttpClientwithTimeout.InfiniteTimeSpan, sharing the oneSocketsHttpHandler(so the connection pool is shared and exactly one owner disposes it). Only those two calls are routed through it; UI-facing calls (list/inspect/stats) keep the 100s ceiling so a wedged socket still fails fast rather than hanging request handlers. Raising the timeout on the single shared client was rejected for that reason.Removing a timeout removes a bound, so every affected wait got an explicit one instead:
TimeoutExceptionand logged — it cannot be silently swallowed by the background service's shutdown guard.ApplyStagecleared with a staleCoordinatorIdand a leaked coordinator container that no later reconcile would clean up.The startup ceiling matters more than it looks:
IHost.StartAsyncreceivesCancellationToken.None(app.RunAsync()with no token) andHostOptions.StartupTimeoutis left infinite, so nothing else would ever cancel that wait. Untimed, a coordinator that reports running but never exits would hold startup open forever — SIGTERM would not help, since the shutdown signal never reaches the startup path. The ceiling surfaces as cancellation (notTimeoutException) on purpose: thecatch (Exception)arm there clears the stage to Idle, which is wrong for a coordinator that is still alive.Verification
dotnet build Watchtower.slnx— succeeded, 0 warnings, 0 errors.dotnet test Watchtower.slnx— 650 passed, 0 failed, 0 skipped (478 Application + 172 Api), run on the merged tree rather than per-branch.npm run typecheckandnpm run buildinwatchtower-web— clean.rpc-schema.jsonre-exported after the merge: no diff (90 methods), so the generated frontend client is current.Reviewer notes
20260811205038_RecordOutdatedImageDigestsadds a non-nulloutdated_image_digestscolumn defaulting to"". Rows written before it are skipped by revalidation and self-correct on the next full check.StackDtois unchanged, so no frontend changes were needed for Updatefehler #25.RemoveAppliedImagessubtracts only positively-confirmed images from the freshly-read row, and only while that row still names the digest this pass verified, so a concurrent full check cannot lose a newly-found update. A microseconds-wide read-then-write window remains (no concurrency token); worst case is one stale flag until the next check. Stated in the code comments rather than glossed as "race-free".GetSystemDfAsyncis the one remaining buffered Docker call whose cost scales with host contents (the daemon walks images/containers/volumes). It is deliberately left on the 100s client because it is UI-facing — the fail-fast ceiling is the right trade there. Noted as the plausible future candidate if it ever bites.SelfUpdateReconcileCeilingTeststemporarily setsHOSTNAME(self-detection reads it) and restores it in afinally.SelfProjectNameProvideralso reads that variable in production, so there is a theoretical coupling with test classes running in parallel; the failure mode is benign (an inspect that cannot succeed under test is treated as inconclusive, the same as unset) and the suite was run repeatedly, including under CPU saturation, without flakes.