issue: #51253
> Rebased on master after #51254 merged. The `db_name`/`db_id` part of
the original PR is now covered by #51254; this PR is reduced to the two
remaining gaps on the same query-by-id path.
## Problem
When `DescribeCollection` is called with **only a `collectionID`** — no
collection name, no db name (e.g. the HTTP management API on `:9091`) —
two gaps remain after #51254:
1. **Top-level `collection_name` is empty** on both the cached provider
path and the `describeCollectionTask` path. The response is initialized
from the (empty) `request.CollectionName` before the name is resolved,
and never backfilled.
2. **The cached provider discards the caller-provided collection id.**
After deriving the name from the id, it re-resolves the id from that
name via `GetCollectionID(db, name)`. Id-only lookups are cached under
the request db name — empty here — so with same-name collections in
different databases, a concurrent cache refresh can rebind the entry
under the `""` key and the request would silently describe the wrong
collection (the derived name matches, the id does not).
## Fix
- `service_provider.go`:
- Backfill `resp.CollectionName` from the resolved cached schema when
the request carried no name. Requests that pass a name (including
aliases) still echo it unchanged.
- Skip the name→id re-resolution when the caller already supplied the
id. `GetCollectionInfo` already validates the cache entry against the id
(`collInfo.collID != collectionID` → refresh by id), so the
caller-provided id is authoritative end to end. Name+id requests keep
the existing name-precedence behavior.
- Resolve identifiers on local copies instead of rewriting
`request.CollectionName`/`CollectionID` in place — the access log
interceptor and the success metric labels serialize the request after
the handler returns, and previously recorded the resolved values instead
of what the client sent.
- `task.go`: backfill `t.result.CollectionName` from the coordinator
result on the non-cached path.
- `meta_cache.go`: fix a stale comment on `ResolveCollectionAlias` —
`update()` now always keys `collInfo` by the real collection name
(aliases live in the separate alias map), the old comment described
pre-refactor behavior.
- Tests:
-
`TestCachedProxyServiceProvider_DescribeCollection_ByIDFillsNameAndUsesRequestID`:
drives the id-only path; `GetCollectionID` is deliberately not mocked,
so any regression back to re-resolving the id panics the test.
- `TestDescribeCollectionTask_FillsNameFromResultWhenQueriedByID`: same
assertion for the non-cached path.
- Removed the now-unused `GetCollectionID` expectation from the test
added in #51254 (that call no longer happens on the id-only path).
## Follow-up commit: entity-keyed meta cache with a cluster-wide id
index
Reviewing this path surfaced a deeper issue in the proxy meta cache,
fixed in the second commit:
- By-id lookups (`GetCollectionName`, `GetCollectionInfo` with an empty
name) linearly scanned a whole db bucket under the read lock, on every
request even on cache hits.
- The bucket scanned/filled was keyed by the *request* db name — empty
for id-only calls — so entries landed in a bogus `""` bucket: a
collection also cached under its real db was duplicated, and same-name
collections of different databases evicted each other from the single
`""`-keyed slot.
The cache is now entity-keyed instead of request-keyed:
- Fills land under the collection's **actual database** (carried in the
describe response); the `""` bucket no longer exists.
- A cluster-wide `collectionID → entry` index serves by-id lookups in
**O(1)** regardless of the request db — matching rootcoord, which
resolves by-id describes straight from `collID2Meta` without consulting
the db name.
- Name lookups normalize an empty db name to `default`, mirroring
rootcoord's backward-compat normalization
(`meta_table.getCollectionByNameInternal`). This also closes a latent
cross-database mis-hit: a name lookup with an empty db could previously
return whichever same-name collection was last id-cached under the `""`
bucket.
- All removal paths maintain the index (pointer-identity-guarded
unindexing); invalidation sweeps stay exhaustive.
## Verification
- Ran locally against a current master core build: the full
`TestMetaCache*`, `TestCachedProxyServiceProvider*`,
`TestDescribeCollectionTask*`, alias and partition test sets all pass,
plus the **full `internal/proxy` suite** (only the pre-existing
etcd-dependent `TestProxyRpcLimit` fails locally — it needs a live etcd,
unrelated to this change).
- New tests:
`TestCachedProxyServiceProvider_DescribeCollection_ByIDFillsNameAndUsesRequestID`
(id-only path; also asserts the request is not rewritten),
`TestDescribeCollectionTask_FillsNameFromResultWhenQueriedByID`,
`TestMetaCache_ByIDIndexRealDBBucket` (same-name collections in two
databases filled by id: no eviction, entries under real dbs, by-name
reuses by-id fill, every removal path cleans the index),
`TestMetaCache_EmptyDBNameSharesDefaultEntry`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
## Update: proxy meta cache rebuilt around an id-primary store (2 new
commits)
Following review discussions on invalidation cost and the alias-DDL
races, this PR now also rebuilds the cache (issue: #51533, design doc:
`docs/design-docs/design_docs/20260716-proxy-metacache-id-inverted-index.md`
in this PR):
1. **`fix: forward the pre-alter alias target id in the AlterAlias
expiration`** — rootcoord resolves the pre-alter target under its
database lock and carries it in the new additive
`AlterAliasMessageHeader.old_collection_id`; the ack callback emits a
second expiration entry so proxies evict BOTH AlterAlias targets by id.
Closes the race where a concurrent Describe re-points the proxy's alias
resolution before the expiration arrives, leaving the old target
permanently stale. Older rootcoords (field 0) fall back to the proxy's
hint resolution.
2. **`enhance: rebuild the proxy meta cache around an id-primary
store`** — the primary store is keyed by the cluster-unique collection
id (single source of truth, with a per-database generation); name/alias
resolution become hints validated against the primary on read; partition
caches are keyed by id; fills are ordered against invalidations by a
fill RWMutex (drain in-flight describes before evicting), replacing the
per-collection timestamp floor. Every invalidation becomes O(1) — no
more full-cache scans under the write lock on
Load/Release/Drop/Rename/Alter broadcasts, and
DropDatabase/AlterDatabase becomes a generation bump. Also removes a
latent stale read (alias-keyed partition entries surviving
DropCollection).
Verification: targeted cache suites green (rename, alias re-point under
concurrent describe, drop+recreate with a reused name, db-generation
invalidation, cross-db isolation, gated-mock fill/invalidation
ordering); three negative controls (hint validation / dbGen check / fill
drain disabled) each fail exactly the test guarding them; full-package
`-race` delegated to CI.
---
## Update: simplification series (final form)
Following design review, the cache was iteratively simplified to an
**id-primary store with declared hints** — every mechanism that could be
replaced by an invariant was deleted (net-negative diffs throughout):
- Primary store keyed by the cluster-unique collection id; liveness IS
presence. Name/alias resolution are hints written ONLY together with
their entry (declared aliases) and validated against the primary on
every read — a stale hint can only cost one extra describe, never a
stale read.
- Fill/invalidation ordering via a fill RWMutex (drain in-flight
describes before evicting); the per-collection timestamp floor, database
generations, background GC, alias negative cache, per-partition cache,
and the resolve-and-forget DescribeAlias path are all **deleted**.
Evictions clean everything the entry owns synchronously; **no
invalidation path performs any RPC**.
- Old-rootcoord fallbacks: id-describes without DbName are served
uncached; alias-DDL broadcasts without ids trigger hint resolution plus
a holder scan **gated on that exact fingerprint** (dead code once
rootcoords are upgraded), closing the ghost-alias case with no healing
event.
- **Accepted gaps (WONT-FIX)** are recorded in the design doc §6 —
notably the upgrade-window new-target `Aliases` display lag (the
association exists only at rootcoord; self-heals on first use;
display-only).
See
`docs/design-docs/design_docs/20260716-proxy-metacache-id-inverted-index.md`
for the full design, invariants and trade-offs.
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## What
Enforce a function–field binding principle for function DDL (add / drop
/ alter function on an existing collection), so a function and its
output field stay coupled and already-stored vectors can never be
silently invalidated by DDL.
BM25 and MinHash follow the strict binding: add/drop only via the
unified `add_function_field` / `drop_function_field`, output field
always coupled. **TextEmbedding is a temporary carve-out** — its legacy
`AddCollectionFunction` / `DropCollectionFunction` paths are retained,
so attach-over-existing-field and detach are still possible for it,
because `add_function_field` embedding backfill is not yet implemented.
It will be folded into the unified path in a follow-up. So the "always
coupled" invariant currently holds for BM25/MinHash, not TextEmbedding.
## Changes
- **AlterCollectionSchema invariant**: reject standalone add-function (a
function must be added together with its new output field), reject
detaching a function without dropping its output field, and reject
function cascade (a new function's input being another function's
output).
- **Legacy RPCs**: `AddCollectionFunction` / `DropCollectionFunction`
are rejected for BM25/MinHash (must use `add_function_field` /
`drop_function_field`) and retained only for TextEmbedding (see
carve-out). `AlterCollectionFunction` is kept and gains the whitelist
below. In the alter and legacy-add paths, function field IDs are always
re-derived from field names (never trusted from the request), so a
request cannot inject an unrelated field ID that a later
`drop_function_field` would delete.
- **alter_function whitelist**: only connection/runtime params may
change (TextEmbedding: `url`, `credential`, `timeout_ms`,
`max_client_batch_size`, `region`, `location`, `projectid`, `user`);
BM25/MinHash have no alterable params. Function identity (type, name,
input/output fields) and output-shaping params (`dim`, `model_name`,
`endpoint` — the TEI model identity, `normalize`, `truncate*`, prompts,
...) are immutable. Params are normalized (keys lowercased, duplicate
keys rejected) so a crafted duplicate key cannot bypass the diff.
- **drop_function_field**: allow any output-producing function (dropping
needs no backfill), removing the previous BM25/MinHash-only restriction.
## Not in scope / follow-up
- Extending `add_function_field` to TextEmbedding (post-creation add
would require backfilling every existing row through the external
embedding model) — deferred; folding the TextEmbedding legacy paths into
the unified binding follows this.
- MinHash input-field analyzer immutability lives on a different DDL
path (`AlterCollectionField`); tracked as a separate follow-up.
## Breaking changes
- `DropCollectionFunction` on a **MinHash** function (detach — remove
the function but keep its output field) is no longer supported; use
`drop_function_field` instead (drops the function together with its
output field). If a released version supported MinHash detach, migrate
any such call to `drop_function_field`.
issue: #51348🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
issue: #51493
### What
#50221 split the `status` label values of `milvus_proxy_req_count` /
`milvus_proxy_grpc_latency` in place. Released in **v2.6.19**, that
silently zeroes every existing dashboard panel and alert rule matching
`status="fail"` / `status="rejected"` — an alert that stops firing
rather than erroring.
This keeps the classification but moves it onto its own dimension,
restoring the status domain to what <= v2.6.18 emitted:
```
status: success | fail | rejected | retry | total | abandon (as before v2.6.19)
cause: user | system | cancel | na (new)
```
| v2.6.19 / v2.6.20 | this PR |
|---|---|
| `status="fail_input"` | `status="fail", cause="user"` |
| `status="fail_system"` | `status="fail", cause="system"` |
| `status="rejected_user"` | `status="rejected", cause="user"` |
| `status="rejected_system"` | `status="rejected", cause="system"` |
| `status="cancel"` | `status="fail"` or `"rejected"`, `cause="cancel"`
|
| `success` / `retry` / `total` / `abandon` | unchanged, `cause="na"` |
### Why a label instead of new status values
- **Old queries work unchanged and count each request once.**
`status="fail"` is again every hard failure; Prometheus aggregates over
`cause` for free. That rollup is what a label dimension *is* — the split
forces every consumer who just wants "how many failures" to hand-write
`status=~"fail_.*"`.
- **Cardinality is unchanged.** `cause` is functionally dependent on the
outcome, so the realized `(status, cause)` pairs are exactly the series
the split already produces. Additive, not multiplicative.
- **New consumers lose nothing**: alert on `status="fail",
cause="system"`; route `cause="user"` to the owning application team.
The alternative — emitting old and new `status` values side by side
during a transition — was rejected: it defers the break rather than
removing it (the old values must still be dropped eventually, forcing
the same migration later), and meanwhile double-counts every request in
unfiltered aggregations.
`cancel` is folded into `cause` for the same reason: before v2.6.19
client cancellations were counted as `fail` (cancel in the response
status) or `rejected` (cancel at the interceptor), so promoting it to a
`status` value quietly makes `status="fail"` under-count versus older
releases. As a cause it stays excludable via `cause!="cancel"` without
redefining `status`.
### Also in this PR
- The 7 `snapshot_impl` sites that emitted a bare `fail` with no
classification now report their real cause via `failMetricLabel(err)`
(e.g. `snapshot_metadata_uri is required` is `cause="user"`, not a
system fault). Their `status` is unchanged.
- `deployments/monitor/grafana/milvus-dashboard.json`: the "Faild
Request Rate" panel goes back to `status="fail"` — a verbatim revert of
what #50221 changed, which is the compatibility claim demonstrated.
- Dev docs and stale comments that named the old label values.
### Verification
- `TestParseMetricLabelStatusDomainIsStable` pins the status domain, so
re-splitting it fails CI rather than shipping.
- Adding a label makes every `WithLabelValues` site arity-sensitive **at
runtime, not compile time** — a missed site panics in production. Unit
tests do not reach all of them (coverage shows
`GetRestoreSnapshotState`, `ListRestoreSnapshotJobs`, `PinSnapshotData`,
`UnpinSnapshotData` at 0%), so all **58 emit sites were verified
statically via AST**, not only the ones tests happen to hit.
- Passing: `pkg/metrics`, `pkg/util/requestutil`, `pkg/util/merr`,
`internal/distributed/proxy` (incl. `httpserver`, which covers the REST
emit path), and the `internal/proxy` snapshot / metric-label tests.
`golangci-lint` clean on every touched package.
- Pre-existing and unrelated: `service_test.go` bind failures (`listen
tcp :19529: address already in use`) reproduce identically on unmodified
master — a local process holds the port.
### Rollout
Should be cherry-picked to 2.6 so the window in which the fine-grained
`status` values exist stays confined to v2.6.19–v2.6.20. Users who
already adopted the new values on those two releases need a one-time
query change (`status="fail_system"` -> `status="fail",
cause="system"`); that is a known, bounded set, and preferable to
leaving every pre-2.6.19 dashboard silently broken.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Keep raw search distances through segment search, iterator output, index
query, and chunk merging so ranking/reduce order is not affected by
`round_decimal`.
Apply `round_decimal` only when producing the final search response,
after reduce/rerank/order-by has already determined result order.
Returned scores keep the requested precision without changing TopK
selection.
issue: #50347
---------
Signed-off-by: xianliang.li <xianliang.li@zilliz.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
issue: #50742
## What this PR does
Optimizes protobuf decoding on Milvus RPC read/write hot paths with a
hand-written decoder in `pkg/util/fastpb`. The final change contains no
gRPC connection-pool or connection-count changes.
## Fast decoder coverage
The gRPC `releaseCodec.Unmarshal` path dispatches these top-level
messages to fastpb:
- `internalpb.RetrieveResults`
- `milvuspb.InsertRequest`
- `milvuspb.UpsertRequest`
`schemapb.SearchResultData` is not a top-level codec-dispatched message.
It is decoded directly from `SlicedBlob` at the search/reduce hot paths
in:
- `internal/proxy/search_reduce_util.go`
- `internal/querynodev2/segments/result.go`
- `internal/querynodev2/local_worker.go`
- `internal/querynodev2/tasks/search_task_go_reduce.go`
Nested hot messages such as `FieldData`, scalar/vector arrays, and IDs
are decoded by the same package.
Main optimizations:
- varchar string arrays use one backing byte arena, reducing per-string
allocations
- packed float vectors use a single memcpy into self-owned memory
- cold, complex, unknown, and mismatched fields delegate or merge
through the official protobuf codec
- no new external dependency or source-buffer aliasing
## Compatibility boundary
The compatibility contract depends on the data path:
- **Untrusted client ingress (`InsertRequest` and `UpsertRequest`)**
validates proto3 strings as UTF-8 and is tested differentially against
`proto.Unmarshal`, including malformed input behavior.
- **Trusted internal result paths (`SearchResultData`,
`RetrieveResults`, and direct `FieldData` decoding)** assume canonical
Milvus/segcore protobuf output and intentionally skip UTF-8 validation
for performance. Therefore these paths do not claim equivalence for
arbitrary adversarial wire containing invalid UTF-8.
Within that boundary, the decoder preserves the protobuf behaviors
exercised by the differential tests:
- proto2 groups fall back to the official codec
- known fields with mismatched wire types fall back to the official
codec
- unknown and future fields are preserved through an official merge pass
- repeated singular messages merge from the raw wire, including
explicitly encoded proto3 default values
- repeated message-valued oneof members merge correctly, while different
variants remain last-wins
- mixed packed and unpacked scalar encodings preserve wire order
- descriptor field-set tripwires force decoder review when the protobuf
schema changes
The unsafe packed-float copy assumes the currently supported
little-endian Milvus targets such as x86-64 and arm64.
## Benchmarks
fastpb vs official `proto.Unmarshal`, using 1000-row payloads:
| path | payload | speedup | allocations/op |
|---|---|---:|---:|
| search / query | varchar | about 2x | about 1000-2035 to about 10-17 |
| search / query | float vector, dim 768 | about 6x | unchanged |
| insert | varchar with UTF-8 validation | about 1.8x | 1019 to about 10
|
## Verification
- unit and differential tests cover field descriptors, UTF-8 contracts,
wire-type mismatches, proto2 groups, unknown fields, packed/unpacked
ordering, oneof behavior, and repeated-message merge semantics
- randomized canonical-message equivalence tests cover search, retrieve,
insert, and upsert payloads
- `go test ./util/fastpb ./util/resource` passes
- `go vet ./util/fastpb ./util/resource` passes
- current full CI and end-to-end status is tracked by the PR checks
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
issue: #51253
### What this PR does
When `proxy.enableCachedServiceProvider` is on and `DescribeCollection`
is called with only a collection id (e.g. the HTTP management API on
port 9091), the cached provider echoed the request db name — empty or
default — instead of the database the collection actually belongs to,
and never filled `db_id`. The remote path does not have this problem
because the coordinator resolves the database by DBID and returns it in
the top-level `DbName`/`DbId` fields.
The proxy meta cache already stores the actual db name of each
collection; this PR also stores the db id, and prefers the cached
`dbName`/`dbID` when assembling the cached DescribeCollection response.
### Verification
- New unit test
`TestCachedProxyServiceProvider_DescribeCollection_ByIDReturnsActualDbName`
(request carries only `collectionID`, asserts real `db_name`/`db_id` are
returned); existing tests in the file still pass.
- Verified end-to-end on a standalone build of this branch: created
`db1`/`coll1`, queried `GET :9091/api/v1/collection` with only
`collectionID` — response now returns `"db_name": "db1"` and the `db_id`
matching `databases/describe`, where it previously returned neither.
- The same logic was also verified on a 2.5-based deployment (real
cluster) before porting to master.
Happy to backport to 2.5/2.6 if needed.
Signed-off-by: fudongying <fudongying@bytedance.com>
issue: #50961
## Summary
Allow AlterCollectionField to set or delete analyzer configuration on
string fields before they are used by text match or BM25. Keep analyzer
params immutable once text match is enabled or a BM25 function depends
on the field.
When analyzer params change, RootCoord revalidates the new analyzer
config, reserves newly referenced file resources before broadcasting,
and lets MetaTable.AlterCollection reconcile file resource refCnt for
request, replay, replicated, and recovery paths.
## Verification
- `git diff --check`
- `go test -tags dynamic,test -gcflags="all=-N -l" -count=1
./util/typeutil -run TestIsBm25FunctionInputField` from `pkg/`
- `go test -tags dynamic,test -gcflags="all=-N -l" -count=1
./internal/rootcoord -run
'TestMetaTableAlterCollectionFileResourceRefCnt|TestDDLCallbacksAlterCollectionFieldAnalyzerValidation'`
blocked locally: missing `milvus_core.pc`
- `go test -tags dynamic,test -gcflags="all=-N -l" -count=1
./internal/proxy -run TestAlterCollectionField` blocked locally: missing
`milvus_core.pc` and `milvus-storage.pc`
Signed-off-by: aoiasd <zhicheng.yue@zilliz.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
issue: #51104
companion SDK PR (merge first): milvus-io/pymilvus#3670
## Summary
`add_function_field` previously accepted a vector output field without
any index meta; bump-schema-version compaction then stalled forever (no
segment index task can be created for a field with no index defined, and
`GetQueryVChanPositions` never hands off unindexed compacted-to segments
— the fail-closed chain is silent). This PR makes the index meta a
mandatory, atomically-bound part of the DDL:
- **Mandatory at every layer**: SDK requires `index_params`; proxy and
rootcoord reject vector function-output fields without an explicit
`index_type` strictly before the broadcast (no silent AUTOINDEX).
- **create_index-aligned materialization**: proxy normalizes the params
with the same logic as `create_index` (name rules via
`validateIndexName`, checker existence, `CheckTrain` incl. dimension
filling, function-type defaults such as `bm25_k1/bm25_b/bm25_avgdl`) and
writes them back into the request; rootcoord prepare allocates index
id/name, rejects name conflicts and unknown index types, and serializes
a complete `indexpb.FieldIndex` into the new
`AlterCollectionMessageUpdates.bound_field_indexes` WAL field.
- **Atomic apply**: the alter-collection ack callback commits the schema
and then applies the bound index by dispatching a synthetic
`CreateIndexMessage` to datacoord's existing `createIndexV2AckCallback`
(same pattern as `cascadeDropFieldIndexesInline`). The callback is
replayed until success across crashes and is fully idempotent (all ids
come from the message body), so `DDL success ⇒ index meta exists` holds
with no partial terminal state.
- **Reuse promotion**: `ValidateIndexParams`/`CheckDuplidateKey` moved
from datacoord to `internal/util/indexparamcheck`; shared
`ExpandIndexParams` / `FillFunctionOutputIndexParams` /
`PrepareFunctionOutputIndexParams` helpers now back both the
create_index path and this DDL.
- **Dead fan-out removed**: the ordinary `CreateIndex` broadcast is
reverted to control-channel-only; the vchannel fan-out had no consumer
and wrote one dead WAL entry per vchannel per index creation.
- **No milvus-proto change**: the request already carried
`FieldInfo.index_name`/`extra_params` (previously unused); this feature
activates them.
- e2e suites updated to the new semantics:
`test_add_function_field_feature.py` rewritten (explicit `index_params`
everywhere, post-DDL `create_index` blocks removed since the bound index
conflicts with a second distinct index per existing semantics, new
negative cases for the mandatory checks); the external-table negative
case updated as well.
## Verification
- New/updated unit tests: rootcoord DDL callbacks
(mandatory/AUTOINDEX/unknown-index-type rejections, bound-index
materialization + ack-callback application via a recording fake), proxy
task validation (invalid index name, normalization write-back), shared
helper tests.
- End-to-end on a live cluster (StorageV3 + bump compaction enabled):
request without params rejected; index meta visible with
create_index-aligned params immediately after the DDL returns; bump
compaction rewrote 2000-row sealed segments and the bound index built on
the results (the exact chain that previously stalled); BM25 search over
pre-DDL rows succeeds once the compacted segments are loaded.
## Limitations (follow-ups)
- Live propagation of the new index meta to already-loaded QueryNode
delegators is intentionally out of scope; a loaded collection observes
the bound index through the segment load path (e.g. reload /
compacted-segment load). Tracked as a separate follow-up PR.
- The python e2e suites require the companion pymilvus change:
milvus-io/pymilvus#3670 is merged and
`tests/python_client/requirements.txt` is bumped to
`pymilvus==3.1.0rc61` in this PR (all 22 rewritten cases + the
external-table case verified locally against the released rc61).
- During a rolling-upgrade window an old coordinator replaying the new
message ignores `bound_field_indexes` (proto3 unknown field), i.e.
pre-existing behavior; avoid the new argument in mixed-version clusters.
- `AddCollectionFunction` on an existing field and plain
add-vector-field keep their current behavior (the WAL carrier is generic
for future extension).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
### What & why
`bump_schema_version`'s full-rewrite path (`runFullSchemaRewrite`,
reached only for a **drop-field** schema bump) builds a from-scratch
output manifest via `NewBinlogRecordWriter` and did **not** carry the
source segment's TEXT LOB references — leaving dangling refs for an
out-of-line (`>=64KB`) TEXT column. This was the `TODO(#50021)`.
### What this does
Wire **REUSE_ALL** LOB handling into `runFullSchemaRewrite`, mirroring
sort compaction. Both are `1->1` with a single output manifest, so
REUSE_ALL is always correct: a schema bump never changes the existing
TEXT LOB data — only its references are carried.
- `internal/compaction/lob_compaction.go`: `GetForcedStrategy` forces
REUSE_ALL for `BumpSchemaVersionCompaction`.
- `internal/datanode/compactor/bump_schema_version_compactor.go`:
collect the source LOB files into a `LOBCompactionContext`, pass
`storage.WithTextRefsAsBinary()`, update per-LOB-file `valid_rows`
(`SetSegmentRowStats`), and merge the LOB references into the output
manifest (`applyLOBCompaction`) **before** building the text-match index
— `createTextIndex` reads the TEXT column through this manifest, so the
carried LOB files must already be referenced (matches sort/mix
ordering). Removed the `TODO(#50021)`.
- Unit tests: forced strategy, init/apply LOB wiring, the
applyLOBCompaction-before-createTextIndex ordering
(`TestFullRewriteMergesLOBRefsBeforeBuildingTextIndex`), and a real
add-nullable-TEXT + drop-field full-rewrite regression driving the
actual packed writer
(`TestFullRewriteFillsMissingNullableTextAsBinary`).
### Also: guard add_function_field behind StorageV3 (#51167)
Adding a function to an existing collection must backfill the new output
field into **pre-existing** sealed segments; that backfill runs through
`bump_schema_version` compaction, which only works on a StorageV3
segment. A pre-existing V2 segment is only backfilled after the
storage-version upgrade compaction rewrites it to V3, and that upgrade
runs only when **both** `common.storage.useLoonFFI` and
`dataCoord.compaction.storageVersion.enabled` are on. The proxy now
rejects add-function unless both are enabled
(`validateAddFunctionRequiresStorageV3`, wired into
`addCollectionFunctionTask` and `alterCollectionSchemaTask`).
`create_collection` with a function is unaffected. Full reasoning in
#51167.
### Notes
- Reuses the LOB machinery from **#50784**.
- The `GenerateEmptyArrayFromSchema` binary-TEXT production fix landed
independently on master via **#51124**; this PR keeps the regression
test guarding the bump path.
issue: #50021, #51167🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make ComposeTSByTime the logical-zero time-based timestamp API, add
ComposeTSByTimeWithLogical for explicit logical composition in tests,
and remove the duplicate GetCurrentTime wrapper.
See also: #51119
Signed-off-by: yangxuan <xuan.yang@zilliz.com>
Signed-off-by: yangxuan <xuan.yang@zilliz.com>
issue: #50882
## Problem
`NULL` is not a grammar token in the plan parser, so the lexer treats a
bare `NULL`/`null` as an ordinary identifier. When it appears where a
value is expected — e.g. inside an `in [...]` value list:
```
id in [6560, NULL, 6722, -7856, -6757]
```
`VisitIdentifier` runs a field lookup on `NULL`:
- **without a dynamic field** it fails with the misleading `field NULL
not exist`, which misleads users into thinking they must add a column
named `NULL` to their schema;
- **with a dynamic field** it is silently mistaken for a JSON key and
accepted.
This affects both `delete()` and `query()` (they share the same
`ParseExpr` path).
## Fix
Treat bare `null`/`NULL` (case-insensitive) as a reserved word in
`VisitIdentifier` and reject it up-front with an actionable message,
regardless of whether a dynamic field exists:
```
NULL literal is not supported in expressions; use '<field> is null' or '<field> is not null' instead
```
This implements option 1 (robust rejection) from the issue. `<field> is
null` / `is not null` are unaffected (they parse via dedicated tokens,
not `VisitIdentifier`).
**The guard is schema-aware for backward compatibility** (review
feedback): `null` only becomes a create-time keyword in this PR, so a
legacy collection may own a field literally named `null`, and the bare
identifier is the only syntax that can reference a top-level scalar
field. The rejection is therefore gated on a strict `GetFieldFromName`
lookup — a real declared field named `null` resolves exactly as before
this PR (including `null is null`, which now parses as a valid predicate
on that field), while the dynamic-field fallback (the source of the
original misparse) still rejects. A JSON **sub-key** literally named
`null` additionally remains reachable via quoting, e.g. `field["null"]`
/ `$meta["null"]`, whose base identifier is the field name, not `null`.
## Test
Added `TestExpr_NullLiteral` covering rejection of `NULL` inside `in
[...]`, as a comparison operand, and as a left operand, plus
confirmation that `is null` / `is not null` still work. Added
`TestExpr_NullLiteral_LegacyNullField` locking the legacy path: a scalar
field named `null` stays queryable via the bare identifier, a JSON field
named `null` stays reachable as `null["x"]`, and any casing that doesn't
name a real field keeps the reserved-word rejection. Full `planparserv2`
package tests pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Note: field-name validation tightened
Alongside the expression-level rejection, `validateFieldName` (proxy)
and
`validateAddedStructFieldName` (rootcoord) now go through
`common.IsFieldNameKeyword`,
which rejects **any casing** of `null` (`null`/`NULL`/`nUlL`/…) as a
field name —
consistent with how a bare `NULL` literal is rejected in expressions.
Previously
`FieldNameKeywords` had no `null` entry, so a field literally named
`null` could be
created. This only affects **new** field creation (validation runs at
create time);
existing collections/fields are unaffected — and thanks to the
schema-aware guard
above, a legacy field literally named `null` also stays queryable.
---------
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Move the RBAC check before the hook interceptor.
Write the resolved roles for the current request into the request
context.
---------
Signed-off-by: aoiasd <zhicheng.yue@zilliz.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
issue: #50573
## What this PR does
This PR fixes `DumpMessages` transaction handling. When the WAL scanner
returns an assembled transaction message, `DumpMessages` now expands it
into separate responses:
- begin transaction message
- data messages that pass the existing message type filter
- commit transaction message
This allows recovery and CDC consumers to decode and replay transaction
data messages correctly.
The PR also supports inclusive start semantics through
`include_start_message`, while preserving the existing default behavior
of starting after `start_message_id` for compatibility.
## Tests
- Added unit coverage for default StartAfter policy.
- Added unit coverage for IncludeStartMessage StartFrom policy.
- Added unit coverage for transaction expansion and send error handling.
- Ran `gofmt` and `git diff --check`.
Linked proto change: milvus-io/milvus-proto#616
Signed-off-by: alohaha22 <shawn.work1229@outlook.com>
issue: https://github.com/milvus-io/milvus/issues/44358
design doc:
docs/design-docs/design_docs/20260609-external-snapshot-export-restore.md
Part 1/3 of the external snapshot cross-bucket restore stack.
This PR defines the API contract and entry surfaces for external
snapshot export and restore:
- Adds the consolidated design document for cross-bucket external
snapshot restore.
- Adds public gRPC, REST, and Go SDK API surfaces for
RestoreExternalSnapshot and ExportSnapshot.
- Adds internal DataCoord proto plumbing and generated code needed by
later implementation PRs.
- Wires Proxy RBAC grouping and database interceptor behavior for the
new APIs.
- Keeps the request contract on a single external_spec field and keeps
db_name for namespace routing rather than permission scoping.
Validation copied from the commit:
- GOTOOLCHAIN=go1.25.10 go test -c -tags dynamic,test -gcflags="all=-N
-l" -ldflags="-r ${RPATH}" -o /tmp/datacoord-commit1.test
github.com/milvus-io/milvus/internal/datacoord
- cd client && GOTOOLCHAIN=go1.25.10 go test -c -o
/tmp/client-milvusclient-commit1.test ./milvusclient
- internal/proxy package compile was blocked locally by missing C++
header internal/core/output/include/segcore/search_result_export_c.h
---------
Signed-off-by: Wei Liu <wei.liu@zilliz.com>
issue: https://github.com/milvus-io/milvus/issues/50571
design doc: docs/design-docs/design_docs/20260624-function-chain-api.md
Add the FunctionChain proto-to-runtime path for ordinary Search L2
rerank. The change converts public FunctionChain protos into ChainRepr,
derives caller-independent read/write metadata, validates
Search-specific inputs in Proxy, and executes public chains through the
existing rerank pipeline.
Add request validation for duplicate stages, unsupported stages,
unsupported system inputs/outputs, unknown schema fields, unsupported
input field types, and hybrid-search usage. Extend REST v2 request
conversion to accept function_chains.
Replace score_combine with num_combine and add typed parameter readers,
repr-based FuncChain construction, chain optimization/pruning helpers,
and model-rerank parameter handling. Add coverage for chain repr
conversion, function/operator behavior, Proxy planning, search pipeline
integration, REST conversion, and REST API cases.
Add the Function Chain API design document and update milvus-proto to
the latest upstream pseudo-version.
Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com>
## Summary
Reject search iterator v2 for hybrid search because the current response
metadata model only supports iterator v2 results for a single query
info. This avoids returning hybrid search hits with empty iterator
metadata that cannot be used for the next page.
issue: #50407
## Test Plan
- [x] `git diff --check`
- [x] `make milvus`
- [x] `MILVUSCONF=/tmp/milvus_issue-50407/configs
PKG_CONFIG_PATH=$(pwd)/internal/core/output/lib/pkgconfig
LD_LIBRARY_PATH=$(pwd)/internal/core/output/lib go test -gcflags="all=-N
-l" -tags dynamic,test ./internal/proxy -run
"^TestSearchTask_ArrayOfVectorHybridSearch$" -count=1 -ldflags="-r
$(pwd)/internal/core/output/lib"`
Signed-off-by: xianliang.li <xianliang.li@zilliz.com>
issue: #46308
## Summary
Normalize analyzer names before running multi-analyzer highlight and
analyzer requests: omitted names use default, one name is broadcast, and
per-text names must match the text count. Also add default analyzer
names for lexical highlight query texts and request the analyzer-name
field for multi-analyzer query-only highlight.
Signed-off-by: aoiasd <zhicheng.yue@zilliz.com>
## Issue
Fixes#50678
## Problem
When `common.security.authorizationEnabled=true`, the Proxy's
`PrivilegeInterceptor` resolved the database for privilege checks solely
from the gRPC connection-context db header (set via
`useDatabase`/`using_database`, defaulting to `default`), via
`GetCurDBNameFromContextOrDefault(ctx)`. It **ignored the `DbName`
carried in the request body**.
Downstream handlers, however, operate on the request's `DbName` (after
`DatabaseInterceptor` normalizes it — request body takes precedence,
then context header, then default). So when a caller does not call
`useDatabase` and instead targets a database directly via the request's
`db_name`, the auth check and the actual operation run against
**different** databases. This produces two classes of problems for every
db-scoped privilege check (`CreateCollection`, `DropCollection`,
`CreateIndex`, `Insert`, `Search`/`Query`, `AlterDatabase`, …):
1. **False denial (functional bug):** a user holding a grant on
`db_target`, whose connection stays on `default` and who targets
`db_target` only through the request's `db_name`, is incorrectly checked
against `default` and receives `PermissionDenied`.
2. **Cross-db privilege escalation (security risk):** a user with a
grant on `default` can set the request's `db_name` to a different
database they are not authorized for; the check passes against `default`
while the operation actually lands on the target database.
## Fix
Resolve the db the request actually operates on: prefer the request-body
`DbName`, falling back to the connection-context db and finally the
cluster default. Privilege checks now use this resolution so
authorization runs against the same database as the operation.
- `internal/proxy/util.go`: add `GetCurDBNameFromRequestOrContext(ctx,
req)` (+ `dbNameGetter` interface). Requests without a top-level
`DbName` field (e.g. grant-management requests carrying `Entity.DbName`)
fall through to the previous context-based behavior, so their semantics
are unchanged.
- `internal/proxy/privilege_interceptor.go`: resolve `dbName` via the
new helper instead of `GetCurDBNameFromContextOrDefault(ctx)`.
## Tests
- `TestPrivilegeInterceptor`: a few assertions had encoded the old
"context decides the db" behavior (e.g. a user granted Global-All only
on `default` was expected to be *allowed* when sending `db_name=db_test`
— which is exactly the escalation bug). Realigned the positive-path
request db with the granted db so the assertions are self-consistent
under correct semantics.
- Added `TestPrivilegeInterceptorRequestDBName` covering the two issue
scenarios plus the fallback:
- granted on `db_target`, no `useDatabase`, request `db_name=db_target`
→ **allowed** (false-denial fixed);
- granted only on `default`, request `db_name=db_escalate` → **denied**
(escalation blocked);
- request omits `db_name` → falls back to the connection-context db
(unchanged behavior).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
issue: https://github.com/milvus-io/milvus/issues/49879
## Summary
- Keep `limit + offset` candidates during reduce for common search with
`order_by_fields`.
- Apply final offset/limit after scalar order_by sorting in the search
pipeline.
- Add targeted proxy unit tests for row-aligned reorder, group
pagination, string IDs, and vector slicing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
issue: #50749
Depends on #50527.
Route proxy PK/channel and partition-key hash helpers through the
routing table while preserving the existing hash modulo behavior.
Query/search are covered through assignPartitionKeys, which delegates to
internal/util/typeutil.HashKey2Partitions.
Signed-off-by: sunby <sunbingyi1992@gmail.com>
issue: https://github.com/milvus-io/milvus/issues/42148
1. Support range search in hybrid search
Enable element-level vector fields to participate in hybrid range search
while preserving element identity and offsets.
2. Support group-by in hybrid search, with constraints
Enable element-level hybrid search with PK group-by and preserve element
offsets. For group-by, sub-searches that need element identity merging
must stay within the same struct array semantic scope.
Also, it fixes 2 related bugs:
1. Fix group-by related issues
Fix lost element indices and incorrect merging behavior in query/search
group-by paths, so multiple matching elements under the same PK can
still be returned with correct offsets.
2. Fix aggregation output issues
Fix aggregation/count output corruption when a collection contains
struct array fields. Proxy struct-field reconstruction now preserves
virtual aggregation fields such as `count(*)` instead of treating
`field_id=0` as a struct sub-field. As a result, `id + count(*)`,
element-level `count(*)`, and group-by `count(*)` return correctly named
fields.
---------
Signed-off-by: SpadeA <tangchenjie1210@gmail.com>
## Description
The proxy access log recorded the collection field as `Unknown` for any
request that does not expose a singular `GetCollectionName() string`.
This made it impossible to attribute traffic to a specific collection
from access logs — most notably when diagnosing per-collection flush
rate-limit rejections
(`milvus_proxy_rate_limit_req_count{msg_type="DDLFlush",
status="fail"}`, default limit
`quotaAndLimits.flushRate.collection.max=0.1`).
Affected proxy-facing requests:
| Request | Collection field | Before | After |
|---------|------------------|--------|-------|
| `Flush` | `collection_names []string` | `Unknown` | `[coll_a coll_b]`
|
| `ShowCollections` | `collection_names []string` | `Unknown` | `[coll_a
coll_b]` |
| `RenameCollection` | `OldName` / `NewName` | `Unknown` | `old_coll` |
| `BatchDescribeCollection` | `collection_name []string` | `Unknown` |
`[coll_a coll_b]` |
## Root cause
`GrpcAccessInfo.CollectionName()` / `RestfulInfo.CollectionName()` only
used `requestutil.GetCollectionNameFromRequest`, which relies on the
singular `CollectionNameGetter` (`GetCollectionName() string`).
`FlushRequest`/`ShowCollectionsRequest` only have the repeated
`GetCollectionNames() []string`, so the assertion failed and the field
fell back to `Unknown`. `RenameCollectionRequest` and
`BatchDescribeCollectionRequest` carry the collection under non-standard
fields.
## Fix
- Add `CollectionNamesGetter` + `GetCollectionNamesFromRequest` to
`requestutil` (mirrors the existing `PartitionNamesGetter`).
- In `CollectionName()`, fall back to the plural getter, then to the
request-specific fields, mirroring the existing `PartitionName()` plural
handling.
- Unit test covering nil / singular / `Flush` / `RenameCollection` /
`BatchDescribe`.
Internal coordinator RPCs (`DescribeSegment`, `ShowSegments`) only carry
`CollectionID` and do not flow through the proxy access log, so they are
out of scope.
Fixes#50741🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
issue: #46308
## Summary
Keep lexical highlight corpus texts aligned with search result rows when
highlighted string fields are nullable or empty. Null and empty rows now
still produce highlight data with an empty fragments list.
Signed-off-by: aoiasd <zhicheng.yue@zilliz.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
related: #46442
This commit adds proper RBAC (Role-Based Access Control) support for the
/expr HTTP endpoint, replacing the previous root-only authentication.
Changes:
- Add new rbac.go with CheckPrivilege function for HTTP endpoints
- Support HTTP Basic Auth only (removed non-standard Bearer token
format)
- Integrate with existing Casbin RBAC framework
- Add PrivilegeExpr to GlobalLevelPrivileges and ClusterAdminPrivileges
- Register GetUserRoleFunc callback in meta_cache.go
- Update tests for new RBAC behavior
Security features:
- Authentication required when authorization is enabled
- Root user bypass when RootShouldBindRole is false
- Proper 401/403 status code differentiation
- Integration with privilege result cache
---------
Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
## Summary
- Disable the proxy search requery path when a collection uses namespace
partition mode.
- Apply the same behavior to normal search and hybrid search
initialization.
- Add coverage for the Always requery policy being overridden by
namespace partition mode.
issue: #50626
---------
Signed-off-by: sunby <sunbingyi1992@gmail.com>
Fixes#50424
## Summary
This PR uses the `DropFunctionOutputFields` request flag to let
`AlterCollectionSchema` distinguish two client-side drop paths:
- `drop_collection_function` detaches the function and keeps its output
fields as normal schema fields.
- `drop_function_field` sets the flag and cascades supported
BM25/MinHash output fields.
MinHash detach keeps the output field because the MinHash signature is a
materialized `BinaryVector` field. Insert and compaction paths persist
the generated signature, raw retrieval is only blocked for BM25 function
outputs, and after detach the MinHash output field is kept with
`IsFunctionOutput=false` so it can stand alone as a normal vector field.
The proxy validation and RootCoord schema mutation logic now follow the
requested semantics, with tests covering detach-only and
output-field-drop behavior.
## Tests
- `make generated-proto-without-cpp`
- `make`
- `go test -v -count=1 -tags dynamic,test -gcflags="all=-N -l"
./internal/proxy -run
"TestAlterCollectionSchemaTask_PreExecute|TestValidateDropFunction"
-timeout 300s`
- `go test -v -count=1 -tags dynamic,test -gcflags="all=-N -l"
./internal/rootcoord -run
"TestBuildSchemaForDetachFunction|TestBuildSchemaForDropFunctionField"
-timeout 300s`
- `/Users/zilliz/dev/milvus_py`: `pytest
test_drop_collection_field.py::test_drop_function_field_bm25_cascades_output_field
test_drop_collection_field.py::test_drop_collection_function_detaches_minhash_output_field
test_drop_collection_field.py::test_drop_whole_struct_array_field_basic
-v -s`
Signed-off-by: sijie-ni-0214 <sijie.ni@zilliz.com>
issue: https://github.com/milvus-io/milvus/issues/50476
This PR rejects dropping a field when it is referenced by the collection
`ttl_field` property. Users need to drop the property first, which
avoids leaving dangling entity-level TTL metadata after Drop Field.
Validation:
- gofumpt -l internal/proxy/task.go internal/proxy/task_test.go
- git diff --check
- TestValidateDropField
Signed-off-by: sijie-ni-0214 <sijie.ni@zilliz.com>
## Summary
- Allow AlterCollectionSchema AddRequest to carry a field-only add
request while keeping the added field as a non-function output.
- Support function-only add requests against existing output fields, and
validate the merged function schema through the shared function
validator.
- Keep the BM25 field-plus-function path explicit, reject invalid BM25
add forms, and normalize TIMESTAMPTZ default values before broadcasting
field-only schema changes.
issue: #50119
## Test Plan
- [x] source scripts/setenv.sh && go test -v -count=1 -tags dynamic,test
-gcflags="all=-N -l" -ldflags="-r ${MILVUS_WORK_DIR}/cmake_build/lib -r
${MILVUS_WORK_DIR}/internal/core/output/lib" ./internal/proxy -run
'TestAlterCollectionSchemaTask($|_)' -timeout 300s
- [x] gofumpt -l internal/proxy/function_task.go
internal/proxy/function_task_test.go internal/proxy/task.go
internal/proxy/task_test.go internal/proxy/util.go
internal/proxy/util_test.go
internal/rootcoord/ddl_callbacks_alter_collection_schema.go
internal/rootcoord/ddl_callbacks_alter_collection_schema_test.go
internal/util/function/validator/validator.go
- [x] git diff --check upstream/master..HEAD
- [ ] source scripts/setenv.sh && go test -v -count=1 -tags dynamic,test
-gcflags="all=-N -l" -ldflags="-r ${MILVUS_WORK_DIR}/cmake_build/lib -r
${MILVUS_WORK_DIR}/internal/core/output/lib" ./internal/rootcoord -run
'TestDDLCallbacksBroadcastAlterCollectionSchema|TestDDLCallbacksAlterCollectionSchemaAddSkipsSchemaDropReady'
-timeout 300s (blocked locally at link time by stale C++/Loon symbols
such as _FreeCFieldMemSizeList and _loon_segment_writer_* in local core
libraries)
Signed-off-by: sijie-ni-0214 <sijie.ni@zilliz.com>
issue: #50557
Summary:
- Add and validate `namespace.sharding.enabled` collection property.
- Default unset property to `false`.
- Reject changing or deleting it via alter collection properties.
Signed-off-by: sunby <sunbingyi1992@gmail.com>
PR #50178 added an unconditional collection-description length check in
proxy `alterCollectionTask.PreExecute`. The check runs on every incoming
`collection.description` value, including values that are not actually
changing.
### Bug
A collection created before the limit existed (e.g. a 2 KB description)
gets rejected when a client resends that unchanged description alongside
a genuine change (e.g. a TTL update). Proxy fails it with
`ErrParameterInvalid`, even though the description is not being
modified. This breaks the PR's own invariant: existing collection
metadata is not re-validated.
### Change
Validate the description only when its value actually differs from the
stored one. A pre-existing collection can resend its (possibly
oversized) description unchanged while altering other properties; that
unchanged value is now skipped. A new or genuinely changed oversized
description still returns `ErrParameterInvalid`.
This keeps the check in `alterCollectionTask.PreExecute`, the same layer
as every other AlterCollection property validator (TTL, mmap,
consistency level), so the fix stays minimal and consistent with the
surrounding code.
## Tests
`internal/proxy/task_test.go` —
`TestAlterCollectionTaskValidateDescription` rewritten as table cases:
- resending an unchanged oversized description while altering TTL is
accepted (the bug);
- changing a valid description to an oversized one is rejected;
- changing an oversized description to a different oversized value is
rejected;
- shrinking an oversized description to a valid value is accepted;
- with duplicate description keys, a changed oversized value is still
rejected per-property.
related: #50173
Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
issue: #50229
## What this PR does
Adds configurable request timeouts for external function model providers
(embedding / rerank / highlight), and standardizes the timeout unit from
**seconds → milliseconds** across all providers and clients.
### Changes
- New global config `function.model.timeout_ms` (default `30000`,
refreshable, exported to `milvus.yaml`).
- New per-function param `timeout_ms` which **overrides** the global
setting (`models.ResolveTimeoutMs`). Invalid / non-positive values fall
back to the `30000` default.
- Converted every provider/client timeout argument from `timeoutSec` to
`timeoutMs`; `PostRequest` now applies the value as milliseconds.
- Zilliz gRPC client now applies a per-request deadline via
`requestContext` (previously had no request timeout).
- Bedrock embedding now derives its call context from the request `ctx`
with the configured timeout (was `context.Background()`).
## Tests
- `models`: `TestParseTimeoutMs` (defaults, override, invalid,
zero/negative, case-insensitive key), `TestResolveTimeoutMs` (global
default + per-function override), and `TestZillizClient_RequestContext`
(per-request deadline + metadata).
- `rerank`: provider param validation for invalid / non-positive
`timeout_ms`.
- `paramtable`: `TestFunctionConfig` asserts the new default.
- Updated zilliz client / rerank / embedding / highlight tests for the
ms unit and the new test constructor.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
- Persist role descriptions when creating roles, expose them in role
list/describe results, and support updating descriptions through the
AlterRole path without changing role names.
- Store role descriptions in the role value body while keeping role
names in keys, and tolerate legacy empty role values plus undecodable
stored values by returning an empty description for that role row.
- Reject built-in/default roles and over-limit descriptions before WAL
writes, and share the role-description length validator between proxy
and rootcoord.
related: #50183
---------
Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
issue: #50341
## Summary
`DumpMessages` (data salvage, added in #47599) validated that
`start_message_id`
is non-empty but then passed the client-controlled `id` bytes straight
into the
panicking `message.MustUnmarshalMessageID`. A syntactically invalid id
(e.g. a
non-base64-decodable string for a Pulsar WAL) panicked the proxy and
**crashed
the whole process** — the client only saw `UNAVAILABLE: Socket closed`.
This uses the non-panicking `message.UnmarshalMessageID` and rejects
malformed
input with an `InvalidParameter` error instead, keeping the server up.
## Changes
- `internal/proxy/impl.go`: `DumpMessages` now unmarshals
`start_message_id` via
`UnmarshalMessageID` and returns `merr.WrapErrParameterInvalidMsg` on
failure.
- `tests/integration/replication/data_salvage_test.go`: add
`TestDumpMessagesWithMalformedStartMessageId`, asserting the RPC errors
and a
follow-up RPC still succeeds (server stayed up).
## Reproduction (before the fix)
```python
from pymilvus import MilvusClient
client = MilvusClient(uri="http://localhost:19530")
for _ in client.dump_messages(
pchannel="by-dev-rootcoord-dml_0",
start_message_id={"id": "not-a-valid-base64-msgid!!!", "wal_name": "Pulsar"},
):
break
# -> server panics in MustUnmarshalMessageID (message_id.go:45) and exits
```
```
panic: unmarshal message id failed: decode pulsar fail when decode base64 ...
message.MustUnmarshalMessageID(...) pkg/streaming/util/message/message_id.go:45
(*Proxy).DumpMessages(...) internal/proxy/impl.go
```
## Test Plan
- [ ] `TestDumpMessagesWithMalformedStartMessageId` passes (integration)
- [ ] Existing `DataSalvageSuite` tests pass
---------
Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
Signed-off-by: Yihao Dai <yihao.dai@zilliz.com>
- Wire credential descriptions through create/update/read paths,
including proxy, HTTP v2, Go SDK options, length validation, optional
update presence handling, and internal credential proto/model
persistence.
- Preserve existing encrypted passwords and descriptions during
RootCoord credential updates when either field is omitted, reject
all-empty or partial direct password updates, and keep sha256 passwords
cache-only.
- Skip auth-cache refresh for description-only credential updates so
existing authentication state is not blanked, while still refreshing
caches for password updates.
- Return persisted user descriptions from select_user and HTTP v2
describe_user, including include_role_info=false, and reuse
already-loaded credential rows in user listing and RBAC backup to avoid
duplicate etcd reads while ignoring malformed credential keys.
- Use the upstream milvus-proto go-api/v3 version that includes the
credential description proto change; the temporary fork replaces in
root, pkg, client, and tests/go_client modules have been removed.
design doc:
docs/design-docs/design_docs/20260601-rbac-user-description.md
related: #50179
---------
Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
issue: #47420
## What this PR does
Project-wide migration of raw `fmt.Errorf` / `errors.New` in function
bodies onto
the `merr` framework, plus the Sys-vs-Input error classification and the
machinery it drives (retriability, fine-grained metrics, segcore
unification),
plus the convention docs and a linter that keeps it from regressing.
Scope: storage, proxy, coordinators (root/data/query), query node, data
node,
`pkg/util` & `internal/util`, expression parser, message queue,
streaming, and
misc packages. Bare raw-error usages went from ~3000 to a ~340 allowlist
(package-level sentinels / build-tag / test sites).
---
## How to review this PR
It is large but the vast majority is mechanical. Changes fall into three
tiers;
spend review budget on Part 2 and Part 3.
### Part 1 — Mechanical standardization (low risk, verify by rule)
Each converted call follows one of a small fixed set of rules. To
review, check
that each site obeys the matching rule rather than reading every line:
| Pattern | Rule |
|---|---|
| `fmt.Errorf("...")` originating a new error | →
`merr.WrapErrXxxMsg("...")` with a code matching the failure's meaning |
| Adding context to an existing typed error | → `merr.Wrap(err, "...")`
/ `merr.Wrapf(...)` — **preserves** the inner code (never `WrapErr*Err`,
which overwrites it) |
| Errors inside the streaming subsystem | → `status.New*` factories
(StreamingError), **not** merr — this is the component-internal dialect
(see `docs/dev/error_handling_guide.md`) |
| Low-level / control-flow signal caught by `errors.Is` | → kept as a
package-level `errors.New` sentinel (lowercase, same-package) |
Conventions are documented in `docs/dev/error_handling_guide.md`
(how-to) and
`docs/dev/error_sentinel_convention.md` (rules + audit). A
`gocritic`/`ruleguard`
rule (`rawmerrerror`, in `rules.go`) enforces "no raw `return
errors.New/fmt.Errorf`"
under `make verifiers`.
### Part 2 — Behavior changes (review these closely)
These are the sites where the wire contract or runtime behavior changes,
not just
the source text. Listed by category; representative locations given,
full set in
the diff.
**A. gRPC wire-code shifts: `UnexpectedError(1)/Code 65535` → typed
code.**
Where a handler previously returned a raw error (collapsed to
`Code=65535` on the
wire), it now returns a typed merr, so the client sees a real code. The
most
common shift is to `IllegalArgument(5)/Code 1100` (ParameterInvalid).
Touch
points include datanode task handlers (CreateTask/Query/Drop), proxy
Upsert,
querynode GetMetrics, datacoord CreateIndex, httpserver query-response
builder,
and typeutil schema validation. One code refinement: an index-param
validation
moved `1100` → `1101` (ParameterMissing). **Client/SDK assertions and
any code
that switched on `Code=65535` for these paths must be re-checked** (the
go_client
e2e assertions were already aligned in this PR).
**B. Prometheus `status` label contract change (externally visible).**
The proxy metric's coarse `fail` / `rejected` values are split into
`fail_input` / `fail_system` and `rejected_user` / `rejected_system` (in
`requestutil.ParseMetricLabel`; auth/privilege rejections count as
`rejected_user`), so dashboards can attribute a failure to caller vs
operator.
**Dashboards/alerts querying `status="fail"` must migrate to
`status=~"fail_.*"`, and `status="rejected"` to
`status=~"rejected_.*"`.** The
in-repo Grafana dashboard is already migrated; external dashboards built
on the
old values silently go empty after upgrade. This is the one change that
requires an ops-side migration.
**C. Retriability semantics.**
- C1: `merr.Status(err)` now forces `Retriable=false` when the error is
an
`InputError` — a malformed request can never succeed on blind retry, so
clients
never get the self-contradictory "your input is wrong but you may
retry".
- C2: `retry.Do` short-circuits an `InputError` (non-retriable) — **but
only when
the caller did not pass a `RetryErr` predicate**. The check is an
`if c.isRetryErr != nil { ... } else if InputError { ... }` *mutually
exclusive*
branch (`pkg/util/retry/retry.go`): an explicit `RetryErr` takes
precedence and
bypasses the InputError abort. `retry.Handle` deliberately does **not**
apply
the InputError abort (its callers signal abort via `shouldRetry=false`).
Four
flusher startup callsites that must retry through transient "not ready"
errors
were given explicit `RetryErr` escape hatches.
**D. segcore (C++→Go) error classification.**
A single shared Go-side table (`pkg/util/merr/segcore.go`) maps each
segcore code
to a merr sentinel + InputError/signal category, replacing scattered
hand-written
`if errorCode == ...` switches in the cgo wrappers. **Wire `Code` values
change
for every segcore pass-through error, not just the remapped ones.**
Named
sentinels remap (C++ `2003` → merr `2001`, `2033` → `2002`,
Folly/Knowhere codes
likewise); **all remaining pass-through codes (`2004`–`2043`, previously
surfaced to clients as raw C++ enum values) now serialize as `2000`**
(`ErrSegcore`), with the original C++ code preserved in the `Reason`
text
(`segcoreCode=...`); unknown/future codes collapse to `2000` as well
(pinned by
the `wire_code_projection` test). Transient segcore classes (object
storage /
file IO / OOM / mmap / FieldNotLoaded — 11 codes) now report
`Retriable=true`.
**Any client switching on raw segcore codes in the `2004`–`2043` range
must be
re-checked**; the in-Reason code remains available for diagnostics.
Signal
codes (PretendFinished / FollyCancel) are recognized centrally.
`errors.Is`-based
control flow on these (e.g. scheduler skip/retry) is preserved.
**E. InputError classification (25 sentinels + dynamic marks).**
25 sentinels in `errors.go` carry `WithErrorType(InputError)` (the
Collection /
ResourceGroup / Database families, `ErrIndexDuplicate`,
`ErrParameterInvalid`,
`ErrPrivilegeNotAuthenticated`, `ErrImportFailed`, `ErrQueryPlan`, ...),
plus dynamic
marks for the 8 segcore input codes (ExprInvalid, DimNotMatch,
MetricTypeInvalid, FieldIDInvalid, ...) and
`WrapErrAsInputError`. The widest blast radius is `ErrParameterInvalid`
(1100):
~2335 `WrapErrParameterInvalid*` callsites now classify as input /
non-retriable. Because of C1/C2 this changes retriability for
any path that returns these. **The audit to confirm no transient path
was
mis-marked is the single most important review item** (see Part 3). One
reverse
correction: storage field-stats parsing moved from `ErrParameterInvalid`
(input)
to `ErrDataIntegrity` — a corrupted stored stat is data corruption, not
user
input.
### Part 3 — Known risks & traps (called out proactively)
1. **`merr.Wrap` vs `WrapErr*Err` (code-masking).** `WrapErr*Err` builds
a
`wrappedMilvusError{sentinel: ErrServiceInternal}` whose `code()`
returns the
*outer* sentinel — it overwrites the inner typed code and hides the
`errors.Is`
chain. This is intentional (use it to *deliberately* downgrade), but it
was a
recurring conversion defect; the rule "add context with `merr.Wrap`,
downgrade
with `WrapErr*Err`" is enforced by convention and reviewed across the
diff.
2. **InputError × `retry.Do` blast radius.** Marking a sentinel
`InputError` makes
any `retry.Do(...)` without a `RetryErr` predicate stop retrying it.
Reviewers
should sanity-check that no transient use of the 19 newly-marked
sentinels
(especially `ErrParameterInvalid`) sits inside a retry loop that needed
to keep
spinning. The known flusher cases were handled (see C2).
3. **The ~340 raw-error allowlist.** What remains as bare `errors.New`
is, by
design: package-level sentinels (caught by `errors.Is`), `//go:build
test`
sites, and out-of-band trees (`cmd/`, `tests/`, codegen, walimpls). The
linter
only bans the *direct-return* form; assignment-then-return escapes and
the full
no-exceptions ban are deferred to an AST-based linter (Tier 2,
documented).
4. **segcore C++ second step deferred.** This PR unifies classification
on the Go
side; splitting the dual-semantic C++ codes at the source is a
follow-up.
---
## Validation
- `make verifiers`: Go side clean (gofmt + static-check across modules,
including
the new `rawmerrerror` rule with a 0-hit baseline repo-wide).
- `make test-go`: passing; the one real regression introduced (a
datanode
`invalid_task_type` assertion shifting `1` → `5` from a ParameterInvalid
conversion) was fixed in-tree.
- go_client e2e CreateIndex assertions aligned to the new merr messages.
---------
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add a refreshable proxy.maxCollectionDescriptionLength setting with a
default 1024-byte limit and document it in milvus.yaml.
- Validate collection descriptions during CreateCollection and
AlterCollection PreExecute, including duplicate alter properties, while
leaving existing metadata loading unchanged.
- Document that restore or replication flows that recreate collections
through CreateCollection must satisfy the configured limit or raise it
first.
- Cover byte-length boundaries, CJK byte overflow, create rejection,
alter rejection, refreshability, duplicate alter-property rejection, and
generated YAML consistency in tests.
related: #50173
---------
Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
issue: #50381
Search results can contain non-empty FieldsData even when there are no
matched rows, because FieldsData may only hold empty output-field
templates.
Use result IDs to detect zero-hit search results before running lexical
or
semantic highlight processing.
Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com>
