Skip to content

Commit ba142cf

Browse files
evakhoniclaude
andcommitted
fix: remove premature OFFLINE status in beforeLease hook failure path
When beforeLease hook fails with onFailure: exit, the handler reported BEFORE_LEASE_HOOK_FAILED then immediately OFFLINE, clobbering the failure status before the client could poll it. This caused a race where wait_for_any_of missed the target status and blocked for the full 300s timeout, producing the e2e B5/C2 test timeouts. Remove the early OFFLINE report — the shutdown/unregistration path reports it reliably. The controller's filterOutNotReadyExporters already prevents lease assignment to exporters with non-Available status, so no lease-assignment regression. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d9efea1 commit ba142cf

2 files changed

Lines changed: 21 additions & 26 deletions

File tree

python/packages/jumpstarter/jumpstarter/exporter/hooks.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -686,11 +686,6 @@ async def run_before_lease_hook(
686686
ExporterStatus.BEFORE_LEASE_HOOK_FAILED,
687687
f"beforeLease hook failed (on_failure=exit, shutting down): {e}",
688688
)
689-
await report_status(
690-
ExporterStatus.OFFLINE,
691-
"Exporter shutting down due to beforeLease hook failure",
692-
)
693-
# Defer shutdown: sets _stop_requested=True, actual stop after lease cleanup
694689
shutdown(exit_code=1, wait_for_lease_exit=True, should_unregister=True)
695690
else:
696691
# on_failure='endLease' - report failure, release in finally block

python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,8 +1261,9 @@ async def test_before_hook_exit_reports_failed_not_available(self, lease_scope)
12611261
"""Issue E2: beforeLease fail+exit should report FAILED, not AVAILABLE.
12621262
12631263
When beforeLease hook fails with on_failure=exit, the last status must be
1264-
BEFORE_LEASE_HOOK_FAILED. It should NOT report AVAILABLE, which would
1265-
incorrectly tell the controller the exporter is ready for new leases.
1264+
BEFORE_LEASE_HOOK_FAILED so the client can detect hook failure reliably.
1265+
It should NOT report AVAILABLE or OFFLINE, which would clobber the
1266+
failure status before the client can poll it.
12661267
"""
12671268
hook_config = HookConfigV1Alpha1(
12681269
before_lease=HookInstanceConfigV1Alpha1(script="exit 1", timeout=10, on_failure="exit"),
@@ -1282,16 +1283,9 @@ async def mock_report_status(status, msg):
12821283
mock_shutdown,
12831284
)
12841285

1285-
# Last status should be OFFLINE (reported before shutdown to prevent new leases)
12861286
last_status, _ = status_calls[-1]
1287-
assert last_status == ExporterStatus.OFFLINE, (
1288-
f"Expected last status to be OFFLINE, got {last_status}"
1289-
)
1290-
1291-
# BEFORE_LEASE_HOOK_FAILED should also be present (reported before OFFLINE)
1292-
failed_statuses = [s for s, _ in status_calls if s == ExporterStatus.BEFORE_LEASE_HOOK_FAILED]
1293-
assert len(failed_statuses) > 0, (
1294-
f"Expected BEFORE_LEASE_HOOK_FAILED status, got: {status_calls}"
1287+
assert last_status == ExporterStatus.BEFORE_LEASE_HOOK_FAILED, (
1288+
f"Expected last status to be BEFORE_LEASE_HOOK_FAILED, got {last_status}"
12951289
)
12961290

12971291
# AVAILABLE should never have been reported
@@ -1379,11 +1373,12 @@ async def mock_report_status(status, msg):
13791373
f"Expected LEASE_READY message to start with '{HOOK_WARNING_PREFIX}', got: '{msg}'"
13801374
)
13811375

1382-
async def test_before_hook_exit_reports_offline_before_shutdown(self, lease_scope) -> None:
1376+
async def test_before_hook_exit_reports_failed_before_shutdown(self, lease_scope) -> None:
13831377
"""When beforeLease hook fails with on_failure=exit, the exporter must
1384-
report OFFLINE status to the controller before initiating shutdown.
1385-
This prevents the controller from assigning new leases to a dying
1386-
exporter during the shutdown window.
1378+
report BEFORE_LEASE_HOOK_FAILED before initiating shutdown. OFFLINE is
1379+
NOT reported here — the shutdown path handles controller notification
1380+
during unregistration, avoiding a race where OFFLINE overwrites the
1381+
failure status before the client can poll it.
13871382
"""
13881383
hook_config = HookConfigV1Alpha1(
13891384
before_lease=HookInstanceConfigV1Alpha1(script="exit 1", timeout=10, on_failure="exit"),
@@ -1406,17 +1401,22 @@ def mock_shutdown(**kwargs):
14061401
mock_shutdown,
14071402
)
14081403

1409-
offline_indices = [
1410-
i for i, (s, _) in enumerate(status_calls) if s == ExporterStatus.OFFLINE
1404+
failed_indices = [
1405+
i for i, (s, _) in enumerate(status_calls) if s == ExporterStatus.BEFORE_LEASE_HOOK_FAILED
14111406
]
1412-
assert len(offline_indices) > 0, (
1413-
f"Expected OFFLINE status before shutdown, got: {status_calls}"
1407+
assert len(failed_indices) > 0, (
1408+
f"Expected BEFORE_LEASE_HOOK_FAILED status before shutdown, got: {status_calls}"
14141409
)
14151410
assert shutdown_called_at_index is not None, "shutdown was never called"
1416-
assert offline_indices[0] < shutdown_called_at_index, (
1417-
f"OFFLINE (index {offline_indices[0]}) must be reported before "
1411+
assert failed_indices[0] < shutdown_called_at_index, (
1412+
f"BEFORE_LEASE_HOOK_FAILED (index {failed_indices[0]}) must be reported before "
14181413
f"shutdown (index {shutdown_called_at_index}). Statuses: {status_calls}"
14191414
)
1415+
offline_statuses = [s for s, _ in status_calls if s == ExporterStatus.OFFLINE]
1416+
assert len(offline_statuses) == 0, (
1417+
f"OFFLINE should NOT be reported by hook handler (shutdown path handles it), "
1418+
f"got: {status_calls}"
1419+
)
14201420

14211421
async def test_after_hook_exit_reports_offline_before_shutdown(self, lease_scope) -> None:
14221422
"""When afterLease hook fails with on_failure=exit, OFFLINE must be

0 commit comments

Comments
 (0)