Ingest partial-write orphans, missing unique constraint, naive timestamps #91
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: LOW-MEDIUM
The problems
Three small integrity issues in the ingest path, grouped because they share a fix area.
1. Files are written before the transaction commits.
services/ingest.py:85,91callsstorage.putfor both scans; the route commits atroutes/ingest.py:79. If the commit fails, thefiles persist with no database row — invisible orphans occupying disk forever. There is no
reconciliation job.
docs/circa-spec.md§6.2 requires that "all multi-step operations usedatabase transactions; partial writes are rolled back," which filesystem writes cannot join as
currently structured.
2. Duplicate detection has no unique constraint behind it. The SHA-256 lookup is a plain
query, so two concurrent identical uploads can both pass the check and create two "canonical"
rows.
3.
datetime.utcnow()is used throughout (models/models.py:41-42and roughly a dozen callsites). It is timezone-naive and deprecated since Python 3.12. Values serialize with no offset, so
the frontend's
new Date(...)reads them as local time and every displayed timestamp isshifted by the viewer's UTC offset. Audit timestamps are evidentiary, and keyset pagination orders
on
created_at, so ambiguity here has real consequences.Fix
staged file if the commit fails.
(#47).
(collection_id, sha256).datetime.now(timezone.utc)throughout, with a migration annotating existing rows asUTC. Fold into the response-model work so the API emits trailing-
ZISO strings.Done when
References
backend/app/services/ingest.py:85,91;backend/app/api/routes/ingest.py:79backend/app/models/models.py:41-42docs/circa-spec.md§6.2Related: #47 (integrity checker), and the response-models issue.
Done in
97e4643. All four "done when" items covered, 21 tests inbackend/tests/test_ingest_integrity.py.1. Orphan files. Ingest reports the keys it created and the route deletes them if the commit raises. The subtlety worth recording: storage is content-addressed, so a second upload of identical bytes maps onto the first photo's file and
put()is a no-op. Reporting that key as "written" would mean a rollback deleted a file another photo depends on — turning a harmless failed upload into data loss.put()now returns whether it actually created the object, and only genuinely new keys are tracked. There's a test for that specifically, because the naive version of this fix is worse than the bug.I kept file-then-commit rather than the staging-then-move ordering the issue suggested. The two orderings trade different failure modes: file-first risks an orphan, commit-first risks a row pointing at a missing photograph. For an archive the first is plainly better — an orphan wastes disk, a dangling row loses a picture. A crash between write and cleanup still leaves an orphan; that's #47's job.
put()also copies to a temp name and renames, so a crash mid-copy can't leave a truncated file at a content-addressed key, where its name would assert a hash its bytes don't have.2. The unique constraint is partial, and that matters:
(collection_id, sha256) WHERE duplicate_of IS NULL. Duplicates are supposed to share a hash — ingest creates a row for the duplicate and links it viaduplicate_of— so the plain unique constraint the issue described would have rejected the exact case the schema exists to record. Only canonical rows are constrained. The loser of a race re-reads the winner and records itself as a duplicate, which is what it would have done had it run a moment later.3. Timestamps — I deviated from the issue here, deliberately. The issue says "switch to
datetime.now(timezone.utc)throughout". I probed it first, and it doesn't work the way it reads: SQLite discardstzinfoon the way in and on the way out, soDateTime(timezone=True)stores an identical naive string and hands back a naive datetime. Aware values would be written by the app and read back naive, and every Python-side comparison mixing the two —session.expires_at - nowin the session renewal path, for one — would raiseTypeError: can't compare offset-naive and offset-aware datetimes. That's a worse bug than the one being fixed, and it surfaces on the paths that happen to compare rather than at the mistake.So storage stays naive UTC by documented convention, enforced by there being exactly one way to get the time (
app/timeutil.py), with a test that greps the app for straydatetime.utcnow()so it can't quietly erode. Correctness lands at the boundary where the ambiguity actually hurt:to_iso_zemits the trailingZ, sonew Date(...)stops reading timestamps as local time. That was the real user-visible bug — every timestamp in the UI was shifted by the viewer's UTC offset. No data migration is needed since the stored representation is unchanged.If this ever moves to PostgreSQL,
timestamptzbecomes real and this should be revisited; the single helper is what makes that a small change. Say if you'd rather I force aware columns now anyway.Not done: the orphan-reconciliation job. That's a periodic sweep comparing storage against the database, and it pairs with the file integrity checker in #47 as the issue notes. It needs the worker runtime (#2) to have somewhere to live. The cleanup path here handles the ordinary case — a constraint violation at commit — which is what was actually reachable.