Skip to content

fix(ci): always unwind pushd in run_conditional_tests.sh#7304

Open
akgitrepos wants to merge 1 commit intogoogleapis:mainfrom
akgitrepos:fix/ci-pushd-cleanup-on-failure
Open

fix(ci): always unwind pushd in run_conditional_tests.sh#7304
akgitrepos wants to merge 1 commit intogoogleapis:mainfrom
akgitrepos:fix/ci-pushd-cleanup-on-failure

Conversation

@akgitrepos
Copy link

Summary

  • fixes a control-flow bug in ci/run_conditional_tests.sh where a failing package test exited the loop before popd, leaving the shell in a pushed directory
  • keeps existing early-exit behavior on first failure (RETVAL + break) while guaranteeing directory stack cleanup
  • adds a small trap-based guard so unexpected exits/signals (EXIT, INT, TERM) also unwind an outstanding pushd

Root Cause

Inside the test loop, the script did:

  1. pushd ${d}
  2. run test script and capture ret
  3. if [ ${ret} -ne 0 ]; then ... break; fi
  4. popd

When ret != 0, break was taken before popd, so cleanup was skipped.

What Changed

  • introduced IN_PUSHED_DIR flag
  • added cleanup_pushd() that conditionally performs popd > /dev/null || true
  • registered trap cleanup_pushd EXIT INT TERM
  • set IN_PUSHED_DIR=true immediately after pushd
  • moved normal-path popd before failure break, then reset IN_PUSHED_DIR=false

Why This Is Safe

  • no change to package selection logic, diff detection, credential gating, or test command execution
  • still exits early on first failed package exactly as before
  • cleanup function is no-op unless a pushd is currently active
  • explicit popd remains in normal flow; trap is fallback for abnormal exits

Validation

  • bash -n ci/run_conditional_tests.sh
  • bash -n ci/run_single_test.sh

Risk Assessment

Low risk. Change is localized to directory-stack cleanup and does not alter core CI decision logic.

@akgitrepos akgitrepos requested a review from a team as a code owner February 25, 2026 18:56
@akgitrepos
Copy link
Author

@pearigee Made some quick fix to run_conditional_tests.sh. Would appreciate a review. Thank you!

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