Skip to content

[worker] Remove legacy upload code, add retries, auto-waive builds#3848

Open
sjchmiela wants to merge 9 commits into
mainfrom
stanley/remove-gcs-upload-urls
Open

[worker] Remove legacy upload code, add retries, auto-waive builds#3848
sjchmiela wants to merge 9 commits into
mainfrom
stanley/remove-gcs-upload-urls

Conversation

@sjchmiela

Copy link
Copy Markdown
Contributor

What changed

  • Removed gcsSignedUploadUrlForApplicationArchive and gcsSignedUploadUrlForBuildArtifacts from worker runtime config and runtime config typing.
  • Removed launcher signed-upload fallback behavior for managed application archive and build artifacts uploads.
  • Classified managed artifact upload failures as SystemErrors with internal tracking codes while keeping the public error code as SERVER_ERROR.
  • Added retry options to worker upload API turtleFetch calls for upload-session creation and artifact metadata saving.

Why

These signed upload URL fields are no longer expected, so keeping fallback paths made the worker handle an obsolete runtime contract. Upload failures in these managed paths are infrastructure/API/storage failures rather than user-actionable project issues.

Validation

  • yarn oxfmt packages/worker/src/config.ts packages/worker/src/upload.ts packages/worker/src/external/turtle.ts packages/worker/src/__unit__/upload.test.ts
  • yarn workspace @expo/worker test:unit src/__unit__/upload.test.ts --runInBand
  • yarn workspace @expo/worker typecheck

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.42%. Comparing base (d2ed1fe) to head (2bc9927).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
.../build-tools/src/steps/functions/uploadArtifact.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3848      +/-   ##
==========================================
+ Coverage   58.26%   58.42%   +0.17%     
==========================================
  Files         917      917              
  Lines       39827    39977     +150     
  Branches     8350     8416      +66     
==========================================
+ Hits        23200    23353     +153     
- Misses      15193    16529    +1336     
+ Partials     1434       95    -1339     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sjchmiela sjchmiela changed the title [codex] remove deprecated worker upload URL fallbacks [worker] Remove legacy upload code, add retries Jun 11, 2026
@sjchmiela sjchmiela changed the title [worker] Remove legacy upload code, add retries [worker] Remove legacy upload code, add retries, auto-waive builds Jun 11, 2026
@sjchmiela sjchmiela added the no changelog PR that doesn't require a changelog entry label Jun 11, 2026
@sjchmiela sjchmiela marked this pull request as ready for review June 11, 2026 09:27
@sjchmiela sjchmiela requested a review from hSATAC June 11, 2026 09:28
@github-actions

Copy link
Copy Markdown

Subscribed to pull request

File Patterns Mentions
**/* @douglowder

Generated by CodeMention

@sjchmiela sjchmiela requested a review from AbbanMustafa June 11, 2026 11:31
Authorization: `Bearer ${robotAccessToken}`,
},
shouldThrowOnNotOk: false,
retries: UPLOAD_API_REQUEST_RETRIES,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/expo/eas-cli/blob/main/packages/worker/src/utils/turtleFetch.ts#L53
so before we explicitly did not retry on POST, are we sure we can now without some form of ensuring idempotency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we did not retry on POST _by default).

i think so:

  • creating upload sessions
    • in builds appends to the array (limit of 5 but i don't think we're going to hit that)
    • in job runs creates workflow artifacts (unique index on name, but if the request failed then i'd rather fail trying again than no trying again?)
  • saving artifacts ensures a given value is set for a given key so it's idempotent

next time can you please try to answer this question yourself and give a conclusive review to unblock?

@hSATAC hSATAC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a deeper look since the retry thread touches the server side too. This looks good to me.

One suggestion and one question:

Suggestion: SystemError.metadata may leak into user-visible build logs

[findAndUploadBuildArtifacts.ts#L29](

stepCtx.logger.error(`Failed to upload application archives.`, err);
), [#L36](
stepCtx.logger.error(`Failed to upload build artifacts.`, err);
), and [uploadArtifact.ts#L131](
logger.error(`Failed to upload ${artifact.type}. Ignoring error.`, error);
) use logger.error('...', err), which inspect-prints enumerable properties and now includes metadata (presigned upload URLs and response payloads from the cause chain).

The practical risk seems low since the URLs are PUT-only and short-lived, but it does add credential-shaped noise to user-visible logs. Using logger.error({ err }, '...') would route through Bunyan's serializers and avoid logging the metadata.

Question: does "auto-waive builds" need a www-side companion?

I may be missing something here, but from my reading of the code, the current SERVER_ERROR auto-waive logic appears to exist only for job runs ([TurtleJobRunsRouter.ts#L183-L186](https://github.com/expo/universe/blob/81a0f3a38f2498c56b4360ca9d6ba3fe944dcb08/server/www/src/apiv2/TurtleJobRunsRouter.ts#L183-L186)). The orchestrator intentionally never sends waivedAt for builds ([update_www.go#L54](https://github.com/expo/workflow-orchestration/blob/9241d2cef9a8a11b29436253c23605e995a43718/internal/utils/update_www.go#L54)), and I couldn't find equivalent handling on the build-update path.

If I'm understanding that correctly, builds failing these uploads would still be recorded as SERVER_ERROR and remain billable. Is there another piece I'm missing, or should the scope here be clarified to job runs only?

@github-actions

Copy link
Copy Markdown

⏩ The changelog entry check has been skipped since the "no changelog" label is present.

@sjchmiela sjchmiela requested a review from hSATAC June 12, 2026 10:48
@sjchmiela

Copy link
Copy Markdown
Contributor Author

Fixed , err vs { err } when logging errors, great catch!

Adding waiving system error builds in https://github.com/expo/universe/pull/28145.

@sjchmiela sjchmiela requested a review from AbbanMustafa June 12, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog PR that doesn't require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants