feat(aws-lambda): publish plan v2 directly to S3

[jrusso1020]

What

• publishes Plan v2 content-addressed artifacts directly from the planner's private staging directory to S3
• commits the Plan v2 manifest only after every referenced artifact is durable
• preserves the existing Step Functions wire result: manifest URI, artifact prefix, plan hash, and render metadata
• makes immutable uploads retry- and race-safe with conditional create plus exact digest/size verification

Why

Distributed Lambda workers do not share a filesystem. Planner, chunk renderers, and assembler must communicate only through durable S3 locators.

This removes the extra local Plan v2 CAS directory and makes the published result independently consumable by any worker. It does not yet eliminate the producer's planner-local v1 staging tree; direct Plan v2 emission is a later layer and is required to fully remove the remaining Plan Too Large staging pressure.

Design invariants

• local paths never cross a worker boundary
• manifest and artifact locators are derived from one validated S3 output prefix
• artifacts are immutable and may be safely reused by retries
• manifest publication is the commit point and happens last
• abort never deletes CAS blobs that another retry may already reference
• a conflicting object at an immutable key fails closed

Test plan

• 130 AWS source tests pass
• AWS package typecheck passes
• AWS package build passes
• changed-file lint, format, fallow, and repository commit gates pass
• tests cover successful publication order, missing artifact rejection, malformed digests, matching retries, conflicting manifests, concurrent-create races, abort behavior, and end-to-end handler S3 handoff

[jrusso1020]

• feat(gcp-cloud-run): publish plan v2 directly to GCS #2799
• feat(aws-lambda): publish plan v2 directly to S3 #2795 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Fresh exact-head review at 09998789b5ff012adcd97e9fb33537e473f1cc52 found no blocking issue.

I traced the full v2 plan path from planner-local staging through S3PlanV2ArtifactPublisher, immutable conditional S3 publication, manifest-last commit, and per-worker target materialization. The source-path lifetime is sound: publication resolves only after every upload has consumed its source, and both the producer pool and the adapter worker pool use Promise.allSettled before scratch cleanup. CAS collisions fail closed; successful/racing writes are reused only when digest metadata and size agree, while workers still verify downloaded bytes against the manifest digest.

The old CodeQL trailing-slash thread is resolved by the linear backward scan and the 10,000-slash regression. Fresh local verification on the stacked checkout: full AWS package 132/132 and typecheck pass. Exact-head required CI is fully green, including Windows, CodeQL, CLI/global-install, producer, and runtime-contract checks.

Verdict: APPROVE
Reasoning: The direct S3 publisher preserves the producer publisher contract, immutable-CAS and manifest-last invariants, and cleanup ordering; the reported security/performance issue is fixed with regression coverage.

— Magi

[vanceingalls]

APPROVE — AWS Lambda's Plan v2 flow now consumes planV2WithPublisher(..., new S3PlanV2ArtifactPublisher(...)); GHAS trailing-slash finding is fixed with a linear scan + 10k-slash regression; the download pool's cleanup race is closed by Promise.allSettled + first-rejection propagation; the publisher preserves the manifest-last invariant end-to-end.

Verification of prior findings

• github-advanced-security[bot] — CodeQL polynomial-regex on trailing / (packages/aws-lambda/src/s3PlanV2Publisher.ts): RESOLVED. The regex path was replaced by a linear backwards scan at packages/aws-lambda/src/s3PlanV2Publisher.ts:55-59: while (end > 0 && value.charCodeAt(end - 1) === 47) end -= 1. No regex reaches planOutputS3Prefix.
• jrusso1020 self-note ("Fixed in 0dfff06 … Added a 10,000-trailing-slash regression test"): CONFIRMED. Regression test at packages/aws-lambda/src/s3PlanV2Publisher.test.ts:88-96 constructs the publisher with planOutputS3Prefix: \s3://bucket/render${"/".repeat(10_000)}`and asserts the resultingartifactPrefix/manifestUri` collapse to the single-slash form.

Claim-by-claim verification

