refactor(webhooks): bound queue waits, route URLs through dynaconf
Addresses round-3 review feedback on PR #747: - webhook_receiver: wrap send_stream.send() in anyio.fail_after(1.0) and return 503 with reason="queue full" if the queue is saturated. Avoids pinning the handler until NC's outbound timeout fires; the 503 retry contract is the same as the existing "sync not running" branch. - webhook_receiver: revise the compare_digest comment to match what the function actually guarantees — it avoids the per-character short-circuit of `==` but is not fully constant-time across length differences. - _get_webhook_uri: read WEBHOOK_INTERNAL_URL and NEXTCLOUD_MCP_SERVER_URL via dynaconf so operators using settings.toml (rather than env vars) aren't silently routed into the docker/localhost fallback. Adds webhook_internal_url to Settings/_DEFAULTS/_field_map; nextcloud_mcp_server_url already existed. Docker-detection markers stay on os.getenv since they're container-runtime signals, not user-facing config. - webhook_routes: sweep remaining f-string logger calls to lazy %s formatting per CLAUDE.md. - client/webhooks: modernise full file's type hints to dict / list / | None per CLAUDE.md. Tests: - New test_returns_503_when_queue_is_full exercises the timeout branch with a saturated buffer and a shortened deadline. - test_webhook_uri tests now patch get_settings (matching the auth-pair tests in the same file) instead of monkeypatching env vars directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
42675e7c20
commit
c1368b9a7f
@@ -165,6 +165,33 @@ def test_returns_500_when_stream_is_closed():
|
||||
assert response.json()["status"] == "error"
|
||||
|
||||
|
||||
def test_returns_503_when_queue_is_full(monkeypatch):
|
||||
"""When the processor queue is saturated, the handler must time out
|
||||
quickly with 503 instead of pinning until NC's outbound timeout fires."""
|
||||
# Speed up the test — a 1s deadline matches production but is overkill
|
||||
# for a unit test that's specifically exercising the timeout branch.
|
||||
# Capture the original BEFORE patching so the override doesn't recurse
|
||||
# into itself (the receiver imports the same anyio module object).
|
||||
real_fail_after = anyio.fail_after
|
||||
monkeypatch.setattr(
|
||||
"nextcloud_mcp_server.vector.webhook_receiver.anyio.fail_after",
|
||||
lambda _seconds: real_fail_after(0.05),
|
||||
)
|
||||
|
||||
# Buffer of 1, no consumer → first send fills it, second blocks.
|
||||
send_stream, _receive_stream = anyio.create_memory_object_stream(max_buffer_size=1)
|
||||
send_stream.send_nowait("sentinel") # type: ignore[arg-type]
|
||||
app = _make_app(send_stream=send_stream)
|
||||
|
||||
with TestClient(app) as client:
|
||||
response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED)
|
||||
|
||||
assert response.status_code == 503
|
||||
body = response.json()
|
||||
assert body["status"] == "unavailable"
|
||||
assert body["reason"] == "queue full"
|
||||
|
||||
|
||||
# --- WEBHOOK_SECRET authentication ---------------------------------------
|
||||
|
||||
|
||||
|
||||
@@ -1,9 +1,13 @@
|
||||
"""Unit tests for ``_get_webhook_uri`` priority order and the
|
||||
``webhook_auth_kwargs`` registration helper.
|
||||
``webhook_auth_pair`` registration helper.
|
||||
|
||||
Cloud deployments register the webhook URI returned by this function with
|
||||
Nextcloud. ECS Fargate also exposes ``/.dockerenv``, so an explicit public
|
||||
URL must win over the docker auto-detection branch.
|
||||
URL must win over the docker auto-detection branch. The URL fields are
|
||||
read via dynaconf (``Settings``), so tests patch ``get_settings`` directly.
|
||||
The docker-detection markers (``/.dockerenv``, ``DOCKER_CONTAINER``,
|
||||
``NEXTCLOUD_MCP_SERVICE_NAME``, ``NEXTCLOUD_MCP_PORT``) remain on
|
||||
``os.getenv`` and are exercised via env-var monkeypatching.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
@@ -15,9 +19,7 @@ from nextcloud_mcp_server.auth.webhook_routes import (
|
||||
)
|
||||
from nextcloud_mcp_server.config import Settings
|
||||
|
||||
ENV_VARS = (
|
||||
"WEBHOOK_INTERNAL_URL",
|
||||
"NEXTCLOUD_MCP_SERVER_URL",
|
||||
DOCKER_ENV_VARS = (
|
||||
"NEXTCLOUD_MCP_SERVICE_NAME",
|
||||
"NEXTCLOUD_MCP_PORT",
|
||||
"DOCKER_CONTAINER",
|
||||
@@ -26,10 +28,21 @@ ENV_VARS = (
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clean_env(monkeypatch):
|
||||
for name in ENV_VARS:
|
||||
for name in DOCKER_ENV_VARS:
|
||||
monkeypatch.delenv(name, raising=False)
|
||||
|
||||
|
||||
def _patch_settings(monkeypatch, **overrides) -> None:
|
||||
"""Make ``get_settings()`` (as called inside webhook_routes) return a
|
||||
Settings instance with the given URL/secret fields set; everything else
|
||||
falls back to the dataclass defaults."""
|
||||
monkeypatch.setattr(
|
||||
webhook_routes,
|
||||
"get_settings",
|
||||
lambda: Settings(**overrides),
|
||||
)
|
||||
|
||||
|
||||
def _no_docker_markers(monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"nextcloud_mcp_server.auth.webhook_routes.os.path.exists",
|
||||
@@ -46,8 +59,11 @@ def _docker_markers(monkeypatch):
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_webhook_internal_url_wins_over_everything(monkeypatch):
|
||||
monkeypatch.setenv("WEBHOOK_INTERNAL_URL", "https://internal.example.com")
|
||||
monkeypatch.setenv("NEXTCLOUD_MCP_SERVER_URL", "https://public.example.com")
|
||||
_patch_settings(
|
||||
monkeypatch,
|
||||
webhook_internal_url="https://internal.example.com",
|
||||
nextcloud_mcp_server_url="https://public.example.com",
|
||||
)
|
||||
_docker_markers(monkeypatch)
|
||||
|
||||
assert _get_webhook_uri() == "https://internal.example.com/webhooks/nextcloud"
|
||||
@@ -57,8 +73,9 @@ def test_webhook_internal_url_wins_over_everything(monkeypatch):
|
||||
def test_public_url_wins_over_docker_detection(monkeypatch):
|
||||
"""The bug-fix case: ECS containers have /.dockerenv but a public URL is
|
||||
set. Docker auto-detection must NOT clobber the explicit public URL."""
|
||||
monkeypatch.setenv(
|
||||
"NEXTCLOUD_MCP_SERVER_URL", "https://holy-bluegill.astrolabecloud.com"
|
||||
_patch_settings(
|
||||
monkeypatch,
|
||||
nextcloud_mcp_server_url="https://holy-bluegill.astrolabecloud.com",
|
||||
)
|
||||
_docker_markers(monkeypatch)
|
||||
|
||||
@@ -72,6 +89,7 @@ def test_public_url_wins_over_docker_detection(monkeypatch):
|
||||
def test_docker_detection_used_when_no_public_url(monkeypatch):
|
||||
"""docker-compose dev: no public URL set, /.dockerenv exists → use the
|
||||
docker-compose service name."""
|
||||
_patch_settings(monkeypatch)
|
||||
_docker_markers(monkeypatch)
|
||||
|
||||
assert _get_webhook_uri() == "http://mcp:8000/webhooks/nextcloud"
|
||||
@@ -79,6 +97,7 @@ def test_docker_detection_used_when_no_public_url(monkeypatch):
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_docker_detection_honors_service_name_and_port_overrides(monkeypatch):
|
||||
_patch_settings(monkeypatch)
|
||||
monkeypatch.setenv("NEXTCLOUD_MCP_SERVICE_NAME", "mcp-login-flow")
|
||||
monkeypatch.setenv("NEXTCLOUD_MCP_PORT", "8004")
|
||||
_docker_markers(monkeypatch)
|
||||
@@ -88,6 +107,7 @@ def test_docker_detection_honors_service_name_and_port_overrides(monkeypatch):
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_docker_container_env_var_triggers_docker_branch(monkeypatch):
|
||||
_patch_settings(monkeypatch)
|
||||
monkeypatch.setenv("DOCKER_CONTAINER", "true")
|
||||
_no_docker_markers(monkeypatch)
|
||||
|
||||
@@ -96,6 +116,7 @@ def test_docker_container_env_var_triggers_docker_branch(monkeypatch):
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_localhost_fallback_when_nothing_set(monkeypatch):
|
||||
_patch_settings(monkeypatch)
|
||||
_no_docker_markers(monkeypatch)
|
||||
|
||||
assert _get_webhook_uri() == "http://localhost:8000/webhooks/nextcloud"
|
||||
@@ -104,23 +125,15 @@ def test_localhost_fallback_when_nothing_set(monkeypatch):
|
||||
# --- webhook_auth_pair() --------------------------------------------------
|
||||
|
||||
|
||||
def _patch_secret(monkeypatch, secret: str | None) -> None:
|
||||
monkeypatch.setattr(
|
||||
webhook_routes,
|
||||
"get_settings",
|
||||
lambda: Settings(webhook_secret=secret),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_auth_pair_returns_none_when_secret_unset(monkeypatch):
|
||||
_patch_secret(monkeypatch, None)
|
||||
_patch_settings(monkeypatch, webhook_secret=None)
|
||||
assert webhook_auth_pair() == ("none", None)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_auth_pair_emits_bearer_header_when_secret_set(monkeypatch):
|
||||
_patch_secret(monkeypatch, "supersecret")
|
||||
_patch_settings(monkeypatch, webhook_secret="supersecret")
|
||||
assert webhook_auth_pair() == (
|
||||
"header",
|
||||
{"Authorization": "Bearer supersecret"},
|
||||
|
||||
Reference in New Issue
Block a user