Skip to content

feat(container-runner): log version and git sha at actor start - #5548

Open
abcxff wants to merge 1 commit into
stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoyfrom
stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv
Open

feat(container-runner): log version and git sha at actor start#5548
abcxff wants to merge 1 commit into
stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoyfrom
stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv

Conversation

@abcxff

@abcxff abcxff commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review

Small, well-scoped change that adds a "container-runner build" log line (version + git SHA) at actor start, with a build.rs that bakes the git SHA in at compile time. A few things worth checking before merge:

Correctness

  1. build.rs will bake a stale git SHA in local/dev builds. The script only emits cargo:rerun-if-env-changed=OVERRIDE_GIT_SHA. Per Cargo's build-script rules, emitting any rerun-if-* instruction switches the build script to "precise" invalidation mode, so the default "rerun on any package file change" behavior goes away. Since there's no rerun-if-changed on the git ref files, git_head_sha() (the "colocated dev builds" fallback the doc comment calls out) only runs once; the SHA baked in won't update on later commits until something else forces the build script to rerun. Compare with the existing pattern in engine/packages/util/build.rs, which uses vergen_gitcl for the equivalent problem, and correctly tracks .git ref changes for invalidation. Recommend reusing vergen_gitcl here for consistency, or adding the missing rerun-if-changed directives if a hand-rolled git rev-parse is intentional.

  2. OVERRIDE_GIT_SHA isn't trimmed before being baked in. In build.rs:

    let git_sha = std::env::var("OVERRIDE_GIT_SHA")
        .ok()
        .filter(|sha| !sha.trim().is_empty())
        .or_else(git_head_sha)
        .unwrap_or_else(|| "unknown".to_string());

    .filter checks sha.trim().is_empty() but keeps the untrimmed string. If the build-arg value has surrounding whitespace/newline (e.g. a CI variable capturing git rev-parse HEAD with a trailing newline), that whitespace gets baked verbatim into CONTAINER_RUNNER_GIT_SHA and later into the git_sha = %git_sha tracing field in actor.rs. An embedded newline there would corrupt the structured single-line log output. The runtime fallback in main.rs::git_sha() does this correctly (.map(|sha| sha.trim().to_string()) before filtering); worth mirroring that here.

  3. Is Dockerfile.release actually part of the shipped release pipeline? I couldn't find it referenced anywhere in .github/workflows/publish.yaml or elsewhere in the repo outside its own header comment. publish.yaml's container-runner (linux-x64-musl) / (linux-arm64-musl) matrix entries, which build the artifacts actually uploaded to R2, go through docker/build/linux-x64-musl.Dockerfile / linux-arm64-musl.Dockerfile instead, and neither passes OVERRIDE_GIT_SHA. Since .git/ is repo-wide .dockerignored, git_head_sha() would also fail inside those builds. If that's the path that actually produces the binaries users get, git_sha will always log as absent ("unknown") in production, and only a manual docker build -f container-runner/Dockerfile.release --build-arg OVERRIDE_GIT_SHA=... invocation would ever see a real SHA. Worth confirming (or wiring OVERRIDE_GIT_SHA into the actual CI build path) so this doesn't ship as effectively dead in the real release artifacts.

Minor / style

  1. In actor.rs, the Some/None match duplicates the whole tracing::info! call to conditionally include git_sha. tracing's Value impl for Option<T: Value> skips the field entirely when None, so this could collapse to one call:
    tracing::info!(
        actor_id = %actor_id,
        version = crate::VERSION,
        git_sha = git_sha.as_deref(),
        "container-runner build"
    );
  2. Consider also logging version/git SHA once at process startup (async_main, next to the existing boot_id log in main.rs) so an instance that never receives an actor placement still surfaces its build info. Optional, given the PR is explicitly scoped to "at actor start."
  3. No test coverage for the new git_sha() resolution/trimming logic in main.rs, even though this file already has a precedent for testing small crate-private helpers (tests/inline/boot_id.rs). A unit test around the OVERRIDE_GIT_SHA trimming/filtering path would likely have caught point 2. (If the reason to skip is env-var mutation being unsafe across parallel tests, worth a short comment saying so.)

Nothing here is blocking-severity, but #1-#3 affect whether this feature actually works as intended in the environments it's meant for (dev iteration and production release artifacts), so I'd nail those down before merging.

@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 9afdbde to 6402b71 Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from 886a208 to 33d70a3 Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 6402b71 to e2a0a13 Compare August 10, 2026 19:03
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch 2 times, most recently from 45f065f to c0e196d Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from e2a0a13 to 5eecc2d Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from c0e196d to 8d63009 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 5eecc2d to 25a2903 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from 8d63009 to cabfb3c Compare August 11, 2026 13:39
@abcxff
abcxff force-pushed the stack/feat-container-runner-log-version-and-git-sha-at-actor-start-qolrvkqv branch from 25a2903 to 72999be Compare August 11, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant