fix(arch-a2): deactivation cleanup - container services, search provider, hook deregistration, notification sync, task state, activation order
Check Cross-Plugin Imports / check (push) Has been cancelled
Check Cross-Plugin Imports / check (push) Has been cancelled
This commit is contained in:
@@ -29,6 +29,14 @@ class ServiceContainer:
|
|||||||
"""Check if a service is registered."""
|
"""Check if a service is registered."""
|
||||||
return name in self._services
|
return name in self._services
|
||||||
|
|
||||||
|
def remove(self, name: str) -> None:
|
||||||
|
"""Remove a service registration (no-op if absent).
|
||||||
|
|
||||||
|
Used by plugin deactivation hooks to clean up services they
|
||||||
|
registered during activation.
|
||||||
|
"""
|
||||||
|
self._services.pop(name, None)
|
||||||
|
|
||||||
async def initialize(self) -> None:
|
async def initialize(self) -> None:
|
||||||
"""Initialize core services."""
|
"""Initialize core services."""
|
||||||
if self._initialized:
|
if self._initialized:
|
||||||
|
|||||||
@@ -62,7 +62,10 @@ class AIUIControlPlugin(BasePlugin):
|
|||||||
from app.plugins.builtins.contracts import get_contract_registry
|
from app.plugins.builtins.contracts import get_contract_registry
|
||||||
get_contract_registry().unregister(self.manifest.name)
|
get_contract_registry().unregister(self.manifest.name)
|
||||||
|
|
||||||
await super().on_deactivate(db, service_container, event_bus)
|
# Remove the WebSocket manager BEFORE super() so that event handlers
|
||||||
|
# being unsubscribed can no longer reach it (ARCH-044).
|
||||||
if service_container.has("ai_ui_control_ws"):
|
if service_container.has("ai_ui_control_ws"):
|
||||||
service_container.remove("ai_ui_control_ws")
|
service_container.remove("ai_ui_control_ws")
|
||||||
logger.info("AI UI Control WebSocket manager removed")
|
logger.info("AI UI Control WebSocket manager removed")
|
||||||
|
|
||||||
|
await super().on_deactivate(db, service_container, event_bus)
|
||||||
|
|||||||
@@ -40,6 +40,10 @@ class GraphRAGPlugin(BasePlugin):
|
|||||||
|
|
||||||
async def on_activate(self, db, service_container, event_bus) -> None:
|
async def on_activate(self, db, service_container, event_bus) -> None:
|
||||||
"""Activate plugin: register GraphRAG search provider."""
|
"""Activate plugin: register GraphRAG search provider."""
|
||||||
|
# Base class first (event subscriptions etc.), then own registrations
|
||||||
|
# so a failure in our code cannot leave the base half-initialized (ARCH-037).
|
||||||
|
await super().on_activate(db, service_container, event_bus)
|
||||||
|
|
||||||
from app.plugins.builtins.graph_rag.provider import GraphRAGSearchProvider
|
from app.plugins.builtins.graph_rag.provider import GraphRAGSearchProvider
|
||||||
from app.plugins.builtins.unified_search.contracts import get_search_registry
|
from app.plugins.builtins.unified_search.contracts import get_search_registry
|
||||||
|
|
||||||
@@ -50,8 +54,6 @@ class GraphRAGPlugin(BasePlugin):
|
|||||||
import logging
|
import logging
|
||||||
logging.getLogger(__name__).exception("Failed to register GraphRAGSearchProvider")
|
logging.getLogger(__name__).exception("Failed to register GraphRAGSearchProvider")
|
||||||
|
|
||||||
await super().on_activate(db, service_container, event_bus)
|
|
||||||
|
|
||||||
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
||||||
"""Deactivate plugin: unregister search provider and contract."""
|
"""Deactivate plugin: unregister search provider and contract."""
|
||||||
from app.plugins.builtins.unified_search.contracts import get_search_registry
|
from app.plugins.builtins.unified_search.contracts import get_search_registry
|
||||||
|
|||||||
@@ -149,8 +149,8 @@ class KnowledgePlugin(BasePlugin):
|
|||||||
|
|
||||||
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
||||||
"""Clean up on deactivation."""
|
"""Clean up on deactivation."""
|
||||||
from app.core.hooks import unregister_actions_by_owner
|
from app.core.hooks import get_hook_registry
|
||||||
unregister_actions_by_owner("knowledge")
|
get_hook_registry().unregister_all_for_plugin("knowledge")
|
||||||
# Unregister knowledge agent tools from the core AI tool registry
|
# Unregister knowledge agent tools from the core AI tool registry
|
||||||
try:
|
try:
|
||||||
from app.ai.tool_registry import get_tool_registry
|
from app.ai.tool_registry import get_tool_registry
|
||||||
|
|||||||
@@ -136,6 +136,13 @@ class KommunikationPlugin(BasePlugin):
|
|||||||
from app.plugins.builtins.contracts import get_contract_registry
|
from app.plugins.builtins.contracts import get_contract_registry
|
||||||
get_contract_registry().unregister(self.manifest.name)
|
get_contract_registry().unregister(self.manifest.name)
|
||||||
|
|
||||||
|
# Remove services registered in on_activate BEFORE super() so that
|
||||||
|
# event handlers being unsubscribed can no longer reach them (ARCH-033).
|
||||||
|
for service_name in ("comm_websocket", "comm_miniapps"):
|
||||||
|
if service_container.has(service_name):
|
||||||
|
service_container.remove(service_name)
|
||||||
|
logger.info("Removed '%s' from service container", service_name)
|
||||||
|
|
||||||
await super().on_deactivate(db, service_container, event_bus)
|
await super().on_deactivate(db, service_container, event_bus)
|
||||||
logger.info("Kommunikation plugin deactivated")
|
logger.info("Kommunikation plugin deactivated")
|
||||||
|
|
||||||
|
|||||||
@@ -115,7 +115,11 @@ async def _auto_sync_loop() -> None:
|
|||||||
class MailPlugin(BasePlugin):
|
class MailPlugin(BasePlugin):
|
||||||
"""Mail plugin for email management: IMAP sync, SMTP send, threading, rules, PGP."""
|
"""Mail plugin for email management: IMAP sync, SMTP send, threading, rules, PGP."""
|
||||||
|
|
||||||
_auto_sync_task: asyncio.Task | None = None
|
def __init__(self) -> None:
|
||||||
|
super().__init__()
|
||||||
|
# Instance attribute: multiple plugin instances must not share the
|
||||||
|
# background task state (ARCH-036).
|
||||||
|
self._auto_sync_task: asyncio.Task | None = None
|
||||||
|
|
||||||
manifest = PluginManifest(
|
manifest = PluginManifest(
|
||||||
name="mail",
|
name="mail",
|
||||||
|
|||||||
@@ -579,14 +579,15 @@ async def request_approval(
|
|||||||
proposal.approval_request_id = req.id
|
proposal.approval_request_id = req.id
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
|
||||||
# Post to Communication via KommunikationContract
|
# Post to Communication via KommunikationContract (ARCH-013: contract
|
||||||
|
# only — no direct plugin imports; skip cleanly when contract is absent)
|
||||||
try:
|
try:
|
||||||
from app.plugins.builtins.contracts import get_contract
|
from app.plugins.builtins.contracts import get_contract
|
||||||
# Find or create a system conversation for improvement proposals
|
|
||||||
_komm = get_contract("kommunikation")
|
_komm = get_contract("kommunikation")
|
||||||
if not _komm or not hasattr(_komm, "create_plugin_room"):
|
if not _komm or not hasattr(_komm, "create_plugin_room"):
|
||||||
from app.plugins.builtins.kommunikation.services import create_plugin_room
|
logger.warning("kommunikation contract unavailable - skipping proposal notification")
|
||||||
room = await create_plugin_room(
|
else:
|
||||||
|
room = await _komm.create_plugin_room(
|
||||||
db=db, tenant_id=tenant_id, user_id=requested_by,
|
db=db, tenant_id=tenant_id, user_id=requested_by,
|
||||||
plugin_name="self_improvement", title="Improvement Proposals",
|
plugin_name="self_improvement", title="Improvement Proposals",
|
||||||
participant_type="system",
|
participant_type="system",
|
||||||
@@ -595,7 +596,7 @@ async def request_approval(
|
|||||||
if not conversation_id and hasattr(room, "id"):
|
if not conversation_id and hasattr(room, "id"):
|
||||||
conversation_id = room.id
|
conversation_id = room.id
|
||||||
if conversation_id:
|
if conversation_id:
|
||||||
await KommunikationContract.send_message(
|
await _komm.send_message(
|
||||||
db=db,
|
db=db,
|
||||||
tenant_id=tenant_id,
|
tenant_id=tenant_id,
|
||||||
conversation_id=conversation_id,
|
conversation_id=conversation_id,
|
||||||
|
|||||||
@@ -27,9 +27,9 @@ class WikiPlugin(BasePlugin):
|
|||||||
try:
|
try:
|
||||||
from app.plugins.builtins.contracts import get_contract
|
from app.plugins.builtins.contracts import get_contract
|
||||||
search_contract = get_contract("unified_search")
|
search_contract = get_contract("unified_search")
|
||||||
if search_contract and hasattr(search_contract, "register_provider"):
|
if search_contract and hasattr(search_contract, "get_search_registry"):
|
||||||
from app.plugins.builtins.unified_search.providers.wiki_provider import WikiSearchProvider
|
from app.plugins.builtins.unified_search.providers.wiki_provider import WikiSearchProvider
|
||||||
search_contract.register_provider(WikiSearchProvider())
|
search_contract.get_search_registry().register(WikiSearchProvider())
|
||||||
logger.info("Registered WikiSearchProvider via contract")
|
logger.info("Registered WikiSearchProvider via contract")
|
||||||
else:
|
else:
|
||||||
logger.warning("unified_search contract not available, skipping WikiSearchProvider registration")
|
logger.warning("unified_search contract not available, skipping WikiSearchProvider registration")
|
||||||
@@ -37,6 +37,17 @@ class WikiPlugin(BasePlugin):
|
|||||||
logger.exception("Failed to register WikiSearchProvider")
|
logger.exception("Failed to register WikiSearchProvider")
|
||||||
|
|
||||||
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
async def on_deactivate(self, db, service_container, event_bus) -> None:
|
||||||
from app.core.hooks import unregister_actions_by_owner
|
from app.core.hooks import get_hook_registry
|
||||||
unregister_actions_by_owner("wiki")
|
get_hook_registry().unregister_all_for_plugin("wiki")
|
||||||
|
|
||||||
|
# Unregister the search provider registered in on_activate (ARCH-012).
|
||||||
|
try:
|
||||||
|
from app.plugins.builtins.contracts import get_contract
|
||||||
|
search_contract = get_contract("unified_search")
|
||||||
|
if search_contract and hasattr(search_contract, "get_search_registry"):
|
||||||
|
search_contract.get_search_registry().unregister("wiki_article")
|
||||||
|
logger.info("Unregistered WikiSearchProvider via contract")
|
||||||
|
except Exception:
|
||||||
|
logger.exception("Failed to unregister WikiSearchProvider")
|
||||||
|
|
||||||
await super().on_deactivate(db, service_container, event_bus)
|
await super().on_deactivate(db, service_container, event_bus)
|
||||||
|
|||||||
@@ -709,6 +709,13 @@ class PluginRegistry:
|
|||||||
await db.flush()
|
await db.flush()
|
||||||
self._db_status[name] = record
|
self._db_status[name] = record
|
||||||
|
|
||||||
|
# Re-sync notification types so entries of the deactivated plugin are
|
||||||
|
# removed from the DB (ARCH-015) — must run AFTER the status update.
|
||||||
|
try:
|
||||||
|
await self.sync_notification_types(db)
|
||||||
|
except Exception:
|
||||||
|
logger.exception("Failed to sync notification types after deactivating '%s'", name)
|
||||||
|
|
||||||
# Invalidate Redis cache for this plugin across all tenants
|
# Invalidate Redis cache for this plugin across all tenants
|
||||||
try:
|
try:
|
||||||
from app.core.redis import get_redis
|
from app.core.redis import get_redis
|
||||||
|
|||||||
@@ -197,3 +197,85 @@ class TestManifestPermissionValidator:
|
|||||||
display_name='X',
|
display_name='X',
|
||||||
permissions=['core:contacts:read'],
|
permissions=['core:contacts:read'],
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# --- A2 deactivation cleanup: lifecycle symmetry ---
|
||||||
|
|
||||||
|
|
||||||
|
class TestServiceContainerRemove:
|
||||||
|
def test_remove_registered_service(self):
|
||||||
|
from app.core.service_container import ServiceContainer
|
||||||
|
|
||||||
|
container = ServiceContainer()
|
||||||
|
container.register('svc', object())
|
||||||
|
assert container.has('svc') is True
|
||||||
|
container.remove('svc')
|
||||||
|
assert container.has('svc') is False
|
||||||
|
|
||||||
|
def test_remove_absent_service_is_noop(self):
|
||||||
|
from app.core.service_container import ServiceContainer
|
||||||
|
|
||||||
|
container = ServiceContainer()
|
||||||
|
container.remove('never_registered') # must not raise
|
||||||
|
assert container.has('never_registered') is False
|
||||||
|
|
||||||
|
|
||||||
|
class TestKommunikationLifecycle:
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_deactivate_removes_container_services(self):
|
||||||
|
from app.core.event_bus import EventBus
|
||||||
|
from app.core.service_container import ServiceContainer
|
||||||
|
from app.plugins.builtins.kommunikation.plugin import KommunikationPlugin
|
||||||
|
|
||||||
|
plugin = KommunikationPlugin()
|
||||||
|
container = ServiceContainer()
|
||||||
|
event_bus = EventBus()
|
||||||
|
|
||||||
|
await plugin.on_activate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
assert container.has('comm_websocket') is True
|
||||||
|
assert container.has('comm_miniapps') is True
|
||||||
|
|
||||||
|
await plugin.on_deactivate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
assert container.has('comm_websocket') is False
|
||||||
|
assert container.has('comm_miniapps') is False
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_reactivate_no_duplicates(self):
|
||||||
|
from app.core.event_bus import EventBus
|
||||||
|
from app.core.service_container import ServiceContainer
|
||||||
|
from app.plugins.builtins.kommunikation.plugin import KommunikationPlugin
|
||||||
|
|
||||||
|
plugin = KommunikationPlugin()
|
||||||
|
container = ServiceContainer()
|
||||||
|
event_bus = EventBus()
|
||||||
|
|
||||||
|
await plugin.on_activate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
await plugin.on_deactivate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
await plugin.on_activate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
|
||||||
|
ws = container.get('comm_websocket')
|
||||||
|
miniapps = container.get('comm_miniapps')
|
||||||
|
assert ws is not None and miniapps is not None
|
||||||
|
assert len(miniapps._apps) == len(set(miniapps._apps.keys()))
|
||||||
|
|
||||||
|
|
||||||
|
class TestWikiProviderLifecycle:
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_deactivate_unregisters_search_provider(self):
|
||||||
|
from app.core.event_bus import EventBus
|
||||||
|
from app.core.service_container import ServiceContainer
|
||||||
|
from app.plugins.builtins.contracts import get_contract
|
||||||
|
from app.plugins.builtins.wiki.plugin import WikiPlugin
|
||||||
|
|
||||||
|
plugin = WikiPlugin()
|
||||||
|
container = ServiceContainer()
|
||||||
|
event_bus = EventBus()
|
||||||
|
|
||||||
|
search_contract = get_contract('unified_search')
|
||||||
|
registry = search_contract.get_search_registry()
|
||||||
|
|
||||||
|
await plugin.on_activate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
assert registry.get('wiki_article') is not None
|
||||||
|
|
||||||
|
await plugin.on_deactivate(db=None, service_container=container, event_bus=event_bus)
|
||||||
|
assert registry.get('wiki_article') is None
|
||||||
|
|||||||
Reference in New Issue
Block a user