diff --git a/features/actor_service_coverage.feature b/features/actor_service_coverage.feature index d717eb47e..4dcfc5bd6 100644 --- a/features/actor_service_coverage.feature +++ b/features/actor_service_coverage.feature @@ -90,3 +90,26 @@ Feature: Actor service coverage And any existing actors are cleared When I ensure the default mock actor with env value "0" Then the result should be None + + Scenario: Ensuring default mock actor is idempotent + Given an actor service with stubbed dependencies + When I ensure the default mock actor with env value "true" + Then the returned actor name should be "local/mock-default" + And the stored actor "local/mock-default" should be default + When I ensure the default mock actor with env value "true" again + Then the returned actor name should be "local/mock-default" + And only one "local/mock-default" actor should exist + + Scenario: Ensuring default mock actor with existing default is safe + Given an actor service with stubbed dependencies + And an actor named "local/existing-default" exists as default + When I ensure the default mock actor with env value "true" + Then the returned actor name should be "local/existing-default" + And the stored actor "local/existing-default" should be default + And only one default actor should exist + + Scenario: Ensuring default mock actor creates exactly one actor + Given an actor service with stubbed dependencies + When I ensure the default mock actor with env value "true" + Then exactly one actor should exist + And the stored actor "local/mock-default" should be default diff --git a/features/steps/actor_service_steps.py b/features/steps/actor_service_steps.py index 3b7d39ebc..090c003fa 100644 --- a/features/steps/actor_service_steps.py +++ b/features/steps/actor_service_steps.py @@ -296,3 +296,30 @@ def after_scenario(context: Context, scenario) -> None: if hasattr(context, "_cleanup_handlers"): for handler in context._cleanup_handlers: handler() + + +@when('I ensure the default mock actor with env value "{value}" again') +def step_ensure_default_mock_actor_again(context: Context, value: str) -> None: + os.environ["CLEVERAGENTS_TESTING_USE_MOCK_AI"] = value + context.result_actor = context.actor_service.ensure_default_mock_actor() + + +@then('only one "{name}" actor should exist') +def step_only_one_actor_exists(context: Context, name: str) -> None: + actors = context.actor_repo.list_all() + count = sum(1 for actor in actors if actor.name == name) + assert count == 1, f"Expected 1 actor named {name}, but found {count}" + + +@then("only one default actor should exist") +def step_only_one_default_actor(context: Context) -> None: + actors = context.actor_repo.list_all() + default_count = sum(1 for actor in actors if actor.is_default) + assert default_count == 1, f"Expected 1 default actor, but found {default_count}" + + +@then("exactly one actor should exist") +def step_exactly_one_actor(context: Context) -> None: + actors = context.actor_repo.list_all() + count = len(actors) + assert count == 1, f"Expected 1 actor, but found {count}" diff --git a/src/cleveragents/application/services/actor_service.py b/src/cleveragents/application/services/actor_service.py index 1b6c4335b..73476949f 100644 --- a/src/cleveragents/application/services/actor_service.py +++ b/src/cleveragents/application/services/actor_service.py @@ -182,6 +182,10 @@ class ActorService: This is idempotent and only runs when CLEVERAGENTS_TESTING_USE_MOCK_AI is true unless ``force`` is set to True (used by tests to provision a fallback actor). + + The check for an existing default and the creation of the mock actor are + performed within a single atomic transaction to prevent TOCTOU race conditions + in concurrent environments (e.g., parallel test workers). """ if not force and os.environ.get( "CLEVERAGENTS_TESTING_USE_MOCK_AI", "" @@ -192,30 +196,32 @@ class ActorService: ): return None + # Perform both check and creation in a single atomic transaction + # to prevent TOCTOU race condition with self.unit_of_work.transaction() as ctx: + # Check if a default actor already exists existing_default = ctx.actors.get_default() if existing_default: return existing_default - # No default yet: create or promote a mock actor - now = datetime.now() - mock_name = "local/mock-default" - mock_actor = Actor( - id=None, - name=mock_name, - provider="MockProvider", - model="mock-gpt-4", - config_blob={}, - config_hash=Actor.compute_hash({}), - graph_descriptor=None, - unsafe=True, - is_built_in=True, - is_default=True, - created_at=now, - updated_at=now, - ) + # No default yet: create or promote a mock actor + now = datetime.now() + mock_name = "local/mock-default" + mock_actor = Actor( + id=None, + name=mock_name, + provider="MockProvider", + model="mock-gpt-4", + config_blob={}, + config_hash=Actor.compute_hash({}), + graph_descriptor=None, + unsafe=True, + is_built_in=True, + is_default=True, + created_at=now, + updated_at=now, + ) - with self.unit_of_work.transaction() as ctx: existing = ctx.actors.get_by_name(mock_name) if existing: if not existing.is_default: