Skip to content

Commit 5991b1e

Browse files
authored
Fix jpd.hostname AssertionError on container stop (#3951)
* Fix jpd.hostname AssertionError on container stop * Add regression tests
1 parent eeb07cd commit 5991b1e

4 files changed

Lines changed: 97 additions & 7 deletions

File tree

src/dstack/_internal/server/background/pipeline_tasks/jobs_terminating.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,10 +660,12 @@ async def _process_terminating_job(
660660

661661
jrd = get_job_runtime_data(job_model)
662662
jpd = get_job_provisioning_data(job_model)
663-
if jpd is not None:
663+
if jpd is not None and jpd.hostname is not None and jpd.ssh_port is not None:
664664
logger.debug("%s: stopping container", fmt(job_model))
665665
ssh_private_keys = get_instance_ssh_private_keys(instance_model)
666666
if not await _stop_container(job_model, jpd, ssh_private_keys):
667+
# Dangling containers (tasks) are cleared periodically on instance checks by
668+
# `remove_dangling_tasks_from_instance()`
667669
logger.warning(
668670
(
669671
"%s: could not stop container, possibly due to a communication error."

src/dstack/_internal/server/services/jobs/__init__.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -338,13 +338,13 @@ async def stop_runner(job_model: JobModel, instance_model: InstanceModel):
338338
`instance_model.project` must be loaded because SSH key resolution uses the project keys.
339339
"""
340340
ssh_private_keys = get_instance_ssh_private_keys(instance_model)
341-
try:
342-
jpd = get_job_provisioning_data(job_model)
343-
if jpd is not None:
344-
jrd = get_job_runtime_data(job_model)
341+
jpd = get_job_provisioning_data(job_model)
342+
if jpd is not None:
343+
jrd = get_job_runtime_data(job_model)
344+
try:
345345
await run_async(_stop_runner, ssh_private_keys, jpd, jrd, job_model)
346-
except SSHError:
347-
logger.debug("%s: failed to stop runner", fmt(job_model))
346+
except SSHError:
347+
logger.debug("%s: failed to stop runner", fmt(job_model))
348348

349349

350350
@runner_ssh_tunnel

src/dstack/_internal/server/services/runner/ssh.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ def wrapper(
5252
Returns:
5353
is successful
5454
"""
55+
if job_provisioning_data.hostname is None or job_provisioning_data.ssh_port is None:
56+
# The callers may try to establish tunnels even if hostname/ssh_port is missing
57+
# and rely on `False` being returned in this case.
58+
return False
59+
5560
if not settings.SERVER_SSH_POOL_ENABLED or not job_provisioning_data.dockerized:
5661
# Connections from dstack-server to runner's sshd are expected to be short
5762
# as the `inactivity_duration` feature distinguishes user and server connections based on duration.

src/tests/_internal/server/background/pipeline_tasks/test_terminating_jobs.py

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -902,3 +902,86 @@ async def test_keeps_related_instance_locked_on_processing_exception(
902902
assert job.lock_owner == JobTerminatingPipeline.__name__
903903
assert instance.lock_token == job_lock_token
904904
assert instance.lock_owner == _get_related_instance_lock_owner(job.id)
905+
906+
async def test_stops_job_gracefully_without_provisioning_data_hostname(
907+
self, test_db, session: AsyncSession, worker: JobTerminatingWorker
908+
):
909+
# Regression test for https://github.com/dstackai/dstack/issues/3950.
910+
# Stopping a job that is still provisioning (no hostname/ssh_port yet) must not raise
911+
# when the graceful stop tries to open an SSH tunnel to the runner.
912+
project = await create_project(session=session)
913+
user = await create_user(session=session)
914+
instance = await create_instance(
915+
session=session,
916+
project=project,
917+
status=InstanceStatus.BUSY,
918+
)
919+
repo = await create_repo(session=session, project_id=project.id)
920+
run = await create_run(session=session, project=project, repo=repo, user=user)
921+
jpd = get_job_provisioning_data(dockerized=True)
922+
jpd.hostname = None
923+
jpd.ssh_port = None
924+
job = await create_job(
925+
session=session,
926+
run=run,
927+
status=JobStatus.TERMINATING,
928+
termination_reason=JobTerminationReason.TERMINATED_BY_USER,
929+
job_provisioning_data=jpd,
930+
instance=instance,
931+
)
932+
job.graceful_termination_attempts = 0
933+
_lock_job(job)
934+
await session.commit()
935+
936+
await worker.process(_job_to_pipeline_item(job))
937+
938+
await session.refresh(job)
939+
assert job.status == JobStatus.TERMINATING
940+
assert job.graceful_termination_attempts == 1
941+
assert job.remove_at is not None
942+
assert job.instance_id == instance.id
943+
944+
async def test_terminates_job_without_provisioning_data_hostname(
945+
self, test_db, session: AsyncSession, worker: JobTerminatingWorker
946+
):
947+
# Regression test for https://github.com/dstackai/dstack/issues/3950.
948+
# The container stop is skipped (and must not raise) when the job has no hostname/ssh_port.
949+
# Dangling containers are cleared later on instance checks by `remove_dangling_tasks_from_instance()`.
950+
project = await create_project(session=session)
951+
user = await create_user(session=session)
952+
instance = await create_instance(
953+
session=session,
954+
project=project,
955+
status=InstanceStatus.BUSY,
956+
)
957+
repo = await create_repo(session=session, project_id=project.id)
958+
run = await create_run(session=session, project=project, repo=repo, user=user)
959+
jpd = get_job_provisioning_data(dockerized=True)
960+
jpd.hostname = None
961+
jpd.ssh_port = None
962+
job = await create_job(
963+
session=session,
964+
run=run,
965+
status=JobStatus.TERMINATING,
966+
termination_reason=JobTerminationReason.TERMINATED_BY_USER,
967+
job_provisioning_data=jpd,
968+
instance=instance,
969+
)
970+
job.graceful_termination_attempts = 1
971+
job.remove_at = get_current_datetime() - timedelta(minutes=1)
972+
_lock_job(job)
973+
await session.commit()
974+
975+
with patch(
976+
"dstack._internal.server.background.pipeline_tasks.jobs_terminating._stop_container",
977+
new=AsyncMock(return_value=True),
978+
) as stop_container:
979+
await worker.process(_job_to_pipeline_item(job))
980+
981+
stop_container.assert_not_awaited()
982+
983+
await session.refresh(job)
984+
await session.refresh(instance)
985+
assert job.status == JobStatus.TERMINATED
986+
assert job.instance_id is None
987+
assert instance.status == InstanceStatus.IDLE

0 commit comments

Comments
 (0)