Skip to content

Commit 3ce26f0

Browse files
smackeseyclaude
authored andcommitted
fix(dagster-aws): guarantee ECS pipes interruption subprocess is reaped (#25004)
## Summary & Motivation On master build [153444](https://buildkite.com/dagster/internal/builds/153444), the `dagster-aws-3-12` step hung: every `pipes_tests` test from `test_ecs_pipes_task_failed_to_start` onward took *exactly* 240.0s — the global `pytest-timeout` fallback (`timeout = "240"`, `method: signal`) killing each hung test and limping to the next. Every pipes test *before* that point ran in seconds, which localizes the problem to mid-module state corruption rather than a slow agent or network issue. The corruption is introduced by `test_ecs_pipes_interruption_forwarding`, which #24924 rewrote. Its happy path was a single `p.terminate()` + `p.join(timeout=30)` + `assert not p.is_alive()`, with `p.kill()` escalation present *only* in the "task never launched within 60s" error branch. The subprocess under test runs `materialize`, which installs Dagster's SIGTERM→graceful-interruption handler — the whole point of the test. When that graceful shutdown outlives the 30s join (or any of the assertions fails), the happy path returns without ever calling `p.kill()`, leaving the child **alive**. That child is `fork()`ed from the test process and holds the inherited moto server socket on `localhost:5193` (the shared, function-scoped `moto_server` fixture port). A leaked orphan wedges that port, so every subsequent moto-dependent pipes test blocks until the 240s timeout fires. The build's 32.7s runtime for `test_ecs_pipes_interruption_forwarding` (≈ startup + the full 30s join cap) is consistent with the join expiring and the child surviving SIGTERM. ## Changes - **Always reap the child.** Wrap the test body in `try/finally` and unconditionally `p.kill()` + `p.join()` the subprocess on every exit path (assertion failure, join timeout, or the "didn't launch" branch), so it can never be leaked. - **Don't inherit the moto socket.** Start the subprocess from a `spawn` multiprocessing context (`multiprocessing.get_context("spawn")`, used for the `Manager`, `Event`, and `Process`). A spawned child gets a fresh interpreter and does not inherit this process's file descriptors — including the moto listening socket — and reaches moto over the network as a client instead. Even a leaked child can then no longer wedge port 5193. Linux CI defaults to `fork`; macOS already defaults to `spawn`. - **Gate on a real completion signal.** Add a `materialization_done_event` that the subprocess sets in its `finally` immediately after writing `return_dict`. The parent waits on this event after `p.terminate()` instead of asserting `not p.is_alive()` within a fixed window. Under spawn, instance/multiprocessing teardown after the interruption can outrun a process-death check even when the materialization data the test cares about is already ready — that timing mismatch was the residual flake #24924 was originally chasing. Together these turn what was a module-wide hang into a robust, deterministic test. ## Test Plan - `ruff format --check` / `ruff check` pass on the changed file. - `test_ecs_pipes_interruption_forwarding` in isolation: passes. - Full `test_ecs.py` (4 tests including the previously-wedged `test_ecs_pipes_task_failed_to_start` and `test_ecs_pipes_waiter_config`): **5/5 consecutive local runs pass cleanly at ~67s each**, no 240s timeouts, no flakes. - Watch the `dagster-aws` steps on this PR's Buildkite run. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Internal-RevId: 8e83e4f0d0a8915c241262bcd52beac7c777376c
1 parent 45c0d68 commit 3ce26f0

1 file changed

Lines changed: 39 additions & 13 deletions

File tree

  • python_modules/libraries/dagster-aws/dagster_aws_tests/pipes_tests

python_modules/libraries/dagster-aws/dagster_aws_tests/pipes_tests/test_ecs.py

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ def test_ecs_pipes(
136136
assert asset_check_executions[0].status == AssetCheckExecutionRecordStatus.SUCCEEDED
137137

138138

139-
def _materialize_asset(env, return_dict, task_started_event):
139+
def _materialize_asset(env, return_dict, task_started_event, materialization_done_event):
140140
ecs_client = boto3.client("ecs", region_name="us-east-1", endpoint_url=_MOTO_SERVER_URL)
141141
cloudwatch_client = boto3.client("logs", region_name="us-east-1", endpoint_url=_MOTO_SERVER_URL)
142142
mock_client = LocalECSMockClient(ecs_client=ecs_client, cloudwatch_client=cloudwatch_client)
@@ -173,36 +173,62 @@ def run_task_and_signal(**kwargs):
173173
return_dict[0] = pipes._client.describe_tasks( # noqa
174174
cluster="test-cluster", tasks=[task_arn]
175175
)
176+
# Signal that return_dict is fully populated. The parent gates on this rather
177+
# than on `p.is_alive()` because spawn/instance teardown after the interruption
178+
# can outrun a fixed process-death window even when the materialization data
179+
# the test cares about is already ready.
180+
materialization_done_event.set()
176181

177182

178183
def test_ecs_pipes_interruption_forwarding(pipes_ecs_client: PipesECSClient):
179-
with multiprocessing.Manager() as manager:
184+
# Use a "spawn" context so the subprocess starts a fresh interpreter and does NOT
185+
# inherit this process's open file descriptors -- in particular the moto server's
186+
# listening socket on localhost:5193. The child reaches moto over the network as a
187+
# client, so it has no need to inherit that socket; a forked child that outlived its
188+
# expected lifetime would otherwise keep the port wedged and hang every subsequent
189+
# pipes test. (Linux CI defaults to fork; macOS already defaults to spawn.)
190+
ctx = multiprocessing.get_context("spawn")
191+
with ctx.Manager() as manager:
180192
return_dict = manager.dict()
181-
task_started_event = multiprocessing.Event()
193+
task_started_event = ctx.Event()
194+
materialization_done_event = ctx.Event()
182195

183-
p = multiprocessing.Process(
196+
p = ctx.Process(
184197
target=_materialize_asset,
185198
args=(
186199
{"SLEEP_SECONDS": "10"},
187200
return_dict,
188201
task_started_event,
202+
materialization_done_event,
189203
),
190204
)
191205
p.start()
206+
try:
207+
if not task_started_event.wait(timeout=60):
208+
raise AssertionError("Subprocess did not launch an ECS task within 60s")
192209

193-
if not task_started_event.wait(timeout=60):
194210
p.terminate()
195-
p.join(timeout=30)
211+
212+
# Gate on the materialization-complete signal -- set by the subprocess
213+
# immediately after return_dict is populated -- rather than asserting the
214+
# process is fully reaped within a fixed window. Under spawn, instance and
215+
# multiprocessing teardown after the interruption can outrun a process-death
216+
# check even when the data we want to assert on is already available.
217+
if not materialization_done_event.wait(timeout=60):
218+
raise AssertionError(
219+
"Subprocess did not complete materialization within 60s of SIGTERM"
220+
)
221+
222+
assert return_dict[0]["tasks"][0]["containers"][0]["exitCode"] == 1
223+
assert return_dict[0]["tasks"][0]["stoppedReason"] == "Dagster process was interrupted"
224+
finally:
225+
# Defense-in-depth: spawn already prevents the child from inheriting our
226+
# fds, and we waited on a real completion signal above, but never leave
227+
# the subprocess alive -- a runaway child would still hold the manager
228+
# connection and consume CI resources. Always reap.
196229
if p.is_alive():
197230
p.kill()
198231
p.join()
199-
raise AssertionError("Subprocess did not launch an ECS task within 60s")
200-
201-
p.terminate()
202-
p.join(timeout=30)
203-
assert not p.is_alive()
204-
assert return_dict[0]["tasks"][0]["containers"][0]["exitCode"] == 1
205-
assert return_dict[0]["tasks"][0]["stoppedReason"] == "Dagster process was interrupted"
206232

207233

208234
class FailToStartECSMockClient(LocalECSMockClient):

0 commit comments

Comments
 (0)