From 15bc6bcf5a69ef3423badfc61311dd0efc072030 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 13:54:20 +0300 Subject: [PATCH] =?UTF-8?q?fix(backend):=20=D0=B8=D1=81=D0=BF=D1=80=D0=B0?= =?UTF-8?q?=D0=B2=D0=B8=D1=82=D1=8C=20CI-=D0=BA=D0=BE=D0=BD=D1=82=D1=80?= =?UTF-8?q?=D0=B0=D0=BA=D1=82=D1=8B=20upload=20(#625)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue: #625 User-Visible: no --- custom_components/houseplan/http_api.py | 38 +++++++++------- scripts/mutation-registry.mjs | 18 +++++--- tests_backend/test_ha_upload.py | 36 ++++++++------- tests_backend/test_ha_websocket.py | 60 ++++++++++++++++++++++--- 4 files changed, 107 insertions(+), 45 deletions(-) diff --git a/custom_components/houseplan/http_api.py b/custom_components/houseplan/http_api.py index 246f8abb..635297a8 100644 --- a/custom_components/houseplan/http_api.py +++ b/custom_components/houseplan/http_api.py @@ -369,10 +369,12 @@ class HouseplanUploadView(HomeAssistantView): if runtime is None: return web.json_response({"error": "not_ready"}, status=503) - # Content-Length includes small multipart overhead, making it a safe - # conservative upper bound. Reject impossible requests before reading - # or creating a temporary file; the exact staged size is checked again - # under the same lock immediately before promotion. + # Content-Length includes multipart boundaries and headers, not just + # file bytes. Reserve one streaming batch for that overhead and only + # preflight the guaranteed payload floor; otherwise a tiny file at the + # exact store boundary would be rejected by its envelope (#498). The + # exact staged size is checked again under the same lock immediately + # before promotion. declared_size = getattr(request, "content_length", None) if declared_size is not None and declared_size > 0: if declared_size > MAX_FILE_BYTES + _FLUSH_AT: @@ -380,21 +382,23 @@ class HouseplanUploadView(HomeAssistantView): {"error": "too_large", "max_mb": MAX_FILE_BYTES // 1024 // 1024}, status=413, ) - try: - async with runtime.upload_lock: - await hass.async_add_executor_job( - partial( - check_quota, - files_root, - declared_size, - MAX_FILES_BYTES, - MAX_FILES_COUNT, + payload_floor = max(0, declared_size - _FLUSH_AT) + if payload_floor: + try: + async with runtime.upload_lock: + await hass.async_add_executor_job( + partial( + check_quota, + files_root, + payload_floor, + MAX_FILES_BYTES, + MAX_FILES_COUNT, + ) ) + except QuotaError as err: + return web.json_response( + {"error": err.reason, "detail": err.detail}, status=507 ) - except QuotaError as err: - return web.json_response( - {"error": err.reason, "detail": err.detail}, status=507 - ) marker_id = "misc" filename: str | None = None # Every temporary file this request creates, promoted or not. The outer diff --git a/scripts/mutation-registry.mjs b/scripts/mutation-registry.mjs index a5539622..fc9303a9 100644 --- a/scripts/mutation-registry.mjs +++ b/scripts/mutation-registry.mjs @@ -10086,16 +10086,20 @@ const MUTANT_DEFINITIONS = [ }], }, { - id: 'quota-ignores-foreign-staged-uploads', + id: 'quota-check-and-promotion-are-not-serialized', guard: 'node scripts/backend-test-guard.mjs ' - + 'issue_498_concurrent_uploads_still_count_each_other ' + + 'issue_625_concurrent_uploads_serialize_exact_quota_check ' + 'tests_backend/test_ha_upload.py', - because: 'only the caller\'s own staged file is exempt: skipping every .upload-* would let two ' - + 'concurrent uploads pass a quota neither of them fits alone (#498 AC1)', + because: 'the exact quota decision and promotion must share one bounded critical section; ' + + 'otherwise concurrent uploads can both decide against stale usage (#625 AC6)', patches: [{ - file: 'custom_components/houseplan/plans.py', - find: ' if exclude is not None and item == exclude:\n', - replace: ' if item.name.startswith(TMP_PREFIX): # mutant: every staged file is invisible\n', + file: 'custom_components/houseplan/http_api.py', + find: ' try:\n' + + ' async with runtime.upload_lock:\n' + + ' name = await hass.async_add_executor_job(_check_and_promote)\n', + replace: ' try:\n' + + ' if True: # mutant: quota check and promotion race\n' + + ' name = await hass.async_add_executor_job(_check_and_promote)\n', }], }, { diff --git a/tests_backend/test_ha_upload.py b/tests_backend/test_ha_upload.py index e60875e9..b93cfdc1 100644 --- a/tests_backend/test_ha_upload.py +++ b/tests_backend/test_ha_upload.py @@ -105,33 +105,38 @@ async def test_issue_498_upload_accepts_the_last_bytes_and_the_last_file_of_the_ assert sorted(p.name for p in (root / "m1").iterdir()) == ["a.pdf", "b.pdf"] -async def test_issue_498_concurrent_uploads_still_count_each_other( +async def test_issue_625_concurrent_uploads_serialize_exact_quota_check( hass: HomeAssistant, hass_client: ClientSessionGenerator, monkeypatch, ) -> None: - """#498 AC1: excluding one's own staged file must not hide the neighbour's. - - Both uploads are held at the quota check while both staged files exist. Each - sees the other's `.upload-*` as usage, so together they cannot exceed the - quota; a check that ignored every staged file would promote both. - """ + """#625 AC6: concurrent uploads cannot race their final quota decisions.""" import asyncio import threading + import time from custom_components.houseplan import http_api as hp_http - from custom_components.houseplan import plans as hp_plans await _setup(hass) client = await hass_client() monkeypatch.setattr(hp_http, "MAX_FILES_BYTES", 1000) - real_check = hp_plans.check_quota - barrier = threading.Barrier(2, timeout=5) + real_check = hp_http.check_quota + state_lock = threading.Lock() + active = 0 + max_active = 0 - def both_staged_check(*args, **kwargs): - barrier.wait() - return real_check(*args, **kwargs) + def observed_check(*args, **kwargs): + nonlocal active, max_active + with state_lock: + active += 1 + max_active = max(max_active, active) + try: + time.sleep(0.05) + return real_check(*args, **kwargs) + finally: + with state_lock: + active -= 1 - monkeypatch.setattr(hp_http, "check_quota", both_staged_check) + monkeypatch.setattr(hp_http, "check_quota", observed_check) responses = await asyncio.gather( client.post("/api/houseplan/upload", data=_pdf_form("a.pdf", 600)), client.post("/api/houseplan/upload", data=_pdf_form("b.pdf", 600)), @@ -139,13 +144,14 @@ async def test_issue_498_concurrent_uploads_still_count_each_other( statuses = sorted(response.status for response in responses) assert statuses != [200, 200], "1200 bytes would be stored against a 1000-byte quota" assert all(status in (200, 507) for status in statuses), [await r.text() for r in responses] + assert max_active == 1 from pathlib import Path from custom_components.houseplan.const import FILES_DIR root = Path(hass.config.path(FILES_DIR)) - assert not list(root.glob(hp_plans.TMP_PREFIX + "*")) + assert not list(root.glob(hp_http.TMP_PREFIX + "*")) stored = sum(p.stat().st_size for p in (root / "m1").iterdir()) if (root / "m1").is_dir() else 0 assert stored <= 1000 diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index cfd9ff95..eebe3d13 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -620,8 +620,12 @@ async def test_issue_340_config_set_without_revision_is_bootstrap_only( assert await runtime.config_store.async_load() == stored_before assert (await runtime.store.async_load())[OPTIMIZE_BACKUP] == backup assert config_events == [] - assert "write rejected" in caplog.text - assert "stale-secret" not in caplog.text + ws_records = [ + record for record in caplog.records + if record.name == "custom_components.houseplan.websocket_api" + ] + assert any("write rejected" in record.getMessage() for record in ws_records) + assert all("stale-secret" not in record.getMessage() for record in ws_records) # Even an exact semantic no-op may not be used to bypass the CAS guard. with caplog.at_level(logging.DEBUG, logger="custom_components.houseplan.websocket_api"): @@ -634,7 +638,11 @@ async def test_issue_340_config_set_without_revision_is_bootstrap_only( assert noop_without_revision["error"]["code"] == "conflict" assert await runtime.config_store.async_load() == stored_before assert config_events == [] - assert sum("config/set without expected_rev" in record.message for record in caplog.records) == 1 + assert sum( + "config/set without expected_rev" in record.getMessage() + for record in caplog.records + if record.name == "custom_components.houseplan.websocket_api" + ) == 1 # The same client succeeds after reading and returning the current rev. await stale_client.send_json_auto_id({ @@ -703,8 +711,12 @@ async def test_issue_356_layout_set_without_revision_is_bootstrap_only( assert "revision is required" in rejected["error"]["message"].lower() assert await runtime.store.async_load() == stored_before assert layout_events == [] - assert "write rejected" in caplog.text - assert "stale-secret" not in caplog.text + ws_records = [ + record for record in caplog.records + if record.name == "custom_components.houseplan.websocket_api" + ] + assert any("write rejected" in record.getMessage() for record in ws_records) + assert all("stale-secret" not in record.getMessage() for record in ws_records) # An equal body is still a write attempt and must not bypass the CAS guard. with caplog.at_level(logging.DEBUG, logger="custom_components.houseplan.websocket_api"): @@ -717,7 +729,11 @@ async def test_issue_356_layout_set_without_revision_is_bootstrap_only( assert noop_without_revision["error"]["code"] == "conflict" assert await runtime.store.async_load() == stored_before assert layout_events == [] - assert sum("layout/set without expected_rev" in record.message for record in caplog.records) == 1 + assert sum( + "layout/set without expected_rev" in record.getMessage() + for record in caplog.records + if record.name == "custom_components.houseplan.websocket_api" + ) == 1 # Reading and returning the current revision preserves the ordinary path. await stale_client.send_json_auto_id({ @@ -3216,6 +3232,38 @@ async def test_attachment_upload_rejects_impossible_content_length_before_multip assert multipart_called is False +async def test_attachment_upload_rejects_impossible_quota_before_multipart( + hass: HomeAssistant, monkeypatch, +) -> None: + """#625 AC5: a payload floor over quota never creates or reads a part.""" + from custom_components.houseplan import http_api + from custom_components.houseplan.http_api import HouseplanUploadView + + await _setup(hass) + monkeypatch.setattr(http_api, "MAX_FILES_BYTES", 0) + multipart_called = False + + class _User: + is_admin = True + + class _Request: + app = {http_api.KEY_HASS: hass} + content_length = http_api._FLUSH_AT + 1 + + def get(self, _key, default=None): + return _User() + + async def multipart(self): + nonlocal multipart_called + multipart_called = True + raise AssertionError("multipart must not be read after early rejection") + + response = await HouseplanUploadView().post(_Request()) + assert response.status == 507 + assert json.loads(response.text)["error"] == "quota_exceeded" + assert multipart_called is False + + async def test_upload_leaves_no_temporary_behind( hass: HomeAssistant, hass_ws_client: WebSocketGenerator, hass_client, monkeypatch ) -> None: