Skip to content

Commit b8b7f3f

Browse files
smackeseyDagster Devtools
authored andcommitted
fix(dagster-aws): make ECS pipes interruption test robust under CI load (#24924)
## Summary & Motivation `test_ecs_pipes_interruption_forwarding` has been flaking on `internal` builds when CI is under load (e.g. https://buildkite.com/dagster/internal/builds/152968, https://buildkite.com/dagster/internal/builds/152985), failing with `KeyError: 0` at the final assertion. Root cause is a race in the test itself, not a CI resource issue. The test spawns a subprocess that runs `_materialize_asset`, then sleeps a hardcoded 4 seconds before sending `SIGTERM` and asserting that the subprocess populated `return_dict[0]` with the stopped ECS task state. `return_dict[0]` is only written from a `finally` block, gated by `assert len(pipes._client._task_runs) > 0`. Under load, Python startup + `DagsterInstance.from_config()` + moto setup take longer than 4 seconds, so the subprocess receives `SIGTERM` before `materialize()` has launched the ECS task. The `finally`-block precondition fails, the child dies without writing `return_dict[0]`, and the parent's assertion blows up. The captured stderr in both failing builds shows the child died inside `instance_for_test()` → `unified_storage = instance_ref.storage`, well before any task was launched. Replace the fixed sleep with an explicit "task launched" signal. The child monkey-patches `mock_client.run_task` to set a `multiprocessing.Event` as soon as `run_task` returns; the parent `event.wait(timeout=60)` before sending `SIGTERM`, guaranteeing there is a live ECS task to interrupt. The unbounded `while p.is_alive(): sleep(4); terminate()` retry loop is replaced with a single `terminate()` + bounded `join(timeout=30)`. ## Test Plan - `tox -e py312 -- -k test_ecs_pipes_interruption_forwarding` in `python_modules/libraries/dagster-aws` (from `dagster-oss/`). - Watch the dagster-aws step on this PR's Buildkite run; it should pass without retries. Internal-RevId: b8eefc49d4bf8091e176569ccbde859c273624fb
1 parent fcca120 commit b8b7f3f

1 file changed

Lines changed: 27 additions & 10 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: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
import os
33
import re
44
import sys
5-
import time
65
from typing import TYPE_CHECKING
76

87
import boto3
@@ -137,11 +136,25 @@ def test_ecs_pipes(
137136
assert asset_check_executions[0].status == AssetCheckExecutionRecordStatus.SUCCEEDED
138137

139138

140-
def _materialize_asset(env, return_dict):
139+
def _materialize_asset(env, return_dict, task_started_event):
141140
ecs_client = boto3.client("ecs", region_name="us-east-1", endpoint_url=_MOTO_SERVER_URL)
142141
cloudwatch_client = boto3.client("logs", region_name="us-east-1", endpoint_url=_MOTO_SERVER_URL)
142+
mock_client = LocalECSMockClient(ecs_client=ecs_client, cloudwatch_client=cloudwatch_client)
143+
144+
# Signal the parent as soon as the ECS task has actually been launched, so
145+
# the parent waits for real work before sending SIGTERM rather than racing
146+
# subprocess startup with a fixed sleep.
147+
original_run_task = mock_client.run_task
148+
149+
def run_task_and_signal(**kwargs):
150+
result = original_run_task(**kwargs)
151+
task_started_event.set()
152+
return result
153+
154+
mock_client.run_task = run_task_and_signal # type: ignore[method-assign] # ty: ignore[invalid-assignment]
155+
143156
pipes = PipesECSClient(
144-
client=LocalECSMockClient(ecs_client=ecs_client, cloudwatch_client=cloudwatch_client), # type: ignore
157+
client=mock_client, # type: ignore
145158
message_reader=PipesCloudWatchMessageReader(
146159
client=cloudwatch_client,
147160
),
@@ -165,25 +178,29 @@ def _materialize_asset(env, return_dict):
165178
def test_ecs_pipes_interruption_forwarding(pipes_ecs_client: PipesECSClient):
166179
with multiprocessing.Manager() as manager:
167180
return_dict = manager.dict()
181+
task_started_event = multiprocessing.Event()
168182

169183
p = multiprocessing.Process(
170184
target=_materialize_asset,
171185
args=(
172186
{"SLEEP_SECONDS": "10"},
173187
return_dict,
188+
task_started_event,
174189
),
175190
)
176191
p.start()
177192

178-
while p.is_alive():
179-
# we started executing the run
180-
# time to interrupt it!
181-
time.sleep(4)
193+
if not task_started_event.wait(timeout=60):
182194
p.terminate()
183-
184-
p.join()
195+
p.join(timeout=30)
196+
if p.is_alive():
197+
p.kill()
198+
p.join()
199+
raise AssertionError("Subprocess did not launch an ECS task within 60s")
200+
201+
p.terminate()
202+
p.join(timeout=30)
185203
assert not p.is_alive()
186-
# breakpoint()
187204
assert return_dict[0]["tasks"][0]["containers"][0]["exitCode"] == 1
188205
assert return_dict[0]["tasks"][0]["stoppedReason"] == "Dagster process was interrupted"
189206

0 commit comments

Comments
 (0)