Skip to content

payload_snapshot: fix JUnit XML discovery timeout for large artifact trees - #641

Closed
redhat-chai-bot wants to merge 1 commit into
openshift-eng:mainfrom
redhat-chai-bot:fix-junit-gcs-timeout
Closed

redhat-chai-bot wants to merge 1 commit into
openshift-eng:mainfrom
redhat-chai-bot:fix-junit-gcs-timeout

Conversation

@redhat-chai-bot

@redhat-chai-bot redhat-chai-bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

The JUnitCollector._list_junit_files() method uses gcloud storage ls with a recursive ** glob and a 30-second timeout to discover JUnit XML files in CI job artifacts. For hypershift e2e jobs that produce 10,000+ artifacts (cluster resource dumps, node YAMLs, deployment YAMLs per test case), this glob times out before finding the junit.xml file.

When the timeout occurs, _run_gcloud() silently returns None, and _list_junit_files() returns an empty list. The collect() method then writes an empty [] to results.json, causing the downstream payload analysis agent to see zero test failures. This data gap led to a confabulation where the agent fabricated test failure output from PR diffs instead of reporting insufficient data.

Fix

  1. Increased glob timeout from 30s to 120s (matching the _run_gcloud default)
  2. Added targeted fallback: when the recursive glob returns empty, the method now lists top-level step directories under artifacts/ and probes each one for junit*.xml files using non-recursive patterns — avoiding the expensive ** glob entirely
  3. Added warning logging when the initial glob returns empty, making the silent failure visible in logs
  4. Bumped plugin version to 0.0.72

@stbenjam requested in Slack thread

Summary by CodeRabbit

  • New Features

    • Improved JUnit report discovery for payload snapshots, including fallback detection across common artifact locations.
  • Bug Fixes

    • Increased reliability when recursive artifact searches return no results.
    • Added clearer warnings and discovery counts when reports are missing or found.
  • Chores

    • Updated the CI plugin version to 0.0.72 across marketplace and documentation metadata.

Hypershift e2e jobs produce 10,000+ artifacts (cluster resource dumps
per test case), causing the recursive `**` glob in `gcloud storage ls`
to time out at the previous 30-second limit. When the timeout fires,
`_run_gcloud` silently returns None, and `_list_junit_files` returns [],
so `collect()` writes an empty results.json — the downstream analysis
agent sees zero test failures and confabulates results.

Changes:
- Increase glob timeout from 30s to 120s (matching _run_gcloud default)
- Add targeted fallback when the glob returns no results: list top-level
  step directories under artifacts/ and probe each for junit files at
  known locations, avoiding the expensive recursive enumeration
- Log a warning when the glob returns empty so the silent failure is
  visible in stderr output
- Bump ci plugin version to 0.0.72

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-ci
openshift-ci Bot requested review from bentito and stbenjam July 23, 2026 14:59
@openshift-ci

openshift-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign rvanderp3 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Changes

CI plugin update

Layer / File(s) Summary
Targeted JUnit discovery fallback
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
JUnit collection increases the recursive lookup timeout, normalizes results, and probes artifact step directories through multiple candidate paths when the recursive search returns no files.
CI plugin version metadata
.claude-plugin/marketplace.json, plugins/ci/.claude-plugin/plugin.json, docs/index.html
The CI plugin version is updated from 0.0.71 to 0.0.72 across marketplace, manifest, and embedded documentation metadata.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JUnitCollector
  participant gcloud_storage
  participant GCS_bucket
  JUnitCollector->>gcloud_storage: Run recursive JUnit glob
  gcloud_storage->>GCS_bucket: Search for junit XML files
  GCS_bucket-->>gcloud_storage: Return matching paths or no results
  gcloud_storage-->>JUnitCollector: Return normalized paths
  JUnitCollector->>gcloud_storage: List artifact step directories when empty
  gcloud_storage->>GCS_bucket: Probe candidate JUnit paths
  GCS_bucket-->>JUnitCollector: Return aggregated fallback matches
Loading

Suggested reviewers: stbenjam, not-stbenjam, petr-muller

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: fixing JUnit XML discovery timeouts in payload snapshot for large artifact trees.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Real People Names In Style References ✅ Passed Scanned the changed files and modified JUnitCollector hunks; no real-person names or style-reference examples were added.
No Assumed Git Remote Names ✅ Passed No new hardcoded remote-name usage was added; the only git fetch origin is pre-existing and unchanged in the PR.
Git Push Safety Rules ✅ Passed The PR only bumps versions and updates JUnit discovery logic; no new git push, force-push, or main/master push instructions were introduced.
No Untrusted Mcp Servers ✅ Passed The PR only bumps version metadata and changes JUnit discovery logic; no new MCP server installs or untrusted MCP dependencies were introduced.
Ai-Helpers Overlap Detection ✅ Passed No changed files match the markdown scopes this check covers, so overlap detection isn’t triggered.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)
.claude-plugin/marketplace.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

plugins/ci/.claude-plugin/plugin.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 771-772: Rename the ambiguous loop variable l to line at all four
affected sites: plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
lines 771-772, 841-843, 852-854, and 863-865. Update each corresponding
reference while preserving the existing filtering and processing 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6f292107-9fca-4e95-9e98-fb714828d19d

📥 Commits

Reviewing files that changed from the base of the PR and between c4c6acb and 42266b3.

📒 Files selected for processing (4)
  • .claude-plugin/marketplace.json
  • docs/index.html
  • plugins/ci/.claude-plugin/plugin.json
  • plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py

Comment thread plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
@stbenjam

Copy link
Copy Markdown
Member

/test ?

@stbenjam

Copy link
Copy Markdown
Member

/test payload-agent

not-stbenjam added a commit to not-stbenjam/ai-helpers that referenced this pull request Jul 26, 2026
…ounded-depth fallback

Incorporates the JUnit discovery hardening from openshift-eng#641 (closed in favour of
this PR), with two fixes and one interaction resolved:

- Primary recursive glob timeout raised 30s -> 120s.
- Bounded-depth fallback when the recursive glob yields nothing.
- Fix: openshift-eng#641's probes checked {step}/, {step}/artifacts/ and
  {step}/*/artifacts/ only. Aggregated jobs keep junit-aggregated.xml
  about six levels below artifacts/, so those probes could never find it.
  The aggregator subtree now gets its own scoped ** probe, which stays
  fast because it covers one small directory.
- Fix: junit_operator.xml lives directly in artifacts/, not inside a step
  directory, so the fallback missed it. Added a top-level probe. The
  fallback now recovers the same file set as the glob.
- Interaction: a recovered failure must not mark the snapshot incomplete.
  Errors resolved by a fallback are flagged `recovered: true`, reported
  separately, and excluded from `data_complete`.

Verified against the same payload three ways:
- glob fails, fallback recovers: 73 failing tests, data_complete true,
  4 errors all recovered, exit 0 — identical results to the glob path
- all reads fail: 20 unrecovered errors, no results.json written,
  junit_collection_failed set, exit 1
- real gcloud: no fallback needed, 73 failing tests, 0 errors, exit 0
@not-stbenjam

Copy link
Copy Markdown
Contributor

Closing in favour of #644, which absorbs the changes here.

#644 keeps both of this PR's improvements — the 30s→120s glob timeout raise and the bounded-depth fallback — and fixes two problems found while incorporating them:

  1. The fallback could not find aggregated JUnit. The probes here cover {step}/, {step}/artifacts/ and {step}/*/artifacts/, but aggregated jobs keep junit-aggregated.xml about six levels below artifacts/:
    artifacts/release-analysis-aggregator/openshift-release-analysis-aggregator/artifacts/release-analysis-aggregator/<job>/<payload>/junit-aggregated.xml
    So for aggregated blocking jobs the fallback would have returned nothing. payload-snapshot: fix silent data loss on unauthenticated gcloud (supersedes #641) #644 adds a ** probe scoped to the aggregator subtree, which stays fast because it covers one small directory.

  2. junit_operator.xml was missed. It sits directly in artifacts/, not inside a step directory. payload-snapshot: fix silent data loss on unauthenticated gcloud (supersedes #641) #644 adds a top-level probe, after which the fallback recovers the same file set as the glob (verified: 73 failing tests either way, vs 71 without it).

#644 also addresses the underlying defect this PR's description identifies but does not fix: _run_gcloud returns None identically for a timeout, a missing binary, an auth failure, and a genuine no-match, and collect() then writes [] regardless. Raising the timeout removes one trigger; the silent-empty behaviour remains for every other cause. In #644, failures are classified and recorded, results.json is not written when the data could not be read, and summary.json carries data_complete / collection_errors[] — so an unreadable job is absent, never a confident test_failure_count: 0. Errors a fallback recovers are marked recovered: true and don't mark the snapshot incomplete.

Worth noting on the premise: in the snapshot that motivated #644, the glob was not actually slow — 0.73s for the aggregated AWS job, 8s for hypershift-e2e-aws — and every gcloud read failed regardless (0 JUnit XMLs, 0 build logs, 15 empty results.json) because gcloud was unauthenticated. So the timeout fix and the error-surfacing fix address genuinely different triggers of the same silent-degradation class, and both are needed.

Thanks — the diagnosis in this PR's description is what pointed at the right area.

@redhat-chai-bot

Copy link
Copy Markdown
Contributor Author

Hi @not-stbenjam -- I can only respond automatically to comments from verified Red Hat associates. Your GitHub account could not be confirmed. If you are a Red Hat employee and believe this is an error, please ask for help in the #chai-users Slack channel.

@stbenjam stbenjam closed this Jul 26, 2026
openshift-merge-bot Bot added a commit that referenced this pull request Jul 26, 2026
payload-snapshot: fix silent data loss on unauthenticated gcloud (supersedes #641)
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.

3 participants