Speed up integration tests#2793
Open
mkocher wants to merge 5 commits into
Open
Conversation
add log line for sandbox reset times
also prevents a bunch of spec files from getting required in spec helper which were then getting run by each parallel worker
Spinning up the sandbox takes 3 to 5 seconds. Using before(:all) when possible to eliminate the overhead. Also fixed an issue with with_reset_sandbox_before_all where it wasn't reconfiguring unless the options were empty.
log runtime to the orphan release-details branch, have parallel_test use that runtime data to balance spec files across parallel threads
WalkthroughThe changes add release-details runtime-log exchange to integration CI, configure ParallelTests to reuse and publish runtime data, and adjust integration specs to use group-level setup. Sandbox resets now emit phase timings. Nginx package preparation downloads fingerprinted blobs directly and tracks platform and package markers. Spec loading excludes support spec files, and nginx-generated artifacts are ignored. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/spec/integration_support/nginx_service.rb (2)
164-168: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix
TypeErrorwhen passing an environment hash toIO.popen.Including the
environmentHash as the first element of thecommandarray will cause aTypeError(no implicit conversion of Hash into String) whenIO.popen(command)is called, becauseIO.popenexpects the array elements to be strings. Additionally,command.join(' ')will stringify the Hash into"{...}", resulting in confusing log output.To correctly pass environment variables, provide the
environmentHash as the first, separate argument toIO.popen.🐛 Proposed fix to correctly pass the environment Hash
def run_command(command, environment = {}) - command = [environment, 'bash', '-c', command] - puts "Running: #{command.join(' ')}" - - io = IO.popen(command) + cmd_array = ['bash', '-c', command] + env_string = environment.map { |k, v| "#{k}=#{v}" }.join(' ') + puts "Running: #{env_string} #{cmd_array.join(' ')}".strip + + io = IO.popen(environment, cmd_array)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 164 - 168, Update run_command so environment is passed as the separate first argument to IO.popen, while the command array contains only string arguments for bash execution. Adjust the “Running” log to display the command without stringifying the environment hash.
164-168: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix
TypeErrorwhen passing an environment hash toIO.popen.Including the
environmentHash as the first element of thecommandarray will cause aTypeError(no implicit conversion of Hash into String) whenIO.popen(command)is called, becauseIO.popenexpects the array elements to be strings. Additionally,command.join(' ')will stringify the Hash into"{...}", resulting in confusing log output.To correctly pass environment variables, provide the
environmentHash as the first, separate argument toIO.popen.🐛 Proposed fix to correctly pass the environment Hash
def run_command(command, environment = {}) - command = [environment, 'bash', '-c', command] - puts "Running: #{command.join(' ')}" - - io = IO.popen(command) + cmd_array = ['bash', '-c', command] + env_string = environment.map { |k, v| "#{k}=#{v}" }.join(' ') + puts "Running: #{env_string} #{cmd_array.join(' ')}".strip + + io = IO.popen(environment, cmd_array)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 164 - 168, Update run_command so the environment Hash is passed as the separate first argument to IO.popen, while the command array contains only string command elements. Keep the execution order and adjust the “Running” log to format only the command arguments, avoiding Hash stringification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/tasks/test-rake-task.sh`:
- Around line 237-248: Update the git add command in the release-details block
to reference the exact copied file under parallel_spec_runtimes, using the
integration_${DB}_${UPDATE_VM_STRATEGY:-delete-create}.log naming pattern. Quote
the complete path and variable expansions to prevent word splitting, while
preserving the existing commit flow.
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 123-125: Replace the hardcoded /tmp/nginx-package.tgz path in the
blob download and extraction commands with a process-specific temporary path,
reusing that same path for both curl output and tar input. Update the commands
around blob_url and run_command without changing the download or extraction
behavior.
- Around line 123-125: Replace the hardcoded /tmp/nginx-package.tgz path in the
nginx package download and extraction flow with a process-specific temporary
path, and use that same path for both run_command invocations. Preserve the
existing blob URL and extraction behavior while ensuring parallel executions
cannot overwrite each other’s archive.
In `@src/spec/integration_support/sandbox.rb`:
- Around line 582-591: Replace the invalid inline rescue in the nats_restart
timing block with an explicit begin/rescue around
read_from_sandbox(NATS_CONFIG), rescuing Errno::ENOENT and assigning nil.
Preserve the existing rendered_nats_config comparison and restart behavior.
In `@src/spec/integration/deploy/local_template_properties_spec.rb`:
- Line 4: The setup hooks in
src/spec/integration/deploy/local_template_properties_spec.rb:44-48 and
src/spec/integration/links/link_parse_validation_spec.rb:26-31 should upload
shared assets only once per example group. Change the local template properties
setup to before(:all), and in link_parse_validation_spec.rb move
upload_links_release and upload_stemcell into a separate before(:all) hook while
keeping upload_cloud_config in before do because it depends on cloud_config.
In `@src/spec/integration/errand/cleanup_obsolete_errand_instances_spec.rb`:
- Line 4: Update the setup around with_reset_sandbox_before_all and the
per-example before hook so each example that deploys or mutates sandbox state
receives an isolated reset. Preserve the existing example setup while moving the
reset to the appropriate per-example lifecycle or otherwise removing the
group-level/per-example state conflict.
In `@src/spec/integration/errand/list_errands_spec.rb`:
- Line 4: Replace the group-level with_reset_sandbox_before_all in the list
errands spec with the per-example reset mechanism used by
cleanup_obsolete_errand_instances_spec.rb, so each example resets the sandbox
before deploying fresh manifests in the existing before block.
In `@src/spec/integration/template_evaluation_spec.rb`:
- Line 4: Replace the group-level with_reset_sandbox_before_all setup in this
spec with per-example sandbox resets so every it block starts from clean state
before running its deploy_from_scratch or deploy_simple_manifest setup. Preserve
the existing assertions and deployment behavior, including the “same id on
recreate” events check.
In `@src/spec/integration/update_settings_spec.rb`:
- Line 4: Update the setup around with_reset_sandbox_before_all in the settings
integration specs so each example receives a fresh sandbox state, preventing
tests from sharing trusted-certificate mutations or service restarts. Preserve
the existing assertions while removing their dependency on execution order.
---
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 164-168: Update run_command so environment is passed as the
separate first argument to IO.popen, while the command array contains only
string arguments for bash execution. Adjust the “Running” log to display the
command without stringifying the environment hash.
- Around line 164-168: Update run_command so the environment Hash is passed as
the separate first argument to IO.popen, while the command array contains only
string command elements. Keep the execution order and adjust the “Running” log
to format only the command arguments, avoiding Hash stringification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64ce9cf8-be10-411b-8061-616a1029af8a
📒 Files selected for processing (21)
.gitignoreci/pipeline.ymlci/tasks/test-rake-task.shci/tasks/test-rake-task.ymlsrc/spec/integration/cli_instances_spec.rbsrc/spec/integration/deploy/dry_run_spec.rbsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/errand/cleanup_obsolete_errand_instances_spec.rbsrc/spec/integration/errand/list_errands_spec.rbsrc/spec/integration/errand/run_errand_success_spec.rbsrc/spec/integration/events_access_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration/links/network_resolution_spec.rbsrc/spec/integration/template_evaluation_spec.rbsrc/spec/integration/unmanaged_persistent_disks_spec.rbsrc/spec/integration/update_settings_spec.rbsrc/spec/integration_support/integration_example_group.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/spec/spec_helper.rbsrc/tasks/spec.rake
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/spec/integration_support/nginx_service.rb (2)
172-180: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the block form of
IO.popento prevent file descriptor leaks.If an exception is raised inside the
each_with_objectblock (e.g., ifputsthrowsErrno::EPIPE), execution will skipio.close. This can leak file descriptors and leave zombie child processes, especially in long-running parallel test runners.Using the block form of
IO.popenguarantees that the IO stream is closed and the process status is reaped regardless of exceptions.🛠️ Proposed fix
- io = IO.popen(environment, cmd_array) - - lines = - io.each_with_object("") do |line, collect| - collect << line - puts line.chomp - end - - io.close + lines = IO.popen(environment, cmd_array) do |io| + io.each_with_object("") do |line, collect| + collect << line + puts line.chomp + end + end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 172 - 180, Update the IO.popen usage in the surrounding command-output collection flow to use its block form, moving the each_with_object processing inside the block and removing the manual io.close call. Preserve the existing line collection and puts behavior while ensuring the popen resource and child process are cleaned up on exceptions.
8-17: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRace condition during parallel NGINX installation.
Although the tarball download path (
tmp_pkg) is correctly isolated per-process, the target directories (packages/nginxfor extraction,WORK_DIRfor compilation, andINTEGRATION_BIN_DIRfor binaries) are shared across all processes in the repository.When CI runs parallel test threads, multiple processes will concurrently attempt to extract blobs to the same directory or delete and recreate
WORK_DIR, causing intermittent corrupted files and flaky test failures. Wrap the installation logic in a file lock so only one process performs the setup; the others will safely wait and then correctly skip compilation since the fingerprints will match.🔒️ Proposed fix to synchronize parallel installations
def self.install - installer = NginxInstaller.new - installer.prepare - - if installer.should_compile? - installer.compile - else - puts 'Skipping compiling nginx because shasums and platform have not changed' + lock_file_path = File.join(IntegrationSupport::Constants::BOSH_REPO_SRC_TMP_DIR, 'nginx_install.lock') + FileUtils.mkdir_p(File.dirname(lock_file_path)) + + File.open(lock_file_path, 'a') do |lock_file| + lock_file.flock(File::LOCK_EX) + + installer = NginxInstaller.new + installer.prepare + + if installer.should_compile? + installer.compile + else + puts 'Skipping compiling nginx because shasums and platform have not changed' + end end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 8 - 17, Wrap the installation workflow in NginxService.install with a shared file lock covering installer.prepare and the conditional compile step, so extraction, workspace cleanup, and binary creation cannot run concurrently. Ensure waiting processes re-enter the locked flow, observe the updated fingerprints through installer.should_compile?, and skip compilation when installation is already current.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 172-180: Update the IO.popen usage in the surrounding
command-output collection flow to use its block form, moving the
each_with_object processing inside the block and removing the manual io.close
call. Preserve the existing line collection and puts behavior while ensuring the
popen resource and child process are cleaned up on exceptions.
- Around line 8-17: Wrap the installation workflow in NginxService.install with
a shared file lock covering installer.prepare and the conditional compile step,
so extraction, workspace cleanup, and binary creation cannot run concurrently.
Ensure waiting processes re-enter the locked flow, observe the updated
fingerprints through installer.should_compile?, and skip compilation when
installation is already current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f980e146-3964-40f2-bc23-0c04edec05e5
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
mkocher
force-pushed
the
speed-up-integration-tests
branch
from
July 21, 2026 07:25
1129a6c to
d789df8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/tasks/test-rake-task.sh`:
- Around line 240-247: Update the release-details handling condition to require
a non-empty parallel_runtime_rspec.log using -s, preventing empty files from
being copied or committed. In this block, replace the cd release-details and
directory-relative Git commands with equivalent git -C release-details commands,
while preserving the existing copy path, staging target, author configuration,
and commit behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23997417-4296-4b7e-9dfe-f69e3b4024ff
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
mkocher
force-pushed
the
speed-up-integration-tests
branch
from
July 21, 2026 17:43
d789df8 to
2f8cebc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/spec/integration_support/nginx_service.rb (2)
172-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the block form of
IO.popenfor safe resource management.If an exception occurs while reading from
io(e.g., an encoding error),io.closewill be skipped, potentially leaking the file descriptor. Using the block form ofIO.popenguarantees that the IO stream is automatically closed and$?is safely set when the block exits.♻️ Proposed fix
- io = IO.popen(environment, cmd_array) - - lines = - io.each_with_object("") do |line, collect| - collect << line - puts line.chomp - end - - io.close + lines = + IO.popen(environment, cmd_array) do |io| + io.each_with_object("") do |line, collect| + collect << line + puts line.chomp + end + end + process_status = $?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 172 - 181, Update the IO.popen usage around the lines collection to use its block form, moving the each_with_object read and output logic inside the block so the stream is closed even if reading raises. Preserve capturing the resulting process status after the block exits, relying on the block form to set $?.
95-131: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winData race on shared
packages/nginxdirectory during concurrent execution.When running
parallel_testslocally, multiple processes share the sameBOSH_REPO_ROOTworkspace. If they start simultaneously, they will all evaluate.blob-fingerprintas missing and concurrently extract their tarballs into the sharedpackages/nginxdirectory. This TOCTOU (time-of-check to time-of-use) race condition can lead to corrupted files, "Text file busy" errors, or incomplete reads by thecp -Rcommand incompile.Wrap the check and extraction logic in a file lock to ensure only one process prepares the shared package directory.
🔒️ Proposed fix to synchronize blob preparation
def prepare Dir.chdir(IntegrationSupport::Constants::BOSH_REPO_ROOT) do - # The nginx package now uses spec.lock. Its package blob lives in the - # public GCS blobstore alongside all other BOSH release blobs and contains - # the nginx source tarballs plus the packaging script that compile() will run below. - # - # We download it directly via curl rather than going through - # `bosh create-release --force`, which tries to resolve *all* release - # packages and fails when any package has a fingerprint that differs from - # what is indexed in .final_builds (e.g. packages with local source - # changes like director, nats, davcli, health_monitor). We only need - # the nginx package blob here, so the direct download is both simpler - # and more reliable for local development. - - # If the packaging script is already present and matches the current - # fingerprint, the blob was already extracted — skip re-downloading. - cached_fingerprint_file = 'packages/nginx/.blob-fingerprint' - if File.exist?('packages/nginx/packaging') && - File.exist?(cached_fingerprint_file) && - File.read(cached_fingerprint_file).strip == package_fingerprint - puts 'nginx package blob already extracted, skipping download' - return - end - - index = YAML.load_file('.final_builds/packages/nginx/index.yml') - blobstore_id = index.dig('builds', package_fingerprint, 'blobstore_id') - raise "nginx fingerprint #{package_fingerprint} not found in .final_builds/packages/nginx/index.yml" unless blobstore_id - - blob_url = "https://storage.googleapis.com/bosh-release-blobs/#{blobstore_id}" - tmp_pkg = "/tmp/nginx-package-#{Process.pid}.tgz" - run_command("curl -fSL -o #{tmp_pkg} '#{blob_url}'") - run_command("tar -xf #{tmp_pkg} -C packages/nginx") - File.write(cached_fingerprint_file, package_fingerprint) - ensure - FileUtils.rm_f(tmp_pkg) if tmp_pkg + File.open('packages/nginx/.blob.lock', File::RDWR | File::CREAT, 0644) do |lock_file| + lock_file.flock(File::LOCK_EX) + + # The nginx package now uses spec.lock. Its package blob lives in the + # public GCS blobstore alongside all other BOSH release blobs and contains + # the nginx source tarballs plus the packaging script that compile() will run below. + # + # We download it directly via curl rather than going through + # `bosh create-release --force`, which tries to resolve *all* release + # packages and fails when any package has a fingerprint that differs from + # what is indexed in .final_builds (e.g. packages with local source + # changes like director, nats, davcli, health_monitor). We only need + # the nginx package blob here, so the direct download is both simpler + # and more reliable for local development. + + # If the packaging script is already present and matches the current + # fingerprint, the blob was already extracted — skip re-downloading. + cached_fingerprint_file = 'packages/nginx/.blob-fingerprint' + if File.exist?('packages/nginx/packaging') && + File.exist?(cached_fingerprint_file) && + File.read(cached_fingerprint_file).strip == package_fingerprint + puts 'nginx package blob already extracted, skipping download' + return + end + + index = YAML.load_file('.final_builds/packages/nginx/index.yml') + blobstore_id = index.dig('builds', package_fingerprint, 'blobstore_id') + raise "nginx fingerprint #{package_fingerprint} not found in .final_builds/packages/nginx/index.yml" unless blobstore_id + + blob_url = "https://storage.googleapis.com/bosh-release-blobs/#{blobstore_id}" + begin + tmp_pkg = "/tmp/nginx-package-#{Process.pid}.tgz" + run_command("curl -fSL -o #{tmp_pkg} '#{blob_url}'") + run_command("tar -xf #{tmp_pkg} -C packages/nginx") + File.write(cached_fingerprint_file, package_fingerprint) + ensure + FileUtils.rm_f(tmp_pkg) if tmp_pkg + end + end end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 95 - 131, Protect the cache check and blob download/extraction sequence in prepare with a shared file lock for packages/nginx, so concurrent processes serialize the entire preparation operation. Recheck .blob-fingerprint after acquiring the lock, then skip or perform extraction accordingly; keep cleanup of the temporary archive in the existing ensure block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spec/integration_support/sandbox.rb`:
- Around line 583-584: Correct the wording in the comment near the restart logic
by changing “only restart of the config has changed” to “only restart if the
config has changed,” without modifying the surrounding behavior or code.
In `@src/tasks/spec.rake`:
- Around line 17-24: Ensure the `tmp` output directory exists before configuring
the RSpec loggers by calling Rake’s `mkdir_p('tmp')`, then replace the combined
`File.exist?` and `File.size` condition with
`File.size?('tmp/parallel_runtime_rspec.log')` while preserving the
runtime-based grouping behavior.
---
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 172-181: Update the IO.popen usage around the lines collection to
use its block form, moving the each_with_object read and output logic inside the
block so the stream is closed even if reading raises. Preserve capturing the
resulting process status after the block exits, relying on the block form to set
$?.
- Around line 95-131: Protect the cache check and blob download/extraction
sequence in prepare with a shared file lock for packages/nginx, so concurrent
processes serialize the entire preparation operation. Recheck .blob-fingerprint
after acquiring the lock, then skip or perform extraction accordingly; keep
cleanup of the temporary archive in the existing ensure block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54d19c83-7e51-4c5d-9ce4-e4b10269f019
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
mkocher
force-pushed
the
speed-up-integration-tests
branch
from
July 21, 2026 23:16
2f8cebc to
9e737c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/spec/integration/deploy/local_template_properties_spec.rb (1)
4-4: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not share sandbox state across these example groups.
with_reset_sandbox_before_allonly resets once at group start, so deployments created by one example can leak into the next; these specs reuse the same deployment names and should keep per-example isolation or explicit cleanup.
src/spec/integration/deploy/local_template_properties_spec.rb#L4src/spec/integration/links/link_parse_validation_spec.rb#L4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration/deploy/local_template_properties_spec.rb` at line 4, The integration specs must isolate sandbox state for each example rather than resetting only once per group. Update the setup at src/spec/integration/deploy/local_template_properties_spec.rb:4 and src/spec/integration/links/link_parse_validation_spec.rb:4 to use per-example isolation or explicit cleanup, preserving independent execution when deployment names are reused.src/spec/integration_support/nginx_service.rb (2)
146-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrite compile markers only after compilation succeeds.
If compilation fails after
COMPILED_PLATFORM_FILEis updated, an existing executable from the old platform plus an unchanged fingerprint marker can make the nextshould_compile?call skip compilation. Move the platform marker write next to the fingerprint marker, after the packaging command succeeds; atomic marker replacement is preferable.Proposed ordering
- File.write(COMPILED_PLATFORM_FILE, RUBY_PLATFORM) - # Make sure packaging script has its own blob copies so that blobs/ directory is not affected ... run_command("bash #{packaging_script_path}", { 'BOSH_INSTALL_TARGET' => IntegrationSupport::Constants::INTEGRATION_BIN_DIR }) - + + File.write(COMPILED_PLATFORM_FILE, RUBY_PLATFORM) File.write(COMPILED_FINGERPRINT_FILE, package_fingerprint)Also applies to: 160-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 146 - 149, Move the COMPILED_PLATFORM_FILE write out of the pre-compilation setup and place it alongside the fingerprint marker update only after packaging/compilation succeeds, ensuring both markers are written together. Prefer atomically replacing the platform marker (and preserve the existing should_compile? behavior) so failed compilation cannot leave stale success markers.
119-127: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the package blob before extracting it.
.final_builds/packages/nginx/index.ymlincludes a digest, but this code trusts onlyblobstore_idand immediately extracts the response. Because the archive contains the packaging inputs later executed byNginxInstaller#compile, a corrupt or incorrect object becomes executable build input. Compare the temporary file with the indexed digest beforetar -xf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 119 - 127, In the package download flow using index and tmp_pkg, read the indexed digest for package_fingerprint and verify the downloaded temporary archive against it before invoking tar extraction. Abort with a clear error when verification fails, while preserving the existing blobstore lookup and extraction behavior for valid packages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 123-126: The GCS download in the nginx package setup can block
indefinitely. Update the curl invocation in the run_command call to include
bounded connection and overall timeouts, and add bounded retry behavior if
supported by the existing command conventions; leave the subsequent tar
extraction unchanged.
- Around line 124-127: The package download flow in NginxInstaller must extract
each changed fingerprint into a clean staging tree instead of overlaying
packages/nginx. Update the extraction logic around NginxInstaller#compile to
clear or replace only generated package contents before extraction, while
preserving spec.lock; then keep the fingerprint cache update after a successful
replacement.
- Around line 167-172: Update run_command to avoid invoking bash -c with the
interpolated command string; parse or accept the executable and arguments
separately and pass them directly to IO.popen, preserving environment handling
and logging while ensuring YAML- and path-derived values remain literal even
when they contain shell metacharacters or spaces.
In `@src/tasks/spec.rake`:
- Around line 17-21: Shell-escape the runtime_log value before interpolating it
into the command string returned by the task, using the existing runtime_log
variable and Shellwords; ensure paths containing spaces or shell metacharacters
remain a single safe argument while preserving the current parallel_rspec
invocation.
---
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 146-149: Move the COMPILED_PLATFORM_FILE write out of the
pre-compilation setup and place it alongside the fingerprint marker update only
after packaging/compilation succeeds, ensuring both markers are written
together. Prefer atomically replacing the platform marker (and preserve the
existing should_compile? behavior) so failed compilation cannot leave stale
success markers.
- Around line 119-127: In the package download flow using index and tmp_pkg,
read the indexed digest for package_fingerprint and verify the downloaded
temporary archive against it before invoking tar extraction. Abort with a clear
error when verification fails, while preserving the existing blobstore lookup
and extraction behavior for valid packages.
In `@src/spec/integration/deploy/local_template_properties_spec.rb`:
- Line 4: The integration specs must isolate sandbox state for each example
rather than resetting only once per group. Update the setup at
src/spec/integration/deploy/local_template_properties_spec.rb:4 and
src/spec/integration/links/link_parse_validation_spec.rb:4 to use per-example
isolation or explicit cleanup, preserving independent execution when deployment
names are reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 858f2823-1ee0-47e1-a969-6e065a8e5bc5
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
mkocher
force-pushed
the
speed-up-integration-tests
branch
from
July 21, 2026 23:44
9e737c0 to
60efd00
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spec/integration_support/nginx_service.rb (1)
146-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrite compiled markers only after a successful build.
If an existing executable was built for another platform and compilation fails, the platform marker is updated while the old fingerprint marker remains current. The next run can then incorrectly skip rebuilding the stale executable. Move the platform marker write next to the fingerprint marker write after packaging completes.
Proposed fix
- File.write(COMPILED_PLATFORM_FILE, RUBY_PLATFORM) ... - File.write(COMPILED_FINGERPRINT_FILE, package_fingerprint) + File.write(COMPILED_PLATFORM_FILE, RUBY_PLATFORM) + File.write(COMPILED_FINGERPRINT_FILE, package_fingerprint)Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 146 - 149, Move the COMPILED_PLATFORM_FILE write out of the pre-compilation setup and place it alongside the fingerprint marker write in the successful post-packaging path. Ensure both markers are updated only after compilation and packaging complete, while preserving the existing directory creation and rebuild behavior on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 146-149: Move the COMPILED_PLATFORM_FILE write out of the
pre-compilation setup and place it alongside the fingerprint marker write in the
successful post-packaging path. Ensure both markers are updated only after
compilation and packaging complete, while preserving the existing directory
creation and rebuild behavior on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de59eca3-0f08-4db2-a802-5a4aed6c4136
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
Co-authored-by: Cursor <cursoragent@cursor.com>
mkocher
force-pushed
the
speed-up-integration-tests
branch
from
July 23, 2026 16:22
60efd00 to
794eaf3
Compare
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spec/integration_support/nginx_service.rb (1)
8-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize the shared nginx installation transaction.
Parallel workers can interleave
prepare’s package-tree deletion/extraction with another worker’scompilecopy, producing incomplete inputs or flaky builds. Lockprepare,should_compile?, andcompiletogether around the shared checkout, work directory, and integration bin.Proposed fix
def self.install - installer = NginxInstaller.new - installer.prepare + lock_path = File.join(IntegrationSupport::Constants::BOSH_REPO_SRC_TMP_DIR, 'nginx-install.lock') + FileUtils.mkdir_p(File.dirname(lock_path)) - if installer.should_compile? - installer.compile - else - puts 'Skipping compiling nginx because shasums and platform have not changed' + File.open(lock_path, 'w') do |lock| + lock.flock(File::LOCK_EX) + installer = NginxInstaller.new + installer.prepare + + if installer.should_compile? + installer.compile + else + puts 'Skipping compiling nginx because shasums and platform have not changed' + end end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spec/integration_support/nginx_service.rb` around lines 8 - 16, Update NginxService.install to serialize the full shared installation transaction with the repository’s existing locking mechanism: acquire the lock before installer.prepare and hold it through should_compile? and the conditional compile, releasing it only after all shared checkout, work-directory, and integration-bin operations complete. Keep the existing skip behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/spec/integration_support/nginx_service.rb`:
- Around line 8-16: Update NginxService.install to serialize the full shared
installation transaction with the repository’s existing locking mechanism:
acquire the lock before installer.prepare and hold it through should_compile?
and the conditional compile, releasing it only after all shared checkout,
work-directory, and integration-bin operations complete. Keep the existing skip
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efe3246e-fce3-44dc-95e1-e15309fc6e56
📒 Files selected for processing (6)
ci/tasks/test-rake-task.shsrc/spec/integration/deploy/local_template_properties_spec.rbsrc/spec/integration/links/link_parse_validation_spec.rbsrc/spec/integration_support/nginx_service.rbsrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this change about?
Improve integration tests in bosh:
What tests have you run against this PR?
I've run this as a one off build with postgres, took it from 2 to 3 hours down to 1.25 hours.
Note
This PR is a few commits, it may be easier to review each commit separately