[ENH]: Make error messages in attach_function clearer #5973

tanujnay112 · 2025-12-06T00:43:53Z

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- Errors fixed:
  - Param validation
  - Output collection already exists error looks nicer now
  - Function already exists error looks cleaner
  - Invalid function to be attached error added
  - Collection already has an attached function
New functionality
- ...

Test plan

How are these changes tested?

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

Migration plan

Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

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

github-actions · 2025-12-06T00:44:08Z

tanujnay112 · 2025-12-06T00:44:13Z

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

propel-code-bot · 2025-12-06T00:44:33Z

Improve attach_function validation and error surfacing across stack

This PR tightens attach_function validation in the Go coordinator, updates Rust clients to surface precise gRPC errors, and expands distributed integration tests to cover newly handled cases. It introduces deterministic function-ID to name mapping for built-in operators, enforces empty params, and ensures output-collection conflicts and mismatched attachments return actionable messages end-to-end.

Key Changes

• Refactored validateAttachedFunctionMatchesRequest in go/pkg/sysdb/coordinator/task.go to return a boolean/matched sentinel, short-circuit mismatches without DB lookups, enforce empty params, and surface rich AlreadyExists / InvalidArgument errors via grpcutils.
• Added built-in function ID→name map and GetFunctionNameByID helper in go/pkg/sysdb/metastore/db/dbmodel/constants.go, with downstream usage in coordinator and Rust conversions to avoid DB fetches for known functions.
• Expanded Rust sysdb client (rust/sysdb/src/sysdb.rs) and frontend (rust/frontend/src/impls/service_based_frontend.rs) error enums to propagate detailed gRPC status messages (already-exists, failed-precondition, invalid-argument, output-collection-exists) and map finish-create conflicts cleanly.
• Augmented integration coverage in chromadb/test/distributed/test_task_api.py for conflicting attachments, invalid params, and pre-existing output collections to assert new error strings.
• Adjusted gRPC service (go/pkg/sysdb/grpc/task_service.go) to translate coordinator errors into explicit NotFound / AlreadyExists responses and preserve existing gRPC statuses from downstream calls.

Affected Areas

• go/pkg/sysdb/coordinator/task.go
• go/pkg/sysdb/grpc/task_service.go
• go/pkg/sysdb/metastore/db/dbmodel/constants.go
• rust/sysdb/src/sysdb.rs
• rust/frontend/src/impls/service_based_frontend.rs
• chromadb/test/distributed/test_task_api.py
• rust/types/src/api_types.rs
• rust/types/src/flush.rs

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

tanujnay112 · 2025-12-06T01:34:24Z

rust/sysdb/src/sysdb.rs

    AlreadyExists,
+    #[error("{0}")]
+    CollectionAlreadyHasFunction(String),
+    #[error("{0}")]


Need to see if this is ok

rust/sysdb/src/sysdb.rs

go/pkg/sysdb/coordinator/task.go

rescrv · 2025-12-08T16:36:27Z

rust/frontend/src/impls/service_based_frontend.rs

+                chroma_types::FinishCreateAttachedFunctionError::OutputCollectionExists => {
+                    chroma_types::AttachFunctionError::OutputCollectionExists(
+                        output_collection.clone(),
+                    )
+                }
+                chroma_types::FinishCreateAttachedFunctionError::AttachedFunctionNotFound => {
+                    chroma_types::AttachFunctionError::Internal(Box::new(e))
+                }
+                chroma_types::FinishCreateAttachedFunctionError::FailedToFinishCreateAttachedFunction(
+                    status,
+                ) => chroma_types::AttachFunctionError::Internal(Box::new(
+                    chroma_error::TonicError(status),
+                )),


Is there a reason AttachFunctionError doesn't just have a Finish variant that takes one of these as its arg?

Fixed and cleaned up

rescrv · 2025-12-08T16:38:46Z

go/pkg/sysdb/coordinator/create_task_test.go

+			// Note: validateAttachedFunctionMatchesRequest uses dbmodel.GetFunctionNameByID (static lookup)
+			// which returns "record_counter" for FunctionRecordCounter - different from requestedOperatorName
+			// Validation returns (false, nil) early, so DatabaseDb.GetDatabases is NOT called


