mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
xarmian
2bb7ac35e4
* feat(attachments): orphan GC sweep with periodic scheduler (TASK-886)
Background job that reclaims attachments past the grace period. Two
qualification criteria, both with a 30-day default grace:
- item_id IS NULL AND deleted_at IS NULL AND created_at < cutoff
(never-attached uploads — editor uploaded then tab-closed before
attaching to an item)
- deleted_at IS NOT NULL AND deleted_at < cutoff
(soft-deleted via the Settings → Storage delete button or the
DELETE /attachments/{id} endpoint)
Reclamation is dedupe-aware: content-addressed storage means the same
hash can be referenced by multiple rows, so the on-disk blob is only
removed when the GC'd row is the LAST live reference to its
content_hash. Otherwise the row drops and the blob stays for the
remaining references. CountLiveAttachmentsForHash is the predicate.
Per-row failures (resolve backend, blob delete, hard-delete) are
logged and skipped; the sweep keeps making progress. Catastrophic
errors (DB failure) return up to the loop, which logs and waits for
the next tick rather than crashing the server.
Lifecycle:
- SetOrphanGCConfig overrides the default 24h interval / 30-day
grace. cmd/pad reads PAD_ORPHAN_GC_INTERVAL / PAD_ORPHAN_GC_GRACE
(Go duration syntax — 1m, 24h, 720h) so operators can tune
without recompiling and tests can crank the interval down to 1ms
to see sweeps land in CI.
- StartOrphanGC kicks the loop. Idempotent — second call is a
no-op so a misconfigured caller can't double-spawn.
- Server.Stop() now signals the loop via stopOrphanGC() before
s.bg.Wait(), so process shutdown drains the goroutine cleanly
(BUG-842 invariant).
- Each tick wraps the sweep in a 30m context timeout so a slow
scan can't pin the goroutine across multiple intervals.
Tests:
- TestOrphanGC_ReclaimsSoftDeleted: upload → soft-delete → sweep
with future cutoff → DB row gone + blob gone from FSStore.
- TestOrphanGC_ReclaimsLongOrphans: upload → backdate created_at
31d → sweep with 30d grace cutoff → row reclaimed.
- TestOrphanGC_KeepsRecentRows: upload → soft-delete → sweep with
past cutoff → row stays. Catches a typo in the WHERE clause that
would silently destroy live attachments.
- TestOrphanGC_PreservesSharedBlob: two uploads with identical
bytes (same hash, same blob), soft-delete only one → sweep →
one row reclaimed BUT BlobsReclaimed=0 because the other row
still references the blob. Pin for content-addressed dedupe.
- TestOrphanGC_StartStop: loop spins up at 1ms interval, second
StartOrphanGC is a no-op, Stop drains via testServer's cleanup.
Parent: PLAN-866. Closes the phase 1 plan with full export →
import → orphan-cleanup round-trip.
* fix(attachments): protect referenced/in-flight blobs from orphan GC per Codex (round 1)
Two real correctness issues Codex caught on PR #307:
P1. The editor's normal upload flow leaves attachments.item_id NULL.
The canonical association lives in markdown content (the editor
PATCHes "pad-attachment:UUID" into the item) — but the GC's
"never-attached past 30d" predicate only checked item_id. So a
legitimate inline image could be hard-deleted 30 days after upload
even though item content still references it.
Added store.AttachmentReferencedInItems(workspaceID, attachmentID)
that scans items.content + items.fields for "pad-attachment:UUID".
The GC sweep now runs this check before reclaiming any
never-attached row; if any live item references the attachment,
the row is left alone (and re-checked next sweep).
P2. Race between concurrent upload and GC. Upload calls
AttachmentStore.Put (blob lands on disk) → THEN inserts the DB row.
Between those two steps an orphan-GC sweep could count zero live
refs for the hash, delete the blob, and the upload's row insert
would then point at a missing blob.
Added Server.inFlightUploadHashes (sync.Map of *atomic.Int64
counters) with markUploadInFlight / uploadInFlight helpers. Every
Put + CreateAttachment site fences itself via markUploadInFlight:
the upload handler, the transform handler, the thumbnail
derivation pipeline, and the bundle-import rehydrate path. The GC
sweep treats an in-flight hash as "another live ref" so it leaves
the blob alone.
Tests:
- TestOrphanGC_KeepsReferencedNeverAttachedRows: upload (item_id
NULL) → create item with pad-attachment: ref → backdate 31d →
sweep with 30d cutoff → row stays.
- TestOrphanGC_RespectsInFlightUploads: upload → soft-delete →
register an in-flight upload at the same hash → sweep → DB row
goes (it's tombstoned past grace) but blob stays so the
in-flight upload can complete cleanly.
The DB row still gets reclaimed in the in-flight case because the
soft-deleted row is independently past grace; only the blob delete
is fenced. That's correct: the blob remains usable for the
incoming upload and the new upload will register its own
attachments row.
* fix(attachments): mutex-protect in-flight tracker + portable JSONB scan per Codex (round 2)
Two fixes for the round-2 findings on PR #307:
P1. Same-hash race in the in-flight upload tracker. The sync.Map +
*atomic.Int64 design split increment from LoadOrStore-then-add and
release-decrement from delete, so a release could see "0" and start
deleting while another upload concurrently reloaded the same map
entry and incremented to "1" — the second upload's signal then
lived in a doomed map slot, invisible to subsequent uploadInFlight
calls.
Replaced with a plain map[string]int64 + sync.Mutex. Inc, dec,
delete-when-zero all run under one critical section, so any
inspection sees a consistent snapshot. Net cost is one mutex per
mark/release; uncontended this is ~10ns and the upload path is
already doing far more expensive work (Put + DB insert).
Stress test: 20 goroutines × 500 iterations of mark→check→release
on a shared hash. Every check must observe in-flight=true while
the calling goroutine holds the mark. Final state must be empty.
Runs cleanly under -race -count=3.
P2. Postgres JSONB compatibility. items.fields is TEXT on SQLite
but JSONB on PostgreSQL (per pgmigrations/001_initial.sql). LIKE
on JSONB fails with a type error, so the orphan GC's reference
scan would error on Postgres and skip every never-attached row —
breaking orphan reclamation for those rows entirely.
Cast fields::text in the Postgres dialect path:
fieldsExpr := "fields"
if s.dialect.Driver() == DriverPostgres {
fieldsExpr = "fields::text"
}
Same approach used elsewhere in the store for dialect-sensitive
text searches.
* fix(attachments): close GC/upload TOCTOU + protect in-grace peers per Codex (round 3)
P1 round 3: TOCTOU race between uploadInFlight check and store.Delete.
The mutex protected the in-flight counter but not the GC's
check-and-delete sequence. A new upload could call markUploadInFlight
between our check and our blob delete, then run Put after the blob
was gone — its CreateAttachment would insert a live row pointing at
the missing hash.
Fixed by holding inFlightHashesMu across the check + FS Delete:
s.inFlightHashesMu.Lock()
inFlight := s.inFlightHashes[hash] > 0
if !inFlight && others == 0 {
store.Delete(ctx, key)
}
s.inFlightHashesMu.Unlock()
A concurrent markUploadInFlight blocks until either we skip (because
we observed in-flight) or finish deleting. Lock window is ms-class
on FSStore; a per-hash lock can replace this server-wide mutex when
S3 lands in Phase 2.
P2 round 3: CountLiveAttachmentsForHash counted only live rows, so
GC could reclaim the blob from row A (soft-deleted 31d ago) even
when row B was also soft-deleted but only 1 day old — within
grace, so its blob must stay reachable until its own grace lapses.
Replaced with CountProtectingAttachmentsForHash which counts rows
where deleted_at IS NULL OR deleted_at >= graceCutoff. The blob is
preserved until every soft-deleted peer has aged past its own
grace window.
Tests:
- TestOrphanGC_RespectsSoftDeletedInGracePeer: two rows sharing a
hash, soft-delete both, backdate only one past 30d → sweep with
30d cutoff → older row reclaimed but blob stays for the still-in-
grace peer.
- existing TestOrphanGC_RespectsInFlightUploads still passes
(still uses the in-flight signal correctly).
* fix(attachments): dedupe blob-reclaim metric across same-hash peers per Codex (round 4)
Codex round 4 noted that when multiple soft-deleted peers share a
content_hash and all are past grace, the GC sweep would inflate
BlobsReclaimed and BytesReclaimed: AttachmentStore.Delete treats a
missing key as success, so the second peer's idempotent no-op
delete still bumped the counter.
Functional cleanup was correct (the blob really was gone after the
first peer); only the metric / log line was wrong, which makes
operator dashboards report fictitious bytes-reclaimed values.
Track per-sweep reclaimed hashes in a map and skip the Delete call
+ counter increment for repeats. The DB row still gets hard-deleted
on each peer.
Test: TestOrphanGC_DedupesBlobReclaimMetric uploads twice with
identical bytes (single shared blob), soft-deletes both, backdates
deleted_at past grace → sweep deletes 2 rows and reports
BlobsReclaimed=1 / BytesReclaimed=blobLen rather than 2 / 2*blobLen.