peter
07328a3412
- #006 (high, data loss): make_room_for() no longer deletes unkept artifacts on a doomed request - checks whether eviction can possibly succeed before deleting anything. Also fixed a related latent bug found while testing: the function returned evicted Artifact objects after calling .delete() on them, so their .id was always None (Django clears pk on delete); now returns ids captured beforehand. - #003: added server-side field-length validation (source/title/ source_name/source_id/theme) on POST and PATCH, matching the model's own max_length limits, which were previously unenforced on the ingest path entirely. - #004: ported daybook_post.py's content-shape validation (html_fragment/html_full/deck/markdown) to the server, closing the gap for any client that doesn't go through the helper script. - #005: PATCH now rejects wrong-typed `tags` (previously silently wiped existing tags) and rejects immutable fields (content/source/kind/day) with 400 instead of silently no-op'ing them. - #002: deleted the misleading skills/daybook-posting/.env.example (a zero-dependency script that never reads .env files) and clarified SKILL.md/docs/llm_posting_helper.md that these must be real process/shell env vars. Adds tests/unit/ for storage-layer tests. 46 tests passing (was 25). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
81 lines
3.6 KiB
Markdown
81 lines
3.6 KiB
Markdown
# 006 — A `POST` that ultimately fails with 413 still permanently deletes unkept artifacts
|
|
|
|
**Status:** Fixed
|
|
**Severity:** High — irreversible data loss on a failed request
|
|
|
|
## Description
|
|
|
|
`make_room_for()` (`artifacts/storage.py`) is called from `create_text_artifact`
|
|
/`create_binary_artifact` *before* the new artifact is persisted. When the
|
|
incoming artifact doesn't fit even after evicting every unkept artifact, it
|
|
raises `QuotaExceeded` and the view returns `413`. But the eviction loop
|
|
inside `make_room_for` deletes each unkept artifact **as it iterates**,
|
|
*before* it can know whether the eventual total will succeed:
|
|
|
|
```python
|
|
for artifact in unkept:
|
|
if used + incoming_bytes <= quota:
|
|
break
|
|
used -= artifact_total_bytes(artifact)
|
|
delete_artifact_files(artifact)
|
|
artifact.delete()
|
|
evicted.append(artifact)
|
|
|
|
if used + incoming_bytes > quota:
|
|
raise QuotaExceeded("Quota is full of kept artifacts; free space to post.")
|
|
```
|
|
|
|
If the loop runs out of unkept artifacts to evict and still hasn't freed
|
|
enough room (because the remaining `is_kept=True` artifacts alone exceed
|
|
the new/current quota), it raises *after* having already deleted every
|
|
unkept artifact — there's no rollback. The caller gets a `413` that reads
|
|
like "nothing happened, free up space and retry," but in reality **every
|
|
unkept artifact the user had was just destroyed** for a request that
|
|
still failed.
|
|
|
|
Reproduced against the `test` account (quota temporarily lowered to
|
|
demonstrate an unwinnable state — 1 kept artifact at 851 bytes vs. an
|
|
800-byte quota):
|
|
|
|
```
|
|
BEFORE: 1 kept artifact (851B) + 5 unkept artifacts (582/321/311/401/428B)
|
|
POST small markdown -> 413 "Quota is full of kept artifacts; free space to post."
|
|
AFTER: only the 1 kept artifact remains — all 5 unkept artifacts are gone
|
|
(both DB rows and on-disk files, confirmed via Django shell)
|
|
```
|
|
|
|
This is a normal, reachable state — not just an artificially shrunk quota.
|
|
Any user whose kept artifacts alone approach their quota will hit this the
|
|
moment they try to post anything that doesn't fit, and lose every unkept
|
|
artifact they had in the process, with a `413` response giving no
|
|
indication that a destructive side effect occurred.
|
|
|
|
## Suggested Fix
|
|
|
|
`make_room_for` should compute whether eviction *can* succeed before
|
|
deleting anything — e.g. sum `unkept` sizes up front and compare
|
|
`used - unkept_total + incoming_bytes` against `quota`; raise
|
|
`QuotaExceeded` immediately if even a hypothetical full eviction wouldn't
|
|
fit, without touching any artifact. Only perform actual deletions once
|
|
it's known they'll be sufficient. This makes the failure path side-effect
|
|
free, matching what a `413` response implies.
|
|
|
|
## Resolution
|
|
|
|
Fixed in `artifacts/storage.py`'s `make_room_for`: computes the total
|
|
recoverable bytes across all unkept artifacts up front and raises
|
|
`QuotaExceeded` immediately if even a full eviction wouldn't fit — no
|
|
artifact is touched on that path. The incremental eviction loop only runs
|
|
once success is already guaranteed.
|
|
|
|
While testing this fix, found and fixed a related latent bug: the function
|
|
returned the evicted `Artifact` *objects* after calling `.delete()` on them,
|
|
but Django clears an instance's pk on delete, so every returned object's
|
|
`.id` was silently `None`. Nothing currently reads this return value, so it
|
|
had no live impact, but it broke the function's own documented contract.
|
|
Now returns the evicted ids (captured before deletion) instead of objects.
|
|
|
|
Regression tests added in `tests/unit/test_storage.py`: an unwinnable
|
|
eviction leaves all unkept artifacts intact, a successful eviction only
|
|
evicts as many (oldest-first) as needed, and an over-quota single artifact
|
|
deletes nothing.
|