Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .server-changes/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,16 @@ The body text (below the frontmatter) is a one-line description of the change. K

These entries are public-facing - they ship verbatim in user-visible release notes. A few rules to keep them clean:

- **Write for the user, not the reviewer.** Lead with what the user notices or has to do. If a reader who doesn't know the codebase can't tell what changed for them, rewrite it.
- **One sentence is usually enough.** The body is the bullet in the changelog. If you need a paragraph, you're probably describing the implementation rather than the change.
- **Describe behavior, not implementation.** Skip internal scopes, middleware names, library specifics, framework internals. Users care about what's different for them, not how it's wired.
- **Never name internal tools or infra.** Observability stacks, internal services, infra components, monitoring backends, CI surfaces, AWS specifics - none of these belong in user-facing notes.

Before / after:

- ❌ _"The image verification step now parses the manifest's layer media types and returns a new result the finalizer rejects."_ (describes the wiring; a user can't act on it)
- ✅ _"Deploying with an outdated CLI could produce an image that fails to start on every run. These deploys are now stopped before going live, with a message asking you to upgrade the CLI and re-deploy."_ (what the user sees and does)

## Lifecycle

1. Engineer adds a `.server-changes/` file in their PR
Expand Down
6 changes: 6 additions & 0 deletions .server-changes/reject-unpullable-deploy-image.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

Deploying with an outdated CLI could produce an image that fails to start on every run. These deploys are now stopped before going live, with a message asking you to upgrade the CLI and re-deploy.
2 changes: 2 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ pnpm run changeset:add

When modifying only server components (`apps/webapp/`, `apps/supervisor/`, etc.) with no package changes, add a `.server-changes/` file instead. See `.server-changes/README.md` for format and documentation.

**Write the description for users, not maintainers.** Both changesets and `.server-changes/` notes ship verbatim in user-visible release notes. Lead with what changed *for the user* - one plain sentence describing behavior, not implementation, and never naming internal tools or infra. The full writing guidance in `.server-changes/README.md` applies to changesets too.

## Dependency Pinning

Zod is pinned to a single version across the entire monorepo (currently `3.25.76`). When adding zod to a new or existing package, use the **exact same version** as the rest of the repo - never a different version or a range. Mismatched zod versions cause runtime type incompatibilities (e.g., schemas from one package can't be used as body validators in another).
Expand Down
6 changes: 6 additions & 0 deletions apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ export class FinalizeDeploymentV2Service extends BaseService {
);
}

if (result === "nonconformant") {
throw new ServiceValidationError(
"Deployment image is not runnable: it contains zstd-compressed layers inside a Docker (v2s2) manifest, which the container runtime cannot pull. This typically comes from an outdated CLI version. Please upgrade to the latest trigger.dev CLI and re-deploy."
);
}

// Fail closed: if we can't confirm the image is present, don't promote a version
// that might not start. Set DEPLOY_IMAGE_VERIFICATION_ENABLED=0 for out-of-band pushes.
if (result === "unknown") {
Expand Down
130 changes: 126 additions & 4 deletions apps/webapp/app/v3/services/verifyDeploymentImage.server.ts
Comment thread
nicktrn marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
} from "@aws-sdk/client-ecr";
import { tryCatch } from "@trigger.dev/core";
import pRetry, { AbortError } from "p-retry";
import { z } from "zod";
import { logger } from "~/services/logger.server";
import {
type AssumeRoleConfig,
Expand All @@ -16,7 +17,85 @@ import { type RegistryConfig } from "../registryConfig.server";

const SHA256_DIGEST = /^sha256:[a-f0-9]{64}$/;

export type ImageLookupResult = "found" | "missing" | "unknown";
export type ImageLookupResult = "found" | "missing" | "unknown" | "nonconformant";

// A zstd layer carried in a Docker (v2s2) manifest rather than an OCI manifest is
// unpullable by cri-o/containerd/podman. OCI zstd (...tar+zstd) is fine - only this
// Docker media type is rejected. An outdated CLI that predates OCI-media-type output
// can emit it when reusing zstd layers from a prior build or the registry cache.
const UNPULLABLE_LAYER_MEDIA_TYPE = "application/vnd.docker.image.rootfs.diff.tar.zstd";

// Nested indexes are exotic (index -> per-platform image manifests is depth 1); cap the
// walk so a pathological manifest can't fan out unbounded.
const MAX_MANIFEST_DEPTH = 3;

// Lenient: we only read what we need. An image manifest carries layers[]; a manifest list
// / OCI index carries manifests[] (per-platform child pointers). Both optional so either
// shape parses cleanly.
const ManifestSchema = z.object({
layers: z.array(z.object({ mediaType: z.string().optional() })).optional(),
manifests: z.array(z.object({ digest: z.string() })).optional(),
});

// Inspect one raw manifest: does it directly carry an unpullable layer, and (if it's an
// index) which child manifests should be walked. Fails open to empty on anything we can't
// parse, so an unreadable manifest never blocks a deploy.
export function inspectManifest(rawManifest: string | undefined): {
hasUnpullableLayer: boolean;
childDigests: string[];
} {
const empty = { hasUnpullableLayer: false, childDigests: [] };

if (!rawManifest) {
return empty;
}

let json: unknown;
try {
json = JSON.parse(rawManifest);
} catch {
return empty;
}

const parsed = ManifestSchema.safeParse(json);
if (!parsed.success) {
return empty;
}

return {
hasUnpullableLayer:
parsed.data.layers?.some((layer) => layer.mediaType === UNPULLABLE_LAYER_MEDIA_TYPE) ?? false,
childDigests: parsed.data.manifests?.map((child) => child.digest) ?? [],
};
}

// Walk a manifest and, for a multi-arch index, its child manifests (fetched by digest).
// Returns true if any layer anywhere in the tree uses the unpullable media type. A child
// that can't be fetched (undefined) is treated as conformant - fail open.
export async function treeHasUnpullableLayer(
rawManifest: string | undefined,
fetchManifest: (digest: string) => Promise<string | undefined>,
depth = 0
): Promise<boolean> {
const { hasUnpullableLayer, childDigests } = inspectManifest(rawManifest);

if (hasUnpullableLayer) {
return true;
}

if (depth >= MAX_MANIFEST_DEPTH) {
return false;
}

for (const digest of childDigests) {
const childManifest = await fetchManifest(digest);
if (await treeHasUnpullableLayer(childManifest, fetchManifest, depth + 1)) {
return true;
}
}

return false;
}

/**
* Split a stored ECR image reference into repository + tag.
Expand Down Expand Up @@ -86,8 +165,10 @@ const sendBatchGetImage: BatchGetImageSender = async ({
imageIds,
}) => {
const ecr = await createEcrClient({ region, assumeRole });
// No acceptedMediaTypes: only the single-manifest types are valid enum values, and
// we only care whether the image exists, not its manifest format.
// Intentionally no acceptedMediaTypes: ECR returns the manifest as stored - which we
// rely on for the layer-media-type check - and omitting it avoids a multi-arch index
// being reported as a failure (i.e. misread as missing). BatchGetImage populates
// imageManifest by default when the image exists; the check fails open if it's absent.
return ecr.send(new BatchGetImageCommand({ repositoryName, registryId, imageIds }));
};

Expand Down Expand Up @@ -183,5 +264,46 @@ export async function ecrImageExists(
return "unknown";
}

return interpretBatchGetImageResponse(response);
const result = interpretBatchGetImageResponse(response);

if (result !== "found") {
return result;
}

// Image exists - now confirm the runtime can actually pull it. Follow index children by
// digest so multi-arch deploys are covered, not just single image manifests.
const fetchManifest = async (digest: string): Promise<string | undefined> => {
const [fetchError, childResponse] = await tryCatch(
_send({
region,
assumeRole,
registryId: accountId,
repositoryName: parsed.repositoryName,
imageIds: [{ imageDigest: digest }],
})
);

if (fetchError) {
logger.warn("Could not fetch child manifest for conformance check", {
imageReference,
digest,
error: fetchError.message,
});
return undefined; // fail open on this child
}

return childResponse.images?.[0]?.imageManifest;
};

const topManifest = response.images?.[0]?.imageManifest;

if (await treeHasUnpullableLayer(topManifest, fetchManifest)) {
logger.error("Deployment image has a runtime-incompatible layer media type", {
imageReference,
repositoryName: parsed.repositoryName,
});
return "nonconformant";
}

return "found";
}
140 changes: 140 additions & 0 deletions apps/webapp/test/verifyDeploymentImage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,37 @@ import { RepositoryNotFoundException } from "@aws-sdk/client-ecr";
import { describe, expect, it } from "vitest";
import {
ecrImageExists,
inspectManifest,
interpretBatchGetImageResponse,
parseEcrImageReference,
treeHasUnpullableLayer,
} from "~/v3/services/verifyDeploymentImage.server";
import { type RegistryConfig } from "~/v3/registryConfig.server";

const ECR_HOST = "123456789012.dkr.ecr.us-east-1.amazonaws.com";
const ecrConfig: RegistryConfig = { host: ECR_HOST, namespace: "deployments-test" };

const DIGEST_A = `sha256:${"a".repeat(64)}`;
const DIGEST_B = `sha256:${"b".repeat(64)}`;
const ZSTD_DOCKER = "application/vnd.docker.image.rootfs.diff.tar.zstd";

const imageManifest = (layerMediaTypes: string[]) =>
JSON.stringify({
schemaVersion: 2,
mediaType: "application/vnd.docker.distribution.manifest.v2+json",
layers: layerMediaTypes.map((mediaType) => ({ mediaType, digest: DIGEST_A })),
});

const indexManifest = (childDigests: string[]) =>
JSON.stringify({
schemaVersion: 2,
mediaType: "application/vnd.oci.image.index.v1+json",
manifests: childDigests.map((digest) => ({
digest,
mediaType: "application/vnd.oci.image.manifest.v1+json",
})),
});

describe("parseEcrImageReference", () => {
it("splits repository and tag for a ref under the configured host", () => {
const ref = `${ECR_HOST}/deployments-test/proj_abc:20240101.1.prod.a1b2c3d4`;
Expand Down Expand Up @@ -61,6 +84,73 @@ describe("interpretBatchGetImageResponse", () => {
});
});

describe("inspectManifest", () => {
it("flags an unpullable zstd layer in an image manifest", () => {
const result = inspectManifest(
imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip", ZSTD_DOCKER])
);
expect(result.hasUnpullableLayer).toBe(true);
expect(result.childDigests).toEqual([]);
});

it("passes OCI zstd and gzip layers (runtime-supported media types)", () => {
const result = inspectManifest(
imageManifest([
"application/vnd.oci.image.layer.v1.tar+gzip",
"application/vnd.oci.image.layer.v1.tar+zstd",
])
);
expect(result.hasUnpullableLayer).toBe(false);
});

it("returns child digests for an index and does not flag it directly", () => {
const result = inspectManifest(indexManifest([DIGEST_A, DIGEST_B]));
expect(result.hasUnpullableLayer).toBe(false);
expect(result.childDigests).toEqual([DIGEST_A, DIGEST_B]);
});

it("fails open (empty) when the manifest is absent or unparseable", () => {
expect(inspectManifest(undefined)).toEqual({ hasUnpullableLayer: false, childDigests: [] });
expect(inspectManifest("not json")).toEqual({ hasUnpullableLayer: false, childDigests: [] });
});
});

describe("treeHasUnpullableLayer", () => {
const neverFetch = async () => undefined;

it("detects an unpullable layer in a flat image manifest", async () => {
expect(await treeHasUnpullableLayer(imageManifest([ZSTD_DOCKER]), neverFetch)).toBe(true);
});

it("follows an index and detects an unpullable layer in a child", async () => {
const children: Record<string, string> = {
[DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+gzip"]),
[DIGEST_B]: imageManifest([ZSTD_DOCKER]),
};
const result = await treeHasUnpullableLayer(
indexManifest([DIGEST_A, DIGEST_B]),
async (digest) => children[digest]
);
expect(result).toBe(true);
});

it("returns false for an index whose children are all conformant", async () => {
const children: Record<string, string> = {
[DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+zstd"]),
[DIGEST_B]: imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip"]),
};
const result = await treeHasUnpullableLayer(
indexManifest([DIGEST_A, DIGEST_B]),
async (digest) => children[digest]
);
expect(result).toBe(false);
});

it("fails open when a child manifest can't be fetched", async () => {
expect(await treeHasUnpullableLayer(indexManifest([DIGEST_A]), neverFetch)).toBe(false);
});
});

describe("ecrImageExists", () => {
it("returns unknown for a non-ECR registry without calling the registry", async () => {
let called = false;
Expand Down Expand Up @@ -174,4 +264,54 @@ describe("ecrImageExists", () => {
);
expect(seen.imageIds).toEqual([{ imageTag: "v1.prod.a1b2c3d4" }]);
});

// Resolve a manifest by tag or digest against a fixture map, mimicking BatchGetImage.
const sendFrom =
(byRef: Record<string, string>) =>
async (input: any): Promise<any> => {
const id = input.imageIds[0];
const manifest = byRef[id.imageDigest ?? id.imageTag];
return manifest ? { images: [{ imageManifest: manifest }] } : { images: [{}] };
};

it("returns nonconformant for a single-arch image with an unpullable zstd layer", async () => {
const result = await ecrImageExists(
{
imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`,
registryConfig: ecrConfig,
},
sendFrom({ "v1.prod.a1b2c3d4": imageManifest([ZSTD_DOCKER]) })
);
expect(result).toBe("nonconformant");
});

it("returns nonconformant when a multi-arch index has an unpullable child", async () => {
const result = await ecrImageExists(
{
imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`,
registryConfig: ecrConfig,
},
sendFrom({
"v1.prod.a1b2c3d4": indexManifest([DIGEST_A, DIGEST_B]),
[DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+gzip"]),
[DIGEST_B]: imageManifest([ZSTD_DOCKER]),
})
);
expect(result).toBe("nonconformant");
});

it("returns found when a multi-arch index's children are all conformant", async () => {
const result = await ecrImageExists(
{
imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`,
registryConfig: ecrConfig,
},
sendFrom({
"v1.prod.a1b2c3d4": indexManifest([DIGEST_A, DIGEST_B]),
[DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+zstd"]),
[DIGEST_B]: imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip"]),
})
);
expect(result).toBe("found");
});
});