Why is it different from recordOperatorName? What's the implication? Tell me that here.

AI wrote this but requestedOperatorName is a variable above. This test seeks to make sure that if you try to attach another function to a collection that already has a different function attached, you will try to see if the requested function attachment matches the previously attached function and fail. This part of the test makes sure that it doesn't make any further database calls during this check/

rescrv

I'd clean up the logging sprintf so it looks good when it errors.

rescrv · 2025-12-08T21:47:29Z

rust/sysdb/src/sysdb.rs

 pub enum AttachFunctionError {
-    #[error("Attached function already exists")]
-    AlreadyExists,
+    #[error("{0}")]


This will sprintf the string with no per-variant string.

tanujnay112 marked this pull request as ready for review December 6, 2025 00:44

This comment has been minimized.

Sign in to view

tanujnay112 force-pushed the fix_errors branch from 8740ade to 6464a8c Compare December 6, 2025 01:33

tanujnay112 commented Dec 6, 2025

View reviewed changes

propel-code-bot bot reviewed Dec 6, 2025

View reviewed changes

rust/sysdb/src/sysdb.rs Outdated Show resolved Hide resolved

tanujnay112 force-pushed the fix_errors branch from 6464a8c to db34d2a Compare December 7, 2025 09:48

propel-code-bot bot reviewed Dec 7, 2025

View reviewed changes

rust/sysdb/src/sysdb.rs Show resolved Hide resolved

tanujnay112 force-pushed the fix_errors branch from db34d2a to 2bc13cf Compare December 7, 2025 10:05

propel-code-bot bot reviewed Dec 7, 2025

View reviewed changes

go/pkg/sysdb/coordinator/task.go Outdated Show resolved Hide resolved

tanujnay112 mentioned this pull request Dec 8, 2025

[BUG]: Allow repairing partially created attached functions #5981

Merged

1 task

tanujnay112 changed the title ~~[ENH]: Make error messages in attach_function better~~ [ENH]: Make error messages in attach_function clearer Dec 8, 2025

tanujnay112 force-pushed the fix_errors branch from 2bc13cf to d05f840 Compare December 8, 2025 09:32

This comment has been minimized.

Sign in to view

tanujnay112 force-pushed the fix_errors branch 2 times, most recently from dfd5bd9 to 2bfc4a0 Compare December 8, 2025 10:48

rescrv approved these changes Dec 8, 2025

View reviewed changes

tanujnay112 force-pushed the fix_errors branch from 2bfc4a0 to f181672 Compare December 8, 2025 21:38

This comment has been minimized.

Sign in to view

rescrv approved these changes Dec 8, 2025

View reviewed changes

tanujnay112 force-pushed the fix_errors branch from f181672 to 4a11b06 Compare December 8, 2025 22:48

tanujnay112 requested a review from rescrv December 8, 2025 23:02

tanujnay112 force-pushed the fix_errors branch from 4a11b06 to 1156e23 Compare December 9, 2025 03:03

[ENH]: Make error messages in attach_function better

8aa4f5f

tanujnay112 force-pushed the fix_errors branch from 1156e23 to 8aa4f5f Compare December 9, 2025 19:46

tanujnay112 merged commit 54d8c8c into main Dec 9, 2025
64 checks passed

[ENH]: Make error messages in attach_function clearer #5973

[ENH]: Make error messages in attach_function clearer #5973

Conversation

tanujnay112 commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Test plan

Migration plan

Observability plan

Documentation Changes

Uh oh!

github-actions bot commented Dec 6, 2025

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Uh oh!

tanujnay112 commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

propel-code-bot bot commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

tanujnay112 Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

rescrv Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

tanujnay112 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

rescrv Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

tanujnay112 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rescrv left a comment

Choose a reason for hiding this comment

Uh oh!

rescrv Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

tanujnay112 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanujnay112 commented Dec 6, 2025 •

edited

Loading

tanujnay112 commented Dec 6, 2025 •

edited

Loading

propel-code-bot bot commented Dec 6, 2025 •

edited

Loading