|
1 | | -"""Issue #589 regression test for webhooks.py Redis client switch. |
| 1 | +"""Webhook rate-limit regression tests. |
2 | 2 |
|
3 | | -The fix in src/backend/routers/webhooks.py replaced |
4 | | - redis.Redis(host="redis", port=6379) |
5 | | -with |
6 | | - redis.from_url(REDIS_URL) |
7 | | -so the credentials embedded in REDIS_URL are actually used. Without this |
8 | | -test, a regression would silently fail-open and rate limiting would be |
9 | | -disabled. |
| 3 | +Two scenarios covered against the live stack: |
10 | 4 |
|
11 | | -Self-contained: creates an agent + schedule + webhook token inline so a |
12 | | -fresh token is used (no pre-existing rate-limit state). |
| 5 | +1. **Sequential** (#589) — verifies the Redis client uses the credentialed |
| 6 | + `REDIS_URL` so rate limiting is actually engaged (the historic regression |
| 7 | + was a silent fail-open on bad auth). |
| 8 | +2. **Concurrent** (#644) — verifies the limiter is TOCTOU-safe: firing |
| 9 | + `WEBHOOK_RATE_LIMIT + 5` simultaneous requests must not let more than |
| 10 | + `WEBHOOK_RATE_LIMIT` slip through. The previous read-then-INCR path |
| 11 | + allowed each concurrent caller to observe `count < limit` before any |
| 12 | + of them incremented — so the actual call rate exceeded the budget by |
| 13 | + the concurrency factor. |
13 | 14 |
|
14 | | -Marked `integration` (not `smoke`) because it needs the full stack — |
| 15 | +Both tests build their own agent + schedule + webhook token so they don't |
| 16 | +share rate-limit state with each other. |
| 17 | +
|
| 18 | +Marked `integration` (not `smoke`) because they need the full stack — |
15 | 19 | backend + Redis with auth + scheduler service. The smoke runner targets |
16 | | -~30s and excludes Docker-dependent tests; this goes through |
| 20 | +~30s and excludes Docker-dependent tests; these go through |
17 | 21 | tests/run-integration.sh. |
18 | 22 | """ |
19 | 23 |
|
| 24 | +import asyncio |
20 | 25 | import uuid |
21 | 26 |
|
22 | 27 | import httpx |
@@ -77,3 +82,79 @@ def test_webhook_rate_limit_returns_429_after_threshold(api_client: TrinityApiCl |
77 | 82 | ) |
78 | 83 | finally: |
79 | 84 | api_client.delete(f"/api/agents/{agent_name}") |
| 85 | + |
| 86 | + |
| 87 | +@pytest.mark.integration |
| 88 | +def test_webhook_rate_limit_holds_under_concurrency(api_client: TrinityApiClient): |
| 89 | + """Concurrent regression for #644. |
| 90 | +
|
| 91 | + Fire `WEBHOOK_RATE_LIMIT + 5` requests simultaneously. The pre-fix |
| 92 | + read-then-INCR path could let all N callers observe `count < limit` |
| 93 | + before any incremented, exceeding the limit by N. After the |
| 94 | + INCR-then-compare fix, at most `WEBHOOK_RATE_LIMIT` calls get a |
| 95 | + non-429 response. |
| 96 | + """ |
| 97 | + agent_name = f"test-644-webhook-{uuid.uuid4().hex[:8]}" |
| 98 | + |
| 99 | + create_resp = api_client.post("/api/agents", json={"name": agent_name}) |
| 100 | + if create_resp.status_code not in (200, 201): |
| 101 | + pytest.skip(f"Cannot create test agent: {create_resp.text}") |
| 102 | + |
| 103 | + try: |
| 104 | + sched_resp = api_client.post( |
| 105 | + f"/api/agents/{agent_name}/schedules", |
| 106 | + json={ |
| 107 | + "name": f"wh-{uuid.uuid4().hex[:6]}", |
| 108 | + "cron_expression": "0 0 1 1 *", # never fires during tests |
| 109 | + "message": "noop", |
| 110 | + "enabled": True, |
| 111 | + "timezone": "UTC", |
| 112 | + }, |
| 113 | + ) |
| 114 | + assert sched_resp.status_code == 201, sched_resp.text |
| 115 | + sid = sched_resp.json()["id"] |
| 116 | + |
| 117 | + gen_resp = api_client.post( |
| 118 | + f"/api/agents/{agent_name}/schedules/{sid}/webhook" |
| 119 | + ) |
| 120 | + assert gen_resp.status_code == 200, gen_resp.text |
| 121 | + webhook_url = gen_resp.json()["webhook_url"] |
| 122 | + token = webhook_url.split("/api/webhooks/")[1] |
| 123 | + |
| 124 | + url = f"http://localhost:8000/api/webhooks/{token}" |
| 125 | + n_concurrent = WEBHOOK_RATE_LIMIT + 5 |
| 126 | + |
| 127 | + async def fire_all(): |
| 128 | + async with httpx.AsyncClient(timeout=10.0) as client: |
| 129 | + tasks = [client.post(url) for _ in range(n_concurrent)] |
| 130 | + return await asyncio.gather(*tasks, return_exceptions=True) |
| 131 | + |
| 132 | + results = asyncio.run(fire_all()) |
| 133 | + |
| 134 | + statuses = [] |
| 135 | + for r in results: |
| 136 | + if isinstance(r, Exception): |
| 137 | + pytest.fail(f"Concurrent webhook call raised: {r!r}") |
| 138 | + statuses.append(r.status_code) |
| 139 | + |
| 140 | + accepted = sum(1 for s in statuses if s in (202, 503)) |
| 141 | + rate_limited = sum(1 for s in statuses if s == 429) |
| 142 | + other = [s for s in statuses if s not in (202, 503, 429)] |
| 143 | + |
| 144 | + assert not other, f"Unexpected statuses: {other} (full set: {statuses})" |
| 145 | + assert accepted <= WEBHOOK_RATE_LIMIT, ( |
| 146 | + f"{accepted} calls succeeded under {n_concurrent}-way concurrency, " |
| 147 | + f"limit is {WEBHOOK_RATE_LIMIT}. TOCTOU race regressed: {statuses}" |
| 148 | + ) |
| 149 | + # Sanity: at least some made it through (otherwise the test isn't |
| 150 | + # exercising the limiter — e.g., backend down). |
| 151 | + assert accepted >= 1, ( |
| 152 | + f"No requests accepted ({statuses}) — limiter or backend broken" |
| 153 | + ) |
| 154 | + # And at least one was rate-limited (proves the limiter ran). |
| 155 | + assert rate_limited >= 1, ( |
| 156 | + f"No 429 in {n_concurrent}-way burst with limit {WEBHOOK_RATE_LIMIT} — " |
| 157 | + f"limiter not engaging: {statuses}" |
| 158 | + ) |
| 159 | + finally: |
| 160 | + api_client.delete(f"/api/agents/{agent_name}") |
0 commit comments