Fix issues #002-#006 found by the LLM test harness
- #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>
This commit is contained in:
parent
c1ea5c7990
commit
07328a3412
13 changed files with 635 additions and 35 deletions
81
issues/006-failed-post-still-evicts-artifacts.md
Normal file
81
issues/006-failed-post-still-evicts-artifacts.md
Normal file
|
|
@ -0,0 +1,81 @@
|
|||
# 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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue