[Fix] Pin Modal base image tags to their current digest - #2474
Open
mrubens wants to merge 4 commits into
Open
Conversation
Modal caches images.fromRegistry(ref) by the ref string, so a mutable tag such as :develop or :latest never gets re-pulled after the first build. Sandboxes silently kept running months-old worker images (and their baked OpenCode CLI), which surfaced as every task failing POST /session with ConfigInvalidError once the generated config used a newer config key. Resolve mutable tags to a digest-pinned reference via the OCI distribution API before handing the image to Modal. New pushes to the tag now produce a new image definition and Modal rebuilds. The lookup is cached per process, probes anonymously (registries answer a rejected Basic header with a placeholder scope), retries the token request anonymously when configured credentials are rejected, and falls back to the tag on any failure so a registry hiccup never blocks sandbox creation. ECR OIDC mode is unchanged. Also include the OpenCode response body in the error thrown for failed harness requests so config rejections are visible in the task error instead of only in the sandbox's harness.log.
Contributor
|
2 issues outstanding. See task
Reviewed 9fd8267 |
![roomote-community[bot]](https://avatars.githubusercontent.com/u/211522643?s=60&v=4){kind=small}
Docker Distribution registries secured with htpasswd-style auth answer the manifest probe with a Basic challenge rather than a Bearer one. Retry the manifest HEAD with the configured credentials in that case instead of falling back to the mutable tag, and fail clearly when no credentials are configured.
Address review findings on the Modal digest pin: - Serve the last successfully resolved digest when a registry refresh fails instead of dropping back to the mutable tag, which would silently re-trigger Modal's stale image cache. The tag is only used when no digest has ever resolved in the process. - Cache failed lookups for 15s so a registry outage costs one timeout per window rather than one per spawn, and thread the sandbox spawn's abort signal into the registry fetches. - Redact the OpenCode response body before it enters the request error message, and render the session-create failure message as a code block in the transcript so JSON bodies do not break markdown.
| `OpenCode request failed method=${method} path=${path} status=${response.status}`, | ||
| `OpenCode request failed method=${method} path=${path} status=${response.status}${ | ||
| responseText | ||
| ? ` body=${redactSecrets(responseText.slice(0, 300))}` |
Contributor
There was a problem hiding this comment.
responseText is redacted only in the thrown error, but the preceding this.logger.warn still writes its first 500 raw characters to harness.log. That log is exposed through the Logs sidebar, so a response body containing credentials can still leak despite this change's redaction goal. Redact the logger's body value as well (before logging it).
Follow-up review fixes for the Modal base image digest pin: - Refuse to send registry credentials to a token realm that is not HTTPS or not on the registry's own site, so a mirror relaying an upstream challenge cannot redirect them. - Parse WWW-Authenticate leniently: bare (unquoted) parameter values and comma-separated challenge lists, picking the Bearer challenge when present and Basic otherwise. - Skip the registry lookup for immutable release tags (develop-<sha>, main-<sha>, v*, raw SHAs) so production deployments carry no per-spawn registry dependency. - Pin the base image on the broker backend too, and warn once when an ECR ref uses a mutable tag (ECR digests cannot be resolved from the controller). Log the pinned ref when creating a sandbox. - Replace the hand-rolled TTL cache with lru-cache (already a package dependency) using its fetch coalescing, stale-on-rejection, and an injectable clock; drop the test-only `now` option from the production signature and parse each ref once. - Document MODAL_BASE_IMAGE_REF and the tag resolution behavior.
| // its image cache on the ref string, so pin the tag to its digest first. | ||
| const imageRef = sourceSnapshotId | ||
| ? undefined | ||
| : await pinModalBaseImageRef({ |
Contributor
There was a problem hiding this comment.
The broker client deliberately has no registry credentials (factory.ts keeps them broker-side), so this anonymous lookup cannot resolve a private base image. It falls back to the mutable tag and the broker path retains the stale Modal-image-cache behavior this change is meant to fix. Resolve the digest in the broker, where its registry credentials are available, or provide a trusted broker-side digest endpoint.
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.
Problem
Modal caches
images.fromRegistry(ref)by the ref string. When the configured base image is a mutable tag (ghcr.io/roocodeinc/roomote-worker:develop, or a:lateststyle tag on a self-hosted deployment), Modal builds it once and never re-pulls, so sandboxes silently keep running whatever the tag pointed at the first time.On one dev deployment this went unnoticed for weeks: the cached image still had OpenCode 1.17.x baked in, and once the generated OpenCode config started emitting
subagent_depth(#2278) every task failed ~50ms after OpenCode started with:The actual cause (
ConfigInvalidError: Unrecognized key: subagent_depth) was only visible in the sandbox'sharness.log, which is gone by the time the failure surfaces.Fix
packages/compute-providers/src/modal/registry-digest.tsresolvesrepo:tagtorepo@sha256:…via the OCI distribution API (HEAD /v2/<repo>/manifests/<tag>→Docker-Content-Digest). A new push produces a new image definition, so Modal rebuilds without any config change.Basicheader with a placeholder scope (repository:user/image:pull), so the scope is built from the repository rather than copied from the challenge.opencode-server/client.ts), so a config rejection shows up in the task error instead of only inharness.log.Verification
images.fromRegistry.Pinned base image … @sha256:…, Modal pulled the current image (sandbox creation went from ~1s cached to ~60s once), and a task completed reportingopencode --version→1.18.10.Follow-up
The worker still trusts
ROOMOTE_BAKED_OPENCODE_CLI_VERSIONfrom the image overDEFAULT_OPENCODE_CLI_VERSIONin the worker release, so a newer worker on an older image never upgrades OpenCode. Treating the worker's default as a floor would make that case self-heal; not included here.