fix: Resolve storage leak upon document deletion#4451
fix: Resolve storage leak upon document deletion#4451SharanRP wants to merge 2 commits intosourcenetwork:developfrom
Conversation
📝 WalkthroughWalkthroughDeletes indexed document, primary key (DS entry), ValueKey, and all FieldKey entries during applyDelete; adds an integration test that enumerates root store keys after deletion to assert data keys are freed across storage backends. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant GoClient as Go Client
participant ApplyDelete as applyDelete
participant RootStore as RootStore
participant IndexStore as IndexStore
TestRunner->>GoClient: request Delete(doc primaryKey)
GoClient->>ApplyDelete: invoke applyDelete(transaction, primaryKey)
ApplyDelete->>IndexStore: parse primaryKey -> delete indexed document
ApplyDelete->>RootStore: begin txn -> delete primary key (DS entry)
ApplyDelete->>RootStore: compute short collection ID -> delete ValueKey (object marker)
ApplyDelete->>RootStore: iterate keys with doc prefix -> delete FieldKey entries
ApplyDelete->>RootStore: commit txn
GoClient->>TestRunner: return delete result
TestRunner->>GoClient: run storageCheckAction
GoClient->>RootStore: iterate all keys -> return key list
TestRunner->>TestRunner: assertNoDataKeys verifies only DeletedKey/PriorityKey remain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Suggested reviewers
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)internal/db/collection_delete.go (3)
tests/integration/mutation/delete/storage_leak_test.go (2)
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/db/collection_delete.go`:
- Around line 189-227: The current deletion loop only removes fields listed in
c.Version().Fields and can miss composite or legacy field keys (e.g., the
DocComposite namespace) and historical field IDs; update the deletion logic in
the collection delete flow to also remove the composite key and any remaining
legacy/unknown field keys by performing a prefix delete for the document
keyspace: after computing shortID and primaryKey.DocID (used in primaryKey and
valueKey), construct and delete the explicit composite key (DocComposite
namespace) using keys.DataStoreKey, and then call the datastore
prefix/scan+delete for the document's field key prefix (using CollectionShortID
and DocID) to remove any leftover historical field entries; ensure these
deletions use txn.Datastore().Delete or the datastore's prefix delete API and
handle errors similarly to existing deletes.
🧹 Nitpick comments (1)
tests/integration/mutation/delete/storage_leak_test.go (1)
62-94: Fail fast on unexpected datastore key decoding.If a key has the datastore prefix but
DecodeDataStoreKeyfails, the currentcontinuecan mask leaks or key-format regressions. Consider surfacing the error to keep the test strict.💡 Suggested tweak
- dsKey, err := keys.DecodeDataStoreKey([]byte(k[1:])) - if err != nil { - continue - } + dsKey, err := keys.DecodeDataStoreKey([]byte(k[1:])) + if err != nil { + return fmt.Errorf("unexpected datastore key format: %s (%v)", k, err) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/db/collection_delete.gotests/integration/mutation/delete/storage_leak_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/mutation/delete/storage_leak_test.go (2)
internal/keys/datastore_doc.go (3)
DecodeDataStoreKey(221-271)InstanceType(23-23)ValueKey(27-27)internal/keys/systemstore_field_id.go (1)
FieldID(21-24)
internal/db/collection_delete.go (5)
internal/core/block/store.go (1)
AddDelta(51-112)internal/db/id/collection.go (1)
GetShortCollectionID(48-64)internal/keys/datastore_doc.go (3)
DataStoreKey(37-42)InstanceType(23-23)ValueKey(27-27)internal/db/id/field.go (1)
GetShortFieldID(28-86)internal/keys/systemstore_field_id.go (1)
FieldID(21-24)
🔇 Additional comments (3)
tests/integration/mutation/delete/storage_leak_test.go (2)
17-60: Nice hook for rootstore inspection.The
storageCheckActionencapsulation and Rootstore iterator usage are clear and make the assertion plug‑in friendly.
97-124: Test scenario coverage looks good.The end-to-end flow (schema → create → delete → storage check) maps well to the regression objective.
internal/db/collection_delete.go (1)
175-182: No action required. Index deletion is properly scoped within the transaction and cannot leave the index out of sync with the datastore. All index and datastore operations use the same transactional context passed through the call chain.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
SharanRP
force-pushed
the
fix/storage-leak-4419
branch
from
4d1729b to
3b7d920
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/integration/mutation/delete/storage_leak_test.go`:
- Around line 61-85: The leak-check currently ignores decode failures in
assertNoDataKeys's AssertKeys, which can hide problematic keys; change the
behavior in storageCheckAction.AssertKeys (inside assertNoDataKeys) to treat
errors from keys.DecodeDataStoreKey([]byte(k[1:])) as failures instead of
continuing — return a descriptive error including the key and decode error so
the test fails fast; keep the existing allowances for keys.DeletedKey and
keys.PriorityKey and only treat remaining active types as leaks.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/db/collection_delete.gotests/integration/mutation/delete/storage_leak_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/db/collection_delete.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/mutation/delete/storage_leak_test.go (1)
internal/keys/datastore_doc.go (4)
DecodeDataStoreKey(221-271)InstanceType(23-23)DeletedKey(31-31)PriorityKey(29-29)
🔇 Additional comments (3)
tests/integration/mutation/delete/storage_leak_test.go (3)
17-24: LGTM: clean test action wiring.
25-59: LGTM: rootstore scan and error handling look solid.
89-116: LGTM: good end-to-end coverage for storage cleanup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
SharanRP
force-pushed
the
fix/storage-leak-4419
branch
from
3b7d920 to
c5a4df1
Compare
2 participants
Relevant issue(s)
Resolves #4419
Description
Fixes a storage leak where document data (DataStore keys and secondary indexes) persisted after deletion. Updated applyDelete to explicitly purge Primary, Value, and Field keys from the projected state while preserving DAG history for P2P sync.
The fix targets the local projected state (DataStore). BlockStore blocks (DAG history) are intentionally preserved to allow P2P sync of the deletion event, which is standard CRDT behavior but may result in non-zero growth of the BlockStore.
Tasks
How has this been tested?
Added TestStorageFreedUponDelete regression test. It performs a raw iteration of the Rootstore to verify that no active data keys remain for a deleted document.
Run:
go test ./tests/integration/mutation/delete/ -v -run TestStorageFreedUponDeleteSpecify the platform(s) on which this was tested: