From eab5faa8a482523f5d02718cfc7ab963c2fa9214 Mon Sep 17 00:00:00 2001 From: q Date: Mon, 19 Oct 2026 04:42:33 +0000 Subject: [PATCH] [hotfix] Give workspace-lock unit owners a live story through the lookup seam The fixture let competing creates reach the real control API, so refusals proved API unavailability and the exhaustive sweep case exceeded the unit budget. Each manager now answers the owner lookup with in_progress and the refusals assert the live-owner message with the owner worker and story. --- docs/CHANGELOG.md | 2 + .../tests/unit/test_workspace_lock_holder.py | 40 ++++++++++++------- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ffc793594..3ee926e44 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,6 +11,8 @@ See the CHANGELOG rule in [AGENTS.md](../AGENTS.md). 15 s, so a finished deploy is seen sooner; the first read stays immediate. - Worker checkout reads the remote default and the story branch from one advertisement and fetches both in one native fetch, so preparation makes four remote Git calls instead of five. +- Workspace-lock unit tests give the existing owner a live story through the status lookup seam, so they + prove live-owner refusal without contacting the control API and fit the unit time budget. ## 2026-10-18 diff --git a/services/worker-manager/tests/unit/test_workspace_lock_holder.py b/services/worker-manager/tests/unit/test_workspace_lock_holder.py index 1f22efb5a..8cb1cada8 100644 --- a/services/worker-manager/tests/unit/test_workspace_lock_holder.py +++ b/services/worker-manager/tests/unit/test_workspace_lock_holder.py @@ -23,6 +23,7 @@ EngineeringExecutionPhase, EngineeringInfrastructureRefusal, ) +from shared.contracts.dto.story import StoryStatus from shared.contracts.dto.worker import WorkerStatus from shared.contracts.queues.worker import WorkerOwnership from shared.contracts.vocab import AgentType @@ -33,6 +34,9 @@ from src.manager import WorkerManager PROJECT = "proj-shared-checkout" +LIVE_OWNER_REFUSAL = ( + rf"Project {PROJECT} already has active worker worker-a owned by story story-1 \(in_progress\)" +) def _make_docker_mock(): @@ -52,6 +56,13 @@ def _make_docker_mock(): return docker +def _manager(redis, docker) -> WorkerManager: + """A real manager whose existing project owner reports a live story.""" + manager = WorkerManager(redis=redis, docker_client=docker) + manager._lookup_story_status = AsyncMock(return_value=StoryStatus.IN_PROGRESS) + return manager + + def _ownership(run_id: str, attempt_id: str) -> WorkerOwnership: return WorkerOwnership( story_id="story-1", project_id=PROJECT, run_id=run_id, attempt_id=attempt_id @@ -96,13 +107,14 @@ async def test_a_rejected_worker_cleanup_leaves_the_live_workers_lock_alone(dock excluded from A's checkout. """ redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await _create(manager, "worker-a", "run-a") assert await redis.sismember("workspace:active_projects", PROJECT) - with pytest.raises(RuntimeError, match="already has active worker"): + with pytest.raises(RuntimeError, match=LIVE_OWNER_REFUSAL): await _create(manager, "worker-b", "run-b") + manager._lookup_story_status.assert_awaited_once_with("story-1") # B took no project lock, ownership, or teardown state. meta_b = decode_redis_fields(await redis.hgetall("worker:meta:worker-b")) @@ -119,7 +131,7 @@ async def test_a_rejected_worker_cleanup_leaves_the_live_workers_lock_alone(dock assert await redis.sismember("workspace:active_projects", PROJECT) assert await manager._check_project_lock(PROJECT) == "worker-a" - with pytest.raises(RuntimeError, match="already has active worker"): + with pytest.raises(RuntimeError, match=LIVE_OWNER_REFUSAL): await _create(manager, "worker-c", "run-c") # And B's cleanup did not spend one of A's project's retries either. @@ -136,11 +148,11 @@ async def test_a_refused_worker_fails_fast_instead_of_timing_out(docker): is exactly how a refusal turned into someone else's lock being released. """ redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await _create(manager, "worker-a", "run-a") - with pytest.raises(RuntimeError, match="already has active worker"): + with pytest.raises(RuntimeError, match=LIVE_OWNER_REFUSAL): await _create(manager, "worker-b", "run-b") assert await redis.hget("worker:status:worker-b", "status") == WorkerStatus.FAILED @@ -158,7 +170,7 @@ async def test_a_refused_worker_fails_fast_instead_of_timing_out(docker): async def test_workspace_lock_refusal_does_not_poison_executor_inventory(docker, monkeypatch): """The retained terminal diagnostic record is not a lease needing a container.""" redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) docker.list_containers = AsyncMock(return_value=[]) monkeypatch.setattr(manager, "_check_project_lock", AsyncMock(return_value="worker-a")) @@ -180,7 +192,7 @@ async def test_the_holder_is_the_one_that_won_the_acquisition(docker): ownership it wrote on the way in is withdrawn with the refusal. """ redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await manager._acquire_workspace_lock("worker-a", _ownership("run-a", "attempt-a")) @@ -198,7 +210,7 @@ async def test_the_holder_is_the_one_that_won_the_acquisition(docker): async def test_failed_container_removal_keeps_the_owner_fenced_workspace_lock(docker): """A delete command is not teardown evidence and cannot free the checkout.""" redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await manager._acquire_workspace_lock("worker-a", _ownership("run-a", "attempt-a")) docker.remove_container.side_effect = RuntimeError("docker daemon unavailable") @@ -218,7 +230,7 @@ async def test_a_qa_executor_does_not_release_a_developer_workers_lock(docker): other way this could reach for a lock it never took. """ redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await _create(manager, "worker-a", "run-a") @@ -252,7 +264,7 @@ async def test_a_finished_qa_executor_is_not_swept_as_the_projects_stale_holder( delete path is forbidden from doing. """ redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) await _create(manager, "worker-a", "run-a") await redis.hset( @@ -321,15 +333,13 @@ async def wrapped(*args, **kwargs): counting_redis = aioredis.FakeRedis(decode_responses=True) counted, sadd_patch, hset_patch = _instrument(counting_redis, sweep_after=0) with sadd_patch, hset_patch: - await _create( - WorkerManager(redis=counting_redis, docker_client=docker), "worker-count", "run-count" - ) + await _create(_manager(counting_redis, docker), "worker-count", "run-count") write_count = counted["writes"] assert write_count > 2 for sweep_after in range(1, write_count + 1): redis = aioredis.FakeRedis(decode_responses=True) - manager = WorkerManager(redis=redis, docker_client=docker) + manager = _manager(redis, docker) state, sadd_patch, hset_patch = _instrument(redis, sweep_after) with sadd_patch, hset_patch: @@ -341,5 +351,5 @@ async def wrapped(*args, **kwargs): assert await redis.sismember("workspace:active_projects", PROJECT), ( f"the sweep took the workspace from its holder at interleaving point {sweep_after}" ) - with pytest.raises(RuntimeError, match="already has active worker"): + with pytest.raises(RuntimeError, match=LIVE_OWNER_REFUSAL): await _create(manager, "worker-b", "run-b")