[build-tools] [ENG-15188] Cancel hanging workflows #525

radoslawkrzemien · 2025-04-04T16:15:35Z

Why

https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

How

Added warn and kill timeouts for workflow command execution, which get reset after logs are printed, and removed after workflow step finishes. If they time out, first a warning is printed and then process gets killed. Similar solution works for builds

Test Plan

Automated tests pass
TODO: add more tests

Functionality will be needed in `steps` and can be imported from there See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Added `infoCallbackFn` to `spawnAsync.SpawnOptions` to allow for tracking when the process writes something at info level See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Functionality will be needed in `steps` and can be imported from there See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Added warn and kill timeouts to build step command execution See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

linear · 2025-04-04T16:15:38Z

ENG-15188 Replicate the cancel hanging build functionality for workflows

Improved readability by extracting timeouts to class fields and setting them up to a private method See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

sjchmiela

Can you add some tests too?

sjchmiela · 2025-04-08T11:46:46Z

packages/steps/src/BuildStep.ts

+const BUILD_STEP_WARN_TIMEOUT_MS = 15 * 60 * 1000;
+const BUILD_STEP_KILL_TIMEOUT_MS = 30 * 60 * 1000;


Can we make this customizable per step, please? Like

steps: - run: maestro test error-no-logs: 30m # not sure about the param name or value format / parsing, maybe you can figure something. Maybe GitHub has something similar?

I'll look into it

GHA has jobs.<job_id>.timeout-minutes and jobs.<job_id>.steps[*].timeout-minutes, although it's not the same as what we are doing since it only looks at total job/step execution time - even if the job/step is alive and printing, if it passes the timeout it gets cancelled. So in this case I think we shouldn't try to mimick GHA as it would be confusing to people familiar with it, maybe something like no-logs-timeout-minutes? WDYT?

packages/steps/src/BuildStep.ts

sjchmiela · 2025-04-08T11:49:00Z

packages/steps/src/BuildStep.ts

+      const ppid = nullthrows(spawnPromise.child.pid);
+      const pids = await getParentAndDescendantProcessPidsAsync(ppid);
+      pids.forEach((pid) => {
+        process.kill(pid);


Isn't killing child.pid sufficient?

I think it should be, but we do it the same way with Install Dependencies and Install Pods build steps and I seem to vaguely remember there was a reason for it, but I can't recall it now

sjchmiela · 2025-04-08T11:51:25Z

packages/steps/src/BuildStep.ts

-      // stdin is /dev/null, std{out,err} are piped into logger.
-      stdio: ['ignore', 'pipe', 'pipe'],
-    });
+    this.commandTimedOut = false;


I suspect we might not need to assign commandTimedOut or commandWarnTimeout or commandKillTimeout on this. Is there a reason to do so?

Just readability, if all is defined within the function executing the command over half of the function body is taken by timeout handing, which is not the main purpose of the function. Having those as properties allows for extracting the timeout setup to a separate private method

Maybe we should move this (setting up timeouts and catching-and-throwing-speciifc-error) to spawnAsync?

That might work. Are you aware of any cases where we use spawnAsync and might not want to have timeouts?

Co-authored-by: Stanisław Chmiela <[email protected]>

Moved the creation, refreshing, handling and removal of optional timeouts into `spawnAsync` itself, while the caller might, but does not have to, define if the timeouts are needed and if they are what their params are See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

sjchmiela · 2025-04-11T11:04:36Z

packages/steps/src/utils/shell/spawn.ts

+    errorClass?: IErrorClass;
+    errorMessage?: string;


Is there a reason we need to customize errorClass and errorMessage? Couldn't we just pass in warnTimeoutMs and killTimeoutMs?

We don't need to, if not passed a default error with a default message will be thrown. But I thought it might be useful to customise them depending on where the functionality is used (workflow command, build step, etc.) and have the appropriate message

sjchmiela · 2025-04-11T11:05:11Z

packages/steps/src/utils/shell/spawn.ts

+          logger.warn(
+            noLogsTimeout.warn?.message ??
+              SPAWN_WARN_TIMEOUT_DEFAULT_MESSAGE.replace(
+                '${minutes}',


Let's make this simpler.

sjchmiela · 2025-04-11T11:06:57Z

packages/steps/src/utils/shell/spawn.ts

+      const errorMessage =
+        noLogsTimeout?.kill?.errorMessage ??
+        SPAWN_KILL_TIMEOUT_DEFAULT_ERROR_MESSAGE.replace('${minutes}', spawnKillTimeout.toString());
+      const ErrorClass = noLogsTimeout?.kill?.errorClass ?? Error;


Can we use some well-known error class for this? Maybe we want to use UserFacingError for this, or add a new subclass of UserFacingError in eas-build-job and use that?

sjchmiela · 2025-04-11T11:08:33Z

packages/steps/src/utils/shell/spawn.ts

+        pipeSpawnOutput(logger, spawnPromise.child, options);
+      }
+    }
+    spawnResult = await spawnPromise;


