From ebc894fa426a51466dd133be5bfd24d4a85f160d Mon Sep 17 00:00:00 2001 From: Worph Date: Wed, 16 Sep 2026 17:10:00 +0000 Subject: [PATCH] deleting a cids/ member also drops its reverse alias --- internal/storage/cid_resolution.go | 36 ++++++ internal/storage/cid_retraction_test.go | 158 ++++++++++++++++++++++++ internal/storage/client.go | 11 ++ 3 files changed, 205 insertions(+) create mode 100644 internal/storage/cid_retraction_test.go diff --git a/internal/storage/cid_resolution.go b/internal/storage/cid_resolution.go index 81efd25..07beac9 100644 --- a/internal/storage/cid_resolution.go +++ b/internal/storage/cid_resolution.go @@ -157,6 +157,42 @@ func (c *Client) addAliasLocked(ctx context.Context, uuid, cidStr string) error return nil } +// removeAliasLocked is the inverse of addAliasLocked: it drops the reverse-index +// entry cid: → . The caller must already hold c.mu, and must delete +// the cids/ key-set member itself (DeleteProperty does both). +// +// ⚠ Why this exists. A cids/ member write touches THREE keys — the flat +// file:/cids/, the __fields__ index entry, and this reverse alias — +// while the delete path only ever removed the first two. A "deleted" CID +// therefore still resolved: GET /meta/, ResolveRoot and GetByCID all kept +// answering with the record. The asymmetry is not theoretical. meta-share +// published dag-pb roots computed over RAR volumes and par2 files onto six +// records on watch.nsl.sh; removing the member was expressible, removing the +// claim was not, so the false identity survived its own retraction. +// +// Guarded, not blind: the alias is removed only when it currently points at +// THIS uuid. Two records can legitimately race to claim a cid (see the +// dual-root guard in addAliasLocked, which leaves the first writer's alias in +// place); deleting a member from the loser must not unpick the winner's index. +func (c *Client) removeAliasLocked(ctx context.Context, uuid, cidStr string) error { + if uuid == "" || cidStr == "" { + return nil + } + indexKey := c.buildCIDIndexKey(cidStr) + existing, err := c.client.Get(ctx, indexKey).Result() + if err == redis.Nil { + return nil + } + if err != nil { + return fmt.Errorf("read existing alias: %w", err) + } + if existing != uuid { + // Somebody else owns this alias. Leave it. + return nil + } + return c.client.Del(ctx, indexKey).Err() +} + // ResolveRoot maps a hash supplied by a caller (a meta-sort write, an editor // lookup, an SSE consumer) to the actual storage root key. // diff --git a/internal/storage/cid_retraction_test.go b/internal/storage/cid_retraction_test.go new file mode 100644 index 0000000..c139842 --- /dev/null +++ b/internal/storage/cid_retraction_test.go @@ -0,0 +1,158 @@ +package storage + +import ( + "context" + "testing" +) + +// Tests for retracting a `cids/` key-set member (cid_resolution.go's +// removeAliasLocked, reached through DeleteProperty). +// +// Why this is worth its own file. Adding a member writes THREE keys — the flat +// `file:/cids/`, the `__fields__` index entry, and the reverse alias +// `cid:` → ``. Until removeAliasLocked existed, deleting removed only +// the first two, so a retracted CID kept resolving to the record: the member was +// gone from the document and the claim was not. +// +// That asymmetry had a live consequence. meta-share minted dag-pb roots and +// midhash256s over whatever `archive::pick_media` elected, which on an +// obfuscated posting was an arbitrary RAR volume or a par2 file. Six records on +// watch.nsl.sh carry such members. `METADATA_KEYS.md` §2 defines a `cids/` +// member as a digest of *this record's file* and notes that every member is a +// reverse index back to the record — so those entries assert that an archive +// volume is the episode. Retracting one has to remove the assertion, not just +// the row. + +// aliasTarget returns the uuid that cid: currently resolves to, or "". +func aliasTarget(t *testing.T, c *Client, cidStr string) string { + t.Helper() + got, err := c.client.Get(context.Background(), c.buildCIDIndexKey(cidStr)).Result() + if err != nil { + return "" + } + return got +} + +func hasField(t *testing.T, c *Client, hashID, field string) bool { + t.Helper() + ok, err := c.client.SIsMember(context.Background(), c.buildFieldsKey(hashID), field).Result() + if err != nil { + t.Fatalf("sismember: %v", err) + } + return ok +} + +// The real shape: `…glnrqztt…` is the nzb-release locator whose record published +// a dag-pb root computed over volume 3 of a 35-volume RAR5 set. +const ( + poisonedRoot = "bafybeicicuzdvuztca2hp2g2k7scgr4nnibilzgoppljeprphybqot7v6i" + goodMidhash = "bagacbabaed6uiulnv7tscpxne22eztkjvnxg5almn43dpfecppixarfe7xhlu" +) + +func TestRetractingACidMemberRemovesTheClaimNotJustTheRow(t *testing.T) { + forEachPrefix(t, func(t *testing.T, c *Client) { + const uuid = "01KSBHMTESTRECORD" + + if err := c.SetMetadataFlat(uuid, map[string]string{ + "cids/" + poisonedRoot: "true", + "cids/" + goodMidhash: "true", + "fileType": "video", + }); err != nil { + t.Fatalf("seed: %v", err) + } + + // All three writes landed, and the CID resolves to the record. + if got := aliasTarget(t, c, poisonedRoot); got != uuid { + t.Fatalf("alias should point at the record before retraction, got %q", got) + } + if !hasField(t, c, uuid, "cids/"+poisonedRoot) { + t.Fatal("field index should list the member before retraction") + } + if got := c.ResolveRoot(poisonedRoot); got != uuid { + t.Fatalf("ResolveRoot should find the record before retraction, got %q", got) + } + + if err := c.DeleteProperty(uuid, "cids/"+poisonedRoot); err != nil { + t.Fatalf("DeleteProperty: %v", err) + } + + // 1. the flat key + if v, err := c.client.Get(context.Background(), + c.buildKeyPrefix(uuid)+"cids/"+poisonedRoot).Result(); err == nil { + t.Fatalf("flat key should be gone, still %q", v) + } + // 2. the field index + if hasField(t, c, uuid, "cids/"+poisonedRoot) { + t.Fatal("field index still lists the retracted member") + } + // 3. the reverse alias — the one that used to survive + if got := aliasTarget(t, c, poisonedRoot); got != "" { + t.Fatalf("reverse alias survived retraction, still points at %q", got) + } + if got := c.ResolveRoot(poisonedRoot); got == uuid { + t.Fatal("a retracted cid must not resolve to the record any more") + } + + // The siblings are untouched: retraction is surgical, not a purge. + if got := aliasTarget(t, c, goodMidhash); got != uuid { + t.Fatalf("an unrelated sibling lost its alias, got %q", got) + } + if !hasField(t, c, uuid, "cids/"+goodMidhash) { + t.Fatal("an unrelated sibling lost its field-index entry") + } + if v, err := c.GetProperty(uuid, "fileType"); err != nil || v != "video" { + t.Fatalf("an unrelated field changed: %q %v", v, err) + } + }) +} + +// Two records can race to claim one cid; addAliasLocked leaves the first +// writer's alias in place. Retracting the loser's member must not unpick the +// winner's index — which is why removeAliasLocked compares before deleting. +func TestRetractingDoesNotStealAnotherRecordsAlias(t *testing.T) { + forEachPrefix(t, func(t *testing.T, c *Client) { + const winner = "01KSBHMWINNER" + const loser = "01KSBHMLOSER" + const shared = "bafybeiaeb2lqi4z3np3q4tarldtyjp2t5lvnoodmtpvpi2tz73azn5td2m" + + if err := c.SetMetadataFlat(winner, map[string]string{"cids/" + shared: "true"}); err != nil { + t.Fatalf("seed winner: %v", err) + } + if err := c.SetMetadataFlat(loser, map[string]string{"cids/" + shared: "true"}); err != nil { + t.Fatalf("seed loser: %v", err) + } + if got := aliasTarget(t, c, shared); got != winner { + t.Fatalf("the first writer should own the alias, got %q", got) + } + + if err := c.DeleteProperty(loser, "cids/"+shared); err != nil { + t.Fatalf("DeleteProperty: %v", err) + } + + if got := aliasTarget(t, c, shared); got != winner { + t.Fatalf("retracting the loser's member stole the winner's alias, now %q", got) + } + if hasField(t, c, loser, "cids/"+shared) { + t.Fatal("the loser's own member should still be gone") + } + }) +} + +// A non-cids field has no alias to clean up, and the delete must not invent one. +func TestRetractingAnOrdinaryFieldTouchesNoAlias(t *testing.T) { + forEachPrefix(t, func(t *testing.T, c *Client) { + const uuid = "01KSBHMORDINARY" + if err := c.SetMetadataFlat(uuid, map[string]string{ + "cids/" + goodMidhash: "true", + "quality": "1080p", + }); err != nil { + t.Fatalf("seed: %v", err) + } + if err := c.DeleteProperty(uuid, "quality"); err != nil { + t.Fatalf("DeleteProperty: %v", err) + } + if got := aliasTarget(t, c, goodMidhash); got != uuid { + t.Fatalf("deleting an ordinary field disturbed a cid alias, got %q", got) + } + }) +} diff --git a/internal/storage/client.go b/internal/storage/client.go index 10b7b61..c1703c9 100644 --- a/internal/storage/client.go +++ b/internal/storage/client.go @@ -460,6 +460,10 @@ func (c *Client) MergeMetadataFlat(hashID string, metadata map[string]string) (i // DeleteProperty deletes a single property // Uses flat key: DEL file:{hashId}/{property} +// +// For a cids/ member this is the full inverse of the write: the flat key, +// the __fields__ entry AND the cid: reverse alias all go. Anything less +// leaves the CID still resolving to this record — see removeAliasLocked. func (c *Client) DeleteProperty(hashID, property string) error { c.mu.RLock() defer c.mu.RUnlock() @@ -475,6 +479,13 @@ func (c *Client) DeleteProperty(hashID, property string) error { if err := c.client.Del(ctx, key).Err(); err != nil { return err } + // Before the field index forgets the name, use it: a cids/ member owns a + // reverse-index entry that nothing else will ever clean up. + if cidStr := cidFromKeysetField(property); cidStr != "" { + if err := c.removeAliasLocked(ctx, hashID, cidStr); err != nil { + return fmt.Errorf("remove alias for %s: %w", cidStr, err) + } + } c.removeFieldLocked(ctx, hashID, property) c.markDirtyLocked(ctx, hashID) return nil