* Stop multi-file uploads from tripping the per-IP concurrency guard The /api/upload concurrency check summed its condition over `files`, but the condition didn't reference the loop variable — so it collapsed to len(files) whenever the IP had any recent upload. A single multi-file batch sent right after another upload therefore counted itself as N concurrent uploads and hit max_concurrent_uploads (3), returning 429. The browser swallows the 429 (no `files` in the body) and sends the chat with no attachments, so the model "doesn't even see" them (issue #1346). Count genuine recent upload events instead, via a pure count_recent_uploads() helper, independent of the current batch's file count. save_upload still enforces the per-minute sliding-window rate limit per file, so throttling is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Also reconcile the per-minute upload rate limit with the batch cap Follow-up within #1346: even after the concurrency-guard fix, a 6+ file batch still failed because save_upload() counts each file against upload_rate_limit (was 5/min) while the composer allows MAX_FILES=10 per batch — the reporter saw "5 attachments work, 6 fail". Raise the per-minute file cap to 60 so a single full batch (and a few of them) isn't self-rejected; burst abuse stays bounded by max_concurrent_uploads. Add a real 6-file regression + a config guard that the cap exceeds the frontend MAX_FILES. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
166 lines
5.8 KiB
Python
166 lines
5.8 KiB
Python
"""Regression tests for issue #1346 — attaching more than one file at once made
|
|
the model "not even see" the attachments.
|
|
|
|
Root cause: the per-IP concurrency guard in routes/upload_routes.py summed its
|
|
condition over `files`, and the condition didn't depend on the loop variable, so
|
|
it collapsed to `len(files)` whenever the IP had any recent upload. A multi-file
|
|
batch sent right after a single upload (the reporter's exact flow) therefore
|
|
counted itself as N concurrent uploads and tripped `max_concurrent_uploads`,
|
|
returning 429. The browser swallowed the 429 (no `files` in the body) and sent
|
|
the chat message with no attachments.
|
|
|
|
The fix counts genuine recent upload *events*, independent of the current
|
|
batch's file count. save_upload still enforces the per-minute rate limit.
|
|
"""
|
|
import io
|
|
import re
|
|
import types
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
from fastapi import APIRouter
|
|
|
|
from src.upload_handler import count_recent_uploads, UploadHandler
|
|
import routes.upload_routes as up
|
|
|
|
_REPO = Path(__file__).resolve().parent.parent
|
|
|
|
|
|
def test_count_recent_uploads_ignores_batch_size():
|
|
now = 1_000.0
|
|
# No prior uploads -> zero, regardless of how big the incoming batch is.
|
|
assert count_recent_uploads([], now) == 0
|
|
# Only events inside the window are counted.
|
|
assert count_recent_uploads([now - 1, now - 2, now - 3], now, window=10) == 3
|
|
assert count_recent_uploads([now - 1, now - 50], now, window=10) == 1
|
|
assert count_recent_uploads([now - 11], now, window=10) == 0
|
|
|
|
|
|
def _fake_handler():
|
|
h = types.SimpleNamespace()
|
|
h.upload_rate_log = {}
|
|
h.max_concurrent_uploads = 3
|
|
|
|
def save_upload(u, client_ip, owner=None):
|
|
# Mimic the real handler: every saved file logs a timestamp.
|
|
h.upload_rate_log.setdefault(client_ip, []).append(_NOW)
|
|
name = getattr(u, "filename", "f")
|
|
return {
|
|
"id": "0" * 32 + "." + "txt",
|
|
"name": name,
|
|
"mime": "text/plain",
|
|
"size": 1,
|
|
"hash": "h",
|
|
"uploaded_at": "now",
|
|
"width": None,
|
|
"height": None,
|
|
"is_duplicate": False,
|
|
}
|
|
|
|
h.save_upload = save_upload
|
|
return h
|
|
|
|
|
|
_NOW = 5_000.0
|
|
|
|
|
|
def _endpoint(router):
|
|
for r in router.routes:
|
|
if getattr(r, "path", None) == "/api/upload" and "POST" in getattr(r, "methods", set()):
|
|
return r.endpoint
|
|
raise AssertionError("upload endpoint not found")
|
|
|
|
|
|
def _request(ip="1.2.3.4", user="tester"):
|
|
return types.SimpleNamespace(
|
|
client=types.SimpleNamespace(host=ip),
|
|
state=types.SimpleNamespace(current_user=user),
|
|
)
|
|
|
|
|
|
def _files(n):
|
|
return [types.SimpleNamespace(filename=f"f{i}.txt") for i in range(n)]
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _reset_router(monkeypatch):
|
|
# Module-level router accumulates routes across setup calls; reset it.
|
|
monkeypatch.setattr(up, "router", APIRouter(prefix="/api/upload", tags=["upload"]))
|
|
# Freeze time so the seeded "recent upload" is deterministic.
|
|
monkeypatch.setattr(up.time, "time", lambda: _NOW)
|
|
|
|
|
|
async def test_multifile_after_a_recent_upload_is_not_rejected():
|
|
"""The bug: one prior upload + a 3-file batch -> 429. Must now succeed."""
|
|
h = _fake_handler()
|
|
h.upload_rate_log["1.2.3.4"] = [_NOW - 1] # step 1: a single file moments ago
|
|
up.setup_upload_routes(h)
|
|
endpoint = _endpoint(up.router)
|
|
|
|
result = await endpoint(_request(), _files(3))
|
|
|
|
assert [f["name"] for f in result["files"]] == ["f0.txt", "f1.txt", "f2.txt"]
|
|
|
|
|
|
async def test_fresh_multifile_upload_succeeds():
|
|
h = _fake_handler()
|
|
up.setup_upload_routes(h)
|
|
endpoint = _endpoint(up.router)
|
|
|
|
result = await endpoint(_request(), _files(5))
|
|
|
|
assert len(result["files"]) == 5
|
|
|
|
|
|
async def test_genuine_recent_volume_still_throttled():
|
|
"""The guard is preserved: enough genuine recent uploads still 429s."""
|
|
from fastapi import HTTPException
|
|
|
|
h = _fake_handler()
|
|
h.upload_rate_log["1.2.3.4"] = [_NOW - 1, _NOW - 2, _NOW - 3] # 3 recent events
|
|
up.setup_upload_routes(h)
|
|
endpoint = _endpoint(up.router)
|
|
|
|
with pytest.raises(HTTPException) as ei:
|
|
await endpoint(_request(), _files(1))
|
|
assert ei.value.status_code == 429
|
|
|
|
|
|
# ── #1346 follow-up: the per-minute rate limit must not reject a single
|
|
# full multi-file batch. The reporter found "5 attachments work, 6 fail":
|
|
# save_upload() counts each file against upload_rate_limit, which was 5 while
|
|
# the composer allows MAX_FILES=10. ──────────────────────────────────────────
|
|
|
|
def _max_files_from_frontend() -> int:
|
|
src = (_REPO / "static/js/fileHandler.js").read_text(encoding="utf-8")
|
|
m = re.search(r"MAX_FILES\s*=\s*(\d+)", src)
|
|
assert m, "MAX_FILES not found in fileHandler.js"
|
|
return int(m.group(1))
|
|
|
|
|
|
def test_rate_limit_accommodates_a_full_batch():
|
|
# The per-minute file cap must comfortably exceed the frontend batch cap,
|
|
# or a single legitimate multi-file attach trips it (issue #1346).
|
|
h = UploadHandler.__new__(UploadHandler)
|
|
UploadHandler.__init__(h, base_dir="/tmp", upload_dir="/tmp/_odysseus_test_uploads_cfg")
|
|
assert h.upload_rate_limit >= _max_files_from_frontend()
|
|
|
|
|
|
def test_six_file_batch_is_not_rate_limited(tmp_path):
|
|
from fastapi import HTTPException
|
|
|
|
h = UploadHandler(base_dir=str(tmp_path), upload_dir=str(tmp_path / "uploads"))
|
|
saved = 0
|
|
for i in range(6):
|
|
u = types.SimpleNamespace(
|
|
file=io.BytesIO(f"file number {i} unique content".encode()),
|
|
filename=f"f{i}.txt",
|
|
)
|
|
try:
|
|
meta = h.save_upload(u, client_ip="9.9.9.9", owner="tester")
|
|
except HTTPException as e:
|
|
raise AssertionError(f"file {i} rejected with {e.status_code}: {e.detail}")
|
|
assert meta and meta.get("id")
|
|
saved += 1
|
|
assert saved == 6
|