Merge branch 'main' into jules/fix-storage-desktop-build

jonsimantov · web-flow · commit 2551c111d253 · 2025-07-02T10:33:31.000-07:00
diff --git 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
diff --git 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'])):
     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)
     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
@@ -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'])
