Update

[ghstack-poisoned]

Author: huydhn

.github/workflows/linux_job_v2.yml

@@ -137,6 +137,7 @@ jobs:
       # Will be blank outside of this
       PR_NUMBER: ${{ github.event.pull_request.number }}
       SCRIPT: ${{ inputs.script }}
+      ALPINE_IMAGE: ${{ startsWith(inputs.runner, 'linux.arm64') && 'arm64v8/alpine' || startsWith(inputs.runner, 'linux.idc') && 'alpine' || '308535385114.dkr.ecr.us-east-1.amazonaws.com/tool/alpine' }}
       filter: ${{ inputs.checkout-mode == 'blobless' && 'blob:none' || inputs.checkout-mode == 'treeless' && 'tree:0' || null }}
     runs-on: ${{ inputs.runner }}
     # TODO: Eventually this should run in a container, we need to make a container that matches up
@@ -311,13 +312,27 @@ jobs:
         uses: ./test-infra/.github/actions/chown-directory
         with:
           directory: ${{ github.workspace }}/${{ env.repository }}
-          ALPINE_IMAGE: ${{ startsWith(inputs.runner, 'linux.arm64') && 'arm64v8/alpine' || startsWith(inputs.runner, 'linux.idc') && 'alpine' || '308535385114.dkr.ecr.us-east-1.amazonaws.com/tool/alpine' }}
+          ALPINE_IMAGE: ${{ env.ALPINE_IMAGE }}
 
-      - name: Chown runner temp
+      # Not all of ${RUNNER_TEMP} — a container-issued chown lands on a subordinate
+      # UID under rootless docker, locking the runner out of _github_workflow/.
+      - name: Chown runner artifacts dir
         uses: ./test-infra/.github/actions/chown-directory
         with:
-          directory: ${{ runner.temp }}
-          ALPINE_IMAGE: ${{ startsWith(inputs.runner, 'linux.arm64') && 'arm64v8/alpine' || startsWith(inputs.runner, 'linux.idc') && 'alpine' || '308535385114.dkr.ecr.us-east-1.amazonaws.com/tool/alpine' }}
+          directory: ${{ env.RUNNER_ARTIFACT_DIR }}
+          ALPINE_IMAGE: ${{ env.ALPINE_IMAGE }}
+
+      - name: Chown runner docs dir
+        uses: ./test-infra/.github/actions/chown-directory
+        with:
+          directory: ${{ env.RUNNER_DOCS_DIR }}
+          ALPINE_IMAGE: ${{ env.ALPINE_IMAGE }}
+
+      - name: Chown runner test results dir
+        uses: ./test-infra/.github/actions/chown-directory
+        with:
+          directory: ${{ env.RUNNER_TEST_RESULTS_DIR }}
+          ALPINE_IMAGE: ${{ env.ALPINE_IMAGE }}
 
       - name: Prepare artifacts for upload
         if: always()

torchci/lib/lambda.ts

