ci: stop hashFiles() from failing jobs whose tests passed - #6003
Merged
Conversation
CI-basictests failed on a run where every test passed:
SUMMARY: ret_rc = [0, 0, 0]
##[error]The template is not valid. System.InvalidOperationException:
hashFiles('proxysql/ci_infra_logs/**/coverage-report/*.info') failed.
Fail to hash files under directory '/home/ci/_work/proxysql/proxysql'
#5995 added that guard to ci-basictests, ci-legacy-g2 and
ci-taptests-pgsql-cluster. The same expression already existed in the g9
workflows and works there, so the expression alone is not the problem -- the
runner is. Those workflows pin 'runs-on: ubuntu-22.04' (GitHub-hosted,
ephemeral, /home/runner/work); these three resolve the runner dynamically and
can land on a self-hosted one (/home/ci/_work), where the workspace PERSISTS
between runs and the test containers leave ci_infra_logs root-owned.
hashFiles() walks the tree to evaluate the glob. On that tree it hits EACCES and
THROWS rather than returning '' -- note the message is "Fail to hash files under
directory", a traversal failure, not a no-match. A throw inside an 'if:' makes
the template invalid, so the job is marked failed after a completely green test
run. That is a false red, and it hides real ones.
Fixes, per workflow:
- Replace the hashFiles() guard with a shell probe ('[ -r <file> ]') exposed as
a step output. A readability test cannot throw. It also checks the EXACT file
the upload step consumes; the old guard globbed '**' across the whole log
tree, which is both wider than needed and what made it walk unreadable
directories in the first place.
- Relax 'Fix artifact permissions' from 'failure() && !cancelled()' to
'!cancelled()'. On a PASSING run nothing chowned ci_infra_logs, which is
exactly the case that broke -- and on a persistent runner that root-owned
tree is inherited by the next job.
Not verified end to end: I cannot inspect the self-hosted runner's filesystem, so
the EACCES step is inference from the error wording plus the hosted/self-hosted
split. What is established is that #5995 introduced the expression, the tests
passed, and the job died evaluating it. The shell probe removes the failure mode
regardless of which of those is the precise trigger, since it cannot throw.
All three files validate as YAML and the probe path matches the upload path in
each.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughWalkthroughThree CI workflows now run artifact-permission fixes for every non-cancelled execution. Coverage handling checks the expected report directly and uses the detection result for missing-report and Codecov-upload steps. ChangesCI workflow updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
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
1 participant
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.
Problem
CI-basictestsfailed on a run where every test passed:#5995 added that guard to
ci-basictests,ci-legacy-g2andci-taptests-pgsql-cluster.Why it breaks here but not on the g9 workflows
The same expression already exists in
ci-mysql84-g9.ymland works, so the expression alone isn't the problem — the runner is:runs-onci-mysql84-g9ubuntu-22.04(GitHub-hosted, ephemeral)/home/runner/work/.../home/ci/_work/...On a self-hosted runner the workspace persists between runs, and the test containers leave
ci_infra_logsroot-owned.hashFiles()walks the tree to evaluate the glob, hits EACCES and throws instead of returning''— note the message is "Fail to hash files under directory", a traversal failure, not a no-match. A throw inside anif:invalidates the template, so the job is marked failed after a completely green run. That's a false red, and false reds hide real ones.Compounding it:
Fix artifact permissionswas gatedfailure() && !cancelled(), so on a passing run nothing ever chownedci_infra_logs— exactly the case that broke.Fix
Per workflow:
hashFiles()guard with a shell probe ([ -r <file> ]) exposed as a step output. A readability test cannot throw. It also checks the exact file the upload consumes — the old guard globbed**across the whole log tree, which is both wider than needed and what made it walk unreadable directories.Fix artifact permissionsto!cancelled(), so the tree is always readable before anything walks it — and, on a persistent runner, isn't inherited root-owned by the next job.Verification and limits
All three files validate as YAML, and the probe path equals the upload path in each.
Not verified end to end: I can't inspect the self-hosted runner's filesystem, so the EACCES step is inference from the error wording plus the hosted/self-hosted split. What is established: #5995 introduced the expression, the tests passed, and the job died evaluating it. The shell probe removes the failure mode regardless of the precise trigger, because it cannot throw.
Summary by CodeRabbit