fix: preserve trending scout image across deploy cleanup

[protostatis]

Summary

• refresh the running scout image pin from the exact current image ID before broad Docker pruning
• fail the release before cleanup if the scheduled scout image is absent, and verify it survives both prune phases
• add a regression check for pin refresh and verification ordering

Validation

• ./.venv/bin/pytest tests/test_deploy_workflow.py
• ./.venv/bin/ruff check tests/test_deploy_workflow.py
• git diff --check
• bash -n deploy/release.sh

Operational note

This intentionally refuses a release when the scout image is already missing; restore or rebuild the known-good image first rather than silently continuing with stale hourly trends.

[protostatis]

Sky's Code Review

This PR hardens the release script against Docker pruning removing the trending scout image by pinning the exact image ID to a long-running placeholder container before cleanup, and verifying the pin survives both prune phases. The core logic is sound: the pin container is created from the resolved image ID (not the mutable :latest tag), verified to retain the expected ID, and renamed atomically into place, which means the image is continuously referenced by a running container across both docker system prune -af and docker image prune -af. The fail-fast behavior on a missing scout image is a reasonable operational choice, and the ordering (refresh -> verify -> prune -> verify) is correct. No security or correctness bugs block this; the remaining items are minor robustness and hygiene suggestions.

Verdict: Comment

Comments

• The atomic rename pattern (create next pin, verify it retains the expected image ID, then swap) is well done and correctly avoids a gap where the image could be pruned between removing the old pin and creating the new one.
• The regression test is string-assertion based and somewhat brittle (it checks for specific literal substrings and ordering by index()), but it does meaningfully enforce the ordering invariants and the schema of the pin mechanism, which is adequate for a shell-script workflow without a heavier harness.
• Consider whether protecting the image via a labeled/retained image tag or a prune filter would be a cleaner long-term alternative to a placeholder container, though the current approach is a legitimate, widely-used idiom and is functionally correct.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

The pin container is created with no resource constraints. Since it's a long-running --restart unless-stopped container living in the root daemon, add --memory and --cpus limits (e.g. --memory 8m --cpus 0.01) for defense-in-depth against accidental resource consumption / a misconfigured entrypoint. The sleep 3600 loop is negligible in normal operation, but limits make the intent explicit.

[protostatis]

Applied in ed05892: the placeholder pin is now capped at 8 MiB and 0.01 CPU; its running state is also verified.

[protostatis]

You set --label panicradar.role=trending-scout-image-pin but this label is never consumed anywhere (the prune commands don't filter by it, and verify uses container/image IDs, not labels). If the goal is to make the pin discoverable or prune-exclusive, that filtering isn't wired up; otherwise the label is dead metadata. Consider documenting its purpose or dropping it.

[protostatis]

Applied in ed05892: removed the unused label. The stable container name is sufficient for operator discovery.

[protostatis]

docker inspect --format '{{.Image}}' "$TRENDING_SCOUT_PIN_NAME" will return a non-zero exit with a cryptic docker error if the pin container does not exist (e.g. it was evicted or never created), rather than your intended 'no longer protects' message. Since set -e (if enabled) would abort here, consider a docker inspect ... >/dev/null 2>&1 || { echo 'ERROR: pin container missing'; exit 1; } guard for a clearer failure.

[protostatis]

Applied in ed05892: verification now explicitly reports a missing pin container before inspecting its image, and checks that the pin remains running.

[protostatis]

docker system prune -af || true swallows all errors from the prune. While the subsequent verify_trending_scout_pin (line 103) provides the real safety check, a prune failure here (e.g. daemon error) is masked and could hide genuine issues unrelated to the scout pin. This is pre-existing behavior, but worth noting that verify only checks the scout image, not general prune health.

[protostatis]

Not changed in this focused fix: the best-effort prune behavior predates this PR and changing it would broaden deployment semantics. The new guard still fails closed if the scout image or pin is affected.

[protostatis]

Sky's Code Review

This PR adds protection so a scheduled trending-scout Docker image isn't destroyed by broad cleanup during an unrelated app release. The approach — pin the exact image ID via a resident 'pin' container before pruning, then verify after each prune phase — is sound and fail-closed (the release refuses to proceed if the image is already missing). The main concern is a Docker image-ID format mismatch: docker image inspect --format '{{.Id}}' emits the full ID with a sha256: prefix, whereas docker inspect --format '{{.Image}}' on a container typically returns the image ID without that prefix. If those formats differ on the target Docker version, the equality checks would always be true and every release would spuriously abort. This should be normalized before merge. Everything else is minor and acceptable.

Verdict: Changes requested

Comments

• Overall design is solid and correctly fail-closed: refusing the release when the scout image is missing is the right safety posture, and the regression test confirms the refresh-before-prune and verify-after-prune ordering.
• The pin container has no --rm and relies on a while :; do sleep 3600; done entrypoint with --restart unless-stopped. This is a reasonable pattern to keep the image referenced, and the --memory 8m --cpus 0.01 limits are good — but note this introduces a permanent resident container purely for image retention, which is a minor operational overhead to document.
• The regression test only asserts string presence and ordering, not the actual ID normalization behavior. Since the ID format mismatch is the main risk, consider adding an assertion (or a note) that both inspect formats are equalized — otherwise the current test would still pass while every deployment aborts.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

deploy/release.sh:60 — Image ID format mismatch risk. docker image inspect --format '{{.Id}}' returns the full digest with a sha256: prefix, while docker inspect --format '{{.Image}}' on a container returns the image ID WITHOUT that prefix on many Docker versions. Since TRENDING_SCOUT_IMAGE_ID is later compared directly against PINNED_NEXT_IMAGE_ID and PINNED_SCOUT_IMAGE_ID, a prefix difference makes those comparisons always unequal, causing every release to abort with 'not running on the expected image ID'. Normalize both sides before comparing (e.g. strip sha256: or use a consistent format) or verify the exact output format of both inspect calls on the target engine.

deploy/release.sh:76 — This equality check depends on the {{.Image}} container field matching {{.Id}} from the image inspect. These are known to differ in format (sha256: prefix) across Docker/containerd versions. See the primary comment on line 60 — recommend normalizing the IDs on both sides of the comparison.

deploy/release.sh:83 — There is a brief window here where neither PIN_NAME nor PIN_NEXT_NAME references the image: docker rm -f of the old pin happens before docker rename completes. If the script is killed in between (or docker rename fails, which is not checked), no pin container exists and a subsequent prune could remove the image. It's self-healing on the next run, but consider checking the rename result or keeping the old container until the rename succeeds to remove the gap.

deploy/release.sh:102 — Same ID-format concern as line 76: PINNED_SCOUT_IMAGE_ID ({{.Image}}) is compared against TRENDING_SCOUT_IMAGE_ID ({{.Id}}). Normalize formats to avoid a spurious failure in verify_trending_scout_pin.