mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 20:29:00 +00:00
fix(backend): исправить CI-контракты upload (#625)
Issue: #625 User-Visible: no
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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',
|
||||
}],
|
||||
},
|
||||
{
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user