feat(host-agent): add single-instance lock to prevent duplicate-process dispatch races
Acquire an exclusive, non-blocking filelock on the identity state directory as the first action of create_application(), before resolve_host_identity() or any enrollment/heartbeat side effect. A second process against the same identity_path exits immediately with InstanceAlreadyRunningError naming the lock path; the lock releases automatically on any process exit (including SIGKILL) via OS-level advisory locking, and explicitly during run_async()'s shutdown finally block. filelock is promoted from transitive to direct dependency (version unchanged at 3.29.7). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -6,6 +6,7 @@ from contextlib import suppress
|
||||
from datetime import UTC, datetime, timedelta
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
from cloud.internal_api.models import (
|
||||
AssignmentModel,
|
||||
@@ -16,6 +17,7 @@ from device.manager import DeviceManager
|
||||
from host_agent.app import HostAgentApplication, create_application
|
||||
from host_agent.config import HostAgentConfig
|
||||
from host_agent.identity import HostIdentityStore
|
||||
from host_agent.instance_lock import InstanceAlreadyRunningError
|
||||
from storage.device_config import DeviceConfigStore
|
||||
|
||||
|
||||
@@ -576,3 +578,123 @@ def test_run_async_starts_supervisor_before_first_heartbeat_connect() -> None:
|
||||
assert events.index("supervisor-stop") < events.index("closed")
|
||||
|
||||
asyncio.run(scenario())
|
||||
|
||||
|
||||
def test_second_create_application_against_held_lock_raises_before_enrollment(
|
||||
tmp_path, monkeypatch
|
||||
) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
identity_path = tmp_path / "host_identity.json"
|
||||
config = HostAgentConfig(
|
||||
control_plane_url="https://control.example",
|
||||
host_id="host-a",
|
||||
token="secret",
|
||||
identity_path=identity_path,
|
||||
)
|
||||
|
||||
class TrackingEnrollmentClient:
|
||||
def __init__(self) -> None:
|
||||
self.config = config
|
||||
self.calls: list[str] = []
|
||||
|
||||
def enroll_host(self, **payload):
|
||||
self.calls.append("host")
|
||||
return HostEnrollmentResponse(host_id="host-a")
|
||||
|
||||
def enroll_device(self, **payload):
|
||||
self.calls.append("device")
|
||||
return DeviceEnrollmentResponse(device_id="device-a")
|
||||
|
||||
def close(self) -> None:
|
||||
return None
|
||||
|
||||
first_client = TrackingEnrollmentClient()
|
||||
first_app = create_application(
|
||||
config=config,
|
||||
identity_store=HostIdentityStore(identity_path),
|
||||
enrollment_client=first_client, # type: ignore[arg-type]
|
||||
)
|
||||
try:
|
||||
second_client = TrackingEnrollmentClient()
|
||||
with pytest.raises(InstanceAlreadyRunningError) as info:
|
||||
create_application(
|
||||
config=config,
|
||||
identity_store=HostIdentityStore(identity_path),
|
||||
enrollment_client=second_client, # type: ignore[arg-type]
|
||||
)
|
||||
assert info.value.lock_path == identity_path.parent / "host_agent.lock"
|
||||
assert second_client.calls == []
|
||||
finally:
|
||||
asyncio.run(first_app.client.aclose())
|
||||
|
||||
|
||||
def test_create_application_with_independent_identity_paths_coexist(
|
||||
tmp_path, monkeypatch
|
||||
) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
config_a = HostAgentConfig(
|
||||
control_plane_url="https://control.example",
|
||||
host_id="host-a",
|
||||
token="secret",
|
||||
identity_path=tmp_path / "identity-a" / "host_identity.json",
|
||||
)
|
||||
config_b = HostAgentConfig(
|
||||
control_plane_url="https://control.example",
|
||||
host_id="host-b",
|
||||
token="secret",
|
||||
identity_path=tmp_path / "identity-b" / "host_identity.json",
|
||||
)
|
||||
|
||||
app_a = create_application(config=config_a, manager=DeviceManager())
|
||||
try:
|
||||
app_b = create_application(config=config_b, manager=DeviceManager())
|
||||
asyncio.run(app_b.client.aclose())
|
||||
finally:
|
||||
asyncio.run(app_a.client.aclose())
|
||||
|
||||
|
||||
def test_lock_released_after_run_async_allows_restart(tmp_path, monkeypatch) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
identity_path = tmp_path / "host_identity.json"
|
||||
config = HostAgentConfig(
|
||||
control_plane_url="https://control.example",
|
||||
host_id="host-a",
|
||||
token="secret",
|
||||
identity_path=identity_path,
|
||||
)
|
||||
|
||||
first_app = create_application(config=config, manager=DeviceManager())
|
||||
|
||||
class StoppedClient:
|
||||
async def claim(self):
|
||||
raise AssertionError("polling must not start")
|
||||
|
||||
async def aclose(self):
|
||||
return None
|
||||
|
||||
class FastHeartbeat:
|
||||
async def run(self, stop):
|
||||
await stop.wait()
|
||||
|
||||
async def sync_once(self):
|
||||
return None
|
||||
|
||||
class IdleProcessor:
|
||||
async def process(self, assignment):
|
||||
raise AssertionError("no assignment expected")
|
||||
|
||||
def request_stop(self):
|
||||
return None
|
||||
|
||||
first_app.client = StoppedClient() # type: ignore[assignment]
|
||||
first_app.heartbeat = FastHeartbeat() # type: ignore[assignment]
|
||||
first_app.processor = IdleProcessor() # type: ignore[assignment]
|
||||
|
||||
stop = asyncio.Event()
|
||||
stop.set()
|
||||
asyncio.run(first_app.run_async(stop))
|
||||
|
||||
# Lock must be free now; a fresh create_application against the same
|
||||
# identity_path must succeed (simulates a clean restart).
|
||||
second_app = create_application(config=config, manager=DeviceManager())
|
||||
asyncio.run(second_app.client.aclose())
|
||||
|
||||
@@ -5,6 +5,7 @@ import sys
|
||||
import pytest
|
||||
|
||||
from host_agent import cli
|
||||
from host_agent.instance_lock import InstanceAlreadyRunningError
|
||||
from host_agent.local_account import LocalAccountStore
|
||||
|
||||
|
||||
@@ -60,7 +61,9 @@ def test_existing_account_fast_path_skips_prompting(monkeypatch, tmp_path) -> No
|
||||
assert captured["config"].display_name == "operator"
|
||||
|
||||
|
||||
def test_interactive_first_run_prompts_and_creates_account(monkeypatch, tmp_path) -> None:
|
||||
def test_interactive_first_run_prompts_and_creates_account(
|
||||
monkeypatch, tmp_path
|
||||
) -> None:
|
||||
_set_env(monkeypatch, tmp_path)
|
||||
monkeypatch.setattr(sys.stdin, "isatty", lambda: True)
|
||||
inputs = iter(["operator"])
|
||||
@@ -120,3 +123,26 @@ def test_setup_subcommand_refuses_overwrite_without_confirmation(
|
||||
cli.main(["setup"])
|
||||
|
||||
assert store.load() == original
|
||||
|
||||
|
||||
def test_duplicate_instance_exits_with_clear_error(
|
||||
monkeypatch, tmp_path, capsys
|
||||
) -> None:
|
||||
_set_env(monkeypatch, tmp_path)
|
||||
LocalAccountStore(tmp_path / "account.json").create(
|
||||
"operator", "correct horse battery staple"
|
||||
)
|
||||
lock_path = tmp_path / "identity_dir" / "host_agent.lock"
|
||||
|
||||
def raising_create_application(*, config=None, **kwargs):
|
||||
raise InstanceAlreadyRunningError(lock_path)
|
||||
|
||||
monkeypatch.setattr(cli, "create_application", raising_create_application)
|
||||
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
cli.main([])
|
||||
|
||||
assert exc_info.value.code == 1
|
||||
err = capsys.readouterr().err
|
||||
assert "another Host Agent instance" in err
|
||||
assert str(lock_path) in err
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from host_agent.instance_lock import (
|
||||
InstanceAlreadyRunningError,
|
||||
InstanceLock,
|
||||
)
|
||||
|
||||
|
||||
def test_acquire_succeeds_when_free(tmp_path) -> None:
|
||||
lock = InstanceLock(tmp_path)
|
||||
lock.acquire()
|
||||
assert lock.lock_path == tmp_path / "host_agent.lock"
|
||||
lock.release()
|
||||
|
||||
|
||||
def test_acquire_raises_when_already_held(tmp_path) -> None:
|
||||
first = InstanceLock(tmp_path)
|
||||
first.acquire()
|
||||
try:
|
||||
second = InstanceLock(tmp_path)
|
||||
with pytest.raises(InstanceAlreadyRunningError) as info:
|
||||
second.acquire()
|
||||
assert info.value.lock_path == tmp_path / "host_agent.lock"
|
||||
assert "another Host Agent instance" in str(info.value)
|
||||
finally:
|
||||
first.release()
|
||||
|
||||
|
||||
def test_release_then_reacquire_succeeds(tmp_path) -> None:
|
||||
lock = InstanceLock(tmp_path)
|
||||
lock.acquire()
|
||||
lock.release()
|
||||
# Same handle can reacquire after release.
|
||||
lock.acquire()
|
||||
lock.release()
|
||||
# A fresh handle against the same path also succeeds.
|
||||
other = InstanceLock(tmp_path)
|
||||
other.acquire()
|
||||
other.release()
|
||||
|
||||
|
||||
def test_release_is_idempotent_and_safe_before_acquire(tmp_path) -> None:
|
||||
lock = InstanceLock(tmp_path)
|
||||
# Releasing before acquire must be a no-op, not raise.
|
||||
lock.release()
|
||||
lock.acquire()
|
||||
lock.release()
|
||||
lock.release()
|
||||
|
||||
|
||||
def test_independent_paths_never_contend(tmp_path) -> None:
|
||||
dir_a = tmp_path / "identity-a"
|
||||
dir_b = tmp_path / "identity-b"
|
||||
lock_a = InstanceLock(dir_a)
|
||||
lock_a.acquire()
|
||||
try:
|
||||
lock_b = InstanceLock(dir_b)
|
||||
lock_b.acquire()
|
||||
lock_b.release()
|
||||
finally:
|
||||
lock_a.release()
|
||||
|
||||
|
||||
def test_context_manager_releases_on_exit(tmp_path) -> None:
|
||||
with InstanceLock(tmp_path):
|
||||
second = InstanceLock(tmp_path)
|
||||
with pytest.raises(InstanceAlreadyRunningError):
|
||||
second.acquire()
|
||||
# After the context exits, a fresh handle can acquire.
|
||||
InstanceLock(tmp_path).acquire()
|
||||
|
||||
|
||||
def test_state_directory_is_created_if_missing(tmp_path) -> None:
|
||||
nested = tmp_path / "nested" / "state"
|
||||
lock = InstanceLock(nested)
|
||||
assert nested.exists()
|
||||
lock.acquire()
|
||||
lock.release()
|
||||
Reference in New Issue
Block a user