Skip to content

Fix Docker build cache command handling#236

Open
TorstenDittmann wants to merge 1 commit into
mainfrom
fix/docker-build-cache-command
Open

Fix Docker build cache command handling#236
TorstenDittmann wants to merge 1 commit into
mainfrom
fix/docker-build-cache-command

Conversation

@TorstenDittmann

Copy link
Copy Markdown
Contributor

Summary

  • keep Docker builds running the original user command when build cache is enabled
  • remove generated cache command script usage from the Docker executor
  • keep cache artifact restore/upload best-effort and preserve cache warnings in build output
  • add unit coverage for avoiding cache wrapper/lifecycle command generation

Tests

  • ./vendor/bin/phpunit --configuration phpunit.xml --testsuite=unit --filter DockerTest
  • ./vendor/bin/pint --test --config pint.json src/Executor/Runner/Docker.php tests/unit/Executor/Runner/DockerTest.php

@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown

Greptile Summary

  • Updates Docker build-cache execution so cache-enabled builds run the original user command instead of a generated cache command script.
  • Removes cache command script generation from the Docker executor.
  • Keeps cache restore/upload best-effort while preserving cache warning output paths.
  • Adds unit coverage for Docker build command generation around cache wrapper behavior.

Confidence Score: 4/5

The Docker cache command changes need attention before merge because cache persistence, failure reporting, and command escaping behavior are affected.

The changed surface is narrow and covered by focused tests, but the cache-enabled runtime paths include regressions that impact expected build cache behavior and command execution semantics.

src/Executor/Runner/Docker.php

T-Rex T-Rex Logs

What T-Rex did

  • Reproduced the getBuildCommands cache lifecycle gap for both v2 and v5, showing builds run only the user command with no cache scripts and stores.sqfs is never created.
  • Created repro artifacts including a PHP script and a failing test output to demonstrate the gap.
  • Reproduced that cache restore warnings are dropped on failure when simulating Docker.php createRuntime control flow for both v2 and v5.
  • Built a shell harness to test the quoting surface in the getBuildCommands path, reproducing the exact surface where command expansion occurs.
  • Produced failing outputs showing shell expansion of $(date), backticks, and $HOME, confirming the quoting issue.
  • Ran the PHPUnit suite to compare base and head results and observed base had no tests executed while head had 3 tests passing with 10 assertions.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: preserve build command with cache" | Re-trigger Greptile

$status = $this->orchestration->execute(
name: $runtimeName,
command: $commands,
command: $this->getBuildCommands($command, $version),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Cache lifecycle skipped

When cacheKey is set, this path now runs the original user command directly and then tries to upload /cache/stores.sqfs. The removed wrapper was the code path that ran the build-cache helper around the command, and the removed script generation was the code path that gave that helper the original build command. Normal build commands such as tar -zxf ... && bash helpers/build.sh "npm install..." do not create /cache/stores.sqfs, so storeBuildCacheArtifact() returns early and no cache artifact is saved. Cache-enabled builds can still succeed, but subsequent builds will not produce the expected saved/hit cache behavior.

Artifacts

Repro: PHP script testing getBuildCommands cache lifecycle gap

  • Contains supporting evidence from the run (text/x-php; charset=utf-8).

Repro: failing test output showing no cache lifecycle in build commands

  • Keeps the command output available without making the summary code-heavy.

View artifacts

T-Rex Ran code and verified through T-Rex

Comment on lines 399 to 400
} catch (Throwable $err) {
throw new ExecutorException(ExecutorException::RUNTIME_FAILED, $err->getMessage(), null, $err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Warnings disappear on failure

If cache restore records a warning and the user command then fails, this rethrow skips the only merge of $cacheWarnings below. The outer catch rebuilds $output from container logs or the throwable message, so the cache warning is dropped from the failed build response. A build with both a restore problem and a command failure will hide the cache problem from the caller.

Artifacts

Repro: PHP script simulating Docker.php createRuntime control flow for cache warning loss

  • Contains supporting evidence from the run (text/x-php; charset=utf-8).

Repro: execution output showing cache warnings present on success but missing on failure

  • Keeps the command output available without making the summary code-heavy.

View artifacts

T-Rex Ran code and verified through T-Rex

return [
'bash',
'-c',
'mkdir -p /tmp/logging && touch /tmp/logging/timings.txt && touch /tmp/logging/logs.txt && script --log-out /tmp/logging/logs.txt --flush --log-timing /tmp/logging/timings.txt --return --quiet --command "' . \str_replace('"', '\"', $command) . '"'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Command quoting changed

Cache-enabled v5 builds now pass the original command through the outer bash -c and script --command "..." string. Before this change, cache-enabled builds wrote the user command to /cache/build-command.sh and ran the cache helper, so the command body was not reinterpreted by this extra shell layer. Commands containing substitutions or shell escapes that were intended for the build shell can now be expanded before script receives them, changing behavior only when cache is enabled.

Artifacts

Repro: shell harness exercising the v5 getBuildCommands quoting surface

  • Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).

Repro: failing test output showing shell expansion of $(date), backticks, and $HOME

  • Keeps the command output available without making the summary code-heavy.

View artifacts

T-Rex Ran code and verified through T-Rex

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