feat(gcp-cloud-run): publish plan v2 directly to GCS#2799
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c881f23 to
0dfff06
Compare
f899d6e to
3ba2b5b
Compare
3ba2b5b to
74d7bfd
Compare
0dfff06 to
0999878
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
APPROVE — clean parity with #2795, publisher validated, drain-before-throw applied.
Verification of prior findings
No prior reviews or review comments on this PR — only the Graphite downstack-mergeability banner (jrusso1020). This is R1.
Claim-by-claim verification
Claim 1: Publisher interface implementation for GCS
packages/gcp-cloud-run/src/gcsPlanV2Publisher.ts:65-136 implements PlanV2ArtifactPublisher with the three methods:
putBlob({sourcePath, sha256, sizeBytes})at lines 83-95 — validates digest format (assertSha256), stats the source file to reconfirmsizeBytes, then delegates touploadContentAddressedFileToGcsatgcsTransport.ts:135-180which enforcesifGenerationMatch: 0(line 166), verifies matching-existing objects, and re-verifies local sha256 before upload. Tracks published digests in#publishedDigests(line 94).commitManifest(manifestBytes)at lines 97-123 — parses & validates everyartifacts[].sha256(manifestDigests), asserts each is present in#publishedDigestsBEFORE any upload (lines 99-105), then computes manifest sha256, writes to scratch, and uploads through the same content-addressed path withapplication/json. Manifest state transition to"committed"happens only after the upload settles.abort()at lines 125-129 — idempotent state flip; deliberately leaves durable remote CAS blobs intact ("Immutable CAS blobs may be shared with or reused by another retry"). Matches the sibling S3 publisher's rationale (relies on bucket intermediate-lifecycle policy).
GCS key layout at line 77-78: <planOutputGcsPrefix>/v2/artifacts/sha256/<aa>/<full_digest> and <planOutputGcsPrefix>/v2/manifest.json. The consumer side (server.ts:689-695) constructs planV2BlobPath/planV2BlobUri from the same <prefix>/<aa>/<full_digest> template — matches byte-for-byte.
commitManifest runs AFTER all putBlob calls by construction: the guard at lines 99-105 rejects committing a manifest that references any digest not in #publishedDigests. Test refuses to expose a manifest that references an unpublished digest (gcsPlanV2Publisher.test.ts:80-94) pins this.
ifGenerationMatch: 0 create-only CAS semantics verified in gcsTransport.ts:166 — used for both blobs and the fixed-key manifest. The manifest URI reuses the CAS upload helper, which gives the fixed-key manifest the same "create-once, reuse-only-if-digest-matches, reject-on-conflict" invariant (test at lines 115-143: "reuses matching objects and rejects a conflicting fixed-key manifest").
Note on abort(): it does NOT use bucket.deleteFiles. This is intentional — the immutable CAS blobs are shared across retries and are governed by the bucket's intermediate-object lifecycle policy. Matches S3 sibling verbatim. This is the correct posture for an immutable CAS scheme.
Claim 2: cleanup-race hardening
server.ts:705-727 — mapConcurrent now uses Promise.allSettled (line 717) to drain every worker before surfacing the first rejection (line 720-726). The inline comment at 723-725 explicitly cites the concern: "Invocation cleanup removes the work directory in finally. Drain all sibling downloads before surfacing an error so a late GCS stream cannot keep writing into scratch after another artifact fails verification."
This is the same shape as the AWS sibling's handler.ts mapConcurrent (verified byte-for-byte via #2795 patch). The fix lives on the DOWNLOAD side (used by downloadAndMaterializePlanV2 at server.ts:667 for chunk/assemble roles), not inside the publisher — because the publisher itself is single-caller sequential (publishPlanV2FromV1 invokes putBlob one at a time).
Claim 3: wire discriminator + terminal-name-list
No new error types are introduced by this PR. The publisher throws PlanV2IntegrityError (imported from producer, already in the NON_RETRYABLE_ERROR_NAMES set at server.ts:906) for manifest validation failures. uploadContentAddressedFileToGcs throws .name = "PLAN_ARTIFACT_DIGEST_MISMATCH" (already in the set at line 896).
normalizeTerminalErrorName at server.ts:178-188 covers code → name aliasing for PLAN_PROTOCOL_UNSUPPORTED, PLAN_TOO_LARGE, PLAN_V2_INTEGRITY_UNRECOVERABLE. Test returns 400 for plan v2 integrity error names and code aliases (server.test.ts:522-558) pins all three shapes: class instance, name alias, code-only. Multi-shape normalization intact.
Cloud Workflows retry predicate at packages/gcp-cloud-run/terraform/workflow.yaml:304-318 (unchanged in this PR) keys off HTTP status (400 = non-retryable, 5xx/429/403/no-code = retryable). Since the discriminator flow is name → HTTP 400 → workflow non-retryable, no workflow.yaml change is needed. Correct.
Cross-adapter parity with #2795 (AWS S3)
Fetched packages/aws-lambda/src/s3PlanV2Publisher.ts at PR #2795 head SHA 09998789 for structural diff:
- Class shape: identical — same three methods, same
#publishedDigestsSet, same#statetransitions, same guard sequencing incommitManifest, same#assertOpenhelper. Only the identifier prefixes (S3PlanV2ArtifactPublishervsGcsPlanV2ArtifactPublisher), transport function names, and error message prefixes differ. Line count matches (137 vs 136). - Key layout: byte-for-byte identical (
<prefix>/v2/artifacts/sha256/<aa>/<digest>+<prefix>/v2/manifest.json). - Abort semantics: identical — leaves durable remote CAS intact, doc-string cites bucket lifecycle.
- Cleanup race fix:
mapConcurrenton both sides uses the samePromise.allSettled+ throw-first-rejection shape. Concurrency constant16matches both at server.ts:667 and aws handler.ts:659. - Test coverage: same 6 tests on both publishers with matching intent (trim-slash linear-time / manifest-last / unpublished-digest guard / malformed-digest guard / reuse + conflict / abort-preserves-CAS).
- Wire discriminator: GCS uses
NON_RETRYABLE_ERROR_NAMESset +normalizeTerminalErrorName(name+code alias). AWS uses its state-machine's own retry config outside the handler (existing divergence, documented invariant — Cloud Run maps status codes to workflow retry, Lambda names go through state-machine retry config).
No undocumented divergence.
Multi-shape throw normalization
Not applicable — no new error types in this PR. Existing coverage (class + name-alias + code-only) verified by server.test.ts:522-558.
Fresh 4-lens pass at 74d7bfd
Standards
Publisher and test file are free of bare as T and non-null ! assertions (grep confirmed). Existing casts in server.ts (lines 143, 199, 201, 204, 208, 934) are all pre-existing envelope-unwrap paths guarded by discriminant checks — not introduced by this PR. The single values[index]! at server.ts:714 is safe: index = cursor++ is guarded by while (cursor < values.length).
Spec forward-check
PR body claims → diff evidence:
- "publishes Plan v2 content-addressed artifacts directly from the Cloud Run planner's private staging directory to GCS" ✅ (
gcsPlanV2Publisher.ts+server.ts:373-380) - "commits the Plan v2 manifest only after every referenced artifact is durable" ✅ (lines 99-105 pre-commit guard)
- "removes the second local Plan v2 CAS directory" ✅ (patch removes
const planV2Dir = join(work, "plan-v2");; test atserver.test.ts:258assertsexistsSync(join(dirname(projectDir), "plan-v2")) === false) - "preserves the existing Cloud Workflows wire contract" ✅ (workflow.yaml untouched; discriminator via HTTP status still valid)
- "exports a reusable GCS publisher adapter" ✅ (index.ts:56-59)
Spec reverse-check
Diff scope beyond claims: server.ts also updates mapConcurrent to drain-before-throw. This is the cleanup-race hardening — implicitly part of the "distributed contract must contain only durable GCS locators" invariant, and the inline comment cites the same rationale as the S3 sibling. Undisclosed but load-bearing and safe.
Sibling-precision divergence
mapConcurrentconcurrency = 16 — same value used in AWS handler. Match.MAX_ENVELOPE_DEPTH = 4— single-site, unchanged.#publishedDigestsSet on both publishers — same shape.- Key template
<prefix>/v2/artifacts/sha256/<aa>/<digest>— matches consumer-sideplanV2BlobPath/planV2BlobUriatserver.ts:689-695and matches S3 sibling. No divergence.
Middle-man wrap/unwrap
gcsPlanV2Publisher.ts:83-95 (putBlob) does one net thing: validate digest, verify size, upload. commitManifest does one net thing: gate on published digests, then upload manifest. No wrap-then-unwrap detours.
server.ts:373-403 (handlePlanV2): previously did planV2 → local staging → readPlanV2Manifest → upload artifacts loop → upload manifest. Now does planV2WithPublisher(publisher) — the publisher inversion eliminates the intermediate staging + manifest re-read, which was pure wrap/unwrap. Net simplification: −48 lines vs +35 lines in server.ts.
Behavior + editor-UI lens complement
Silent-catch + error-invariant
server.ts:842-849 (cleanupDir) swallows filesystem errors on the cleanup path — pre-existing, documented ("leak is preferable to crashing on the success path"). Acceptable.
server.ts:965 (module-boot guard) is unchanged.
Publisher itself has no silent catches. commitManifest's try/finally (lines 110-122) always calls rmSync(stagingDir, ..., force: true); failure inside the try re-throws with staging removed. Correct.
Concurrency/lifecycle
Publisher is stateful (#state: "open" | "committed" | "aborted") — every mutating method calls #assertOpen first. abort() is idempotent (line 126 gates on "open"). The publisher is not thread-safe against parallel putBlob calls from a single instance, but the producer's publishPlanV2FromV1 invokes putBlob sequentially, so this matches the interface contract. Same shape as S3.
Upload-pool drain: mapConcurrent awaits every worker via Promise.allSettled before surfacing the first rejection. In-flight createWriteStream writes complete before the request-level finally at server.ts:507 fires cleanupDir(work).
PR-body-vs-diff parity
Every PR body bullet lands in the diff. The one unlisted change (mapConcurrent drain fix) is a bug-fix delta that the AWS sibling also carries in matched shape — not a scope-widening surprise.
Test plan checkboxes claim "all 95 GCP Cloud Run package tests pass"; test file adds 6 new publisher tests + updates the v2 e2e test to use planV2WithPublisher. Not independently re-run here — CI checks all pass (no failing required checks; only Graphite / mergeability_check pending, expected for a downstack).
Perf audit
putBlobre-stats the source file (line 86) — one extrastatSyncper blob, negligible against the sha256+upload roundtrip.- Manifest write goes through
mkdtempSync+writeFileSync+uploadContentAddressedFileToGcs+rmSync— one filesystem round-trip per commit, one-shot at the end of planning. No hot-path concern. mapConcurrentfanout of 16 is unchanged from prior behavior.
Rollout gate flip verification (GCP)
Cloud Run's handlePlanV2 at server.ts:358-403 now wires new GcsPlanV2ArtifactPublisher(...) and calls planV2WithPublisher(projectDir, config, publisher, { stagingParentDir: work }). No LocalPlanV2ArtifactPublisher in packages/gcp-cloud-run/** (verified via GitHub code search: 0 hits). E2E test asserts the second CAS directory (plan-v2 under work) is NOT created (server.test.ts:258). Gate flip lands cleanly on the GCP side, symmetric with the S3 sibling.
Verdict
APPROVE
Publisher is structurally identical to the S3 sibling (same interface, same key layout, same abort posture, same test matrix). Cleanup-race hardening applied at mapConcurrent in matched shape across both adapters. Wire discriminator + terminal-name list already covers the paths this PR touches. Rollout gate correctly wires the GCS publisher on Cloud Run with the second local CAS directory retired.
— Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Fresh exact-head review at 74d7bfde4870bbc1c6c4471cfc004964807df3e7 found no blocking issue.
The GCS implementation preserves the same contract as the reviewed S3 parent: planner-local source paths never cross the worker boundary, ifGenerationMatch: 0 makes CAS creation race-safe, matching winners are reused only after metadata/size verification, the publisher tracks durable blobs before manifest commit, and both publication/download pools drain before scratch removal. The end-to-end adapter test exercises plan → target-scoped chunk → assemble without a shared destination filesystem.
Fresh local verification after building the stack dependencies: GCP package 96/96 and typecheck pass; the combined stack also builds producer, AWS, and GCP packages successfully. GitHub only reports change-detected regression checks on this stacked head, so the inevitable post-parent restack still needs fresh exact-head required CI and approval before merge.
Verdict: APPROVE
Reasoning: The direct GCS publisher is behaviorally and failure-semantically aligned with the S3 parent, with no new correctness or lifecycle gap found at this head.
— Magi
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
LGTM at 74d7bfde4. This is a clean symmetric port of the AWS S3 publisher from #2795, plus the download-side mapConcurrent cleanup-race hardening catching GCS up to the AWS handler side.
Interface + contract verification
GcsPlanV2ArtifactPublisher(gcsPlanV2Publisher.ts:68-75) explicitlyimplements PlanV2ArtifactPublisherwithputBlob,commitManifest,abortmatching the interface signatures exactly.putBlobfully awaitsuploadContentAddressedFileToGcs(which awaits bothsha256File(localPath)andbucketHandle.upload(localPath, ...)) before resolving —sourcePathcontract satisfied.commitManifestiteratesmanifestDigests(manifestBytes)and rejects withPlanV2IntegrityErrorif any digest is missing from the in-memory#publishedDigestsset. Set is populated only afteruploadContentAddressedFileToGcsreturns success, so it's authoritative for what this publisher put in GCS during its lifetime.abortis a state-flip no-op that leaves durable CAS blobs intact — same design as AWS, and the render bucket'srenders/lifecycle expiration (verified in AWS atHyperframesRenderStack.ts:88-93; GCP equivalent presumably lives in the Terraform module) sweeps orphans. Test atgcsPlanV2Publisher.test.ts:206-...pins the idempotent no-delete contract.
Cleanup-race hardening at server.ts:717-727 is exactly right:
const results = await Promise.allSettled(
Array.from({ length: Math.min(concurrency, values.length) }, () => worker()),
);
const failure = results.find(
(result): result is PromiseRejectedResult => result.status === "rejected",
);
// Invocation cleanup removes the work directory in `finally`. Drain all
// sibling downloads before surfacing an error so a late GCS stream cannot
// keep writing into scratch after another artifact fails verification.
if (failure) throw failure.reason;Closes the same race the AWS handler shipped in #2795: Promise.all would surface the first rejection immediately, then the outer finally { cleanupDir(work) } would rmSync while sibling downloadGcsObjectToFileVerified writes were still landing under the same work dir. The Promise.allSettled drain guarantees every in-flight download settles before the workdir is deleted.
GCS-specific CAS semantics — bucketHandle.upload(..., { preconditionOpts: { ifGenerationMatch: 0 } }) in gcsTransport.ts (pre-existing) provides atomic create-only-if-doesn't-exist. On 412, isReusableContentAddressedObject re-verifies size + sha256 metadata against the winner; only matching content is treated as "reused", otherwise PLAN_ARTIFACT_DIGEST_MISMATCH is thrown. Symmetric with AWS's IfNoneMatch: "*" + inspectContentAddressedObject.
AWS/GCS symmetry review — the two publishers are essentially isomorphic (interface, state machine, putBlob/commitManifest/abort shape, error typing, key construction, injection defense, CAS retry-recovery). One informational asymmetry worth capturing:
Defense-in-depth asymmetry (pre-existing, not new to this PR): AWS's uploadContentAddressedFileToS3 sends ChecksumSHA256 on the PutObjectCommand so S3 server-side verifies the body's sha256 against the header and rejects mid-flight corruption. GCS's bucketHandle.upload(...) only carries the sha256 as custom user metadata (metadata: { metadata: { sha256: expectedSha256 } }) — the GCS server does not verify body-to-sha256 correspondence. The SDK's default MD5 checksum still catches transit corruption (GCS server verifies MD5), and downloadGcsObjectToFileVerified re-hashes at read time — so the defense-in-depth cost is not zero, just weaker at rest until someone reads back. Worth remembering when cross-cloud replication / integrity-at-rest stories land. Not a bug and not new to this PR — it lives in gcsTransport.ts from earlier.
Test coverage — six tests in gcsPlanV2Publisher.test.ts mirror the six in s3PlanV2Publisher.test.ts line for line. Gaps also mirror (see my #2795 review body for the enumerated missing scenarios — same holes apply here). Non-blocking; symmetric backfill would be a nice follow-up.
Nothing left blocking on my side.
The base branch was changed.
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head restamp at 74d7bfde4870bbc1c6c4471cfc004964807df3e7.
This is an ancestry/restack restamp; the five-file GCP Plan v2 implementation is unchanged. The adapter preserves manifest-last immutable CAS publication, create-only generation preconditions, storage-only locators across stages, v1 default behavior, and all-settled download cleanup.
Fresh verification: ordered studio-server, producer, and GCP Cloud Run builds pass; GCP tests are 96/96; typecheck and diff check pass. I held the stamp until every current exact-head regression shard was terminal; the full matrix is now green with no failures.
Verdict: APPROVE
Reasoning: The unchanged GCP object-storage implementation retains its reviewed safety invariants, local verification is clean, and the complete restacked exact-head CI matrix has now finished green.
— Magi

What
Why
Cloud Run planner, chunk, and assembler requests are independent containers and do not share a filesystem. The distributed contract must contain only durable GCS locators.
This layer makes the Plan v2 publication path object-store-native. Like the AWS parent PR, it still relies on one planner-local frozen v1 tree inside the producer; eliminating that remaining staging tree requires direct Plan v2 emission in a later layer.
Design invariants
Test plan