Claim 1: GHAS trailing-slash fix + 10k regression — MET

• Fix is at packages/aws-lambda/src/s3PlanV2Publisher.ts:55-59. Character-code 47 is /; the loop walks backwards from value.length and slices once. O(n) time, O(1) space, no backtracking surface.
• Regression: packages/aws-lambda/src/s3PlanV2Publisher.test.ts:88-96 (10,000 trailing slashes) — synchronous constructor path, no timeout budget, so if a polynomial regex ever creeps back in the same test will surface it under CI wall-clock (P3 nit: the assertion is on output correctness, not elapsed time — a mildly polynomial regex could still pass, but Fallow/GHAS is the backstop).
• Sibling site: packages/aws-lambda/src/handler.ts:808-810 still holds a separate trimTrailingSlash(prefix: string) { return prefix.endsWith("/") ? prefix.slice(0, -1) : prefix; }. It strips at most one slash. Not ReDoS-prone (no regex), but same-named function, different semantics — a sibling-precision divergence. In this PR the multi-slash input path funnels through the publisher's stricter version before reaching handler.ts's helper (which only trims the publisher-produced artifactPrefix at handler.ts:686, already normalized), so it is not exploitable. Called out under sibling-precision below.

Claim 2: upload pool drain-before-cleanup — MET

• packages/aws-lambda/src/handler.ts:697-719 now runs the cursor-based worker pool via Promise.allSettled, then finds the first rejection and rethrows it. Comment at :715-717 states the intent: "Do not reject while sibling workers may still be writing into invocation scratch."
• Failure-path trace: worker A's await fn(...) rejects → A's worker promise rejects → other workers keep pulling from the shared cursor, finish or fail independently → outer Promise.allSettled resolves only after every worker settles → results.find((r) => r.status === "rejected") → throw failure.reason → propagates to caller → caller's finally { cleanupDir(work) } runs after the last in-flight download's stream has settled. This is exactly the shape the review prompt calls for (Promise.allSettled, not Promise.all).
• Publisher-side race: the publisher itself doesn't kick off a pool — putBlob calls are serial from the producer's viewpoint. commitManifest's scratch dir (packages/aws-lambda/src/s3PlanV2Publisher.ts:117-131) uses finally { rmSync(stagingDir, ...) } after the awaited uploadContentAddressedFileToS3, and the inner uploader's finally { body.destroy(); } (s3Transport.ts:180-185) settles the stream first. No orphan-file-under-cleanup path.

Claim 3: Publisher interface implementation — MET

• packages/aws-lambda/src/s3PlanV2Publisher.ts:63 declares class S3PlanV2ArtifactPublisher implements PlanV2ArtifactPublisher.
• putBlob at :81-93 validates digest shape, verifies statSync(sourcePath).size === blob.sizeBytes (throws PlanV2IntegrityError on mismatch), builds a key of the form <artifactPrefix>/<digest[:2]>/<digest>, delegates to uploadContentAddressedFileToS3 (which re-hashes the local file and mismatch-checks before PUT), and records the digest in #publishedDigests.
• commitManifest at :95-121 parses manifest, extracts every artifact sha256, and refuses to publish if any referenced digest is missing from #publishedDigests (packages/aws-lambda/src/s3PlanV2Publisher.ts:99-106). Manifest bytes are staged to a fresh mkdtempSynced dir, hashed with createHash("sha256"), and PUT via uploadContentAddressedFileToS3 with IfNoneMatch: "*" (from s3Transport.ts:165-179). #state transitions to "committed" only on successful upload.
• abort at :133-137 flips #state from "open" → "aborted" (idempotent from "committed" / "aborted") and — by design — does not delete uploaded CAS blobs. The inline rationale: blobs are content-addressed, may be reused by a concurrent retry, and expire under the render bucket's intermediate-object lifecycle policy. Legitimate tradeoff; the alternative (delete-on-abort) would race a concurrent retry that already dereferenced the same digest. Would prefer the comment mention the lifecycle-rule expectation as an operator-facing contract, but not blocking.
• Key layout: <planOutputS3Prefix>/v2/artifacts/sha256/<digest[:2]>/<digest> (s3PlanV2Publisher.ts:76-84) matches the read side at handler.ts:681-687 (planV2BlobPath + planV2BlobUri). Both use digest.slice(0, 2) as the shard prefix; layout is symmetric.

