Skip to content

fix: admit already_stored to the uploads status constraint and delete path - #156

Open
Nic-dorman wants to merge 7 commits into
masterfrom
fix/already-stored-status
Open

fix: admit already_stored to the uploads status constraint and delete path#156
Nic-dorman wants to merge 7 commits into
masterfrom
fix/already-stored-status

Conversation

@Nic-dorman

Copy link
Copy Markdown
Member

⚠️ Stacked on #155 — merge that first, review the last commit only

Migration 014 must apply after #155's 013 (goose skips out-of-order versions, and the rebuilt table includes cache_key), so this branch is based on #155's head. Once #155 merges: gh pr update-branch (or local master-merge) collapses this diff to the final commit.

What

The already_stored status (V2-399 content-addressed dedup — a re-upload whose chunks were all already on the network) has been written by the worker since V2-399 shipped, but the schema never allowed it: both dialects' initial CHECK constraints omit it, so on any database built from the migrations the transition fails — the user sees "Failed to save upload record" after the network store succeeded. Unnoticed because e2e always uploads fresh content. Found by the #154 panel review.

Second half: Delete only allowed failed/completed, so an already_stored upload was permanently undeletable — which since #154/#155 also meant no erasure path for its row, DataMap, or cached bytes.

How

  • Migration 014: Postgres drops and re-adds uploads_status_check with the five statuses. SQLite rebuilds the table (CHECKs can't be altered); two sqlite subtleties handled: renaming uploads rewrites the FK clauses of every referencing table (file_tags, collection_files, transactions — all three rebuilt against the new table, the 011 pattern; legacy_alter_table is ignored inside goose's transaction), and index names follow the renamed table until it drops, so the uploads indexes are recreated only after uploads_old is gone. Down folds already_stored into completed.
  • Delete's status list gains already_stored (error text updated). Quota queries and the web UI already handled the status — verified, no changes needed there.

Tests

  • TestMarkAlreadyStoredPersists — the direct regression: both MarkAlreadyStored variants succeed against the freshly-migrated schema in both CI dialects (fails on the pre-014 constraint), and stamp cache_key.
  • TestDeleteAlreadyStoredUpload — deletable, row gone, purge-log fan-out runs like any other delete.
  • Full -race suite green in both dialects locally (sqlite) + CI (postgres).

🤖 Generated with Claude Code

Nic-dorman and others added 6 commits August 5, 2026 14:56
download_cache_private (default off) admits private uploads to the
download cache — read-through and seeding, keyed on the serve path's
DataMap derivation. Deletes now propagate to every instance: the delete
appends the upload's cache keys to cache_purge_log before any row is
removed, each instance's 1-minute sweep tick consumes the tail and
never advances past a failed unlink, and boot reconciliation validates
the whole cache directory against live rows (uploads.cache_key, stamped
at terminal statuses and backfilled at writer boot) for instances that
were down past log retention. Purge window: synchronous on the handling
instance, ~one tick everywhere else.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Boot reconciliation (correctly) purged the sweep tests' fixtures as
orphans before the eviction assertions ran — the entries stood in for
legitimately cached live content and now have matching rows, which is
also what production looks like.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Finding 1: the purge-log append and upload-row delete now commit in one
transaction — no interleaving lets a reconciling instance record a log
high-water mark, still see the live row, and miss the delete between.
A refused delete rolls its log rows back (regression-tested).

Finding 2: the full liveness reconciliation now re-runs every 12 hours
(and on demand) as the guaranteed backstop, so a purge whose log row
was pruned while its unlink was stuck is re-derived from live rows and
retried until the bytes are gone; a stuck key no longer delays later
purges — every entry is attempted per tick, only the high-water mark
waits for the contiguous prefix. Both panel reproductions are now
regression tests.

Finding 3: reconciliation is policy-aware — turning
download_cache_private off purges already-cached private plaintext
(next tick online, boot reconciliation for instances that were down)
instead of stranding it; no-op setting updates do no spurious work.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completes the panel's no-op-update matrix — false-to-false was already
asserted; both are the same non-transition branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… path

The V2-399 dedup status has been written by the worker since it
shipped, but both dialects' schema CHECK constraints never listed it —
on any database built from the migrations, a duplicate re-upload failed
at the status flip after the network store succeeded. It was also
absent from the delete path's allowed statuses, leaving such uploads
with no erasure path.

Migration 014 extends the CHECK. SQLite needs a table rebuild; renaming
uploads rewrites the FK clauses of file_tags, collection_files, and
transactions to follow the old table, so those three are rebuilt
against the new uploads (the 011 pattern), and the uploads indexes are
recreated only after uploads_old is dropped, which is when their names
free up.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…status

# Conflicts:
#	internal/services/upload.go
@dirvine

dirvine commented Aug 6, 2026

Copy link
Copy Markdown
Member

Hermes Agent review — Verdict: No blockers ✅

Reviewed at head fae661e. All CI green (Docker build+smoke, Frontend, Lint, Smoke, Test postgres, Test sqlite). Collapsed diff is exactly the 4 intended files (2 migrations + upload.go + test); the stacked-#155 merge resolved cleanly with no drift.

What this fixes (confirmed)

already_stored (V2-399 dedup) was written by the worker but never in the schema CHECK, so the status flip was rejected on any migration-built DB after a successful network store — the upload got stuck and surfaced as "Failed to save upload record". Migration 014 widens the constraint in both dialects; Delete now admits already_stored, restoring the erasure path (row + DataMap + cached bytes) that the #154/#155 purge-log depends on.

Checks that held

  • Migration integrity — recreated sqlite uploads table is column-for-column identical to 001 + cache_key (013); the three referencing tables rebuilt (file_tags, collection_files, transactions) are the complete set that reference uploads; collection_files keeps its composite PK + ON DELETE CASCADE; postgres constraint name uploads_status_check resolves; Down folds already_storedcompleted correctly.
  • Delete/erasure — the DELETE's status IN-list is the real terminal gate (no non-terminal bypass); purge-log insert sits in the same transaction before the row delete; CacheKeys() covers both data_map (private) and datamap_address (public) derivations; quota is computed live so deletion correctly frees usage.
  • Security — authorization unchanged (handler-enforced, uploads.go); all SQL parameterized; cache keys are SHA-256 digests. No injection/integrity surface.

Non-blocking follow-ups

  1. Release note (recommended): DBs that ran the worker pre-014 accumulated dedup re-uploads in failed state. Migration 014 does not touch those rows — operators/users must manually Retry them once for MarkAlreadyStored to succeed.
  2. Minor test gaps (optional): the Down-migration fold (already_storedcompleted) has no row-level assertion, and the public variant's delete→purge path isn't directly tested. Low-stakes — the core regressions (status-flip persistence, delete admission) are covered in both CI dialects.

…delete test

Review follow-ups from #156: operators with pre-014 databases have dedup
re-uploads stuck in failed state that migration 014 does not rewrite —
one manual Retry completes them at zero cost. Plus the public-variant
delete/purge-fan-out regression.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Nic-dorman

Copy link
Copy Markdown
Member Author

Both follow-ups handled in the new head:

  1. Release note — added an [Unreleased] CHANGELOG entry covering the fix and the operator note verbatim: pre-014 databases hold dedup re-uploads stuck in failed; migration 014 does not rewrite them, and a single Retry completes them at zero cost (dedup re-Prepare).
  2. Public delete/purge testTestDeleteAlreadyStoredPublicUpload covers the address-derived purge fan-out.

On the Down-migration row-level assertion: deliberately skipped — this repo's test harness never exercises goose Down migrations, and introducing that machinery for one CASE expression is out of proportion; the fold is documented in the migration and reviewed above.

CI re-running on the new head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants