alishahryar1--free-claude-code
3081a72f41
## Problem\n\nShutdown could report success after bounded messaging cleanup, hidden persistence failures, or failed managed-process stops. An Admin restart could then construct a replacement while the old runtime still owned work.\n\n## Changes\n\n| Before | After |\n| --- | --- |\n| Runtime reused bounded interactive stop semantics for terminal messaging cleanup. | Workflow close cancels work, stops managed sessions, awaits every claim and recovery task, then flushes persistence. |\n| Explicit persistence failures were logged and treated as successful writes. | Explicit flushes and authoritative writes propagate failure and stay dirty for retry; timer writes remain best effort. |\n| Managed sessions and aliases were removed before subprocess termination was confirmed. | Manager and session terminal states prevent reuse, retain failed owners and PIDs, reject ID collisions, and retry exact sessions. |\n| Admin restart followed the restart request even after incomplete shutdown. | Supervisor restarts only when the prior runtime reports its entire ownership graph closed. |\n| Partial messaging startup cleanup could fail while application startup continued. | Incomplete partial cleanup fails startup and retains the exact graph for a later close attempt. |\n| Messaging task failures read process-global settings. | Runtime injects diagnostic policy and the messaging package depends only on core. |\n| Lifecycle edge cases were verified only in isolated components. | Deterministic and live product coverage proves composed retry, drain, privacy, and customer command behavior. | <!-- greptile_comment --> <details open><summary><h3>Greptile Summary</h3></summary> This PR makes application shutdown wait for owned work to finish before restart or exit. The main changes are: - Runtime close now waits for messaging work, managed sessions, and persistence flushes. - Admin restart now requires the previous runtime to report full closure. - Managed Claude sessions now keep aliases and PIDs until stop is confirmed. - Session persistence now propagates explicit write failures and keeps dirty state for retry. - Messaging no longer reads global config from task-failure callbacks. </details> <h3>Confidence Score: 5/5</h3> This looks safe to merge. No blocking issues found in the changed code. No files need attention. <details><summary><h3><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="20" align="absmiddle"></a> T-Rex Logs</h3></summary> **What T-Rex did** - I reviewed the general contract validation proof and confirmed that the lifecycle\_session pytest run completed with 73 passed in 2.68s (EXIT\_CODE: 0) and the messaging pytest run completed with 95 passed in 1.96s (EXIT\_CODE: 0). <a href="https://app.greptile.com/trex/runs/14094038/artifacts"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifactsDark.svg?v=4"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"><img alt="View all artifacts" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"></picture></a> <sub><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="14" align="absmiddle"></a> Ran code and verified through T-Rex</sub> </details> <details open><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | src/free_claude_code/messaging/trees/processor.py | Completion callback failures now release task ownership and surface through the idle waiter. | | src/free_claude_code/messaging/workflow.py | Terminal workflow close now cancels tasks, waits for processor cleanup, and flushes persistence. | | src/free_claude_code/runtime/application.py | Runtime shutdown now keeps incomplete ownership cleanup retryable and exposes closure state. | | src/free_claude_code/cli/entrypoints.py | The supervisor now restarts only after the old runtime reports full closure. | | src/free_claude_code/cli/managed/manager.py | Managed session shutdown now blocks reuse and retains failed owners for retry. | | src/free_claude_code/cli/managed/session.py | Managed sessions now mark terminal state under a lifecycle lock and retain PID ownership until exit. | </details> <!-- greptile_failed_comments --> <h3>Comments Outside Diff (1)</h3> 1. `src/free_claude_code/messaging/trees/processor.py`, line 194-202 ([link](https://github.com/alishahryar1/free-claude-code/blob/b736bad1aacb66784e7b4d1e09d27a32b2a380c7/src/free_claude_code/messaging/trees/processor.py#L194-L202)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=9" align="top"></a> **Idle Event Stays Cleared** When `_claim_finished_callback` raises a non-cancellation exception, `_finish_and_continue` exits before `slot.transitioned` is set, before the slot is removed from `_tasks`, and before `_idle` is set. `MessagingWorkflow.close()` now waits on `wait_idle()`, so a finish-path error can leave shutdown waiting forever instead of returning a failed close. <details><summary><strong>Artifacts</strong></summary><br /> **[Repro: standalone async harness that drives TreeQueueManager and forces a finish callback RuntimeError](https://app.greptile.com/trex/artifacts/da413ba3-7f69-459d-b8c8-3141c2bb6c41)** - Contains supporting evidence from the run (text/x-python; charset=utf-8). **[Repro: uv run output showing finish callback RuntimeError, retained task\_count, cleared idle event, and wait\_idle timeout](https://app.greptile.com/trex/artifacts/35b2beaa-ec52-47bd-b36b-2735b3ebc62c)** - Keeps the command output available without making the summary code-heavy. <a href="https://app.greptile.com/trex/runs/14093236/artifacts?artifact=da413ba3-7f69-459d-b8c8-3141c2bb6c41"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifactsDark.svg?v=4"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifacts.svg?v=4"><img alt="View artifacts" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifacts.svg?v=4"></picture></a> </details> <sub><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="14" align="absmiddle"></a> Ran code and verified through T-Rex</sub> <a href="https://app.greptile.com/api/ide/codex?prompt=IMPORTANT%3A%20Work%20in%20the%20repository%20%22alishahryar1%2Ffree-claude-code%22%20on%20the%20existing%20branch%20%22ali%2Fcompletion-driven-shutdown%22.%20Checkout%20that%20branch%20%E2%80%94%20do%20NOT%20create%20a%20new%20branch%20or%20open%20a%20new%20PR.%20Push%20your%20changes%20to%20%22ali%2Fcompletion-driven-shutdown%22.%0A%0AThis%20is%20a%20comment%20left%20during%20a%20code%20review.%0APath%3A%20src%2Ffree_claude_code%2Fmessaging%2Ftrees%2Fprocessor.py%0ALine%3A%20194-202%0A%0AComment%3A%0A**Idle%20Event%20Stays%20Cleared**%0A%0AWhen%20%60_claim_finished_callback%60%20raises%20a%20non-cancellation%20exception%2C%20%60_finish_and_continue%60%20exits%20before%20%60slot.transitioned%60%20is%20set%2C%20before%20the%20slot%20is%20removed%20from%20%60_tasks%60%2C%20and%20before%20%60_idle%60%20is%20set.%20%60MessagingWorkflow.close%28%29%60%20now%20waits%20on%20%60wait_idle%28%29%60%2C%20so%20a%20finish-path%20error%20can%20leave%20shutdown%20waiting%20forever%20instead%20of%20returning%20a%20failed%20close.%0A%0AHow%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20please%20make%20it%20concise.&repo=alishahryar1%2Ffree-claude-code&pr=1056&platform=github"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodexDark.svg?v=6"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodex.svg?v=6"><img alt="Fix in Codex" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodex.svg?v=6"></picture></a> <!-- /greptile_failed_comments --> <sub>Reviews (2): Last reviewed commit: ["Surface messaging completion failures"](https://github.com/alishahryar1/free-claude-code/commit/39a874c8f0e0614847d80321dd51c9654707a7ff) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=43514893)</sub> <!-- /greptile_comment -->
242 行
8.0 KiB
Python
242 行
8.0 KiB
Python
"""Tests for messaging/ module."""
|
|
|
|
import asyncio
|
|
import json
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
# --- Existing Tests ---
|
|
|
|
|
|
class TestMessagingModels:
|
|
"""Test messaging models."""
|
|
|
|
def test_incoming_message_creation(self):
|
|
"""Test IncomingMessage dataclass."""
|
|
from free_claude_code.messaging.models import IncomingMessage
|
|
|
|
msg = IncomingMessage(
|
|
text="Hello",
|
|
chat_id="123",
|
|
user_id="456",
|
|
message_id="789",
|
|
platform="telegram",
|
|
)
|
|
assert msg.text == "Hello"
|
|
assert msg.chat_id == "123"
|
|
assert msg.platform == "telegram"
|
|
assert msg.is_reply() is False
|
|
|
|
def test_incoming_message_with_reply(self):
|
|
"""Test IncomingMessage as a reply."""
|
|
from free_claude_code.messaging.models import IncomingMessage
|
|
|
|
msg = IncomingMessage(
|
|
text="Reply text",
|
|
chat_id="123",
|
|
user_id="456",
|
|
message_id="789",
|
|
platform="discord",
|
|
reply_to_message_id="100",
|
|
)
|
|
assert msg.is_reply() is True
|
|
assert msg.reply_to_message_id == "100"
|
|
|
|
|
|
class TestMessagingPorts:
|
|
"""Test explicit messaging platform component ports."""
|
|
|
|
def test_components_bundle_runtime_and_outbound(self):
|
|
"""Verify the factory handoff shape is explicit."""
|
|
from free_claude_code.messaging.platforms.ports import (
|
|
MessagingPlatformComponents,
|
|
)
|
|
|
|
runtime = MagicMock()
|
|
runtime.name = "telegram"
|
|
runtime.start = AsyncMock()
|
|
runtime.quiesce = AsyncMock()
|
|
runtime.close = AsyncMock()
|
|
runtime.on_message = MagicMock()
|
|
outbound = MagicMock()
|
|
outbound.queue_send_message = AsyncMock()
|
|
outbound.queue_edit_message = AsyncMock()
|
|
outbound.queue_delete_messages = AsyncMock()
|
|
outbound.fire_and_forget = MagicMock()
|
|
components = MessagingPlatformComponents(
|
|
name="telegram",
|
|
runtime=runtime,
|
|
outbound=outbound,
|
|
voice_cancellation=None,
|
|
)
|
|
assert components.runtime is runtime
|
|
assert components.outbound is outbound
|
|
|
|
|
|
class TestSessionStore:
|
|
"""Test SessionStore."""
|
|
|
|
def test_session_store_init(self, tmp_path):
|
|
"""Test SessionStore initialization."""
|
|
from free_claude_code.messaging.session import SessionStore
|
|
|
|
store = SessionStore(storage_path=str(tmp_path / "sessions.json"))
|
|
assert store.load_conversation_snapshot().is_empty
|
|
|
|
# --- Tree Tests ---
|
|
|
|
def test_save_and_get_tree(self, tmp_path):
|
|
"""Test saving and retrieving trees."""
|
|
from free_claude_code.messaging.models import MessageScope
|
|
from free_claude_code.messaging.session import SessionStore
|
|
from free_claude_code.messaging.trees import TreeIdentity, TreeSnapshot
|
|
|
|
store = SessionStore(storage_path=str(tmp_path / "sessions.json"))
|
|
scope = MessageScope(platform="telegram", chat_id="chat")
|
|
|
|
tree_data = {
|
|
"scope": {"platform": scope.platform, "chat_id": scope.chat_id},
|
|
"root_id": "r1",
|
|
"nodes": {
|
|
"r1": {"node_id": "r1", "status_message_id": "s1"},
|
|
"n1": {"node_id": "n1", "status_message_id": "s2"},
|
|
},
|
|
}
|
|
snapshot = TreeSnapshot.from_json(tree_data)
|
|
assert snapshot is not None
|
|
store.save_tree_snapshot(snapshot)
|
|
|
|
identity = TreeIdentity(scope=scope, root_id="r1")
|
|
loaded = store.load_conversation_snapshot().get_tree(identity)
|
|
assert loaded is not None
|
|
assert loaded == snapshot
|
|
assert loaded.lookup_ids() == {"r1", "s1", "n1", "s2"}
|
|
|
|
# --- Persistence & Edge Cases ---
|
|
|
|
def test_load_existing_file_with_trees(self, tmp_path):
|
|
"""Test loading file with trees (legacy sessions ignored)."""
|
|
from free_claude_code.messaging.models import MessageScope
|
|
from free_claude_code.messaging.session import SessionStore
|
|
from free_claude_code.messaging.trees import TreeIdentity
|
|
|
|
data = {
|
|
"sessions": {},
|
|
"trees": {
|
|
"r1": {
|
|
"root_id": "r1",
|
|
"nodes": {
|
|
"r1": {
|
|
"node_id": "r1",
|
|
"incoming": {
|
|
"platform": "telegram",
|
|
"chat_id": "chat",
|
|
},
|
|
}
|
|
},
|
|
}
|
|
},
|
|
"node_to_tree": {"r1": "r1"},
|
|
"message_log": {},
|
|
}
|
|
|
|
p = tmp_path / "sessions.json"
|
|
with open(p, "w") as f:
|
|
json.dump(data, f)
|
|
|
|
store = SessionStore(storage_path=str(p))
|
|
identity = TreeIdentity(
|
|
scope=MessageScope(platform="telegram", chat_id="chat"),
|
|
root_id="r1",
|
|
)
|
|
assert store.load_conversation_snapshot().get_tree(identity) is not None
|
|
|
|
def test_load_corrupt_file(self, tmp_path):
|
|
"""Test loading corrupt/invalid json file."""
|
|
p = tmp_path / "sessions.json"
|
|
with open(p, "w") as f:
|
|
f.write("{invalid json")
|
|
|
|
from free_claude_code.messaging.session import SessionStore
|
|
|
|
# Should log error and start empty, avoiding crash
|
|
store = SessionStore(storage_path=str(p))
|
|
assert store.load_conversation_snapshot().is_empty
|
|
|
|
def test_save_error_handling(self, tmp_path):
|
|
"""Test error during save."""
|
|
from free_claude_code.messaging.models import MessageScope
|
|
from free_claude_code.messaging.session import SessionStore
|
|
from free_claude_code.messaging.trees import TreeIdentity, TreeSnapshot
|
|
|
|
store = SessionStore(storage_path=str(tmp_path / "sessions.json"))
|
|
scope = MessageScope(platform="telegram", chat_id="chat")
|
|
snapshot = TreeSnapshot(scope=scope, root_id="r1", nodes={"r1": {}})
|
|
store.save_tree_snapshot(snapshot)
|
|
|
|
with (
|
|
patch(
|
|
"free_claude_code.messaging.session.persistence.os.replace",
|
|
side_effect=OSError("Disk full"),
|
|
),
|
|
pytest.raises(OSError, match="Disk full"),
|
|
):
|
|
store.flush_pending_save()
|
|
|
|
assert store.dirty is True
|
|
identity = TreeIdentity(scope=scope, root_id="r1")
|
|
assert store.load_conversation_snapshot().get_tree(identity) is not None
|
|
|
|
|
|
class TestTreeQueueManager:
|
|
"""Test TreeQueueManager."""
|
|
|
|
def test_tree_queue_manager_init(self):
|
|
from free_claude_code.messaging.trees import TreeQueueManager
|
|
|
|
async def process(_claim):
|
|
return None
|
|
|
|
mgr = TreeQueueManager(process)
|
|
assert mgr.get_tree_count() == 0
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_admit_creates_tree_and_claim(self):
|
|
from free_claude_code.messaging.models import IncomingMessage
|
|
from free_claude_code.messaging.trees import TreeQueueManager
|
|
|
|
processed = asyncio.Event()
|
|
|
|
async def processor(claim):
|
|
assert claim.node.node_id == "1"
|
|
processed.set()
|
|
|
|
incoming = IncomingMessage(
|
|
text="test",
|
|
chat_id="1",
|
|
user_id="1",
|
|
message_id="1",
|
|
platform="test",
|
|
)
|
|
|
|
mgr = TreeQueueManager(processor)
|
|
decision = await mgr.admit(incoming, "status_1")
|
|
|
|
assert decision.accepted is True
|
|
assert decision.claim is not None
|
|
await processed.wait()
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_cancel_unknown_node_is_empty(self):
|
|
from free_claude_code.messaging.models import MessageScope
|
|
from free_claude_code.messaging.trees import TreeQueueManager
|
|
|
|
async def process(_claim):
|
|
return None
|
|
|
|
mgr = TreeQueueManager(process)
|
|
scope = MessageScope(platform="test", chat_id="1")
|
|
cancelled = await mgr.cancel_node(scope, "nonexistent")
|
|
assert cancelled.effects == ()
|