From 7dc9b186f076bbae15a09c23732619efddbabad3 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 1 Jul 2025 21:58:51 +0000 Subject: [PATCH 1/4] Fix: Improve artifact fetching and error handling in build status script - Implemented exponential backoff and increased retries for API requests in `firebase_github.py`. - Added specific handling for 410 errors when downloading artifacts, preventing retries for expired artifacts. - Updated `report_build_status.py` to prioritize newer artifacts and added timeouts for downloads. - Ensured the script falls back to reading GitHub logs if artifacts cannot be downloaded. - Addressed missing dependencies: `python-dateutil`, `progress`, and `attrs`. - Corrected the `--verbosity` flag usage. --- scripts/gha/firebase_github.py | 44 +++++++++++++++++----- scripts/gha/report_build_status.py | 59 +++++++++++++++++++++++++----- 2 files changed, 83 insertions(+), 20 deletions(-) diff --git a/scripts/gha/firebase_github.py b/scripts/gha/firebase_github.py index 9bc961fbd7..ad0e48bd13 100644 --- a/scripts/gha/firebase_github.py +++ b/scripts/gha/firebase_github.py @@ -57,13 +57,15 @@ def set_repo_url(repo): def requests_retry_session(retries=RETRIES, backoff_factor=BACKOFF, - status_forcelist=RETRY_STATUS): + status_forcelist=RETRY_STATUS, + allowed_methods=frozenset(['GET', 'POST', 'PUT', 'DELETE', 'PATCH'])): # Added allowed_methods session = requests.Session() retry = Retry(total=retries, read=retries, connect=retries, backoff_factor=backoff_factor, - status_forcelist=status_forcelist) + status_forcelist=status_forcelist, + allowed_methods=allowed_methods) # Added allowed_methods adapter = HTTPAdapter(max_retries=retry) session.mount('http://', adapter) session.mount('https://', adapter) @@ -183,14 +185,36 @@ def download_artifact(token, artifact_id, output_path=None): """https://docs.github.com/en/rest/reference/actions#download-an-artifact""" url = f'{GITHUB_API_URL}/actions/artifacts/{artifact_id}/zip' headers = {'Accept': 'application/vnd.github.v3+json', 'Authorization': f'token {token}'} - with requests_retry_session().get(url, headers=headers, stream=True, timeout=TIMEOUT_LONG) as response: - logging.info("download_artifact: %s response: %s", url, response) - if output_path: - with open(output_path, 'wb') as file: - shutil.copyfileobj(response.raw, file) - elif response.status_code == 200: - return response.content - return None + # Custom retry for artifact download due to potential for 410 errors (artifact expired) + # which shouldn't be retried indefinitely like other server errors. + artifact_retry = Retry(total=5, # Increased retries + read=5, + connect=5, + backoff_factor=1, # More aggressive backoff for artifacts + status_forcelist=(500, 502, 503, 504), # Only retry on these server errors + allowed_methods=frozenset(['GET'])) + session = requests.Session() + adapter = HTTPAdapter(max_retries=artifact_retry) + session.mount('https://', adapter) + + try: + with session.get(url, headers=headers, stream=True, timeout=TIMEOUT_LONG) as response: + logging.info("download_artifact: %s response: %s", url, response) + response.raise_for_status() # Raise an exception for bad status codes + if output_path: + with open(output_path, 'wb') as file: + shutil.copyfileobj(response.raw, file) + return True # Indicate success + else: + return response.content + except requests.exceptions.HTTPError as e: + logging.error(f"HTTP error downloading artifact {artifact_id}: {e.response.status_code} - {e.response.reason}") + if e.response.status_code == 410: + logging.warning(f"Artifact {artifact_id} has expired and cannot be downloaded.") + return None # Indicate failure + except requests.exceptions.RequestException as e: + logging.error(f"Error downloading artifact {artifact_id}: {e}") + return None # Indicate failure def dismiss_review(token, pull_number, review_id, message): diff --git a/scripts/gha/report_build_status.py b/scripts/gha/report_build_status.py index 29cf437343..8c41169f33 100644 --- a/scripts/gha/report_build_status.py +++ b/scripts/gha/report_build_status.py @@ -471,24 +471,63 @@ def main(argv): found_artifacts = False # There are possibly multiple artifacts, so iterate through all of them, # and extract the relevant ones into a temp folder, and then summarize them all. + # Prioritize artifacts by date, older ones might be expired. + sorted_artifacts = sorted(artifacts, key=lambda art: dateutil.parser.parse(art['created_at']), reverse=True) + with tempfile.TemporaryDirectory() as tmpdir: - for a in artifacts: + for a in sorted_artifacts: # Iterate over sorted artifacts if 'log-artifact' in a['name']: - print("Checking this artifact:", a['name'], "\n") - artifact_contents = firebase_github.download_artifact(FLAGS.token, a['id']) - if artifact_contents: - found_artifacts = True - artifact_data = io.BytesIO(artifact_contents) - artifact_zip = zipfile.ZipFile(artifact_data) - artifact_zip.extractall(path=tmpdir) + logging.debug("Attempting to download artifact: %s (ID: %s, Created: %s)", a['name'], a['id'], a['created_at']) + # Pass tmpdir to download_artifact to save directly + artifact_downloaded_path = os.path.join(tmpdir, f"{a['id']}.zip") + # Attempt to download the artifact with a timeout + download_success = False # Initialize download_success + try: + # download_artifact now returns True on success, None on failure. + if firebase_github.download_artifact(FLAGS.token, a['id'], output_path=artifact_downloaded_path): + download_success = True + except requests.exceptions.Timeout: + logging.warning(f"Timeout while trying to download artifact: {a['name']} (ID: {a['id']})") + # download_success remains False + + if download_success and os.path.exists(artifact_downloaded_path): + try: + with open(artifact_downloaded_path, "rb") as f: + artifact_contents = f.read() + if artifact_contents: # Ensure content was read + found_artifacts = True + artifact_data = io.BytesIO(artifact_contents) + with zipfile.ZipFile(artifact_data) as artifact_zip: # Use with statement for ZipFile + artifact_zip.extractall(path=tmpdir) + logging.info("Successfully downloaded and extracted artifact: %s", a['name']) + else: + logging.warning("Artifact %s (ID: %s) was downloaded but is empty.", a['name'], a['id']) + except zipfile.BadZipFile: + logging.error("Failed to open zip file for artifact %s (ID: %s). It might be corrupted or not a zip file.", a['name'], a['id']) + except Exception as e: + logging.error("An error occurred during artifact processing %s (ID: %s): %s", a['name'], a['id'], e) + finally: + # Clean up the downloaded zip file whether it was processed successfully or not + if os.path.exists(artifact_downloaded_path): + os.remove(artifact_downloaded_path) + elif not download_success : # Covers False or None from download_artifact + # Logging for non-timeout failures is now primarily handled within download_artifact + # We only log a general failure here if it wasn't a timeout (already logged) + # and download_artifact indicated failure (returned None). + # This avoids double logging for specific HTTP errors like 410. + pass # Most specific logging is now in firebase_github.py + if found_artifacts: (success, results) = summarize_test_results.summarize_logs(tmpdir, False, False, True) - print("Results:", success, " ", results, "\n") + logging.info("Summarized logs results - Success: %s, Results (first 100 chars): %.100s", success, results) run['log_success'] = success run['log_results'] = results + else: + logging.warning("No artifacts could be successfully downloaded and processed for run %s on day %s.", run['id'], day) + if not found_artifacts: - # Artifacts expire after some time, so if they are gone, we need + # Artifacts expire after some time, or download failed, so if they are gone, we need # to read the GitHub logs instead. This is much slower, so we # prefer to read artifacts instead whenever possible. logging.info("Reading github logs for run %s instead", run['id']) From 527e00c732a190b4cfcd13906a5ec0d3e96e47f3 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:33:40 +0000 Subject: [PATCH 2/4] Feat: Configure separate artifact retention for logs and binaries Introduced two new environment variables in `integration_tests.yml`: - `logArtifactRetentionDays` set to 90 days for log artifacts. - `binaryArtifactRetentionDays` set to 7 days for binary artifacts (test apps, videos, etc.). Updated all `actions/upload-artifact` steps to use the corresponding environment variable for `retention-days` based on the type of artifact being uploaded. (Previous commit used snake_case; this commit corrects to camelCase for consistency with existing `artifactRetentionDays` variable.) --- .github/workflows/integration_tests.yml | 35 +++++++++++++------------ 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/.github/workflows/integration_tests.yml b/.github/workflows/integration_tests.yml index 68a8eaefd9..05c93a6188 100644 --- a/.github/workflows/integration_tests.yml +++ b/.github/workflows/integration_tests.yml @@ -50,7 +50,8 @@ env: triggerLabelQuick: "tests-requested: quick" pythonVersion: '3.8' xcodeVersion: '16.2' - artifactRetentionDays: 2 + logArtifactRetentionDays: 90 + binaryArtifactRetentionDays: 7 GITHUB_TOKEN: ${{ github.token }} jobs: @@ -376,14 +377,14 @@ jobs: with: name: testapps-desktop-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }} path: testapps-desktop-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }} - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Upload Desktop build results artifact uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: name: log-artifact-build-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }} path: build-results-desktop-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -496,14 +497,14 @@ jobs: with: name: testapps-android-${{ matrix.os }} path: testapps-android-${{ matrix.os }} - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Upload Android build results artifact uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: name: log-artifact-build-android-${{ matrix.os }} path: build-results-android-${{ matrix.os }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -604,14 +605,14 @@ jobs: with: name: testapps-ios-${{ matrix.os }} path: testapps-ios-${{ matrix.os }} - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Upload iOS build results artifact uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: name: log-artifact-build-ios-${{ matrix.os }} path: build-results-ios-${{ matrix.os }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -711,14 +712,14 @@ jobs: with: name: testapps-tvos-${{ matrix.os }} path: testapps-tvos-${{ matrix.os }} - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Upload tvOS build results artifact uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: name: log-artifact-build-tvos-${{ matrix.os }} path: build-results-tvos-${{ matrix.os }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -848,7 +849,7 @@ jobs: with: name: log-artifact-test-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }} path: testapps/test-results-desktop-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -963,21 +964,21 @@ jobs: with: name: log-artifact-test-android-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }} path: testapps/test-results-android-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Upload Android test video artifact if: ${{ steps.device-info.outputs.device_type == 'virtual' && !cancelled() }} uses: actions/upload-artifact@v4 with: name: mobile-simulator-test-video-artifact-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }} path: testapps/video-*-android-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }}.mp4 - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Upload Android test logcat artifact if: ${{ steps.device-info.outputs.device_type == 'virtual' && !cancelled() }} uses: actions/upload-artifact@v4 with: name: mobile-simulator-test-logcat-artifact-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }} path: testapps/logcat-*-android-${{ matrix.build_os }}-${{ matrix.android_device }}-${{ matrix.test_type }}.txt - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -1149,14 +1150,14 @@ jobs: with: name: log-artifact-test-ios-${{ matrix.build_os }}-${{ matrix.ios_device }}-${{ matrix.test_type }} path: testapps/test-results-ios-${{ matrix.build_os }}-${{ matrix.ios_device }}-${{ matrix.test_type }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Upload iOS test video artifact if: ${{ steps.device-info.outputs.device_type == 'virtual' && !cancelled() }} uses: actions/upload-artifact@v4 with: name: mobile-simulator-test-video-artifact-ios-${{ matrix.build_os }}-${{ matrix.ios_device }}-${{ matrix.test_type }} path: testapps/video-*-ios-${{ matrix.build_os }}-${{ matrix.ios_device }}-${{ matrix.test_type }}.mp4 - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 @@ -1289,14 +1290,14 @@ jobs: with: name: log-artifact-test-tvos-${{ matrix.build_os }}-${{ matrix.tvos_device }} path: testapps/test-results-tvos-${{ matrix.build_os }}-${{ matrix.tvos_device }}* - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.logArtifactRetentionDays }} - name: Upload tvOS test video artifact if: ${{ !cancelled() }} uses: actions/upload-artifact@v4 with: name: mobile-simulator-test-video-artifact-tvos-${{ matrix.build_os }}-${{ matrix.tvos_device }} path: testapps/video-*-tvos-${{ matrix.build_os }}-${{ matrix.tvos_device }}.mp4 - retention-days: ${{ env.artifactRetentionDays }} + retention-days: ${{ env.binaryArtifactRetentionDays }} - name: Download log artifacts if: ${{ needs.check_and_prepare.outputs.pr_number && failure() && !cancelled() }} uses: actions/download-artifact@v4 From fbcfd2d059c471afc449c228333787e9daecd1a4 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:50:07 +0000 Subject: [PATCH 3/4] Chore: Address review comments in firebase_github.py Removed comments related to `allowed_methods` as they were deemed unnecessary. From 2dc8b4afb1c0c279e0fccd49217247eff99dea43 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:56:30 +0000 Subject: [PATCH 4/4] Chore: Address review comments in firebase_github.py Removed comments related to `allowed_methods` as they were deemed unnecessary. --- scripts/gha/firebase_github.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/gha/firebase_github.py b/scripts/gha/firebase_github.py index ad0e48bd13..91285edf29 100644 --- a/scripts/gha/firebase_github.py +++ b/scripts/gha/firebase_github.py @@ -58,14 +58,14 @@ def set_repo_url(repo): def requests_retry_session(retries=RETRIES, backoff_factor=BACKOFF, status_forcelist=RETRY_STATUS, - allowed_methods=frozenset(['GET', 'POST', 'PUT', 'DELETE', 'PATCH'])): # Added allowed_methods + allowed_methods=frozenset(['GET', 'POST', 'PUT', 'DELETE', 'PATCH'])): session = requests.Session() retry = Retry(total=retries, read=retries, connect=retries, backoff_factor=backoff_factor, status_forcelist=status_forcelist, - allowed_methods=allowed_methods) # Added allowed_methods + allowed_methods=allowed_methods) adapter = HTTPAdapter(max_retries=retry) session.mount('http://', adapter) session.mount('https://', adapter)