Take the review: cache the stopped probe, and never let it cost an answer
Secret Scan / scan (push) Successful in 6s
Build App (Preview) / compute-version (pull_request) Successful in 5s
Secret Scan / scan (pull_request) Successful in 4s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m58s
Build App (Preview) / build-linux (pull_request) Successful in 4m43s
Build App (Preview) / build-windows (pull_request) Successful in 5m9s
Build App (Preview) / prune-previews (pull_request) Successful in 2s
Secret Scan / scan (push) Successful in 6s
Build App (Preview) / compute-version (pull_request) Successful in 5s
Secret Scan / scan (pull_request) Successful in 4s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m58s
Build App (Preview) / build-linux (pull_request) Successful in 4m43s
Build App (Preview) / build-windows (pull_request) Successful in 5m9s
Build App (Preview) / prune-previews (pull_request) Successful in 2s
Six findings, all real. The one that mattered: `getContainerStaleness` is
called from a `useEffect` that fires whenever the container settles, so
merely opening a stopped project's Overview now committed its whole writable
layer — 44 s on a real project, against ~3 s for the snapshot probe it
replaced. Shipping that would have traded one bad banner for a bad page.
A stopped container's writable layer cannot change, so the probe is exactly
cacheable: `STOPPED_MANIFEST_CACHE` keys on the container's `FinishedAt`,
which moves on every stop. Cold 2967 ms, warm 1 ms, measured. A live test
asserts the restart case as well as the hit, because a cache that failed to
invalidate would plan a migration against a filesystem the project no longer
has — verified by breaking the token and watching that assertion fail.
Skipping the probe for projects that are not stale looked like the cheaper
fix and is unsafe: the deltas would be empty while `probeSettled` stayed
true, and the migrate action in the project menu is not gated on the banner,
so the pre-flight would report nothing to copy while the backend was told to
copy nothing. That is the hazard `canMigrate`'s comment already warns about.
Not done, and written down so it is not tried again.
Also from the review:
- A failed commit no longer costs an answer the snapshot could have given.
Before this feature a stopped project read its snapshot directly, so
surfacing this error would have made the banner worse than it was — and
the failure modes are where the fallback earns its keep: a full disk (the
commit allocates the whole layer, the snapshot probe allocates nothing)
and a 409 from a concurrent claim.
- The probe no longer commits while the project is claimed. The collision is
not symmetric: the probe losing is a retryable `probe_error`, but
`start_project_container` removes the old container with a hard `?`, so a
remove that raced a commit would fail the user's Start with an opaque
error. `stopped_probe_policy` reads `project_lock::held` and probes the
snapshot instead, or defers with a message that says so.
- The cleanup-failure warning claimed the next probe of the same container
would reclaim the leftover. Unique names made that false the moment they
landed; it is `reap_probe_images` that collects it.
- The TS binding still called the command read-only, which is how the
auto-refresh got added in the first place.
- CLAUDE.md still documented the stable `triple-c-probe-{cid}:latest` name
this PR removed as unsafe.
548 unit tests, 752 frontend tests, 4 live-Docker tests. Clippy unchanged at
44 warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019RSaoDLovVV2wmH4H8VVxz
This commit is contained in:
@@ -441,7 +441,7 @@ security update. Migration is the non-destructive way out; Reset is the destruct
|
||||
or inside a migration. **Never on stop.** So a project in daily use for a year can legitimately
|
||||
have no `triple-c-snapshot-{id}:latest` at all, and one that has is stale by everything installed
|
||||
since. `pick_probe_source` therefore reads a *stopped* container directly — commit its writable
|
||||
layer to `triple-c-probe-{cid}:latest`, probe that, drop it — and ranks it **above** the snapshot,
|
||||
layer to a unique `triple-c-probe-*` image, probe that, drop it — and ranks it **above** the snapshot,
|
||||
for the same reason a running container already outranked it. Assuming a snapshot existed is what
|
||||
made a stopped, never-recreated project report "no container or snapshot image yet" with its
|
||||
container sitting right there, and left Update disabled on the projects furthest behind.
|
||||
@@ -466,6 +466,30 @@ security update. Migration is the non-destructive way out; Reset is the destruct
|
||||
other was still reading, reporting a bogus `probe_error` on a healthy project. `get_container_staleness`
|
||||
takes no `project_lock` claim (the migration banner needs it to answer *during* a migration), so
|
||||
uniqueness is what makes overlapping probes safe.
|
||||
- **The stopped-container probe is cached per stop, and that is not an optimisation you may drop.**
|
||||
`getContainerStaleness` is called from a `useEffect` that fires whenever the container settles, so
|
||||
merely opening a stopped project's Overview probes it. Uncached that is a `docker commit` of the
|
||||
whole writable layer per visit — measured at 44 s on a real project, against ~3 s for the snapshot
|
||||
probe it replaced. `STOPPED_MANIFEST_CACHE` is keyed on the container's `FinishedAt`, which is
|
||||
exact rather than merely plausible: nothing can write to a stopped container's writable layer, and
|
||||
`FinishedAt` moves on every stop. A live test asserts the restart case, because a cache that
|
||||
failed to invalidate would plan a migration against a filesystem the project no longer has.
|
||||
- **Do not "skip the probe when the project is not stale" to save that cost.** It was tried. The
|
||||
deltas would be empty while `probeSettled` (`!probing && staleness && !probe_error`) stayed *true*,
|
||||
which leaves the migrate action in the project menu enabled — that action is not gated on the
|
||||
banner — so the pre-flight would report nothing to copy while the backend was told to copy
|
||||
nothing. That is the exact hazard `ProjectHome.tsx`'s `canMigrate` comment already warns about.
|
||||
- **A failed stopped-container probe falls back to the snapshot whenever one exists.** Before this
|
||||
feature a stopped project read its snapshot directly, so surfacing a commit failure where the
|
||||
snapshot could have answered would make the banner *worse* than it was — and the failure modes are
|
||||
exactly the ones where the fallback earns its keep: a full disk (the commit allocates the whole
|
||||
writable layer; the snapshot probe allocates nothing) and a 409 from a concurrent claim.
|
||||
- **`get_container_staleness` never commits while the project is claimed.** It takes no
|
||||
`project_lock` claim itself, deliberately — the banner has to answer *during* a migration — so it
|
||||
reads `project_lock::held` instead and probes the snapshot rather than the container. The
|
||||
collision is not symmetric: the probe losing is a retryable `probe_error`, but
|
||||
`start_project_container` removes the old container with a hard `?`, so a remove that raced a
|
||||
commit would fail the user's Start with an opaque error.
|
||||
- **An image's `Created` is the image's own, not its tag's.** Tagging an existing image gives you
|
||||
that image's age; BuildKit stamps `docker build` output with a fixed epoch. Only `docker commit`
|
||||
stamps *now* — which is what real probe images do, and what any fixture for them must do.
|
||||
|
||||
Reference in New Issue
Block a user