Update (base update)

[ghstack-poisoned]

Author: huydhn

aws/lambda/gha-log-uploader/README.md

@@ -31,22 +31,51 @@ Payload:
 `conclusion` is optional. A malformed payload raises, which means Lambda retries
 twice and then DLQs it.
 
+## What fails how
+
+Callers invoke asynchronously, so *raising* is what reaches Lambda's retries and
+then the dead-letter queue; *returning* records the invocation as a success no
+matter what the return value says. Which failures do which:
+
+| Failure | Behaviour |
+| --- | --- |
+| Network error reaching GitHub | raises — retried, then DLQ |
+| GitHub 5xx or 429 | raises — retried, then DLQ |
+| No usable credential | raises — retried, then DLQ |
+| Malformed payload | raises — retried, then DLQ |
+| GitHub 404 (log has aged out), 401, 403 | returns `stored: false`, no retry |
+| Classifier call fails or times out | returns `classified: false`, no retry |
+
+The bottom two are deliberate. A log GitHub has already dropped does not come
+back on the third attempt, and re-running a whole download to retry a classifier
+handoff would re-fetch megabytes to redo something that takes milliseconds.
+`stored: false` is therefore not visible on the DLQ; if you want to alarm on it,
+match the terminal `ERROR <status> downloading log` line specifically. Do **not**
+alarm on `ERROR` generally: `installation_token` logs one every time it falls
+back to the PAT pool, which is a path that then usually succeeds.
+
 ## Classification
 
 After a log is stored, `log_classifier` is called through its function URL —
 byte for byte the call `github-status-test` makes today.
 
-That call is synchronous. Function URLs only support the `RequestResponse`
-invocation type, so this function's duration includes the classification, and
-`github-status-test`'s 274s/344s/400s/900s duration maxima come from exactly
-this. **Keep the timeout at 900s**: on a slow classification a shorter one would
-kill the invocation mid-wait, and since callers invoke asynchronously, Lambda
-would then retry the whole thing and re-download the log.
+Function URLs only support the `RequestResponse` invocation type, so there is no
+way to ask for fire-and-forget. `CLASSIFIER_TIMEOUT` gets close enough: after 30s
+this stops waiting for the reply. Disconnecting does not cancel the classifier —
+it runs to completion regardless — so nothing is lost by hanging up, and
+`github-status-test`'s 274s/344s/400s/900s duration maxima do not carry over.
+
+That bound is load-bearing, not tidiness. `urlopen` with no `timeout` has none at
+all, so a connection that is accepted and never answered raises nothing and burns
+the entire function timeout. Since callers invoke asynchronously, Lambda counts
+that as a failure and replays the whole invocation twice more, re-downloading the
+same log each time and eventually DLQ-ing a job whose log was archived fine on
+the first attempt. `github-status-test` does hit its 900s ceiling, so this is an
+observed tail, not a theoretical one.
 
-Unlike in `github-status-test` the tail is no longer harmful. There it ran behind
-API Gateway on the webhook's critical path, so a slow classification risked a
-GitHub webhook timeout. Here the caller has already returned, and a long
-invocation costs GB-seconds and a concurrency slot, nothing more.
+With the wait bounded, every step has an explicit ceiling — two 30s log fetches
+at most, then a 30s classifier call — so a **300s function timeout** is
+comfortable, rather than the 900s `github-status-test` needs.
 
 The way out is `lambda:InvokeFunction` with `InvocationType: "Event"`, which
 needs `log_classifier` to accept a plain `{"job_id", "repo"}` payload — it
@@ -82,7 +111,7 @@ on the repo.
 
 | Env var | Required | Purpose |
 | --- | --- | --- |
-| `GITHUB_APP_ID` | no | App id used to mint installation tokens (e.g. `4550824`, `pytorch-bot-preview`) |
+| `GITHUB_APP_ID` | no | Numeric app id used to mint installation tokens, e.g. `4550824` — the id of the `pytorch-bot-preview` app. Must be the number: it goes through `int()`, so an app slug fails |
 | `GITHUB_APP_PRIVATE_KEY` | no | The app's private key, base64-encoded PEM (same encoding torchci uses) |
 | `GITHUB_TOKENS` | yes | Comma-separated PAT pool, used as the fallback and when no app is configured |
 
@@ -109,16 +138,17 @@ Notes on the app path:
 Not done by CI. Needed before the deploy workflow can run.
 
 1. Create the function: python3.12, x86_64, handler `lambda_function.lambda_handler`,
-   512 MB, **900s timeout** — matching `github-status-test`, because the
-   synchronous classifier call means a slow classification is a slow invocation.
+   512 MB, **300s timeout** — every step is individually bounded, so this does
+   not need `github-status-test`'s 900s. See Classification above.
 2. Give its execution role `s3:PutObject` on `arn:aws:s3:::ossci-raw-job-status/log/*`
    plus the usual CloudWatch Logs permissions. No `lambda:InvokeFunction` is
    needed while the classifier is reached over its function URL.
 3. Set the env vars above. Prefer fresh credentials over copying
    `github-status-test`'s, whose PATs sit in plaintext env vars and are due for
    rotation.
-4. Configure an on-failure destination or DLQ, and alarm on it. That queue is the
-   only signal that a trunk-only job lost its log.
+4. Configure an on-failure destination or DLQ, and alarm on it. For a trunk-only
+   job that is the only signal its log went missing — Dr.CI's self-heal only
+   covers PR jobs. See "What fails how" for what does and does not land there.
 5. Add the invoke grant for torchci, and nothing else:
    ```
    aws lambda add-permission --function-name gha-log-uploader \

aws/lambda/gha-log-uploader/lambda_function.py

@@ -7,11 +7,11 @@
 API Gateway integration and no Lambda function URL: the only way in is
 ``lambda:InvokeFunction``, which is IAM-authenticated.
 
-Classification is kicked off exactly as ``github-status-test`` does it, through
-log_classifier's function URL. That call is synchronous, so this function's
-duration includes the classification -- which is why its timeout has to stay at
-900s. Switching to an async ``lambda:InvokeFunction`` needs log_classifier to
-accept a plain payload first; see the README.
+Classification is kicked off through log_classifier's function URL, as
+``github-status-test`` does it, but with a bounded wait for the reply so a hung
+classifier cannot burn this function's whole timeout. Switching to an async
+``lambda:InvokeFunction`` needs log_classifier to accept a plain payload first;
+see the README.
 """
 
 import base64
@@ -37,6 +37,10 @@
 LOG_CLASSIFIER_URL = (
     "https://vwg52br27lx5oymv4ouejwf4re0akoeg.lambda-url.us-east-1.on.aws"
 )
+# How long to wait for the classifier's reply before giving up on it. Classifying
+# a big log can take minutes, and waiting that out is pointless: nothing here
+# reads the result, and disconnecting does not cancel the classifier.
+CLASSIFIER_TIMEOUT = 30
 
 GITHUB_API_URL = "https://api.github.com"
 # Installation tokens last an hour. Refresh early so a warm invocation never
@@ -59,6 +63,15 @@
 _token_cache = {}
 
 
+class RetryableDownloadError(Exception):
+    """A download failure worth another attempt.
+
+    Raised rather than returned so the async invocation fails: Lambda retries it
+    twice and then hands it to the dead-letter queue, which is the only place a
+    permanently lost log gets reported.
+    """
+
+
 def app_private_key():
     key = GITHUB_APP_PRIVATE_KEY
     if "PRIVATE KEY" not in key:
@@ -138,14 +151,30 @@ def log_object_path(full_name, job_id):
 
 
 def classify_log(full_name, job_id):
-    """Kick off classification for a log we just stored. Returns True on success.
-
-    Same call github-status-test makes. The function URL only supports the
-    RequestResponse invocation type, so this blocks until classification
-    finishes; the function's 900s timeout has to cover that.
+    """Kick off classification for a log we just stored.
+
+    Same call github-status-test makes, except that it waits a bounded time for
+    the reply. The function URL only supports the RequestResponse invocation
+    type, so there is no way to ask for fire-and-forget -- but giving up on the
+    reply is close enough, because a client disconnect does not cancel the
+    classifier. It runs to completion either way; we just stop waiting.
+
+    That bound matters. `urlopen` with no `timeout` has none at all, so a
+    connection that is accepted and never answered raises nothing and instead
+    burns the whole function timeout. Callers invoke this asynchronously, which
+    means Lambda would count that as a failure and replay the entire invocation
+    twice more, re-downloading the same multi-megabyte log each time -- and
+    github-status-test really does hit its 900s ceiling, so this is an observed
+    tail rather than a theoretical one.
+
+    Returns True when the classifier answered. False means we stopped waiting or
+    the call failed, and only the second of those actually skips classification.
     """
     try:
-        urlopen(f"{LOG_CLASSIFIER_URL}/?job_id={job_id}&repo={full_name}")
+        urlopen(
+            f"{LOG_CLASSIFIER_URL}/?job_id={job_id}&repo={full_name}",
+            timeout=CLASSIFIER_TIMEOUT,
+        )
         return True
     except Exception as err:
         # Best effort, deliberately. Raising would make Lambda retry the whole
@@ -155,8 +184,25 @@ def classify_log(full_name, job_id):
         return False
 
 
+def is_retryable(status_code):
+    """Whether GitHub answering with this is worth another attempt.
+
+    Server errors and rate limits go away on their own; 404 (the log has aged
+    out, which GitHub does after a couple of months) and the auth failures do
+    not, and replaying those only re-runs a download that will fail identically.
+    """
+    return status_code >= 500 or status_code == 429
+
+
 def download_log(full_name, conclusion, job_id):
-    """Fetch a job log from GitHub and archive it. Returns True when stored."""
+    """Fetch a job log from GitHub and archive it. Returns True when stored.
+
+    Returns False for failures that a retry cannot fix, and raises
+    RetryableDownloadError for the ones it can. Because callers invoke this
+    function asynchronously, raising is what puts a job in front of Lambda's
+    retries and, if they all fail, on the dead-letter queue -- returning False
+    records the invocation as a success and the lost log goes unreported.
+    """
     response = None
 
     app_token = installation_token(full_name)
@@ -174,12 +220,19 @@ def download_log(full_name, conclusion, job_id):
 
     if response is None:
         if not GITHUB_TOKENS:
-            print(f"ERROR no usable credential for {full_name} job {job_id}")
-            return False
+            # A misconfiguration, so retrying will not fix it -- but it costs
+            # every repo every log, and the DLQ is where that has to show up.
+            raise RetryableDownloadError(
+                f"no usable credential for {full_name} job {job_id}"
+            )
         response = fetch_log(full_name, job_id, random.choice(GITHUB_TOKENS.split(",")))
 
     if not response.ok:
         # Bail out rather than archive the API error body as if it were the log
+        if is_retryable(response.status_code):
+            raise RetryableDownloadError(
+                f"{response.status_code} downloading log for {full_name} job {job_id}"
+            )
         print(
             f"ERROR {response.status_code} downloading log for {full_name} job {job_id}"
         )

aws/lambda/gha-log-uploader/test_lambda_function.py

@@ -55,16 +55,43 @@ def test_calls_the_classifier(self):
             self.assertTrue(classify_log("pytorch/pytorch", 123))
 
         urlopen.assert_called_once_with(
-            f"{lambda_function.LOG_CLASSIFIER_URL}/?job_id=123&repo=pytorch/pytorch"
+            f"{lambda_function.LOG_CLASSIFIER_URL}/?job_id=123&repo=pytorch/pytorch",
+            timeout=lambda_function.CLASSIFIER_TIMEOUT,
         )
 
+    def test_waits_a_bounded_time(self):
+        # Without a timeout there is none at all, and a hung classifier burns the
+        # whole function timeout -- which Lambda then treats as a failed async
+        # invocation and replays, re-downloading the log every time.
+        self.assertIsNotNone(lambda_function.CLASSIFIER_TIMEOUT)
+        self.assertLess(lambda_function.CLASSIFIER_TIMEOUT, 300)
+
     def test_a_failed_call_is_reported_not_raised(self):
         # Raising would make Lambda retry the whole function and re-download a
         # multi-megabyte log, when the log is already safe in S3.
         with patch.object(lambda_function, "urlopen") as urlopen:
             urlopen.side_effect = RuntimeError("connection reset")
             self.assertFalse(classify_log("pytorch/pytorch", 123))
 
+    def test_a_hang_is_reported_not_raised(self):
+        # The case the timeout exists for: urlopen raises TimeoutError once it
+        # fires, which must come back down the same branch as any other failure.
+        with patch.object(lambda_function, "urlopen") as urlopen:
+            urlopen.side_effect = TimeoutError("timed out")
+            self.assertFalse(classify_log("pytorch/pytorch", 123))
+
+    def test_a_hang_still_leaves_the_log_stored(self):
+        # Giving up on the reply must not undo the upload or fail the invocation.
+        with patch.object(lambda_function, "s3"), patch.object(
+            lambda_function, "download_log", return_value=True
+        ), patch.object(lambda_function, "urlopen", side_effect=TimeoutError):
+            result = lambda_function.lambda_handler(
+                {"repo": "pytorch/pytorch", "job_id": 5}, None
+            )
+
+        self.assertTrue(result["stored"])
+        self.assertFalse(result["classified"])
+
 
 class TestInstallationToken(unittest.TestCase):
     def setUp(self):
@@ -221,6 +248,29 @@ def test_error_response_is_not_archived(self, s3):
 
         s3.Object.assert_not_called()
 
+    def test_a_terminal_status_does_not_retry(self, s3):
+        # GitHub drops logs after a couple of months, and no number of retries
+        # brings one back -- nor does replaying an auth failure fix it.
+        for status in (401, 403, 404, 410, 422):
+            with self.subTest(status=status), patch.object(
+                lambda_function, "installation_token", return_value=None
+            ), patch.object(
+                lambda_function, "fetch_log", return_value=make_response(status)
+            ):
+                self.assertFalse(download_log("pytorch/pytorch", "failure", 123))
+
+    def test_a_transient_status_raises_so_lambda_retries(self, s3):
+        for status in (429, 500, 502, 503, 504):
+            with self.subTest(status=status), patch.object(
+                lambda_function, "installation_token", return_value=None
+            ), patch.object(
+                lambda_function, "fetch_log", return_value=make_response(status)
+            ):
+                with self.assertRaises(lambda_function.RetryableDownloadError):
+                    download_log("pytorch/pytorch", "failure", 123)
+
+        s3.Object.assert_not_called()
+
     def test_non_pytorch_repo_is_prefixed(self, s3):
         with patch.object(
             lambda_function, "installation_token", return_value=None
@@ -231,13 +281,16 @@ def test_non_pytorch_repo_is_prefixed(self, s3):
             "ossci-raw-job-status", "log/pytorch/executorch/999"
         )
 
-    def test_no_credentials_at_all_is_a_noop(self, s3):
+    def test_no_credentials_at_all_reaches_the_dlq(self, s3):
+        # Retries will not conjure a credential, but this costs every repo every
+        # log, so it has to fail loudly rather than report a successful no-op.
         with patch.object(
             lambda_function, "installation_token", return_value=None
         ), patch.object(lambda_function, "GITHUB_TOKENS", None), patch.object(
             lambda_function, "fetch_log"
         ) as fetch_log:
-            self.assertFalse(download_log("pytorch/pytorch", "failure", 123))
+            with self.assertRaises(lambda_function.RetryableDownloadError):
+                download_log("pytorch/pytorch", "failure", 123)
 
         fetch_log.assert_not_called()
         s3.Object.assert_not_called()