Hmm, I think in some places we don't await the SpawnPromise by design. E.g. here.

Instead of try-catch and awaiting the spawnPromise, could we attach the catch as spawnPromise.catch(err => ...)?

Hm, doing it like this breaks one of the tests, and I'm not exactly sure why. Might be the .catch() interferes with the original implementation of spawnAsync from @expo/spawn-async. Investigating

Moved the creation of timeout messages to helper functions to unclutter the main function body See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Removed the iterative killing of descendant pids See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

This reverts commit 138b2bb.

This reverts commit 3094b81.

Even though it's always set at this point, TS still thinks it can be undefined See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Returning the SpawnPromise unawaited, as before, but with `catch` and `finally` callbacks attached See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Added a new subclass of UserFacingError for spawn timeouts See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Allowed for setting the timeout in minutes on the step definition See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Inner promise from `@expo/spawn-async` rejecting caused the outer process to exit unexpectedly See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Updated test for Step See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Updated tests for StepsConfigParser checking for customizable timeouts See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

radoslawkrzemien added 4 commits April 4, 2025 17:42

[eas-build] Move processes.ts to steps

3094b81

Functionality will be needed in `steps` and can be imported from there See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Add callback to spawn options

f2d9526

Added `infoCallbackFn` to `spawnAsync.SpawnOptions` to allow for tracking when the process writes something at info level See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Move processes.ts to steps

138b2bb

Functionality will be needed in `steps` and can be imported from there See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Add timeouts

ea8dfff

Added warn and kill timeouts to build step command execution See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Reformat

9555ec9

Improved readability by extracting timeouts to class fields and setting them up to a private method See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

radoslawkrzemien marked this pull request as ready for review April 8, 2025 09:45

radoslawkrzemien requested a review from sjchmiela as a code owner April 8, 2025 09:45

radoslawkrzemien requested a review from khamilowicz April 8, 2025 09:45

sjchmiela requested changes Apr 8, 2025

View reviewed changes

radoslawkrzemien and others added 3 commits April 8, 2025 13:57

Update packages/steps/src/BuildStep.ts

9a76072

Co-authored-by: Stanisław Chmiela <[email protected]>

Update packages/steps/src/BuildStep.ts

6206192

Co-authored-by: Stanisław Chmiela <[email protected]>

sjchmiela requested changes Apr 11, 2025

View reviewed changes

radoslawkrzemien added 11 commits April 14, 2025 11:29

[eas-build] Simplify message creation

08b8fd5

Moved the creation of timeout messages to helper functions to unclutter the main function body See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Kill just child pid

2890b87

Removed the iterative killing of descendant pids See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

Revert "[eas-build] Move processes.ts to steps"

cca6e8b

This reverts commit 138b2bb.

Revert "[eas-build] Move processes.ts to steps"

d7e6e29

This reverts commit 3094b81.

[eas-build] Make param optionally undefined

459a096

Even though it's always set at this point, TS still thinks it can be undefined See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Don't await SpawnPromise

a40fbe6

Returning the SpawnPromise unawaited, as before, but with `catch` and `finally` callbacks attached See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Use specific error class

6f17ed5

Added a new subclass of UserFacingError for spawn timeouts See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Parameterize the timeouts

c2885de

Allowed for setting the timeout in minutes on the step definition See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Fix inner promise rejection

dc9d523

Inner promise from `@expo/spawn-async` rejecting caused the outer process to exit unexpectedly See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Update test

4b9aa51

Updated test for Step See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

[eas-build] Update tests

d2ba058

Updated tests for StepsConfigParser checking for customizable timeouts See: https://linear.app/expo/issue/ENG-15188/replicate-the-cancel-hanging-build-functionality-for-workflows

		const BUILD_STEP_WARN_TIMEOUT_MS = 15 * 60 * 1000;
		const BUILD_STEP_KILL_TIMEOUT_MS = 30 * 60 * 1000;

[build-tools] [ENG-15188] Cancel hanging workflows #525

Are you sure you want to change the base?

[build-tools] [ENG-15188] Cancel hanging workflows #525

Uh oh!

Conversation

radoslawkrzemien commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How

Test Plan

Uh oh!

linear bot commented Apr 4, 2025

Uh oh!

sjchmiela left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radoslawkrzemien Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

radoslawkrzemien commented Apr 4, 2025 •

edited

Loading

radoslawkrzemien Apr 8, 2025 •

edited

Loading