[ENH]: sysdb changes to support moving collection hard deletes to garbage collector

[codetheweb]

Description of changes

• sysdb no longer has a flag for soft delete mode, soft delete is always enabled
• sysdb no longer automatically transitions soft deleted collections to hard deleted
• lineage file name is now uuidv7
  • It was previously based on the new target collection ID that was just added to the file, which works when adding dependencies but not when removing dependencies. UUIDv7 is time-sortable and random.
• added BatchGetCollectionSoftDeleteStatus method (is called/tested in the next PR)

Test plan

How are these changes tested?

• Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

n/a

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[github-actions]

⚠️ The Helm chart was updated without a version bump. Your changes will only be published if the version field in k8s/distributed-chroma/Chart.yaml is updated.

[codetheweb]

• [ENH]: soft delete databases, add FinishDatabaseDeletion gRPC method to hard delete databases #4627
• [ENH]: perform collection hard deletes from garbage collector #4605
• [ENH]: sysdb changes to support moving collection hard deletes to garbage collector #4607 👈 (View in Graphite)
• [ENH]: add validation during garbage collection for empty file paths #4586
• [ENH]: garbage collector v2 orchestrator (supports forking) #4468
• [ENH]: allow deleting v0 from version file & correctly propagate errors on DeleteCollectionVersion #4579
• [ENH]: add operator for garbage collection to list all files used by collection version #4466
• [ENH]: add operator to compute versions to garbage collect from version graph #4464
• [ENH]: add orchestrator to construct version graph for garbage collection #4463
• [ENH]: add batch get version file paths method to Sysdb #4432 : 1 other dependent PR (#4447 )
• [ENH]: SysDb should return lineage, version file paths and root collection ID on collections #4557
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codetheweb]

review: should we instead extend the GetCollection method to allow returning soft deleted collections, and then get the statuses with multiple calls?

[Sicheng-Pan]

I would prefer a single call to reduce the roundtrips. GetCollection should not return soft deleted collections for its current usages.

[propel-code-bot]

[ENH]: sysdb changes to support moving collection hard deletes to garbage collector

This major PR re-architects the sysdb collection deletion flow by enabling soft delete as the only deletion mode and shifting responsibility for hard deletion of collections to an external garbage collector. It removes all prior server-side soft delete flags and automatic hard deletion logic, introduces distinct 'SoftDeleteCollection' and 'FinishCollectionDeletion' (hard delete) code paths, adapts the proto/grpc APIs accordingly, and ensures correct lineage file management with UUIDv7 filenames. A new batch API for efficiently querying soft delete status across multiple collections is also added.

Key Changes:
• Removed the soft-delete flag and automatic hard delete logic from sysdb; soft deletes are now always used
• Split deletion logic: 'SoftDeleteCollection' marks collections as deleted, while 'FinishCollectionDeletion' (hard delete) is meant for external GC use
• Removed in-process SoftDeleteCleaner/background cleanup from server implementation
• Migration of the hard-deletion API to the responsibility of an external GC, and added gRPC proto methods reflecting this flow
• Changed lineage file naming to use UUIDv7 for better time-ordering and conflict avoidance
• Added BatchGetCollectionSoftDeleteStatus API and backend for bulk-lookup of soft-deleted status for collections
• Removed all configuration options related to soft delete management from server and CLI
• Adapted table_catalog and related code to enforce invariants and protect lineage file updates with appropriate row-level locking
• Refactored tests to use direct calls to new code paths ('SoftDeleteCollection', 'FinishCollectionDeletion') instead of legacy unified deletion logic
• Updated proto definitions, mocks, and downstream call sites throughout the sysdb module

Affected Areas:
• sysdb/coordinator (deletion API, collection lifecycle, lineage management)
• sysdb/grpc (gRPC server and service implementation, removal of background cleaner)
• idl/chromadb/proto (protocol and service contract, collection deletion and batch status APIs)
• sysdb/metastore/db/dao (database access for deletion, batch soft delete status)
• sysdb/metastore/s3 (lineage/version file handling)
• Command-line flags, configuration, and test suites

Potential Impact:

Functionality: Collection hard deletes are no longer triggered by sysdb itself but must be issued explicitly (e.g., by a garbage collector process). All code and clients must now follow the new two-step deletion process. Legacy flows and flags for soft/hard delete are removed.

Performance: Reduced server-side cron-like activity for purging collections; batch APIs for soft delete status improve efficiency for clients like GC. Slight impact in GC latency depending on garbage collector implementation.

Security: No direct changes to authorization, but deletion pathways are now more explicit and controlled, reducing accidental permanent data loss.

Scalability: Should improve with externalization of collection purging and batch status fetches, and more scalable lineage management with UUIDv7.

Review Focus:
• Correctness of the new deletion flow: no accidental permanent deletes; no collection left in inconsistent state
• Race conditions around the updating or deleting of lineage files (check locking and error paths)
• Correct adaptation/removal of soft delete cleaner code and cron logic across sysdb/grpc
• Proto and gRPC API changes compatibility (ensure all call sites and generated code match)
• BatchGetCollectionSoftDeleteStatus implementation (edge cases on empty and mixed inputs)
• Removal/renaming of configs-ensure CLI, configs, and documentation are updated in sync

Testing Needed

• Verify the two-step deletion flow: collections should move to soft-delete state and only be hard-deleted via the FinishCollectionDeletion API
• Ensure lineage files are managed (including concurrent forks and deletes) without race
• Batch soft delete status API correctness for both existing, new, and missing collections
• All new and adapted integration/unit tests pass (especially for deletion edge-cases, lineage, and batched status)

Code Quality Assessment

go/pkg/sysdb/coordinator/coordinator.go: Cleaner interface for clients, well-abstracted two-step flow.

go/pkg/sysdb/metastore/s3/impl.go: Improved nil checks. Reviewer requested additional guards for robustness, which is acknowledged but could be further improved.

go/pkg/sysdb/coordinator/table_catalog.go: Substantial refactor, mostly clear, includes explicit row-level locking for potential race conditions. Some error handling improvements; edge cases are considered.

go/pkg/sysdb/grpc/collection_service.go: Clear separation of soft vs. hard delete, improved logging. New gRPC methods are well-documented.

go/pkg/sysdb/metastore/db/dao/collection.go: Batch queries implemented as expected; suggestion to check empty input for further efficiency; mostly concise.

idl/chromadb/proto/coordinator.proto: Major service definition changes; structure is consistent and well-documented.

Best Practices

Testing:
• Integration and unit tests updated and improved for new behavior

Config Hygiene:
• Removal of obsolete flags/configs

API Design:
• Separation of concern between soft and hard delete
• Batch status API for efficiency and atomicity

Robustness:
• UUIDv7 for file naming prevents collisions and enables efficient sorting
• Reviewer suggests additional guards and comments for further robustness

Potential Issues

• Potential for race conditions on lineage file updates (mitigated with row locks, but should be carefully verified under load/concurrent operations)
• Some downstream consumers may expect the legacy unified DeleteCollection API behavior; must ensure comprehensive migration
• Lineage or version file handling could panic on malformed data or unexpected proto structures (as flagged by review comments)
• Danger if GC process is not deployed or if clients do not issue the hard delete step-collections would remain in limbo

This summary was automatically generated by @propel-code-bot

[Sicheng-Pan]

nit: I think there is an error constant for this

[codetheweb]

did not see one

[Sicheng-Pan]

in ForkCollection we directly used common.ErrCollectionNotFound. Maybe we should have a separate error constant for this. I'm ok with this now

[Sicheng-Pan]

what happens when dependency.SourceCollectionId == deleteCollection.ID?

[codetheweb]

dependency.SourceCollectionId == deleteCollection.ID should never evaluate to true at this point because only leaf nodes in the dependency tree are eligible for hard deletion
but this would be good to add as an invariant 👍

[Sicheng-Pan]

warn: potential race condition with forkCollection

[codetheweb]

should be fixed with the lock I added

[Sicheng-Pan]

the lock seems to be on the deleted collection, while we need to lock the root collection to prevent race right

[codetheweb]

🤦 whoops yes, we need to lock both
sorry tired brain today

[Sicheng-Pan]

careful about the lock order. not sure if a deadlock is possible as forking lock two collections as well