Skip to content

Speed up deploy action#4595

Merged
MartinquaXD merged 12 commits into
mainfrom
speed-up-image-builds
Jul 7, 2026
Merged

Speed up deploy action#4595
MartinquaXD merged 12 commits into
mainfrom
speed-up-image-builds

Conversation

@MartinquaXD

@MartinquaXD MartinquaXD commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Description

Currently the deploy action takes ~15m. This makes preparing and rolling out hotfixes extremely tedious.
The main issue is that we can't make good use of caches because so far our builds happen inside docker. While it is possible to use cargo-chef to cache the dependencies it's not optimal and relatively complex.

Changes

The goal of this PR is to do the main work of compiling the binaries in the GH runner while making optimal use of caching.
This requires a few changes:

  • rust-cache does not cache the build artifacts of the generated contract bindings. To support that I needed to move them out of their separate workspace and made them part of the main workspace. Dissolving this extra workspace and merging it into the main workspace also slightly changed the behavior of formatting and doc tests which is why the just commands for those had to be adjusted a bit.
  • each main binary has a build script that bakes the commit it was built from into the binary. This means even if no code changed we at least have to rebuild each binary so that it knows the right commit. To avoid that I switched to an approach where the branch name and git commit gets set via an env variable in the docker file. That way binaries that didn't change at all don't need to be recompiled and we simply update the commit information in the docker file.
  • additionally I dropped the build script of the observe crate that was only there to silence a warning. Now the warning gets silenced via a lint option in the workspace cargo.toml file.
  • cargo figures out which files changed by looking at the modified timestamp. This gets set to the current time during the checkout action. Since this invalidates the cache I introduced another action that sets each source file's modified date to the date of the commit that last changed it.

To help rust-cache pick the most relevant cache the cache key was chosen specifically. The most important parts are $CARGO_LOCK_HASH-$BRANCH_NAME-$RUN_ID. The cargo lock hash gets added by default but the branch name and run id are added explicitly. They are here to ensure that each branch operates on their own cache to avoid experiments interfering with the main cache which is primarily used to build the images that get released to staging and always just have small incremental changes. The run id was added to force rust-cache to effectively store an updated cache snapshot after every new build. Otherwise changes from previous commits would always be compiled in addition to the latest commit.

How to test

Checked how long it takes to compile minimal changes. In these runs I only changed 1 line in the driver or solvers binary which only took 3:30 instead of the usual 15m.

Also tested on sepolia staging that the autopilot and init container run as expected and that the version string is reasonable.
Screenshot 2026-07-06 at 07 21 21
Screenshot 2026-07-06 at 07 22 28

@MartinquaXD
MartinquaXD requested review from a team as code owners July 5, 2026 14:52
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 3m 46s —— View job


PR Review: Speed up deploy action

Nice change — moving compilation to the runner with rust-cache is the right call, and the caching-key rationale in the description is sound. I verified the concerns below and left inline comments.

Verified safe:

  • The glibc reasoning (build on 22.04/2.35 → run on bookworm/2.36) is correct (forward-compatible).
  • The old Dockerfile is still required — the playground compose files use its named per-binary targets, so keeping it untouched is correct.
  • version_handler returning String works fine with axum's IntoResponse.

Inline comments posted:

  • deploy.yaml:119build-contexts: binaries=target/release ships the entire multi-GB target/release/ tree to buildkit, not just the 6 binaries. Suggest staging binaries into a small dir first.
  • deploy.yaml:77cache-workspace-crates doesn't look like a real rust-cache input (likely a silent no-op); please verify/remove.
  • .cargo/config.toml:2debug = 1 is now applied to every release build repo-wide, not just deploy (scope expansion vs. the old Dockerfile-scoped env var).

Minor note (no inline): The /version endpoint format changes from git describe output (e.g. v1.2.3-4-gabc) to branch@sha, and git_version() falls back to unknown@unknown when the env vars aren't set (e.g. playground binaries built via the old Dockerfile). Fine for production images, but worth confirming no consumer relies on the old /version format.

None of these are blockers; the build-contexts one is the most worth addressing since it directly affects the deploy time you're optimizing.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request removes the compile-time vergen build dependency across multiple crates, replacing it with a runtime version check in a new observe::version module that reads GIT_SHA and GIT_BRANCH environment variables. It also consolidates the workspace members in the root Cargo.toml and adds a new Dockerfile.deploy-ci. The review feedback recommends optimizing the runtime version retrieval by caching the environment variables using std::sync::LazyLock to avoid performance overhead and string allocations on every request to the version endpoint.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/observe/src/version.rs Outdated
Comment thread crates/orderbook/src/api/version.rs Outdated
push: true
tags: ${{ steps.meta_services.outputs.tags }}
labels: ${{ steps.meta_services.outputs.labels }}
build-contexts: binaries=target/release

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.

