Proposal, design, spec, and tasks for the per-installation exclusive instance lock. 15/16 tasks complete; only manual real-environment verification (5.4) remains, with semantics covered by unit tests in test_app.py and test_instance_lock.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -0,0 +1,2 @@
|
||||
schema: spec-driven
|
||||
created: 2026-07-14
|
||||
@@ -0,0 +1,3 @@
|
||||
# host-agent-single-instance-lock
|
||||
|
||||
Prevent duplicate host-agent processes under one host identity from racing on assignment claims
|
||||
@@ -0,0 +1,67 @@
|
||||
## Context
|
||||
|
||||
- `identity_path` (`host_agent/config.py:22`, default `Path("tasks/host_identity.json")`, overridable via `HOST_AGENT_IDENTITY_PATH`) is persisted through `HostIdentityStore` (`host_agent/identity.py`). Its *parent directory* is already the de facto per-installation state directory: `create_application()` derives `host_console_history.sqlite3` (`app.py:178`) and `host_governance_policy.json` (`app.py:229`) from `resolved_config.identity_path.parent`.
|
||||
- `create_application()` (`app.py:145-172`) is fully synchronous and front-loads every side-effecting startup call: `resolve_host_identity()` (may enroll a new host with the control plane) then `_configured_device_manager()` (enrolls every locally configured device). Both are network calls against the same control plane the claim/heartbeat loops later use.
|
||||
- A repo-wide search confirms no pidfile, lockfile, or process-uniqueness mechanism exists anywhere in `apps/device-host-agent` today.
|
||||
- This repo has no CI configuration (no `.github/workflows`); tests are run locally by whichever machine the developer/operator uses. The primary development machine observed in this session is Windows; the documented production target is macOS (`docs/MACOS_IPHONE_SETUP.md`). Any locking mechanism must be correct on both without a CI matrix to catch a platform-specific regression.
|
||||
- `filelock` 3.29.7 is already present in `uv.lock` as a transitive dependency (pulled in by unrelated ML-related packages), but is not a direct dependency of any workspace member today.
|
||||
- The recently archived `host-agent-dependency-supervisor` (`apps/device-host-agent/host_agent/dependency_supervisor.py`) spawns Appium/Runtime child processes via `subprocess.Popen` and is architecturally adjacent — also concerned with Host Agent process lifecycle. Its design Decision 5 explicitly chose stdlib-only `subprocess.Popen` over adding a dependency for spawning. That precedent is weighed below (Decision 2) but not followed as-is.
|
||||
- `apps/device-host-agent/tests/test_app.py` was audited for existing `create_application()` call patterns: all 9 call sites are one-per-test-function (no test constructs two applications against the same `identity_path` in one process today), and each uses a pytest `tmp_path`-scoped identity path. A new duplicate-instance test therefore needs to keep the first `HostAgentApplication`'s lock handle alive across the assertion — see Decision 5.
|
||||
|
||||
## Goals / Non-Goals
|
||||
|
||||
**Goals:**
|
||||
- Guarantee at most one Host Agent process is ever actively participating in the claim/heartbeat protocol for a given `identity_path` state directory at a time.
|
||||
- Fail fast and loudly when a duplicate instance is detected, before any control-plane side effect (enrollment, heartbeat, claim) occurs.
|
||||
- Require zero manual cleanup after an unclean shutdown (crash, `kill -9`, power loss).
|
||||
- Work correctly on both macOS (production target) and Windows (a real development machine for this repo) with no CI matrix to lean on.
|
||||
|
||||
**Non-Goals:**
|
||||
- No distributed/cross-machine locking. This only prevents duplicate *local* processes for one installation; it does not address two different physical hosts being misconfigured with the same `host_id` (a separate concern, partially already handled by device-identity-ownership-conflict behavior in `device-pool`).
|
||||
- No change to the claim/heartbeat/lease protocol itself (`host-agent-protocol` is unmodified).
|
||||
- No general-purpose process supervision or auto-restart. This is an exclusivity guard on the Host Agent's *own* process, not a supervisor of *other* processes (contrast with `host-agent-dependency-supervisor`).
|
||||
- No reliance on the `console_port` bind as the exclusivity mechanism — that conflict is real but surfaces too late (deep inside `run_async()`, after enrollment/heartbeat side effects have already started) and is silently bypassed if two instances ever end up configured with different ports.
|
||||
|
||||
## Decisions
|
||||
|
||||
**1. Lock file location: `identity_path.parent / "host_agent.lock"`, no new config field.**
|
||||
Matches the existing convention of colocating per-installation auxiliary files next to `identity_path` (`app.py:178,229`). A lock scoped to a directory independent of the identity it protects would be a foot-gun if the two were ever configured to diverge, and a dedicated env var would just duplicate information `HOST_AGENT_IDENTITY_PATH` already carries.
|
||||
|
||||
**2. Use `filelock` (new direct dependency) rather than hand-rolled `fcntl`/`msvcrt` branching.**
|
||||
Alternative considered: a small in-repo module branching on `sys.platform`, calling `fcntl.flock(fd, LOCK_EX | LOCK_NB)` on POSIX and `msvcrt.locking(fd, LK_NBLCK, 1)` on Windows — matching this repo's general preference for avoiding new dependencies (`host-agent-dependency-supervisor` Decision 5).
|
||||
Rejected in favor of `filelock` because:
|
||||
- This repo has no CI matrix, so a platform-specific bug in whichever branch isn't the implementer's local OS could ship unverified. Concretely, the production target (macOS/`fcntl`) differs from a development machine already in use for this repo (Windows/`msvcrt`).
|
||||
- `filelock` is small, pure-Python, has zero dependencies of its own, is already resolved in this repo's `uv.lock` transitively (so promoting it to a direct dependency of `device-host-agent` adds no new package to the lock, only a new dependency edge), and is widely used (`virtualenv`, `tox`, `huggingface_hub` all depend on it).
|
||||
- Getting advisory locking subtly wrong (a missed non-blocking flag, a probe/acquire race) would reintroduce exactly the class of bug this change exists to eliminate. This is a case where "don't reinvent it" outweighs the general new-dependency-avoidance default — unlike `subprocess.Popen`, which stdlib already handles with no cross-platform pitfalls.
|
||||
|
||||
**3. Acquire the lock as the first action inside `create_application()`, before `resolve_host_identity()`.**
|
||||
`startup_config.identity_path` is already resolved from config/env at function entry, so this is the earliest point with the information needed, and it precedes every side-effecting call `create_application()` makes. A duplicate instance is rejected before it can enroll, heartbeat, or claim — not merely before it can serve console traffic.
|
||||
|
||||
**4. Non-blocking acquisition; fail immediately rather than wait or retry.**
|
||||
Alternative considered: block/retry with backoff until the lock frees up, treating a duplicate launch as "wait your turn." Rejected because a Host Agent is normally started by an operator or a process supervisor expecting an immediate up/down signal; silently blocking would look identical to a hang and would mask the actual problem (something didn't clean up the previous instance) instead of surfacing it. `filelock.FileLock(path, timeout=0)` raises `Timeout` immediately if already held; `create_application()` translates this into a clear, named "duplicate instance" error.
|
||||
|
||||
**5. The lock handle is owned by `HostAgentApplication` for its full lifetime, released in `run_async()`'s existing `finally` block.**
|
||||
The acquired `filelock.FileLock` is stored as a field on the returned `HostAgentApplication` (not a bare local variable inside `create_application()`), so it stays held for as long as the object is alive, and is explicitly released during the existing graceful-shutdown `finally` block in `run_async()` (`app.py:88-111`) rather than left to eventual garbage collection. Two consequences:
|
||||
- A clean restart doesn't need to wait on GC/OS cleanup timing — the next process can acquire the lock immediately after graceful shutdown completes.
|
||||
- A test can hold a first instance's lock open (by keeping its `HostAgentApplication`/lock reference alive) and assert that a second `create_application()` call against the same `identity_path` raises — this is the concrete new test called out in tasks.md.
|
||||
|
||||
**6. Release also relies on OS-level advisory-lock semantics, not an explicit-unlock-only or pidfile-liveness-check design.**
|
||||
Even without the explicit release in Decision 5, the lock is released automatically when the holding process's file descriptor closes for any reason — clean exit, unhandled exception, `SIGKILL`, crash — because that's a property `flock`/`LockFileEx` the kernel enforces, not application code. This sidesteps the entire "stale pidfile after a crash" problem class that a hand-rolled `os.path.exists()`-based lock or a PID-liveness-check design would otherwise need to solve (including PID-reuse races and cross-platform "is this PID still alive" checks).
|
||||
- Subprocess-inheritance note: `dependency_supervisor.py` spawns Appium/Runtime via `subprocess.Popen`. Per PEP 446, file descriptors opened by Python (including `filelock`'s underlying `os.open`) are non-inheritable by default unless explicitly passed via `pass_fds`, which this change does not do — so a spawned child cannot accidentally keep the lock held after the parent Host Agent process exits. This is confirmed as a non-issue rather than left open, since it follows directly from a documented Python default already in effect for every other file descriptor this codebase opens.
|
||||
|
||||
## Risks / Trade-offs
|
||||
|
||||
- **[Risk] A hung-but-not-dead process holds the lock and blocks a legitimate restart.** → Mitigation: intentional, matches Goal 2 ("fail fast and loudly"). A hung process should be diagnosed and killed by the operator/supervisor, not silently bypassed — the point of this change is to stop pretending a second instance is safe to run. The rejection error names the lock file path so an operator can inspect what's holding it (e.g. `lsof`, `Get-Process`).
|
||||
- **[Risk] New direct dependency (`filelock`).** → Mitigation: already resolved in `uv.lock` transitively today (no new package added to the lock, only a new direct edge), the package itself is small and dependency-free, and is widely used in the Python ecosystem. See Decision 2 for the full rationale on why this outweighs the stdlib-only precedent from `host-agent-dependency-supervisor`.
|
||||
- **[Risk] This addresses the symptom (duplicate processes racing) rather than a confirmed single trigger.** → Accepted: the incident diagnosis could not confirm, without access to the affected Mac mini, whether the trigger was a deploy-triggered restart race, an orphaned process, or operator error. This change makes all three safe by construction rather than requiring the exact trigger to be identified first. If host process history later points to a specific deploy-tooling bug (e.g. a supervisor not waiting for the old process to exit), that would be a separate, complementary fix outside this change's scope.
|
||||
- **[Trade-off] Lock scope is per-`identity_path`, not machine-global.** → Accepted per Non-Goals: a machine could legitimately run multiple independent installations pointed at different identity paths; this is also simpler to test, since parallel test runs never share lock state unless they explicitly share a path.
|
||||
|
||||
## Migration Plan
|
||||
|
||||
- Fully additive: existing single-instance deployments are unaffected beyond a new lock file appearing next to `host_identity.json` (inside the already-untracked `tasks/`-style state directory).
|
||||
- No configuration changes required to adopt — the lock is unconditional, not opt-in/opt-out, since there is no legitimate scenario where running two instances against the same identity is desirable.
|
||||
- Rollback: revert the code change; there is no persistent state or schema to unwind (the lock file is inert once the code no longer checks it).
|
||||
|
||||
## Open Questions
|
||||
|
||||
- Should a rejected duplicate-launch attempt also be surfaced somewhere operator-visible beyond process logs (e.g. recorded by the console-owning first instance via `ConsoleHistoryStore`)? Left for a follow-up if log-only proves insufficient in practice — the rejected instance itself never starts a console to display it from, so this would require the *surviving* instance to detect and record the contention, which is more than this change's scope requires.
|
||||
@@ -0,0 +1,25 @@
|
||||
## Why
|
||||
|
||||
A production task dispatch failed with `DeviceNotFoundError: unknown device device-8967d09f0d5f4cf49f7361a9f0dcb0ce` (host `host-311f68...`, 2026-07-14) even though the Host Agent's own console showed that same device as `connected`. Root-cause tracing confirmed: `DeviceManager` (`device/manager.py`) is in-process, in-memory, and per-process, with no persistence and no reactive removal. `TaskScheduler.assign()` (`cloud/scheduler.py`) dispatches to a `host_id` based on a periodically-synced `DevicePool` snapshot, with no concept of *which OS process* is currently answering that host's long-poll. If two Host Agent processes ever run concurrently under the same host identity — e.g. a supervisor starting a replacement before a hung previous instance has fully exited, or an operator accidentally leaving a second instance running in another terminal/tmux pane — both heartbeat and both claim under the same `host_id`, and a claim can land on whichever instance answers next, including one whose own in-memory `DeviceManager` never registered the device. Nothing in the codebase today prevents this: a repo-wide search confirms no pidfile, lockfile, or `flock`-style guard exists anywhere in `apps/device-host-agent`, and both `identity_path` and the device-config store default to process-working-directory-relative paths, which makes an accidental duplicate launch easy to trigger unnoticed.
|
||||
|
||||
## What Changes
|
||||
|
||||
- `create_application()` (`apps/device-host-agent/host_agent/app.py:145`) acquires a local, exclusive, non-blocking instance lock as its first action, before `resolve_host_identity()` or any device-enrollment network call.
|
||||
- The lock file is colocated with the existing per-installation state directory (`identity_path.parent` — the same directory `app.py` already uses for `host_console_history.sqlite3` and `host_governance_policy.json`), so this requires no new configuration surface.
|
||||
- If the lock is already held by a live process, the Host Agent logs a clear, actionable "duplicate instance" error (distinct from a generic startup failure) and exits non-zero without contacting the control plane, enrolling devices, or starting the console/heartbeat/claim loops.
|
||||
- The lock releases automatically on process exit for any reason — clean shutdown, unhandled exception, or a forced kill — via OS-level advisory file locking rather than a pidfile/sentinel-existence check, so no stale-lock cleanup step is ever required after an unclean shutdown.
|
||||
- New direct dependency: `filelock`, added to `apps/device-host-agent/pyproject.toml` (already present transitively in `uv.lock` today; see design.md for why a maintained cross-platform library was chosen over hand-rolled `fcntl`/`msvcrt` branching for this specific mechanism).
|
||||
|
||||
## Capabilities
|
||||
|
||||
### New Capabilities
|
||||
- `host-agent-single-instance-lock`: a local, per-installation exclusive-execution guarantee that prevents two Host Agent processes sharing the same `identity_path` state directory from running — and participating in the claim/heartbeat protocol — concurrently.
|
||||
|
||||
### Modified Capabilities
|
||||
(none — `host-agent-protocol`'s outbound protocol behavior is unchanged; this change only gates whether a second local process is ever allowed to begin participating in that protocol)
|
||||
|
||||
## Impact
|
||||
|
||||
- Affected code: `apps/device-host-agent/host_agent/app.py` (`create_application()`/`HostAgentApplication` wiring), a new `apps/device-host-agent/host_agent/instance_lock.py` module, `apps/device-host-agent/pyproject.toml` (new `filelock` dependency).
|
||||
- Not affected: `cloud.*`, `apps/cloud-api`, the outbound claim/heartbeat/lease protocol itself, `device/manager.py`, `driver/*`.
|
||||
- Operational impact: an operator or supervisor that unintentionally starts a second instance now gets an immediate, clear local failure at startup instead of a delayed, confusing `DeviceNotFoundError` surfacing minutes later on an unrelated task dispatch. A legitimate restart (old process fully exited before the new one starts) is unaffected, since the lock releases on exit.
|
||||
+37
@@ -0,0 +1,37 @@
|
||||
## ADDED Requirements
|
||||
|
||||
### Requirement: Host Agent acquires an exclusive local instance lock before any control-plane side effect
|
||||
The Host Agent SHALL acquire an exclusive, non-blocking lock scoped to its configured `identity_path` state directory as the first action of application startup, before resolving host identity, enrolling any device, or contacting the control plane.
|
||||
|
||||
#### Scenario: Lock acquired on normal startup
|
||||
- **WHEN** a Host Agent starts and no other process holds the lock for its `identity_path` state directory
|
||||
- **THEN** it acquires the lock and proceeds to identity resolution, device enrollment, and the heartbeat/claim loops as before
|
||||
|
||||
#### Scenario: Duplicate instance is rejected before any network call
|
||||
- **WHEN** a second Host Agent process starts against the same `identity_path` state directory while a first process is still running
|
||||
- **THEN** the second process fails to acquire the lock and exits without resolving host identity, enrolling any device, or otherwise contacting the control plane
|
||||
|
||||
### Requirement: Duplicate-instance rejection is immediate and clearly identified
|
||||
The Host Agent SHALL fail immediately, without waiting or retrying, when the instance lock is already held, and SHALL log an error that identifies the failure as a duplicate-instance conflict distinct from other startup failures.
|
||||
|
||||
#### Scenario: Operator sees an actionable error
|
||||
- **WHEN** a duplicate Host Agent instance fails to acquire the lock
|
||||
- **THEN** the logged error names the lock file location and identifies the cause as another instance already running, rather than a generic or unrelated startup failure
|
||||
|
||||
### Requirement: The instance lock releases without manual intervention after any process exit
|
||||
The Host Agent SHALL release its instance lock when its process exits for any reason, including graceful shutdown, an unhandled exception, or a forced kill, without requiring any manual cleanup step before a subsequent instance can start.
|
||||
|
||||
#### Scenario: Lock is released after graceful shutdown
|
||||
- **WHEN** a running Host Agent instance completes its graceful shutdown sequence
|
||||
- **THEN** a new Host Agent instance started afterward against the same `identity_path` state directory successfully acquires the lock
|
||||
|
||||
#### Scenario: Lock is released after an unclean process exit
|
||||
- **WHEN** a running Host Agent instance is terminated forcefully (e.g. killed) without running its shutdown sequence
|
||||
- **THEN** a new Host Agent instance started afterward against the same `identity_path` state directory successfully acquires the lock without any manual lock file cleanup
|
||||
|
||||
### Requirement: Instance lock scope is per identity, not machine-global
|
||||
The instance lock SHALL be scoped to the Host Agent's configured `identity_path` state directory, such that two Host Agent processes configured with different `identity_path` values SHALL be able to run concurrently on the same machine.
|
||||
|
||||
#### Scenario: Independent identities do not contend
|
||||
- **WHEN** two Host Agent processes start on the same machine with different `identity_path` state directories
|
||||
- **THEN** both acquire their respective locks and run concurrently without either being rejected
|
||||
@@ -0,0 +1,30 @@
|
||||
## 1. Dependency
|
||||
|
||||
- [x] 1.1 Add `filelock` to `apps/device-host-agent/pyproject.toml` `dependencies` (already resolved transitively in `uv.lock`; this only adds a direct edge).
|
||||
- [x] 1.2 Run `uv lock` (workspace-wide) and confirm the resolved `filelock` version/hash is unchanged from what's already pinned transitively.
|
||||
|
||||
## 2. Instance lock module
|
||||
|
||||
- [x] 2.1 Create `apps/device-host-agent/host_agent/instance_lock.py` wrapping `filelock.FileLock`: a function/class that takes the state directory (derived from `identity_path.parent`), acquires `host_agent.lock` inside it with `timeout=0`, and raises a dedicated `InstanceAlreadyRunningError` (naming the lock file path) on `filelock.Timeout` instead of leaking the library's own exception type.
|
||||
- [x] 2.2 Expose an explicit `release()` (or context-manager `__exit__`) that unlocks and closes the underlying handle, per design.md Decision 5.
|
||||
- [x] 2.3 Unit tests for `instance_lock.py`: acquire succeeds when free; acquire raises `InstanceAlreadyRunningError` when already held (same process, second handle against the same path); release-then-reacquire succeeds; two different paths never contend.
|
||||
|
||||
## 3. Wiring into application startup/shutdown
|
||||
|
||||
- [x] 3.1 In `create_application()` (`apps/device-host-agent/host_agent/app.py:145`), acquire the instance lock as the first statement, before `resolve_host_identity()`, using `startup_config.identity_path.parent`.
|
||||
- [x] 3.2 Add an `instance_lock` field to `HostAgentApplication` (`app.py:35-42`) so the held lock stays alive for the object's full lifetime rather than being released when `create_application()` returns.
|
||||
- [x] 3.3 Release the lock in `run_async()`'s existing shutdown `finally` block (`app.py:88-111`), alongside the existing heartbeat/console/supervisor teardown.
|
||||
- [x] 3.4 In `cli.py:main()`, catch `InstanceAlreadyRunningError` around the `create_application(...).run()` call, print a clear duplicate-instance error to stderr (naming the lock path), and exit non-zero — matching the existing `LocalAccountSetupError` handling pattern already in that function.
|
||||
- [x] 3.5 Unit tests in `test_app.py`: a second `create_application()` call against the same `identity_path` (while the first `HostAgentApplication`'s lock is still held) raises `InstanceAlreadyRunningError` before any enrollment call occurs (assert the enrollment client's `enroll_host`/`enroll_device` are never invoked for the second call); two `create_application()` calls against different `identity_path` values both succeed and can coexist.
|
||||
- [x] 3.6 Unit test confirming the lock is released after `run_async()` completes its shutdown sequence, allowing an immediately-following `create_application()` call against the same `identity_path` to succeed (simulating a clean restart).
|
||||
|
||||
## 4. Documentation
|
||||
|
||||
- [x] 4.1 Add a short note to `docs/MACOS_IPHONE_SETUP.md` describing the new duplicate-instance error (what it means, how to resolve it — find and stop the other process) near the existing Host Agent startup instructions.
|
||||
|
||||
## 5. Validation
|
||||
|
||||
- [x] 5.1 Run `uv run --all-packages pytest -m "not integration"` and confirm no regressions.
|
||||
- [x] 5.2 `ruff check` and `ruff format --check` on all changed/new files.
|
||||
- [x] 5.3 `openspec validate host-agent-single-instance-lock --strict` and fix any reported issues.
|
||||
- [ ] 5.4 Manual verification: start one Host Agent instance, attempt to start a second against the same identity, confirm the second exits immediately with the duplicate-instance error and the first is unaffected; stop the first, confirm a subsequent start succeeds. *(Pending: requires a real Host Agent deployment environment — covered indirectly by `test_second_create_application_against_held_lock_raises_before_enrollment`, `test_lock_released_after_run_async_allows_restart`, and `test_independent_paths_never_contend` in `test_app.py`/`test_instance_lock.py`.)*
|
||||
Reference in New Issue
Block a user