Claim 4: wire discriminator + terminal-name-list — N/A (no new terminal codes introduced)

• The publisher throws only PlanV2IntegrityError (existing) and PLAN_ARTIFACT_DIGEST_MISMATCH via the transport (existing, verified by the new retry.commitManifest(...marker="two") case at s3PlanV2Publisher.test.ts:190-194).
• handler.ts:145-155's normalizeTerminalErrorName still maps PLAN_V2_INTEGRITY_UNRECOVERABLE, PLAN_TOO_LARGE, PLAN_PROTOCOL_UNSUPPORTED to .name. No new code introduced in this PR, so nothing to add to the CDK/SAM terminal list here. If PlanV2IntegrityError's bare class name needs to appear in the Step Functions non-retryable list, that's producer / #2792's scope — grep confirms no aws-lambda side of it in this diff.

Claim 5: AWS/CDK/SAM parity — N/A (no CDK/SAM touched)

• Diff excludes packages/aws-lambda/src/cdk/** and packages/aws-lambda/sam/**. No new terminal error codes, no new environment variables, no new IAM permissions required (S3 PUT/HEAD on the render bucket already granted). Parity is preserved by omission.

Cross-adapter parity with #2799 (GCP)

• Fetched packages/gcp-cloud-run/src/gcsPlanV2Publisher.ts at 74d7bfde4 (head of #2799). Structural mirror is faithful:
  • Identical implements PlanV2ArtifactPublisher seam.
  • Same trimTrailingSlash linear-scan shape (character-code 47 loop).
  • Same #publishedDigests: Set<string> manifest-durability guard.
  • Same #state: "open" | "committed" | "aborted" state machine.
  • Same rationale-preserving abort() no-delete semantics.
• No AWS-specific concept leaks to a would-be shared abstract base — the shared abstraction is the producer's PlanV2ArtifactPublisher interface + PlanV2PublishBlob DTO; each adapter owns its transport (uploadContentAddressedFileToS3 vs uploadContentAddressedFileToGcs). Clean separation.
• Note for the #2799 reviewer: nothing in this PR obligates the GCP side to a specific terminal wire code.

Multi-shape throw normalization

• Not applicable: no new error class introduced in this PR. Existing PLAN_ARTIFACT_DIGEST_MISMATCH is thrown by .name, no .code alias present in this diff (matches the pre-PR shape).

Fresh 4-lens pass at 0999878

Standards

• No bare as T in s3PlanV2Publisher.ts. Test-file uses this as unknown as import("@aws-sdk/client-s3").S3Client inside FakeS3.asClient() (s3PlanV2Publisher.test.ts:24-26) — acceptable test-boundary duck-typing.
• No non-null ! assertions in the new publisher source.
• #publishedDigests, #state, #temporaryRoot use private-field syntax; nothing leaks into the public surface.
• parseS3Uri(outputPrefix) at s3PlanV2Publisher.ts:77 is called for effect-only (side-effect validation, discarded return) — mildly unusual but intentional as a fail-fast guard. Fine.

Spec forward-check

Every PR-body bullet has a matching diff site:

• "publishes … directly from the planner's private staging directory to S3" → putBlob streams blob.sourcePath via uploadContentAddressedFileToS3 (s3PlanV2Publisher.ts:91).
• "commits the manifest only after every referenced artifact is durable" → durability set-check at commitManifest:99-106.
• "preserves the existing Step Functions wire result" → handler.ts:376-391 returns the same result shape (manifest URI, artifact prefix, plan hash, render metadata).
• "makes immutable uploads retry- and race-safe with conditional create plus exact digest/size verification" → IfNoneMatch: "*" at s3Transport.ts:174-179 + HeadObject-based reuse/conflict discrimination at inspectContentAddressedObject:225-243 + the two new race regression tests at s3Transport.test.ts:136-165.

Spec reverse-check

• The producer-side sha256File and uploadContentAddressedFileToS3 imports are removed from handler.ts because their logic moved into the publisher; both remain exported from s3Transport.ts and are consumed by the publisher + tests. No dead exports.
• handler.ts's handlePlanV2 no longer creates planV2Dir locally (previous "extra local Plan v2 CAS directory" the PR body says was removed) — verified: the diff deletes const planV2Dir = join(work, "plan-v2"); and the subsequent read-side pool that used it. The comment in the test at handler.test.ts:544-545 even asserts existsSync(join(dirname(projectDir), "plan-v2")) is false after the primitive runs, which is a nice guard against the CAS-dir reintroduction.
• The scope carve-out noted in the PR body — "does not yet eliminate the producer's planner-local v1 staging tree; direct Plan v2 emission is a later layer" — is honored: this PR does not touch packages/producer/**. Legitimate carve-out.

Sibling-precision divergence

• trimTrailingSlash exists twice with different behavior: s3PlanV2Publisher.ts:55-59 (linear-scan, strips all trailing /) vs handler.ts:808-810 (.endsWith("/") ? .slice(0, -1) : ..., strips at most one). Different semantics under multi-slash input. P3 nit: consider hoisting the linear-scan version to s3Transport.ts or a shared util and having handler.ts reuse it — protects against a future contributor "re-fixing" ReDoS by adding a regex to the handler-side helper. Not blocking because the multi-slash user-controlled input (event.PlanOutputS3Prefix) reaches the publisher's stricter helper before it ever reaches handler.ts's helper (which only sees the publisher-normalized artifact prefix at handler.ts:686).
• mapConcurrent(uniqueArtifacts, 16, ...) at handler.ts:659 — the 16 concurrency knob is single-use; no divergent constant to reconcile.

Middle-man wrap/unwrap

• Publisher delegates to uploadContentAddressedFileToS3 without repackaging errors (PlanV2IntegrityError, PLAN_ARTIFACT_DIGEST_MISMATCH, PreconditionFailed all bubble raw). No middle-man wrapping.

Behavior + editor-UI lens complement

Silent-catch + error-invariant

• No swallow of PlanV2IntegrityError. The only catch in the publisher is inside manifestDigests at :36-39 — it re-throws as PlanV2IntegrityError("S3 publisher received invalid manifest JSON"). That's an intentional wrap (SyntaxError → typed integrity error), not a swallow.
• s3Transport.ts:181-186 catches only isS3PreconditionFailed errors, re-inspects, and either returns "reused", throws digest-mismatch, or rethrows the original. All non-precondition errors propagate.

Concurrency/lifecycle

• Publisher state machine is single-caller (producer awaits each putBlob before the next per the PlanV2ArtifactPublisher contract). Even if concurrent, #publishedDigests.add(digest) and #state mutations are single-tick-safe in V8.
• abort() is idempotent (test s3PlanV2Publisher.test.ts:215 calls .abort() twice and asserts objects survive).
• #assertOpen at :139-143 guards against post-abort/post-commit re-use, throwing PlanV2IntegrityError("cannot publish a blob after publisher is aborted") — tested at s3PlanV2Publisher.test.ts:218-220.

PR-body-vs-diff parity

• Every PR-body claim is grep-verifiable at head. No undisclosed scope. Test-plan checkboxes match the tests added (s3PlanV2Publisher.test.ts covers publication order, missing artifact rejection, malformed digests, matching retries, conflicting manifests, abort behavior; handler.test.ts covers end-to-end handler S3 handoff via publishPlanV2FromV1 mock + upload-order assertion at :610-618; s3Transport.test.ts covers concurrent-create races).

Perf audit

• No O(n²) upload: one PUT per unique digest (via uniqueArtifacts de-dup at handler.ts:656-658), one HEAD per PUT for the reuse check (early return on match).
• No unbounded retries in the publisher: the retry loop lives in the state machine's outer non-retryable-name contract, not here.
• No memory blowup: manifest is a single string (small), blobs are streamed via createReadStream, #publishedDigests is a Set<string> whose cardinality is the artifact count (bounded by manifest size).
• One P3 optimization not required for this PR: uploadContentAddressedFileToS3 re-hashes every source file locally before HEAD/PUT (s3Transport.ts:161). For very large blobs a HEAD-first-then-hash-only-if-missing would save I/O on the reuse path, but adds a new failure-mode (mid-upload corruption slipping past). Current shape is safer; defer.

Rollout gate flip verification

Confirmed. packages/aws-lambda/src/handler.ts:26,354 imports and calls planV2WithPublisher; packages/aws-lambda/src/handler.ts:365-370 constructs new S3PlanV2ArtifactPublisher({ s3, planOutputS3Prefix: event.PlanOutputS3Prefix, temporaryRoot: work }) and passes it into the primitive with { stagingParentDir: work }. No LocalPlanV2ArtifactPublisher reference anywhere under packages/aws-lambda/** (search verified: matches only in packages/producer/**). The consumer wiring is correct.

Verdict

APPROVE — GHAS ReDoS closed with a linear scan + 10k-slash regression, cleanup-race hardened via Promise.allSettled + drain-then-throw in mapConcurrent, publisher cleanly implements the three-method interface with manifest-last invariant enforcement, key layout mirrors the read side, GCP sibling PR #2799 mirrors the shape faithfully, and the AWS Lambda plan-v2 handler now consumes planV2WithPublisher with an S3PlanV2ArtifactPublisher — this is the #2792 rollout-gate flip landed cleanly.

— Via

[james-russo-rames-d-jusso]

LGTM at 09998789b with a small handful of forward-looking concerns — none blocking. This is a clean first-cut S3-direct plan v2 publisher on top of the storage-neutral interface from merged #2792.

Contract compliance verified

• S3PlanV2ArtifactPublisher (s3PlanV2Publisher.ts:65) explicitly implements PlanV2ArtifactPublisher with putBlob / commitManifest / abort matching signatures exactly.
• sourcePath lifetime: putBlob awaits uploadContentAddressedFileToS3 which awaits both sha256File(localPath) and client.send(new PutObjectCommand({ ..., Body: createReadStream(localPath), ContentLength: size })). The AWS SDK v3 .send(PutObjectCommand) promise doesn't resolve until the stream is fully consumed and the S3 response received. Contract satisfied.
• commitManifest durability: client-side check via #publishedDigests Set is sufficient because the set is only populated after uploadContentAddressedFileToS3 returned success (either PUT or verified-reused). Not race-safe against out-of-band DELETE, but that's guarded by the immutable-CAS + IfNoneMatch: "*" conditional-create + PLAN_ARTIFACT_DIGEST_MISMATCH throw on conflict.
• abort cleanup posture: publisher intentionally does NOT delete S3 objects — relies on the 7-day ExpireIntermediates lifecycle rule on the renders/ prefix at HyperframesRenderStack.ts:88-93 (pinned by contract test at HyperframesRenderStack.contract.test.ts:64-80, mirrored in template.yaml:123-131). Design intent documented and tested. Reused blobs are safe because they're content-addressed. Good posture.

GHAS trailing-slash fix + 10k-slash regression

• trimTrailingSlash at s3PlanV2Publisher.ts:52-56 is the linear-scan implementation.
• s3PlanV2Publisher.test.ts:105-113 pins the 10,000-slash input case verifying artifactPrefix and manifestUri normalize correctly (the linear-time property is implicitly tested by the fact that the test completes at all).

Upload pool drain

handler.ts:697-719 mapConcurrent switches from Promise.all to Promise.allSettled + throw-first-rejected, with the rationale correctly documented inline. This is the download-side pool (used inside handleRenderChunkV2 / handleAssembleV2 for materializing artifacts), not the publisher's upload path. Closes a genuine race: previously a rejected worker would bubble up immediately, the outer finally { cleanupDir(work) } would rmSync while sibling downloadS3ObjectToFileVerified writes were still landing. Correct fix.

CAS semantics — uploadContentAddressedFileToS3 uses IfNoneMatch: "*" for atomic create-only-if-doesn't-exist; on 412 it inspects the existing object and either returns "reused" (matching size+sha256 metadata) or throws PLAN_ARTIFACT_DIGEST_MISMATCH (conflict). Server-verified via the ChecksumSHA256 header. Race-safe, tested at s3PlanV2Publisher.test.ts:176-205.

Key injection defense — assertSha256 runs before URL interpolation (test at :154-167 pins the ../outside-prefix case). planOutputS3Prefix is bucket-checked by validateEventS3Uris upstream. No user data enters the S3 key beyond validated inputs.

Forward-looking concerns (non-blocking)

C1 — Publisher-adjacent integrity errors don't share the PlanV2IntegrityError class. s3Transport.ts throws PLAN_ARTIFACT_DIGEST_MISMATCH as new Error(...) with error.name = "PLAN_ARTIFACT_DIGEST_MISMATCH" (in two places: uploadContentAddressedFileToS3's local sha256-mismatch guard, and throwImmutableObjectConflict for the CAS 412-with-different-content path). Step Functions routes this correctly because the name is in the SAM/CDK terminal lists (verified in #2789), but upstream code that catches via instanceof PlanV2IntegrityError will silently miss it. The publisher itself already imports PlanV2IntegrityError from planV2Errors.ts and uses it for its own validation throws (s3PlanV2Publisher.ts:28, :38, :41, etc.); tightening the transport helpers to throw the same class (or a sibling that shares an ancestor) would let callers switch on type instead of stringly on name.

C2 — HasAudio uses a hardcoded string. handler.ts:386 has HasAudio: manifest.artifacts.some((artifact) => artifact.path === "audio.aac") — the exported constant PLAN_AUDIO_RELATIVE_PATH from packages/producer/src/services/distributed/shared.ts:39 is exactly this value. Using the constant would prevent a silent break if the audio artifact ever gets renamed. Two-line fix (import + swap the literal).

C3 — Double file read per putBlob. uploadContentAddressedFileToS3 reads the source file twice: sha256File(localPath) for the pre-upload integrity check, then createReadStream(localPath) for the PUT body. For a 2 GiB plan directory that's real Lambda ephemeral-storage I/O cost. The defense-in-depth motivation (guard against local disk corruption between planning and upload) is well-founded, but a stream-and-hash-in-one-pass pattern (Node Readable piped through a Hash transform then to the SDK body) would halve the I/O without giving up the check. Worth revisiting when large-plan support goes GA.

C4 — Two trimTrailingSlash implementations coexist. handler.ts:808-810 uses prefix.endsWith("/") ? prefix.slice(0, -1) : prefix (single-slash strip); s3PlanV2Publisher.ts:52-56 uses a linear-scan strip-all. No v2 user-visible impact because v2 flows only touch the publisher's version, but v1 code paths still normalize "s3://bucket/render///" → "s3://bucket/render//" (leaves a double slash). Consistency win to share one implementation. If the GHAS finding was against handler.ts's version, that one may still trip whatever check flagged it.

C5 — Test coverage gaps. s3PlanV2Publisher.test.ts has six clean happy-path tests but doesn't exercise: (a) putBlob failing mid-batch on a non-precondition S3 error; (b) commitManifest called twice (should trip #assertOpen on committed state); (c) putBlob after commitManifest (same); (d) abort() after commitManifest (implicit no-op — worth pinning); (e) manifestBytes that is invalid JSON (manifestDigests try/catch at :35-39); (f) constructor rejecting a non-s3:// planOutputS3Prefix. GCS mirrors these same holes in #2799 — symmetric backfill would tighten both.

Nothing left blocking on my side.