Skip to content

Commit 08a386c

Browse files
dotsdlclaudemikemhenry
authored
Improve robustness of service termination (#503)
* Make ComputeManager and compute service sleeps interruptible ComputeManager used a plain time.sleep() between cycles, so stop() (and SIGINT) did not take effect until the full sleep_interval elapsed. It now sleeps via the InterruptableSleep functor, woken by stop(). SynchronousComputeService already constructed an InterruptableSleep and interrupted it in stop(), but never slept through it: every sleep used plain time.sleep(), leaving the mechanism (and the SleepInterrupted handler in start()) as dead code. Its cycle sleeps now go through int_sleep, and start() clears the event so a stopped service can be cleanly restarted. Also removed the unused sched.scheduler instances and corrected the InterruptableSleep docstring, which described a scheduler integration that was never wired up. Adds tests asserting stop() promptly wakes a sleeping manager/service. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add news fragment for #503 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Extract InterruptableSleep to alchemiscale.sleep; make strategist sleep interruptible Moves InterruptableSleep/SleepInterrupted out of compute.service into a new dependency-free alchemiscale.sleep module, now that a third consumer (the strategist) needs them from a different subpackage. They are re-exported from compute.service for backwards compatibility, and compute.manager imports from the canonical location. StrategistService now sleeps between cycles via the InterruptableSleep functor (interrupted by stop()), so termination takes effect promptly instead of waiting out the sleep interval. No KeyboardInterrupt is raised: the cooperative _stop checks and ProcessPoolExecutor teardown are preserved, since raising into the pool-managing thread risks orphaning child processes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add install_stop_handlers helper and use it in the compute service CLI Adds alchemiscale.compute.signals.install_stop_handlers, a small reusable helper that wires SIGHUP/SIGINT/SIGTERM to a service's stop(). Signal disposition is process-global and main-thread-only, so it belongs at the entry point; the helper keeps each CLI from re-implementing (and forgetting) the wiring. The compute service 'synchronous' CLI command now uses it. Also updates the news fragment to cover the broader service-termination work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * DO NOT MERGE: skip 3 new threaded tests to bisect CI hang Marks the three new tests that spawn services in background threads with `@pytest.mark.skip` to isolate whether they are the cause of the 6h hang at `test_validate_network_nonself` on Python 3.11/3.13. If CI goes green on 3.11/3.13 with this commit, the trigger lives in one of these tests (or the threads they leave behind on failure). To be reverted once root cause is identified. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * bisect: re-enable test_manager_interruptible_sleep Step 2 of bisect: previous push (skipping all three new threaded tests) went green on 3.11/3.13 in ~16m, confirming the hang is triggered by one of those three tests. Re-enabling the compute-manager test first because it spawns a non-daemon thread and exercises the most stateful service (Neo4j registration, polling). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * bisect: confirm test_manager_interruptible_sleep is culprit; re-skip Bisect step 2 (re-enabling test_manager_interruptible_sleep) reproduced the 3.11/3.13 hang while 3.12 passed cleanly. The test itself runs to completion on gw0 in ~4.5s, but gw0 stops making progress immediately after — strongly suggesting an issue with what the test leaves behind, not what it asserts. See log analysis in PR thread. Re-skipping so CI is green and the remaining bisect work (strategist and compute-service threaded tests) can proceed without re-burning 6h. * Fix test_manager_interruptible_sleep CI hang: use n4js_fresh With n4js_preloaded, the manager's first cycle saw num_tasks > 0 and called create_compute_services -> multiprocessing.Process(...). Because integration tests force fork as the global start method (see running_service in alchemiscale/tests/integration/utils.py) and this test runs manager.start() in a background thread, that Process forks a multi-threaded Python worker. Locks held by other threads at the time of fork are inherited as held-with-no-owner in the child, which then deadlocks on its first acquire (typically the logging lock). The deadlocked child hung the xdist worker on Python 3.11 and 3.13; 3.12 happened to miss the trip. n4js_fresh has no preloaded tasks, so cycle() reports num_tasks == 0 and no Process is spawned. The test still exercises the actual code path it cares about (start() entering its interruptible sleep, stop() waking it). Also drops the @pytest.mark.skip added during bisection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix test_manager_interruptible_sleep: monkeypatch create_compute_services Previous attempt swapped n4js_preloaded for n4js_fresh to avoid the fork-from-thread deadlock, but n4js_fresh leaves Neo4j empty including the compute identities (n4js_preloaded writes those via create_credentialed_entity), so the manager's registration POST hit /token -> 500 and start() never reached its sleep. Keep n4js_preloaded (credentials are in place), but replace manager.create_compute_services with a no-op so cycle() does not fork a Process. The test still exercises register / get_instruction / update_status / int_sleep / stop / deregister, which is what it's about. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Re-enable test_start_interruptible_sleep and test_service_interruptible_sleep Both were temporarily skipped during the PR #503 bisection. They are predicted innocent under the fork-from-thread hypothesis: - test_start_interruptible_sleep (SynchronousComputeService): the service runs tasks in-thread, never forks a multiprocessing.Process during its cycle. The test thread is also daemon=True. - test_service_interruptible_sleep (StrategistService): when the strategist does spawn subprocesses, it uses mp.get_context("spawn") (see strategist/service.py:589), which avoids the fork-from-thread pitfall entirely. Drop the skip markers and let CI confirm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Make SynchronousComputeService.heartbeat interruptible Previously heartbeat() slept with plain time.sleep(self.heartbeat_interval), which stop() could not wake. The daemon heartbeat thread therefore stayed alive — and the worker stayed multi-threaded — for up to heartbeat_interval seconds after the main loop had already torn down. In production this means SIGTERM on the CLI service has a long tail where fork-from-a-multi-threaded-process is possible (any later multiprocessing call inherits other threads' locks as held-with-no-owner -> child deadlock). In tests this is exactly what tripped up test_compute_api / test_compute_synchronous on 3.13 after test_start_interruptible_sleep ran on the same xdist worker: those tests fork a server via running_service, which deadlocked because the worker still had the heartbeat thread alive. Python flagged it explicitly: DeprecationWarning: This process (pid=...) is multi-threaded, use of fork() may lead to deadlocks in the child. Switch heartbeat to the same int_sleep / SleepInterrupted dance the main loop uses. stop() now wakes both threads in one call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review feedback - install_stop_handlers: add raise_keyboard_interrupt kwarg (default True) so the strategist CLI can opt out. The strategist's stop() triggers a ProcessPoolExecutor shutdown that must not be interrupted partway through. Strategist CLI now uses install_stop_handlers instead of an inline signal.signal loop, which removes the last in-tree use of the signal module from cli.py. New unit test covers the raise_keyboard_interrupt=False path. - compute/service.py: document the fact that self.int_sleep is shared between the main loop and the heartbeat thread, with the gotcha that any future split must interrupt both from stop() --- otherwise the heartbeat thread keeps the worker multi-threaded until its sleep naturally expires (the bug fixed in be8fc49). - Rename test_start_interruptible_sleep -> test_service_interruptible_sleep for naming parity with test_manager_interruptible_sleep and the strategist's test_service_interruptible_sleep. - Replace direct attribute assignment in the two test monkeypatches with monkeypatch.setattr, the idiomatic form in this codebase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Adapt test_manager_interruptible_sleep monkeypatch to #502 signature PR #502 ("Move autoscaling sizing logic into ComputeManager base", merged to main as 7dfb3de) changed ComputeManager.create_compute_services from ``(self, data)`` to ``(self, data, target)``. Our monkeypatch was still the one-arg lambda; once the post-#502 ``cycle()`` calls it with two positional arguments, TypeError fires, ``start()``'s ``except Exception`` catches it and ERROR-deregisters, and the loop exits before logging "Sleeping for ...". The test's 30s deadline then trips with "manager never reached its sleep". Switch the no-op to ``lambda data, target: 0`` so the call signature matches and cycle proceeds normally into the interruptible sleep that the test is actually exercising. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * do min fix + add regression tests * use a context manager instead * run black * Harden heartbeat() against per-beat exceptions The pre-refactor SynchronousComputeService.start() papered over a fragility with an inline check at the top of each main-loop iteration: if the heartbeat thread had died, it was resurrected with a fresh Thread before the next cycle. Mike's _running() refactor (30747fd) drops that check, which is correct structurally but exposes the underlying problem: client.heartbeat() can raise after exhausting its retry policy (sustained outage, auth-token expiry, persistent 5xx), and an uncaught exception in heartbeat() would silently kill the daemon thread while the main loop kept happily claiming and executing tasks. The cleanest fix is at the source rather than restoring the resurrection: catch per-beat failures inside heartbeat(), log them, and try again on the next interval. The thread now only exits on stop() (via SleepInterrupted) or process teardown. No resurrection check needed in start(). Regression test monkeypatches beat() to raise once, runs heartbeat() in a thread for a few intervals, and asserts (a) the thread is still alive, (b) beat was called more than once, (c) the failure was logged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Consistency changes to manager for parity with compute service --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Mike Henry <11765982+mikemhenry@users.noreply.github.com>
1 parent b146148 commit 08a386c

11 files changed

Lines changed: 725 additions & 133 deletions

File tree

alchemiscale/cli.py

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import click
88
import yaml
99
import json
10-
import signal
1110

1211
from .security.models import (
1312
CredentialedEntity,
@@ -382,6 +381,7 @@ def synchronous(config_file, name, compute_manager_id):
382381
from alchemiscale.models import Scope
383382
from alchemiscale.compute.service import SynchronousComputeService
384383
from alchemiscale.compute.settings import ComputeServiceSettings
384+
from alchemiscale.compute.signals import install_stop_handlers
385385

386386
params = yaml.safe_load(config_file)
387387

@@ -393,14 +393,8 @@ def synchronous(config_file, name, compute_manager_id):
393393

394394
service = SynchronousComputeService(ComputeServiceSettings(**params))
395395

396-
# add signal handling
397-
for signame in {"SIGHUP", "SIGINT", "SIGTERM"}:
398-
399-
def stop(*args, **kwargs):
400-
service.stop()
401-
raise KeyboardInterrupt()
402-
403-
signal.signal(getattr(signal, signame), stop)
396+
# install handlers so SIGHUP/SIGINT/SIGTERM stop the service cleanly
397+
install_stop_handlers(service)
404398

405399
try:
406400
service.start()
@@ -548,6 +542,7 @@ def strategist(config_file):
548542
from alchemiscale.models import Scope
549543
from alchemiscale.strategist.service import StrategistService
550544
from alchemiscale.strategist.settings import StrategistSettings
545+
from alchemiscale.compute.signals import install_stop_handlers
551546

552547
params = yaml.safe_load(config_file)
553548

@@ -556,18 +551,12 @@ def strategist(config_file):
556551

557552
service = StrategistService(StrategistSettings(**params))
558553

559-
# add signal handling
560-
for signame in {"SIGHUP", "SIGINT", "SIGTERM"}:
561-
562-
def stop(*args, **kwargs):
563-
service.stop()
564-
565-
signal.signal(getattr(signal, signame), stop)
554+
# install handlers so SIGHUP/SIGINT/SIGTERM stop the service cleanly.
555+
# do *not* raise KeyboardInterrupt --- the strategist's stop() triggers a
556+
# ProcessPoolExecutor shutdown that must not be interrupted partway through.
557+
install_stop_handlers(service, raise_keyboard_interrupt=False)
566558

567-
try:
568-
service.start()
569-
except KeyboardInterrupt:
570-
pass
559+
service.start()
571560

572561

573562
@cli.group(help="Subcommands for managing identities")

alchemiscale/compute/manager.py

Lines changed: 69 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
"""
66

77
from abc import abstractmethod
8+
from contextlib import contextmanager
89
import logging
910
import time
1011

@@ -18,6 +19,7 @@
1819
AlchemiscaleComputeManagerClientError,
1920
)
2021
from .settings import ComputeManagerSettings, ComputeServiceSettings
22+
from ..sleep import InterruptableSleep, SleepInterrupted
2123

2224

2325
class ComputeManager:
@@ -44,6 +46,7 @@ def __init__(
4446
)
4547

4648
self._stop = False
49+
self.int_sleep = InterruptableSleep()
4750

4851
logger = logging.getLogger("AlchemiscaleComputeManager")
4952
logger.setLevel(self.settings.loglevel)
@@ -75,34 +78,73 @@ def _register(self, steal=False):
7578
def _deregister(self):
7679
self.client.deregister(self.compute_manager_id)
7780

78-
def start(self, max_cycles: int | None = None, steal=False):
79-
self.logger.info(f"Starting up compute manager '{self.settings.name}'")
80-
self._register(steal=steal)
81-
self.logger.info(f"Registered compute manager '{self.compute_manager_id}'")
82-
self._stop = False
81+
@contextmanager
82+
def _running(self, steal=False):
83+
"""Register this compute manager for the lifetime of the context.
84+
85+
This guarantees that if anything interrupts startup after registration,
86+
including int_sleep.clear(), the manager is still deregistered.
87+
"""
88+
registered = False
89+
8390
try:
84-
count = 0
85-
self.logger.info("Starting main loop")
86-
while self.cycle():
87-
count += 1
88-
if max_cycles and count >= max_cycles:
89-
self.logger.info("Reached maximum number of cycles")
90-
break
91-
self.logger.info(f"Sleeping for {self.settings.sleep_interval} seconds")
92-
time.sleep(self.settings.sleep_interval)
93-
except Exception as e:
94-
self.logger.error(f"Unknown exception raised: '{str(e)}'")
95-
self.logger.info(f"Updating manager status to 'ERROR'")
96-
self.client.update_status(
97-
self.compute_manager_id, ComputeManagerStatus.ERROR, detail=repr(e)
98-
)
99-
raise e
100-
except KeyboardInterrupt:
101-
self.logger.info("Caught SIGINT/Keyboard interrupt.")
91+
self._register(steal=steal)
92+
registered = True
93+
94+
self.logger.info(f"Registered compute manager '{self.compute_manager_id}'")
95+
96+
self._stop = False
97+
self.int_sleep.clear()
98+
99+
yield
100+
102101
finally:
103-
self.logger.info(f"Deregistering '{self.compute_manager_id}'")
104-
self._deregister()
105-
self.logger.info(f"Deregistration successful")
102+
if registered:
103+
self.logger.info(f"Deregistering '{self.compute_manager_id}'")
104+
105+
# kept here in case we add additional cleanup to stop later, such as other threads
106+
self.stop()
107+
108+
self._deregister()
109+
self.logger.info("Deregistration successful")
110+
111+
def start(self, max_cycles: int | None = None, steal=False):
112+
self.logger.info(f"Starting up compute manager '{self.settings.name}'")
113+
114+
with self._running(steal=steal):
115+
try:
116+
count = 0
117+
self.logger.info("Starting main loop")
118+
119+
while self.cycle():
120+
count += 1
121+
122+
if max_cycles and count >= max_cycles:
123+
self.logger.info("Reached maximum number of cycles")
124+
break
125+
126+
self.logger.info(
127+
f"Sleeping for {self.settings.sleep_interval} seconds"
128+
)
129+
self.int_sleep(self.settings.sleep_interval)
130+
131+
except SleepInterrupted:
132+
self.logger.info("Compute manager stopping.")
133+
134+
except KeyboardInterrupt:
135+
self.logger.info("Caught SIGINT/Keyboard interrupt.")
136+
137+
except Exception as e:
138+
self.logger.error(f"Unknown exception raised: '{str(e)}'")
139+
self.logger.info("Updating manager status to 'ERROR'")
140+
141+
self.client.update_status(
142+
self.compute_manager_id,
143+
ComputeManagerStatus.ERROR,
144+
detail=repr(e),
145+
)
146+
147+
raise
106148

107149
@abstractmethod
108150
def create_compute_services(self, data: dict, target: int) -> int:
@@ -189,6 +231,7 @@ def _compute_jobs_to_create(self, num_tasks: int, num_active_services: int) -> i
189231
return jobs or 1
190232

191233
def stop(self):
234+
self.int_sleep.interrupt()
192235
self._stop = True
193236

194237
def cycle(self) -> bool:

0 commit comments

Comments
 (0)