implement blob/release and blob/reject; migrate jobqueue into harmony task engine - #30
Conversation
84688ca to
c268234
Compare
cb62fcf to
7de588e
Compare
…blob lifecycle Ports the blob-removal protocol surface from the pre-Curio branch (frrist/fil-623-blob-remove) onto the Curio-based core, under the names settled in libforge PR #46: the lifecycle is allocate → accept | reject, with /blob/remove as the accepted-blob claim-release primitive. - /blob/remove (subject = provider, space in args): deletes the space's location claim (found via the new digest→claim index), acceptance, and allocation; queues byte release only when no space holds any claim. Idempotent. - /blob/reject (rename of the donor's /blob/unallocate): retires a parked blob — refuses accepted blobs with the stable BlobAccepted failure, deletes the space's allocation, queues byte release at zero allocations. - Accept handler: the acceptance row is written BEFORE the pipeline enqueue (with a compensating delete if the enqueue fails) so 'an acceptance exists' is a conservative superset of 'entered the pipeline' — the ordering reject's guard and the removal machinery's claim re-checks rely on. Acceptance records gain a Claim link (cbor regenerated). - Stores: acceptance/allocation gain Delete + ListSpaces; invocation store gains Delete. - types.PieceRemoverAPI + PDPService.RemovePiece: defer-by-default — a removal request is only recorded in the new pdp_pending_piece_removals table (migration 00000002); nothing is deleted inline. The machinery that advances these rows (claims re-check, pipeline-state check, schedulePieceDeletions for proven pieces, byte release) lands separately. - Registration delegations += blob/remove, blob/reject; pdpfake gains a recording RemovePiece. Tests: handler unit suites (remove/reject/accept-ordering) + ucanfxtest RPC coverage, all ported from the donor with the rename applied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Full adoption of Curio's harmony scheduler for the aggregation trio and the blob-removal engine, replacing the jobqueue-based commp/aggregator/ manager services and their separate aggregator_db. Pipeline state lives in harmonydb (pdp_blob_pipeline, pdp_root_submissions — migration 00000003), making every stage transition transactional with the tables the removal machinery inspects: - PDPCommP: per-blob commP + park, claimed via commp_task_id; idempotent re-runs (piecerefs-existence guard around ParkPiece); IAmBored scavenges rows whose spawn was lost (AddTask swallows errors by design). - PDPAggregate: derived-buffer fold under Max=1, executed in a single transaction with the candidate rows locked (FOR UPDATE) so the removal sweep's cancel serializes against it; Follows[PDPCommP] as the fast crash net, IAmBored as the slow one. Threshold semantics unchanged (fold.go keeps the pure functions). - PDPAddRoots: batched root submission behind the kept RootSubmitter seam; IAmBored is the poll-interval partial-batch flush (old manager loop); retires pipeline + submission rows after AddRoots (idempotent per root). - PDPRemoveSweep: harmonytask singleton (SingletonTaskAdder, 30s) driving PDPService.ProcessPendingRemovals — the ported four-hole-safe sweep: claims re-check per pass, transactional pre-aggregation cancel, waits out staged-but-unconfirmed adds and in-flight deletions (NextPP confirms or clears rm_message_hash, self-rescheduling), retires roots only when every live sub-piece is pending removal, re-checks claims at the last instant before releasing bytes and bookkeeping rows. commp.Calculator survives as the ingress seam (interface-only package); Enqueue commits the pipeline row before returning so accept's compensating delete still has an error signal. PDPService drops its vestigial TaskEngine field (would otherwise cycle: engine -> tasks -> service) and gains the allocation store for claim re-checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New pkg/internal/testutil.NewHarmonyDB: lazy shared Postgres testcontainer, fresh database per test with the curated PDP closure schema applied (moved into leaf package pkg/curiopdp/schema so packages pkg/curiopdp itself imports — like pkg/pdp/service — can use the harness without an import cycle). pkg/pdp/service/piece_remove_test.go ports the four-hole regression suite from the pre-Curio branch onto the curio schema: RemovePiece never deletes inline; never-aggregated blobs finalize; pre-commp pipeline rows cancel transactionally; buffered (unfolded) pieces cancel and release their parked chain; folded-but-unstaged aggregates are never deleted; staged-but-unconfirmed adds wait; revived claims (acceptance or allocation) cancel the removal; roots retire only when every live sub-piece is pending (exactly one schedulePieceDeletions, no re-schedule while the rm tx is in flight); confirmed removals finalize everything; failed rm txs (rm_message_hash cleared by NextPP) re-schedule. pkg/pdp/pipeline/calculator_test.go pins Enqueue's ingress guards: insert+claim, re-enqueue dedup, live-content skip, and — the revival hole — content whose deletion tx is in flight re-enters the pipeline instead of being treated as live. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the temporary local-checkout replace with the merged libforge commit aac837a (blob remove/abort/reject with Space). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Accept logged the raw multihash bytes where every other blob handler logs digestutil.Format's zQm form — breaking log-based test assertions (and grep-ability generally) for accepts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c268234 to
adc9c71
Compare
…ct guard The blob-removal RFC splits the removal chain into per-leg commands: the node-side claim release is now /blob/release (space explicit in the arguments, subject = provider), while /blob/remove stays the upload service's client-facing verb. Rebind the handler and the registration delegation accordingly. Scope the reject BlobAccepted guard to the invoking space, per the RFC: another space's acceptance of the same content-addressed bytes must not strand this space's parked allocation — the space's allocation is dropped and the bytes retained for whoever still claims them. Consequently the reject byte-release gate now checks acceptances as well as allocations, since the two can coexist across spaces. The BlobAccepted error name now comes from libforge, where sprue can match it to re-surface the refusal in abort receipts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alanshaw
left a comment
There was a problem hiding this comment.
Is there a libforge PR that goes with this?
The replace pointed at a sibling working copy and broke any other checkout; the referenced commits are now pushed (libforge PR #49). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@alanshaw yes here: fil-forge/libforge#49 |
Splits the provider leg out of `/blob/remove` into a new `/blob/release` binding, per the blob-removal RFC ([fil-one/RFC#13](fil-one/RFC#13), merged): `/blob/remove` and `/blob/abort` are now space-subject client verbs whose arguments no longer carry the space (it is the invocation subject), while `/blob/release` carries `{space, digest}` under the provider subject, matching `/blob/allocate` and `/blob/accept`. Cause references say `/blob/add` throughout — the `/space` command prefix died with the UCAN 1.0 transition. Defines the RFC's named errors where every service can share them: `BlobAccepted` (reject refused because **the invoking space** accepted the blob — the guard is space-scoped, not digest-scoped, so another tenant's acceptance never strands a parked allocation) and `MissingCause` (abort's cause is missing or doesn't resolve to a known `/blob/add` task). ## Landing order **This PR is the root of the removal chain** — fil-forge/piri#30, fil-forge/sprue#33, fil-forge/smelt#19, and fil-forge/ingot#40 all pin this branch's head (`3e5e6ba`) and re-pin to the merge commit once this lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…(FIL-522) (#33) Serves the upload-service half of blob removal, aligned with the blob-removal RFC ([fil-one/RFC#13](fil-one/RFC#13)): - `/blob/remove` (subject = the space, args `{digest}`) — releases a space's claim on an accepted blob: recovers every provider holding it (primary via the registration's receipt chain, plus non-failed replicas), forwards a provider-rooted **`/blob/release`** `{space, digest}` to each (best-effort), and deregisters last so the receipt chain survives for a retry if every forward fails. - `/blob/abort` (subject = the space, args `{digest, cause}`) — abandons a parked (never-accepted) blob: the provider is recovered from the cause's receipt chain and the abort is forwarded as `/blob/reject`. A cause that doesn't resolve to a known `/blob/add` task fails with the named error `MissingCause`; a node refusing because the space accepted the blob has its `BlobAccepted` re-surfaced in the abort receipt, so clients can distinguish "use `/blob/remove`" from a retryable fault. No local state mutates, so aborts are safely retryable. - `/upload/remove` (subject = the space, args `{root}`) — deletes the upload's root→shards index entry only. Shard blobs are the client's per-digest `/blob/remove` decision (content addressing shares shards between uploads). All handlers are idempotent (unknown/already-removed → success). Named errors come from libforge, shared with piri. ## Landing order fil-forge/libforge#49 (`/blob/release` binding + named errors; pinned at its branch head, re-pin on merge) → fil-forge/piri#30 (piri serves `/blob/release` + `/blob/reject`) → **this** — the forwards here invoke piri capabilities that only exist on #30. Same chain: fil-forge/smelt#19 (devnet delegations), fil-forge/ingot#40 (S3 surface). Validated end-to-end by ingot's delete-finality and deferred-multipart itests against piri #30 + this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
alanshaw
left a comment
There was a problem hiding this comment.
Leaving what feedback I have so far - not completed yet sorry!
Just wondering if there is any code that automatically removes a blob that is allocated but never accepted or rejected?
| -- task spawned for this blob (spawn dedup); the fold itself operates on | ||
| -- every unaggregated row under the task's Max=1 serialization. | ||
| agg_task_id bigint, | ||
| aggregate_root text, -- aggregate root piece CID (v2), set when folded in |
There was a problem hiding this comment.
It's kinda odd for commp and aggregate_root to be text and blob to be bytea no?
There was a problem hiding this comment.
It mirrors the curio tables this pipeline joins against: pdp_piece_mh_to_commp is exactly mhash bytea + commp text, and piece CIDs are text throughout harmonydb (pdp_piecerefs.piece_cid, pdp_data_set_pieces.sub_piece, ...). So the convention is: multihash digests as raw bytes, piece CIDs as text strings. I've added a comment to the schema documenting this, and renamed the column to digest (per your other comment) so it no longer reads as holding the blob's bytes — 12b1710.
| -- every unaggregated row under the task's Max=1 serialization. | ||
| agg_task_id bigint, | ||
| aggregate_root text, -- aggregate root piece CID (v2), set when folded in | ||
| created_at timestamptz NOT NULL DEFAULT now(), |
There was a problem hiding this comment.
IDK is it a good idea to have timestamps for lifecycle events, like commp_hashed_at and aggregated_at?
There was a problem hiding this comment.
Took the idea in f6ea205 — added commp_hashed_at and aggregated_at, stamped in the same UPDATEs that complete each stage. What tipped me from neutral to yes: aggregated_at isn't derivable after the fact (the fold that aggregates a row is generally not the task recorded in agg_task_id, and harmony_task_history gets pruned), and per-stage age is exactly the stuck-row triage query an operator wants. One caveat on expectations: pipeline rows are deleted once their batch lands on-chain, so these are in-flight diagnostics, not a durable latency log — long-term stage metrics would still need an exporter.
| pieces = append(pieces, p) | ||
| } | ||
|
|
||
| _, aggregates, err := aggregator.AggregatePieces(aggtypes.Buffer{}, pieces) |
There was a problem hiding this comment.
Why do we throw away the buffer return value here?
There was a problem hiding this comment.
Because the buffer is derived, not persisted: each fold rebuilds it from the unaggregated pdp_blob_pipeline rows, and pieces still below the threshold simply keep their aggregate_root IS NULL rows and get re-folded on the next run — so the returned buffer carried no information the DB didn't already hold. That said, discarding it was a smell worth fixing at the source: as of 12b1710 the aggregator API is Append(pieces) ([]types.Aggregate, error) and there's no buffer return to throw away — pieces absent from every returned aggregate are the 'still buffered' set.
- aggregator: replace the Buffer-round-trip AggregatePiece/AggregatePieces with Append(pieces) ([]Aggregate, error). The fold's buffer is derived from pdp_blob_pipeline rows on every run and was discarded by the only caller, so the Buffer in/out contract was a fossil of the deleted persistent-buffer jobqueue design; it also returned a non-nil aggregate alongside a non-nil error. types.Buffer is gone (cborgen regenerated). - schema: rename the bytea multihash column in pdp_blob_pipeline and pdp_pending_piece_removals from blob to digest — a bytea column named blob read as holding the blob's bytes — and document the typing convention: digests are raw bytes, piece CIDs are text, mirroring curio's pdp_piece_mh_to_commp (mhash bytea, commp text). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
add comment Co-authored-by: ash <alan138@gmail.com>
Add commp_hashed_at / aggregated_at to pdp_blob_pipeline, set in the same UPDATEs that complete each stage. aggregated_at is not derivable after the fact — the fold that aggregates a row is generally not the task recorded in agg_task_id, and harmony_task_history is pruned — and per-stage age is the natural stuck-row triage query. Rows are deleted once their batch lands on-chain, so these are in-flight diagnostics, not a durable latency log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| return fmt.Errorf("counting unclaimed roots: %w", err) | ||
| } | ||
| if unclaimed == 0 || (!force && unclaimed < batchSize) { | ||
| return nil |
There was a problem hiding this comment.
Maybe add a debug log line here?
| // followCommP is the Follows crash net: when a PDPCommP task completes but | ||
| // its aggregation handoff was lost, the follow poller (~1 min cadence) | ||
| // spawns the missing PDPAggregate task. Dedup is the engine's previous_task | ||
| // check plus the agg_task_id guard. |
There was a problem hiding this comment.
Not sure I follow this. What does "its aggregation handoff was lost" mean?
There was a problem hiding this comment.
Fair — the comment assumed too much context. The "handoff" is CommPTask.Do's last act: it enqueues the PDPAggregate task for the piece it just hashed. That spawn is best-effort (AddTask swallows errors, and the process can die between the commp result committing and the spawn committing), which would leave a row with a commp set and nothing to fold it. Follows is harmonytask's recovery poller over completed tasks: it re-checks completed PDPCommP tasks and spawns the missing PDPAggregate. I've rewritten the comment to spell all that out.
| IAmBored: func(add harmonytask.AddTaskFunc) error { | ||
| add(func(id harmonytask.TaskID, tx *harmonydb.Tx) (bool, error) { | ||
| n, err := tx.Exec(` | ||
| UPDATE pdp_blob_pipeline SET commp_task_id = $1 |
There was a problem hiding this comment.
Is it ok to set the task ID to the same value for multiple rows?
There was a problem hiding this comment.
In this query it can't happen: digest is the table's primary key and the subquery is LIMIT 1, so exactly zero or one rows get stamped — same for every other claim (spawn keys on a single digest, followCommP on a commp_task_id that's only ever stamped on one row). Multi-row stamping is fine in general — the *_task_id columns are our claim bookkeeping, not engine state — and PDPAddRoots does it deliberately for batch claiming, processing every row bearing its ID. For CommP the 1:1 invariant does matter (Do reads a single row, and a claimed commp_task_id is never cleared), so I've added a comment stating it where Do relies on it.
| // task then either confirms the deletion (removed=TRUE) or clears the | ||
| // stamp on a failed transaction, which reschedules it here. Roots with | ||
| // any live sub-piece keep being proven — and the pending blob's bytes | ||
| // retained — until the whole root is retirable. |
There was a problem hiding this comment.
So, a 1KB blob can potentially cost the operator up to 256MB of space (minus 1KB) if all the other blobs in the aggregate are removed?
Is the follow up to add a "defrag" task that reclaims the space?
There was a problem hiding this comment.
So, a 1KB blob can potentially cost the operator up to 256MB of space (minus 1KB) if all the other blobs in the aggregate are removed?
Yes on the mechanics: one live sub-piece pins its whole root (up to 256MB padded), because PDPVerifier's removal unit is the whole root and the prove task reads sub-piece bytes to answer challenges — removed blobs' bytes must be retained while their root is live. As this PR stands there's also no time bound: a sparse root is reclaimed only when its last live blob goes.
One disambiguation on "cost the operator", though: under our S3 billing the client stops paying at the delete request (I assume, since that would be my exp as a client, maybe a product question here), and fil.one pays the SP for proven bytes — so retained-but-removed bytes are fil.one's bill, not the SP's. The SP is compensated for proving them. This is exactly the "zombie storage" quantity in the pdp-sim fil.one workload study: FIL_ONE_FINDINGS.md.
Is the follow up to add a "defrag" task that reclaims the space?
Roughly yes — the follow-up is a compaction task, but the sim reshaped its design. The dominant lever is the ratio of delete SLA to proving period, not a space threshold: compacting eagerly per delete is the catastrophic regime (SLA = proving period → un-batchable rewrites, ~110× the gas, and every rewrite double-proves survivors during the proving lag), while past ~5 proving periods fil.one gives margin back for no further gain. So the follow-up is SLA-bounded batched compaction (target 2–3 proving periods), where the compaction threshold is second-order. Filed #46 with the sketch.
| // the blob was revived after this sweep pass classified it, the removal is | ||
| // cancelled instead. commp/commpV1 are empty when the blob never had a | ||
| // mapping. | ||
| func (p *PDPService) finalizeRemoval(ctx context.Context, blob multihash.Multihash, commp, commpV1 string) error { |
There was a problem hiding this comment.
Urgh, do we really have to pass both commp versions? Can we parse the string and, pass commp v2 as a multihash and derive the v1 if/when we need it? I feel that we've taken a step backwards with the DB using both versions in different tables.
There was a problem hiding this comment.
Good idea, finalizeRemoval now takes just the v2 string and the sweep derives the v1 form at the point of use.
The commp_v1 column has to stay though as its a requirement of the Curio task engine schema for now. i.e. https://github.com/filecoin-project/curio/blob/main/tasks/pdpv0/task_prove.go (our imported proving task) only uses CommpV1
There was a problem hiding this comment.
Can we make this non-nillable? There's no no-PDP flow anymore.
| // Claim is a link to the `/assert/location` claim minted at acceptance — | ||
| // the digest→claim index `/blob/release` uses to delete the location | ||
| // claim when this space's acceptance is removed. Nil for acceptances | ||
| // recorded before this field existed. | ||
| Claim *cid.Cid `cborgen:"claim,omitempty"` |
There was a problem hiding this comment.
Lets not retain backwards compatibility for a service not in production yet and not allow it to be nil.
In the /blob/accept OK type we refer to this as Site - I suggest we follow the same naming convention:
| // Claim is a link to the `/assert/location` claim minted at acceptance — | |
| // the digest→claim index `/blob/release` uses to delete the location | |
| // claim when this space's acceptance is removed. Nil for acceptances | |
| // recorded before this field existed. | |
| Claim *cid.Cid `cborgen:"claim,omitempty"` | |
| // Site is a link to the location commitment minted at acceptance — | |
| // the digest→claim index `/blob/release` uses to delete the location | |
| // claim when this space's acceptance is removed. | |
| Site cid.Cid `cborgen:"site"` |
|
Related to this for your consideration fil-forge/libforge#50 |
Co-authored-by: ash <alan138@gmail.com>
Co-authored-by: ash <alan138@gmail.com>
- Submit buffers all roots in one INSERT ... SELECT unnest() and gains a harmonydb-backed test (batch, overlap idempotency, empty call). - finalizeRemoval takes only the v2 commp; the sweep derives the v1 form at the pdpv0 boundary via pieceCIDv1String (thin adapter over asPieceCIDv1). The commp_v1 column stays for v1-string SQL lookups (resolver reverse lookup, calculator joins). - Spell out the followCommP crash net (what a lost aggregation handoff is) and the one-row-per-commp_task_id invariant Do relies on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Acceptance.PDPAccept is a value promise.AwaitOK (no no-PDP flow remains) and Claim becomes Site cid.Cid, matching the /blob/accept OK type's naming. cborgen regenerated; the map encoders now enforce both fields at write time (undefined cids refuse to encode), so every test fixture seeds them like production does. The legacy nil-claim-link release path and its compat test are gone — nothing is deployed. - flush logs unclaimed/batchSize/force at debug so a non-flushing manager is diagnosable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ored A released digest leaves its bytes in the piece store until the pending- removal sweep runs, with zero allocations. A re-add of the same content in that window (S3 delete -> immediate re-put) hit the !received path, and AllocatePiece reported the piece present (Allocated=false, no upload ID) — but the handler wrapped the zero-value upload URL in a BlobAddress anyway, sending the client a PUT to "" (surfaced as a deterministic 500 in ingot's versity conformance run: GetObject_overrides_success re-puts the content GetObject_overrides_presign_success just deleted). Emit an address only when AllocatePiece actually allocated an upload slot. When the bytes are already present the receipt carries no address — the caller proceeds straight to accept — and the allocation row written by the handler is a claim, so the sweep cancels the queued removal instead of deleting the revived bytes (the revival contract ProcessPendingRemovals already implements). Residual (pre-existing) race: the allocation row is written after the piece-presence check, so a finalizeRemoval pass landing in between can still delete the bytes before the claim appears; the failure is then a clean accept error rather than a PUT to "". Closing it needs claim-first ordering in allocate, which interacts with allocation expiry — left for the allocation-lifecycle proposal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the removal half of the blob lifecycle and moves the aggregation pipeline onto the harmony scheduler. Aligned with the blob-removal RFC (fil-one/RFC#13).
Protocol:
/blob/release— the claim-release inverse of/blob/allocate, invoked by the upload service translating a client/blob/remove— drops a space's claim on an accepted blob (claim + acceptance + allocation deleted; bytes released once no space holds an allocation or acceptance)./blob/rejectretires a parked blob — the "don't accept" exit of allocate→accept|reject. ItsBlobAcceptedguard is scoped to the invoking space per the RFC: only that space's own acceptance refuses the reject; another tenant's acceptance of the same content-addressed bytes just drops this space's allocation and retains the bytes (multi-tenant liveness). Both named errors come from libforge, shared with sprue. The registration delegation grants/blob/release+/blob/rejectto the upload service. Physical deletion is always deferred to a sweep that re-verifies claims before every destructive step.Pipeline: the commp/aggregator/manager jobqueues (and their separate aggregator_db) are replaced by harmonytask tasks —
PDPCommP,PDPAggregate,PDPAddRoots,PDPRemoveSweep— with all state in harmonydb (pdp_blob_pipeline,pdp_root_submissions). Stage transitions are transactional with the tables the removal machinery inspects, which closes the accepted-blob removal races by construction: pre-commp rows cancel transactionally, the fold locks its candidates (a row is either cancelled or folded, never both), staged pieces ride the root lifecycle, and roots retire on-chain (schedulePieceDeletions) only when every live sub-piece is pending removal.Validated: full suite including a harmonydb-backed (Postgres testcontainer) regression harness for the removal races — including the cross-space reject case the RFC mandates; smelt e2e with the injected binary — upload/retrieve plus the full proving loop through the new pipeline, proof verified by the PDPVerifier contract.
Landing order: fil-forge/libforge#49 (the
/blob/releasebinding + shared named errors; pinned at its branch head, re-pin on merge) → this. Downstream: fil-forge/sprue#33 (upload-service handlers, forwards/blob/release) + fil-forge/smelt#19 (devnet delegations), then fil-forge/ingot#40 (S3 surface).🤖 Generated with Claude Code