@@ -6,13 +6,36 @@ import {
 
 export const GHA_LOG_UPLOADER_FUNCTION = "gha-log-uploader";
 
+export class MissingAwsCredentialsError extends Error {
+  constructor() {
+    // The SDK's own failure here is "Resolved credential object is not valid",
+    // which says nothing about which variable to go set.
+    super(
+      "OUR_AWS_ACCESS_KEY_ID / OUR_AWS_SECRET_ACCESS_KEY are not set, " +
+        "cannot reach the log uploader"
+    );
+    this.name = "MissingAwsCredentialsError";
+  }
+}
+
 export function getLambdaClient(): LambdaClient {
+  const accessKeyId = process.env.OUR_AWS_ACCESS_KEY_ID;
+  const secretAccessKey = process.env.OUR_AWS_SECRET_ACCESS_KEY;
+  if (!accessKeyId || !secretAccessKey) {
+    throw new MissingAwsCredentialsError();
+  }
+
   return new LambdaClient({
     region: "us-east-1",
-    credentials: {
-      accessKeyId: process.env.OUR_AWS_ACCESS_KEY_ID!,
-      secretAccessKey: process.env.OUR_AWS_SECRET_ACCESS_KEY!,
-    },
+    credentials: { accessKeyId, secretAccessKey },
+    // This call sits on the webhook ack path, so it has to fail fast rather than
+    // fail well. The SDK defaults to 3 attempts and no socket timeout, which on a
+    // black-holed endpoint hangs for the OS default (~75s) per attempt; measured,
+    // a connectionTimeout of 1s aborts at ~1.5s instead. Worst case here is
+    // roughly 4s, against a 10s GitHub webhook timeout that every other handler
+    // also has to fit inside.
+    maxAttempts: 2,
+    requestHandler: { connectionTimeout: 1000, requestTimeout: 2000 },
   });
 }
 
@@ -32,8 +55,9 @@ export interface LogUploadRequest {
  * handoff has to be both awaited and bounded.
  *
  * Delivery failures are Lambda's problem from here -- it retries twice and then
- * DLQs. Failures to hand off at all are the caller's, and are non-fatal: Dr.CI
- * re-requests a missing log through backfillMissingLog on its next run.
+ * DLQs. Failures to hand off at all reject, and every caller treats that as
+ * non-fatal: Dr.CI re-requests a missing log through backfillMissingLog on its
+ * next run, so losing a handoff costs a log, never a webhook.
  */
 export async function invokeLogUploader(
   request: LogUploadRequest

torchci/test/backfillMissingLog.test.ts

@@ -0,0 +1,37 @@
+import { backfillMissingLog } from "lib/jobUtils";
+import * as lambda from "lib/lambda";
+import { RecentWorkflowsData } from "lib/types";
+
+const job = { id: 999, conclusion: "failure" } as RecentWorkflowsData;
+
+describe("backfillMissingLog", () => {
+  afterEach(() => jest.restoreAllMocks());
+
+  test("queues an upload and reports success", async () => {
+    const invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
+
+    await expect(
+      backfillMissingLog("pytorch", "executorch", job)
+    ).resolves.toBe(true);
+    expect(invoke).toHaveBeenCalledWith({
+      repo: "pytorch/executorch",
+      job_id: 999,
+      conclusion: "failure",
+    });
+  });
+
+  test.each([
+    ["credentials are missing", new lambda.MissingAwsCredentialsError()],
+    ["the Lambda API is unreachable", new Error("TimeoutError")],
+    ["the role cannot invoke", new Error("AccessDeniedException")],
+  ])("returns false rather than throwing when %s", async (_label, error) => {
+    // Dr.CI calls this mid-comment-render for every failed job with no log. An
+    // exception here would take down the whole Dr.CI run over one missing log.
+    jest.spyOn(lambda, "invokeLogUploader").mockRejectedValue(error);
+    jest.spyOn(console, "error").mockImplementation(() => {});
+
+    await expect(backfillMissingLog("pytorch", "pytorch", job)).resolves.toBe(
+      false
+    );
+  });
+});

torchci/test/lambdaClient.test.ts

@@ -0,0 +1,65 @@
+import {
+  getLambdaClient,
+  invokeLogUploader,
+  MissingAwsCredentialsError,
+} from "lib/lambda";
+
+describe("getLambdaClient", () => {
+  const saved = {
+    id: process.env.OUR_AWS_ACCESS_KEY_ID,
+    secret: process.env.OUR_AWS_SECRET_ACCESS_KEY,
+  };
+
+  afterEach(() => {
+    process.env.OUR_AWS_ACCESS_KEY_ID = saved.id;
+    process.env.OUR_AWS_SECRET_ACCESS_KEY = saved.secret;
+  });
+
+  function setCredentials(id?: string, secret?: string) {
+    if (id === undefined) {
+      delete process.env.OUR_AWS_ACCESS_KEY_ID;
+    } else {
+      process.env.OUR_AWS_ACCESS_KEY_ID = id;
+    }
+    if (secret === undefined) {
+      delete process.env.OUR_AWS_SECRET_ACCESS_KEY;
+    } else {
+      process.env.OUR_AWS_SECRET_ACCESS_KEY = secret;
+    }
+  }
+
+  test.each([
+    ["neither is set", undefined, undefined],
+    ["only the key id is set", "AKIA", undefined],
+    ["only the secret is set", undefined, "shh"],
+    ["the key id is empty", "", "shh"],
+  ])("throws a named error when %s", (_label, id, secret) => {
+    setCredentials(id, secret);
+    expect(() => getLambdaClient()).toThrow(MissingAwsCredentialsError);
+  });
+
+  test("the error names the variables to set", () => {
+    setCredentials(undefined, undefined);
+    // The SDK's own message is "Resolved credential object is not valid", which
+    // gives whoever is paged nothing to act on.
+    expect(() => getLambdaClient()).toThrow(/OUR_AWS_ACCESS_KEY_ID/);
+  });
+
+  test("builds a client when both are set", () => {
+    setCredentials("AKIA", "shh");
+    expect(getLambdaClient()).toBeDefined();
+  });
+
+  test("bounds retries and socket waits", async () => {
+    setCredentials("AKIA", "shh");
+    const config = getLambdaClient().config;
+    expect(await config.maxAttempts()).toBe(2);
+  });
+
+  test("invokeLogUploader rejects rather than hanging with no credentials", async () => {
+    setCredentials(undefined, undefined);
+    await expect(
+      invokeLogUploader({ repo: "pytorch/pytorch", job_id: 1 })
+    ).rejects.toThrow(MissingAwsCredentialsError);
+  });
+});

torchci/test/logUploader.test.ts

@@ -138,10 +138,17 @@ describe("logUploader", () => {
     );
   });
 
-  test("a failed invoke does not fail the webhook", async () => {
+  test.each([
+    ["the role cannot invoke the function", "AccessDeniedException"],
+    ["credentials are missing entirely", "MissingAwsCredentialsError"],
+    ["the Lambda API is unreachable", "TimeoutError"],
+    ["Lambda throttles us", "TooManyRequestsException"],
+  ])("a failed invoke does not fail the webhook when %s", async (_l, name) => {
     // Throwing here would make GitHub redeliver the event and re-run every other
     // handler, to retry something Dr.CI repairs on its own.
-    invoke.mockRejectedValue(new Error("AccessDeniedException"));
+    const error = new Error(name);
+    error.name = name;
+    invoke.mockRejectedValue(error);
 
     await expect(receive(workflowJobPayload())).resolves.not.toThrow();
   });

torchci/test/logUploaderBackfill.test.ts

@@ -114,8 +114,13 @@ describe("/api/log-uploader/backfill", () => {
     expect(res.status).toHaveBeenCalledWith(400);
   });
 
-  test("reports a failure to reach the uploader", async () => {
-    invoke.mockRejectedValue(new Error("AccessDeniedException"));
+  test.each([
+    ["credentials are missing", new lambda.MissingAwsCredentialsError()],
+    ["the role cannot invoke", new Error("AccessDeniedException")],
+    ["the Lambda API is unreachable", new Error("TimeoutError")],
+  ])("reports 502 rather than throwing when %s", async (_label, error) => {
+    invoke.mockRejectedValue(error);
+    jest.spyOn(console, "error").mockImplementation(() => {});
     const res = mockRes();
     await handler(mockReq(), res);