Using binaries=target/release as a named build context sends the entire target/release/ directory to buildkit, not just the six binaries. With cache-all-crates: true + debug = 1, that directory (deps/, build/, .fingerprint/, incremental/, …) is easily multiple GB. Since setup-buildx-action uses the docker-container driver by default, that whole tree gets tarred and transferred to the builder on every deploy — which eats into the speedup this PR is trying to achieve.

Consider staging just the needed binaries into a small directory first and pointing the context at that, e.g.:

- name: Stage binaries
  run: |
    mkdir -p dist
    cp target/release/{autopilot,driver,orderbook,refunder,solvers,pool-indexer} dist/

and build-contexts: binaries=dist.

Comment thread .github/workflows/deploy.yaml
Comment thread .cargo/config.toml
push: true
tags: ${{ steps.meta_services.outputs.tags }}
labels: ${{ steps.meta_services.outputs.labels }}
build-contexts: binaries=target/release

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.

Using binaries=target/release as a named build context sends the entire target/release/ directory to buildkit, not just the six binaries. With cache-all-crates: true + debug = 1, that directory (deps/, build/, .fingerprint/, incremental/, …) is easily multiple GB. Since setup-buildx-action uses the docker-container driver by default, that whole tree gets tarred and transferred to the builder on every deploy — which eats into the speedup this PR is trying to achieve.

Consider staging just the needed binaries into a small directory first and pointing the context at that, e.g.:

- name: Stage binaries
  run: |
    mkdir -p dist
    cp target/release/{autopilot,driver,orderbook,refunder,solvers,pool-indexer} dist/

and build-contexts: binaries=dist.

Comment thread .github/workflows/deploy.yaml
Comment thread .cargo/config.toml
@MartinquaXD
MartinquaXD marked this pull request as draft July 5, 2026 15:06
@MartinquaXD
MartinquaXD force-pushed the speed-up-image-builds branch from f06b4f9 to bf4cc57 Compare July 5, 2026 15:17
@MartinquaXD
MartinquaXD marked this pull request as ready for review July 6, 2026 06:31
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 3m 42s —— View job


PR Review: Speed up deploy action

  • Read PR context
  • Analyze the diff
  • Review key files
  • Verify workspace-merge side effects

Overall this is a solid change and the caching strategy is well reasoned. Since the earlier review round, the LazyLock version caching and doc-test/format fixes have been folded in nicely. I focused this pass on the workspace-merge fallout and re-verifying the still-open items.

Verified clean:

  • The generated contract crates now inherit the main workspace lints (clippy.cast_possible_wrap = "deny"), replacing the old generated-workspace clippy.all = "warn". I checked the generated sources for wrapping as iN casts — none exist, so cargo clippy --workspace --all-targets won't newly fail on generated code.
  • Doc-test exclusion is correct: the facade package is contracts with [lib] doctest = false, and the per-contract cow-contract-* crates are excluded via the --exclude 'cow-contract-*' glob.
  • The deploy-ci final image (all 6 binaries + tini entrypoint + netcat-openbsd) matches the old Dockerfile's untargeted final stage, so the deployed image layout is preserved.
  • Moving check-cfg for tokio_unstable from observe/build.rs into [workspace.lints.rust] is equivalent and now applies workspace-wide.

Minor cleanup (not on a diff line, so noting here):

  • Cargo.toml:156 still declares vergen = "8" in [workspace.dependencies], but every consumer (build.rs in autopilot/driver/orderbook/solvers/observe) was deleted in this PR. It's now an unused workspace dependency and can be removed. Fix this →

Still open from the earlier review round (unchanged in the latest commits, worth addressing before merge):

  • deploy.yaml build-contexts: binaries=target/release ships the whole multi-GB target/release/ tree to buildkit — the highest-impact item since it eats into the speedup. Stage the 6 binaries into a small dir and point the context at that.
  • deploy.yaml cache-workspace-crates: true isn't a recognized Swatinem/rust-cache input (silently ignored) — remove it.
  • .cargo/config.toml debug = 1 now applies to every --release build repo-wide, not just deploy. Consider scoping it back to the deploy job via CARGO_PROFILE_RELEASE_DEBUG=1.

Note: I intentionally did not re-post duplicate inline comments for the three still-open items since they're already on the PR from the prior review.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

Comment thread .github/workflows/deploy.yaml Outdated
Comment thread contracts/src/codegen.rs
Comment thread .github/workflows/deploy.yaml Outdated
@MartinquaXD
MartinquaXD added this pull request to the merge queue Jul 7, 2026
Merged via the queue into main with commit f9e9d0f Jul 7, 2026
22 checks passed
@MartinquaXD
MartinquaXD deleted the speed-up-image-builds branch July 7, 2026 13:02
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants