thejayman77/PartsInventorySystem
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix upload concurrency, bound the request body, make backups consistent

Browse Source 
Four audit findings on the photo feature.

The upload route was async, so its blocking SQLite work ran on the event loop:
under contention it stalled every other request for SQLite's busy timeout, not
just its own. It also read the photo count without the write lock, so
overlapping uploads all observed the same total and stored past the ceiling
together. It is a synchronous endpoint now, running in the threadpool, taking
BEGIN IMMEDIATE before re-checking the part, the ceiling and the position, and
committing before it returns. Reverting either half makes the new test die with
the same TimeoutError the audit reported.

The 8MB cap protected nothing: Starlette parses and spools an entire multipart
body before a route's dependencies run — before the login check — so the bytes
were already on disk by the time anything rejected them, and an anonymous
caller could make us write them. A plain ASGI middleware outside routing now
refuses an over-large body first, and Caddy enforces the same ceiling at the
edge.

The documented backup captured the database and the photos at two different
moments while the app stayed writable, so a photo deleted in between left the
saved database pointing at a file the archive did not contain. tools/backup.sh
stops the app for the few seconds the copy takes and verifies afterwards that
every referenced photo is in the archive.

Cleanup could destroy data rather than merely litter: prune-images could delete
a file between an upload writing it and inserting its row, and deletions
unlinked before their transaction committed. Pruning now ignores anything under
an hour old unless forced, and deletes commit before unlinking — an orphaned
file is recoverable, a row without its photo is not. check-images reports drift
in both directions and fails only on the direction that loses data.

Checks go from 271 to 291.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jay
2026-08-25 09:57:18 -04:00
parent 96d2a1a087
commit a56cea6e2a
6 changed files with 362 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/admin.py
+58 -9
View File
@@ -5,6 +5,7 @@ cannot help with — a forgotten password — and for the initial handover.
docker exec -it parts python -m app.admin set-password
docker exec parts python -m app.admin show-status
docker exec parts python -m app.admin check-images
docker exec parts python -m app.admin prune-images
"""
@@ -41,25 +42,72 @@ def clear_password(_argv):
return 0
def prune_images(_argv):
# An upload writes its file before inserting the row, so for a moment a live
# file legitimately has no row. Pruning anything younger than this would delete
# a photo out from under a request that is still in flight.
PRUNE_MIN_AGE_SECONDS = 3600
def prune_images(argv):
"""Delete image files with no row pointing at them.
Uploads write the file before the row, so an ill-timed crash can strand
one. Nothing else creates orphans; this is a sweeper, not a routine step.
Only files older than an hour, unless --all is given — and --all is only
safe with the app stopped, because a file younger than its row is exactly
what an in-flight upload looks like.
"""
import os
import time
ignore_age = "--all" in argv
with db.session() as conn:
known = {
os.path.basename(db.image_path(r["token"], r["mime"]))
for r in conn.execute("SELECT token, mime FROM part_images").fetchall()
}
removed = 0
removed = skipped = 0
now = time.time()
for name in os.listdir(db.IMAGE_DIR):
if name not in known:
os.remove(os.path.join(db.IMAGE_DIR, name))
removed += 1
print(f"{removed} orphaned image file(s) removed; {len(known)} kept.")
if name in known:
continue
path = os.path.join(db.IMAGE_DIR, name)
if not ignore_age and now - os.path.getmtime(path) < PRUNE_MIN_AGE_SECONDS:
skipped += 1
continue
os.remove(path)
removed += 1
print(f"{removed} orphaned image file(s) removed; {len(known)} referenced file(s) kept.")
if skipped:
print(f"{skipped} skipped as too recent to be certain they are orphans "
f"(stop the app and re-run with --all to include them).")
return 0
def check_images(_argv):
"""Report drift in both directions between the database and the files."""
import os
with db.session() as conn:
rows = conn.execute("SELECT token, mime, part_id FROM part_images").fetchall()
expected = {os.path.basename(db.image_path(r["token"], r["mime"])): r for r in rows}
present = set(os.listdir(db.IMAGE_DIR))
missing = sorted(set(expected) - present)
orphans = sorted(present - set(expected))
print(f"{len(expected)} referenced photo(s), {len(present)} file(s) on disk")
for name in missing:
print(f" MISSING FILE part {expected[name]['part_id']} {name}")
for name in orphans:
print(f" orphan file {name}")
# A referenced photo with no file is data loss; a stray file is only clutter.
return 1 if missing else 0
def list_image_files(_argv):
"""Every filename the database expects to exist. Used by the backup script."""
import os
with db.session() as conn:
for r in conn.execute("SELECT token, mime FROM part_images ORDER BY id").fetchall():
print(os.path.basename(db.image_path(r["token"], r["mime"])))
return 0
@@ -79,7 +127,8 @@ def show_status(_argv):
COMMANDS = {"set-password": set_password, "clear-password": clear_password,
"show-status": show_status, "prune-images": prune_images}
"show-status": show_status, "prune-images": prune_images,
"check-images": check_images, "list-image-files": list_image_files}
def main(argv=None):
app/main.py
+97 -18
View File
@@ -32,6 +32,9 @@ STATIC_DIR = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file
# that nothing can fill the volume by accident.
MAX_IMAGE_BYTES = 8 * 1024 * 1024
MAX_IMAGES_PER_PART = 12
# Multipart framing adds boundaries and headers around the file, so the whole
# request is allowed slightly more than one image.
MAX_BODY_BYTES = MAX_IMAGE_BYTES + 1024 * 1024
CSP = (
"default-src 'self'; "
@@ -45,6 +48,62 @@ CSP = (
)
class BodySizeLimit:
"""Reject an over-large request before anything parses it.
Starlette parses and spools an entire multipart body *before* the route's
dependencies run, so a route-level cap — and the login check — both happen
after the bytes have already landed in the container's temporary storage.
A caller who doesn't know the password could still make us write them.
This is plain ASGI so it sits outside routing. Caddy enforces the same
ceiling at the edge; this is the backstop for anything that reaches the app
directly.
"""
def __init__(self, app, max_bytes: int):
self.app = app
self.max_bytes = max_bytes
async def __call__(self, scope, receive, send):
if scope["type"] != "http":
return await self.app(scope, receive, send)
for name, value in scope.get("headers") or []:
if name == b"content-length":
try:
declared = int(value)
except ValueError:
break
if declared > self.max_bytes:
return await self._too_large(send)
break
seen = 0
async def limited_receive():
nonlocal seen
message = await receive()
if message.get("type") == "http.request":
seen += len(message.get("body", b""))
if seen > self.max_bytes:
# Chunked upload with no declared length: cut the stream so
# the parser fails rather than letting it run unbounded.
return {"type": "http.request", "body": b"", "more_body": False}
return message
await self.app(scope, limited_receive, send)
async def _too_large(self, send):
body = json.dumps(
{"detail": f"Request body must be {self.max_bytes // (1024 * 1024)}MB or smaller"}
).encode()
await send({"type": "http.response.start", "status": 413,
"headers": [(b"content-type", b"application/json"),
(b"content-length", str(len(body)).encode())]})
await send({"type": "http.response.body", "body": body})
@asynccontextmanager
async def lifespan(_app: FastAPI):
db.init()
@@ -52,6 +111,8 @@ async def lifespan(_app: FastAPI):
app = FastAPI(title="Parts Inventory", docs_url=None, redoc_url=None, lifespan=lifespan)
# Added last, so it wraps everything else and runs before routing or body parsing.
app.add_middleware(BodySizeLimit, max_bytes=MAX_BODY_BYTES)
# --- validated field types --------------------------------------------------
@@ -609,6 +670,10 @@ def delete_part(part_id: int, conn=Depends(db.get_db)):
if cur.rowcount == 0:
raise HTTPException(404, "Part not found")
conn.execute("DELETE FROM parts_fts WHERE rowid = ?", (part_id,))
# Commit before unlinking. If the order were reversed and the commit failed,
# the rows would come back pointing at files that no longer exist — and a
# missing photo is unrecoverable, where a stranded file is just clutter.
conn.commit()
db.delete_image_files(images)
return {"deleted": part_id}
@@ -672,26 +737,24 @@ def list_images(part_id: int, conn=Depends(db.get_db)):
@app.post("/api/parts/{part_id}/images", dependencies=[Depends(auth.require_auth)],
status_code=201)
async def upload_image(
def upload_image(
part_id: int,
file: UploadFile = File(...),
caption: str = Form(default=""),
conn=Depends(db.get_db),
):
if conn.execute("SELECT 1 FROM parts WHERE id = ?", (part_id,)).fetchone() is None:
raise HTTPException(404, "Part not found")
"""Store one photo against a part.
existing = conn.execute(
"SELECT COUNT(*) AS n FROM part_images WHERE part_id = ?", (part_id,)
).fetchone()["n"]
if existing >= MAX_IMAGES_PER_PART:
raise HTTPException(409, f"A part can hold at most {MAX_IMAGES_PER_PART} images")
# Read with a ceiling rather than trusting Content-Length, which a client
# controls. One byte over the cap is enough to know it is too big.
Deliberately a synchronous endpoint. FastAPI runs these in the threadpool,
whereas an async one runs on the event loop — and this does blocking SQLite
work, so under contention it would stall every other request for the length
of SQLite's busy timeout rather than just queueing this one.
"""
# Read and sniff before taking any lock: the bytes are the slow part, and
# holding SQLite's write lock across them would serialise unrelated writes.
data = b""
while len(data) <= MAX_IMAGE_BYTES:
chunk = await file.read(256 * 1024)
chunk = file.file.read(256 * 1024)
if not chunk:
break
data += chunk
@@ -710,25 +773,39 @@ async def upload_image(
path = db.image_path(token, mime)
with open(path, "wb") as fh:
fh.write(data)
try:
nxt = conn.execute(
# Everything that reads-then-writes happens under the write lock: the
# existence check, the ceiling, and the position. Without it, overlapping
# uploads all observe the same count and sail past the limit together.
db.begin_immediate(conn)
if conn.execute("SELECT 1 FROM parts WHERE id = ?", (part_id,)).fetchone() is None:
raise HTTPException(404, "Part not found")
existing = conn.execute(
"SELECT COUNT(*) AS n FROM part_images WHERE part_id = ?", (part_id,)
).fetchone()["n"]
if existing >= MAX_IMAGES_PER_PART:
raise HTTPException(409, f"A part can hold at most {MAX_IMAGES_PER_PART} images")
position = conn.execute(
"SELECT COALESCE(MAX(position), -1) + 1 AS n FROM part_images WHERE part_id = ?",
(part_id,),
).fetchone()["n"]
conn.execute(
"INSERT INTO part_images(part_id, token, mime, bytes, caption, position) "
"VALUES (?,?,?,?,?,?)",
(part_id, token, mime, len(data), caption.strip()[:300], nxt),
)
conn.execute(
"UPDATE parts SET updated_at = datetime('now') WHERE id = ?", (part_id,)
(part_id, token, mime, len(data), caption.strip()[:300], position),
)
conn.execute("UPDATE parts SET updated_at = datetime('now') WHERE id = ?", (part_id,))
# Commit here rather than leaving it to dependency cleanup, so the write
# lock is released before the response is built rather than after.
conn.commit()
except Exception:
# Don't leave a file behind for a row that never landed.
db.delete_image_files([{"token": token, "mime": mime}])
raise
return {"token": token, "mime": mime, "bytes": len(data),
"caption": caption.strip()[:300], "position": nxt}
"caption": caption.strip()[:300], "position": position}
@app.get("/api/parts/{part_id}/images/{token}", dependencies=[Depends(auth.require_auth)])
@@ -770,6 +847,8 @@ def delete_image(part_id: int, token: str, conn=Depends(db.get_db)):
row = _image_row(conn, part_id, token)
conn.execute("DELETE FROM part_images WHERE part_id = ? AND token = ?", (part_id, token))
conn.execute("UPDATE parts SET updated_at = datetime('now') WHERE id = ?", (part_id,))
# Commit first: an orphaned file is recoverable, a row without its file is not.
conn.commit()
db.delete_image_files([row])
return {"deleted": token}