## 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: #51376
## What this fixes
The in-place partial-materialization path of
`bump_schema_version_compaction` commits onto a live segment's manifest,
and DataCoord adopted the result on a version-newer check only — it
never verified the result was built on the segment's *current* manifest.
A bump built on a plan-pinned base could therefore be adopted after a
concurrent stats/index task advanced the manifest pointer, silently
dropping that index (correct version number, wrong content lineage). The
loss is permanent and non-self-healing, because the segment meta stats
placeholder stops the stats inspector from re-triggering.
## Fix — application-layer adoption CAS (mirrors the stats path)
- The schema-bump result carries the plan-pinned base manifest
(`CompactionSegment.base_manifest`, like `StatsResult.base_manifest`);
the DataNode fills it with the manifest it materialized on.
- DataCoord adopts the in-place result only if `result.base_manifest ==
segment.ManifestPath` (the current pointer); a missing or stale base is
rejected (`ErrIllegalCompactionPlan`) so the task re-triggers and
rebuilds on the current manifest instead of overwriting the concurrent
commit. This mirrors the stats path's `errStatsResultStale`.
- The storage commit stays on `OverwriteResolver` (unchanged). Because
the CAS is at the adoption layer (against the meta pointer, not
storage-latest), a result lost after a DataNode crash self-heals on
retry: the pointer stays put, so the retry re-pins the same base and is
adopted — no wedge.
- The bump policy is restricted to internal collections (matching every
sibling compaction policy), so a rejected attempt's orphan is reclaimed
by internal dropped-segment GC and bump never churns on external
collections.
## Verification
Both interleavings converge with no data loss: stats-first → bump's base
no longer matches the advanced pointer → reject + retrigger on the
current manifest; bump-first → stats' existing base-check rejects and
retries. Added meta-level adoption tests (matching/stale base) and
updated the in-place adoption tests to carry the base.
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: #51275
### Problem
Under StorageV3 growing-source flush, a manifest legally keeps column
groups whose fields were later dropped: drop does not rewrite flushed
data, and compaction removes the leftover lazily. Loaders however
projected such groups against the **current** schema and treated the
mismatch as an error:
- **Growing recovery** (`SegmentGrowingImpl::LoadColumnsGroups`)
submitted every manifest group with a full-schema projection; a group
whose fields were all dropped has an empty projection, which
milvus-storage rejects by contract (`ChunkReaderImpl::open`: "No needed
columns found in column group"). The channel-recovery level retried the
deterministic failure every second — observed locally: **4140 identical
failures over ~3 minutes**, the channel unavailable (`no available shard
leaders`) the whole time, until a compaction rewrite happened to remove
the group.
- **Sealed load** (`ChunkedSegmentSealedImpl::LoadColumnGroups`)
resolved every manifest column to a field id without a membership check
and tripped the schema-lookup assert on any group still carrying a
dropped column — mixed groups included.
### Fix
Treat a dropped-field column as a legal leftover at the loaders:
- Growing: skip a column group before submitting its load task when none
of its columns exist in the segment schema (logged).
- Sealed: filter resolved field ids absent from the schema snapshot
while building `cg_field_ids` (logged); a group filtered to empty
produces no load task. Mixed groups keep loading their live columns via
projection, unchanged.
### Verification
- Local A/B with a schema-evolution chaos workload (concurrent add/drop
ordinary + BM25 function field, DML/query/search, `useLoonFFI` +
growing-source flush enabled, StreamingNode SIGKILL/restart injections):
before the fix, recovery wedged in the retry loop (4140 errors / ~3 min
/ final query 503); after the fix the same scenario logs 2 group skips,
the channel recovers in seconds, and the full serial consistency
validation passes.
- New UT `LoadGrowingSegmentSkipsDroppedFieldColumnGroup`: flushes a
manifest with a field-exclusive column group (writer pattern
`"0|1|100,101"`), reloads the segment under a schema without that field,
and asserts the load succeeds with the group skipped (plus a guard
assertion that the manifest really contains the exclusive group).
- The sealed-path filter is exercised by code inspection and mirrors the
growing-side semantics; on current master the external-table load path
derives its projection from schema-owned column names and is not exposed
to this class.
🤖 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>
issue: #51117
Supersedes #51207 per team decision to ship the skipped-field tolerance
for 3.0.0; the era-consistent replay consume path (#51146) remains a
follow-up.
### Problem
A growing segment created while a field was absent from the schema never
materializes that column: the InsertRecord ctor only allocates fields
present at creation, Reopen fills later-added ordinary fields but skips
function outputs by design, and a field dropped before segment creation
is skipped at consume. Flushing such a segment with a schema snapshot
that still carries the field hit `Assert "data_.find(field_id) !=
data_.end()"` in the growing-source flush path and crash-looped the
StreamingNode (exit 134 / CrashLoopBackOff in the schema-evolution L3
runs).
### Design
Principle: the flush writes exactly `flushSchema ∩ materialized`. A
non-materialized column is legally absent — a field dropped from the
segment's own schema, or a function output recomputed by bump-schema
compaction backfill. Real schema/data inconsistency is segcore's own
concern and is verified inside the flush.
- **Go (layout source of truth)**: trim the column groups to the source
segment's materialized field ids (plus system fields) right after layout
derivation — `Fields` and the parallel `Columns` array in lockstep — so
the writer pattern, binlog meta and metacache current split all describe
the same layout. An empty materialized report is refused. On a
committed-flush ack retry the layout is trimmed to the committed binlogs
(`ChildFields` union), the persisted truth.
- **C++ (segment-internal judgement)**: a column missing from the insert
record (`HasFieldData` semantics: allocated-but-empty counts as missing)
is skipped when the field is gone from the segment's own schema
(dropped) or is a function output; a regular field still present in the
segment schema is materialized by ctor/Reopen by construction, so that
case hard-errors instead of asserting.
- Skipped columns are backfilled by bump-schema compaction via
`RecordMaterializer`.
### Review responses (@liliu-z)
- stale `Columns` / phantom column in writer pattern →
`filterColumnGroupFields` trims `Fields`+`Columns` in lockstep at all
three trim sites
- committed ack-retry currentSplit divergence → layout trimmed to
committed binlogs' `ChildFields` union
- allocated-but-empty function-output column wedging the flush →
materialized report and the flush skip both gate on `HasFieldData`
semantics (empty = not materialized)
- silent skip of a missing regular field → three-way judgement: dropped
from segment schema → skip; function output → skip;
present-in-segment-schema regular field → hard error (`has no field data
in growing segment ... but is present in the segment schema`)
- retryability routing of the missing-field error: the legally-absent
cases no longer produce an error at all; the remaining hard error
indicates real data loss and is construction-unreachable via public APIs
(the consume path asserts regular fields carry data)
### Verification
- Go UT (`growing_source_test.go`): trim semantics — dropped ordinary
field trimmed, `Columns` kept in lockstep, empty materialized report
refused, committed-retry trim by `ChildFields`
- C++ UT (`test_storage.cpp`): flush skips a non-materialized function
output; skips a dropped field carried by a staler flush schema; skips an
allocated-but-empty function-output column
- Local schema-evolution chaos runs (concurrent add/drop field + BM25
function field + DML/query/search with `useLoonFFI` + growing-source
flush enabled, incl. StreamingNode force-kill/restart injections): no
panic, no crash loop, final serial consistency validation passes. The
exact original assert requires an era-mismatch replay not reproduced
locally; that path is covered by the unit tests above.
🤖 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>
## Summary
After schema evolution adds an `enable_match` field (e.g.
`drop_collection_field` + `add_collection_field` of a same-name text
field), `text_match` could fail with `text index not found for field
<id>` on a growing segment even though `describe_index` reported
`Finished`.
## Root cause
A growing segment builds its per-segment text indexes exactly once, in
the constructor (`CreateTextIndexes`), against the construction-time
schema — `text_indexes_` has a single writer, reachable only from that
path. When a query carrying the new schema drives `LazyCheckSchema ->
Reopen(SchemaPtr)`, Reopen only backfilled empty column data
(`fill_empty_field`) and never built the text index for the newly added
field — unlike the sealed path, where both load and schema-only reopen
go through `ComputeDiff -> text_indexes_to_create -> CreateTextIndex`.
So an old growing segment upgraded by Reopen had no
`text_indexes_[new_field]`, and `text_match` hit `GetTextIndex()` and
threw `TextIndexNotFound`. The window closes once the segment is
flushed/sealed, loaded (sealed load rebuilds the index) and released
from the delegator, which is why the failure is intermittent and
self-healing but hard, non-retryable while it lasts.
## Fix
In `SegmentGrowingImpl::Reopen(SchemaPtr)`, for newly added
`IsStringDataType && enable_match` fields, **before publishing
`schema_`** (LazyCheckSchema readers don't take `sch_mutex_`, so a
concurrent query observing the new schema skips Reopen and queries
immediately — the index must already exist by then):
1. Build the text index via the new `BuildTextIndexForMeta(const
FieldMeta&)` (the field is not yet in the published `schema_`, so the
meta is passed in; mirrors sealed's `CreateTextIndexWithSchema`). The
index is **staged as a local object** — it is not visible in
`text_indexes_` yet.
2. **Index the pre-existing rows with the same default-value-or-nulls
fill the column received** (constructed directly — the content is
knowable without reading the column back, since the exclusive
`sch_mutex_` blocks concurrent inserts). This keeps the index consistent
with the column, matching sealed's create-from-raw behavior
(`BulkRawStringAt -> AddTextSealed`): when the added field carries a
`default_value`, old rows must match text queries against the default
text and report not-null. Backfilling every row (nulls included) also
keeps text-index doc offsets dense and aligned with `insert_record_`
rows, which query-side bitmap sizing relies on.
3. Force `Commit()+Reload()` on each staged index; after **every** new
field succeeds, publish all staged indexes into `text_indexes_` together
(an O(1) map insert per field under one `mutex_` acquisition).
Publish-after-success makes the failure path sound across fields as well
as within one: a present `text_indexes_` entry is always a fully
backfilled, committed index; a throw anywhere during any field's
build/backfill/commit publishes nothing and leaves `schema_`
un-advanced, so the next query rebuilds everything from scratch (no
per-field partial publication that a retry's presence guard would skip
over). It also shrinks the `mutex_` hold during Reopen from the O(N)
backfill to the final map insert, so concurrent `GetTextIndex` readers
(text_match on existing fields) are not stalled by the backfill.
Function-output fields (e.g. BM25 sparse output) remain skipped: they
are non-string and never carry a text index. The related-but-distinct
`field index meta not found` symptom for BM25 function-output fields
(follow-up of #50783: `index_meta_` is not updated on growing reopen) is
a separate fix and not covered here.
## Test
`test_schema_reopen.cpp`:
- `ReopenBuildsTextIndexForNewEnableMatchField`: growing segment on
schema v1, old rows inserted, Reopen to v2 adding a nullable
`enable_match` VARCHAR (no default) — `GetTextIndex` throws before
Reopen; after Reopen it returns the index, `MatchQuery` finds nothing
and `IsNotNull` reports all rows null, consistent with the column.
- `ReopenTextIndexIndexesDefaultValueForOldRows`: same flow with a
`default_value` on the added field — after Reopen
`MatchQuery("default")` matches all N pre-existing rows and `IsNotNull`
reports them non-null, matching sealed semantics.
- `ReopenBuildsTextIndexesForMultipleNewFields`: one Reopen adds two
enable_match fields (one nullable-null, one with default) — both indexes
come out complete through the batched staged publish.
All tests query immediately after Reopen with no explicit
`Commit()/Reload()`, asserting the forced-visibility behavior.
## Known limitation (pre-existing, follow-up)
`EnsureArrayOffsetsForStructField` still runs after `schema_` is
published, so a throw there can leave a schema-advanced segment without
array offsets — pre-existing behavior, unchanged by this PR; reworking
Reopen's publication order for non-text state is left as a follow-up.
(The text-index half of this concern is resolved: the index is now built
before publication and a build failure leaves the reopen retryable.)
issue: #50484🤖 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: #51170
Expose the existing server-side query ORDER BY capability (query param
key `order_by_fields`, already consumed by proxy `task_query.go`)
through the two remaining client surfaces:
- **Go SDK**: `queryOption.WithOrderByFields(fields ...string)` — each
spec is `"field"`, `"field:asc"` or `"field:desc"`, joined into the
`order_by_fields` query param.
- **RESTful v2**: `QueryReqV2` gains `orderByFields` (`[]string`, same
spec format), forwarded by the query handler as the `order_by_fields`
query param.
Validation (sortable type, explicit-limit requirement, iterator
exclusion) stays server-side in the proxy, consistent with how
limit/offset/group-by are handled at these layers. Interface shape
mirrors pymilvus (`order_by=["price:desc"]`).
Tests: Go SDK option unit test, RESTful v2 handler unit test (asserts
the KV pair is forwarded, and absent when not requested), and a
go_client e2e case (desc / asc-default / no-limit rejection).
Follow-up (out of scope here): search-side `order_by` first-class parity
for RESTful v2 / Go SDK (currently reachable via raw search params
only).
🤖 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>
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>
## What
Filter out insert payload columns whose field is absent from the current
collection schema when converting an insert message on the QueryNode
consumption path (`TransferInsertMsgToInsertRecord`), instead of
forwarding them into segcore where `SegmentGrowingImpl::Insert`
hard-asserts on unknown fields and panics the whole StreamingNode.
## Why (root cause, verified from Loki logs of the failing run)
1. A growing segment was created under schema v104 and received one
insert whose payload — written at WAL-append time under v104 — contains
field 158 (a field generation that was later dropped; by v112 it no
longer exists in the schema).
2. The StreamingNode died once for an unrelated reason. On every
subsequent restart, `WatchDmChannels` seeds the collection with the
**latest** schema (v112, without field 158 — coord meta is updated right
after the DDL's WAL append ack, so it always runs ahead of any replaying
consumer), and WAL replay starts from the channel checkpoint, which
predates the segment's data.
3. The replayed v104 insert still carries field 158; the growing segment
rebuilt with v112 has no such column and can never gain it. Replayed
schema events (v1→v103) are correctly skipped by the schema-version gate
(#50577), so the mismatch is structural, not a race.
4. `SegmentGrowingImpl::Insert` tolerates only the inverse staleness
(segment schema newer than payload → fill empty columns). An unknown
payload field hits `AssertInfo(insert_record_.is_data_exist(field_id),
...)` → SegcoreError → `delegator.ProcessInsert` panics → the checkpoint
never advances past the message → deterministic CrashLoopBackOff.
Dropped-field data is unqueryable by definition, so skipping those
columns is semantically lossless. On the live path the WAL-append
schema-version gate plus segment fencing on schema change guarantee the
payload is a subset of the current schema, making this filter a no-op
outside replay.
A WARN log records the skipped field IDs for observability.
## Notes
- Related latent issues intentionally out of scope (to be filed
separately): compaction can leave a flushed segment out of the watch
request's flushed/dropped exclusion lists (duplicate-data risk on
replay); `Reopen`/`FillAbsentFields` never create function-output
columns on pre-existing growing segments.
issue: #51117
---------
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>
## Summary
A sealed segment that predates a function-output field added by
`add_function_field`
lazy-loads that field's raw column (so `HasFieldData` becomes true) but
has **no index
meta** for it on this segment. A BM25 brute-force search then reached
`GetFieldIndexMeta(field_id)`, which dereferenced an `end()` iterator —
the guarding
`assert` is stripped in release builds — and the QueryNode died with
`SIGSEGV
SI_ADDR=(nil)` while copying the garbage index-params map.
## Fix
- `FieldAccessible` now requires `HasIndex || (HasFieldData &&
HasFieldIndexMeta)`.
`HasIndex` is checked **first** so the field becomes serviceable again
the moment its
index loads — recovery does not depend on `col_index_meta_` ever being
refreshed
(it is set once at construction and never updated on a stale segment).
- New `virtual HasFieldIndexMeta` (default `true`;
`ChunkedSegmentSealedImpl` /
`SegmentGrowingImpl` override via `col_index_meta_` / `index_meta_ ->
HasField`).
- Defense-in-depth: `CollectionIndexMeta::GetFieldIndexMeta` `assert` ->
`AssertInfo`,
so a missing field throws a `SegcoreError` even under NDEBUG instead of
UB.
With the gate in place the existing empty-result branch in
`segment_c.cpp` AsyncSearch
serves a graceful partial result (the not-yet-indexed old segment
contributes nothing)
instead of crashing.
issue: #50783
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 <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: #50729
### What
Dropping a BM25 function field and re-adding it with the same function
name and output field left the new sparse index stuck `InProgress`. The
flow was blocked on two layers, fixed here:
- **DataCoord**: accept expanded pre-allocated segment ID ranges when
completing the schema-bump replacement compaction (previously only a
single-ID range was allowed, causing `BumpSchemaVersionCompaction failed
... compaction plan illegal`). Still require the replacement segment to
use the range begin, matching DataNode full-rewrite behavior.
- **QueryNode**: allow a BM25 function field to be dropped and re-added
with a new output field ID (reject only incompatible in-place changes to
a shared output field), and prune dropped BM25 stats from
current/growing/sealed state and local disk so reclaimed disk size stays
accurate.
### Tests
- Cover expanded-range completion in the schema-bump metadata regression
test.
- Cover drop/re-add validation and dropped-stats pruning
(current/growing/sealed + on-disk) in the delegator and idf_oracle
tests.
🤖 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: #48808
## Summary
- sync delegator function/analyzer runtime state on schema update
- allow additive BM25 function fields in the IDF oracle without dropping
existing stats
- route Reopen BM25 stats through the delegator oracle with field-level
idempotency
🤖 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 (1M context) <noreply@anthropic.com>
issue: #49841
issue: #49842
## Summary
- Reject search iterator when search_aggregation is enabled instead of
returning an empty page.
- Reject JSON and dynamic fields across search_aggregation context
building for group_by, metric sources, and top_hits sort fields.
- Add regression coverage for unsupported iterator and JSON/dynamic
aggregation inputs.
🤖 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: #49869
## Summary
- Align search ORDER BY null placement with query/PostgreSQL defaults:
ASC NULLS LAST and DESC NULLS FIRST.
- Support explicit `nulls_first` / `nulls_last` options in search
`order_by_fields`.
- Cover scalar, JSON path, and dynamic-field null ordering behavior in
proxy tests.
🤖 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>
## Summary
- Add per-level `search_size` handling for search aggregation context
building.
- Preserve `size` as the response bucket limit while using `search_size`
for downstream candidate bucket budget derivation.
- Bump the root module milvus-proto dependency to the version that
contains `SearchAggregationSpec.search_size`.
## Test Plan
- [x] `git diff --check trunk/master...HEAD`
- [x] `go test -tags dynamic,test -gcflags=\"all=-N -l\" -count=1
./internal/proxy/search_agg/...`
related: #48971🤖 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 (1M context) <noreply@anthropic.com>
issue: #49046
## What this PR does
Marks `test_search_aggregation_nullable_metric_field` as non-strict
xfail because the case is unstable when `top_hits` does not include NULL
nullable metric rows.
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>
## Summary
- Remove proxy-side schema version consistency gate for schema-change
DDLs.
- Allow mix and clustering compaction to process mixed schema-version
segments.
issue: #49475
## Test plan
- [x] git diff --check
- [ ] go test -tags dynamic,test -gcflags=\"all=-N -l\" -count=1
./internal/proxy -run
'TestProxy_AddCollectionField_DoesNotBlockOnSchemaVersion|TestProxy_AlterCollectionSchema'
- [ ] go test -tags dynamic,test -gcflags=\"all=-N -l\" -count=1
./internal/datacoord -run
'TestCompactionTriggerKeepsMixedSchemaVersionSegments|TestClusteringCompactionPolicySuite/TestTriggerOneCollectionAllowsMixedSchemaVersionGroup|TestMetaBasicSuite/TestCompleteCompactionMutation/(mixed
schema version mix compaction uses task schema version|mixed schema
version clustering compaction uses task schema version)'
Note: Go test commands were blocked locally because pkg-config could not
find milvus_core / milvus-storage.
🤖 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 (1M context) <noreply@anthropic.com>
## Summary
- add `search_agg` package in proxy for context build, recursive
aggregation compute, and bucket ordering
- wire `group_by` aggregation through `task_search` and built-in
pipeline (`searchWithAggPipe` + aggregate operator serialization)
- upgrade querynode reduce key extraction for agg multi-field grouping,
and update related proxy/querynode tests
- keep local proto iteration path and local core build job defaults
aligned for this development workflow
## Test plan
- [x] bash
/home/zilliz/hc-claude-projects/hc-milvus-projects/search-aggregation/scripts/verify_baseline.sh
- [x] direct association tests passed
- [x] indirect association tests passed
issue: #49046🤖 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 Sonnet 4.6 (1M context) <noreply@anthropic.com>
## Summary
- Add schema version check on insert path in streaming node shard
interceptor, rejecting inserts with mismatched schema version
- Add
`AlterCollection`/`AppendNewCollectionSchema`/`CheckIfCollectionSchemaVersionMatch`
methods to shard manager for schema lifecycle management
- Propagate schema version through segment allocation chain:
`shard_manager_segment` → `partition_manager` → `segment_alloc_worker` →
`CreateSegment` message header
- Add `SchemaVersionMismatch` streaming error type (classified as
unrecoverable to force proxy metadata refresh)
- Track collection schema in `CollectionInfo` with
`CollectionSchemaOfVChannel` for version gating
related: https://github.com/milvus-io/milvus/issues/48808
design doc:
https://github.com/milvus-io/milvus-design-docs/blob/main/design_docs/20260129-add-function-field-design.md
## Test plan
- [x] partition_manager_test.go updated for new `schemaVersion`
parameter
- [x] wal_test.go updated with CollectionSchema in CreateCollection
messages
- [ ] CI: code-check, ut-go, integration-test
🤖 Generated with [Claude Code](https://claude.ai/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.6 <noreply@anthropic.com>
## Summary
Refactor search-side GROUP BY from single-field to multi-field composite
key in the **segcore (C++) layer**, aligning with Elasticsearch's
`multi_terms` aggregation semantics.
**Scope: segcore layer only.** Go-layer changes (proxy search reduce,
delegator multi-field wiring, SDK API) will be tracked and submitted in
a separate PR.
## Core Changes
- **CompositeGroupKey** (Types.h): New composite key type
(vector<GroupByValueType>) with CompositeGroupKeyHash using
bits::hashMix, consistent with query-agg HashTable.h
- **MultiFieldDataGetter** (SearchGroupByOperator.{h,cpp}): Type-erased
multi-field data reader via std::function<GroupByValueType(int64_t)>,
replacing ~300 lines of per-type template dispatch
- **GroupByMap<CompositeGroupKey>** specialization for composite key
grouping with hash collision handling
- **SearchInfo.group_by_field_ids_** (QueryInfo.h): Replaced
optional<FieldId> group_by_field_id_ with vector<FieldId>
group_by_field_ids_; added has_group_by() / has_multi_field_group_by()
helpers
- **Proto**: New `repeated int64 group_by_field_ids = 17` in QueryInfo,
with backward compat for old group_by_field_id
- **Reduce pipeline**: GroupReduce, Reduce::SortEqualScoresOneNQ, and
AssembleCompositeGroupByValues all updated for composite keys
- **JSON group_by**: Restored proper json_type dispatch
(bool/int8/16/32/64/varchar) in MultiFieldDataGetter
- **Dead code cleanup**: Removed old AssembleGroupByValues() (~290
lines), removed write-only SearchResultPair.group_by_value_ field
## What This Enables (for follow-up Go-layer PR)
This commit provides the segcore foundation for the **Proxy-Only
architecture**:
1. Proxy amplifies group_count and group_size, flattens all nested
levels into a flat multi_terms composite key
2. Segcore / Delegator / QN execute standard flat group_by with
amplified parameters — zero awareness of nesting or metrics
3. Proxy reconstructs the nested hierarchy, computes metrics from
returned docs, and trims to user-requested group_size
The Go-layer PR will wire up: proto group_by_field_values (plural)
reading, proxy search reduce with composite keys, metric field injection
into output_fields, and nested reconstruction logic.
The full feature surface (per-group metrics R4, intra-bucket sort R5,
**bucket-level ordering / per-level-orderby R7**) is described in the
design doc below.
## Test plan
- [x] 5 existing single-field GroupBY.* tests adapted to composite key
path — all pass
- [x] 3 existing GroupBYJSON.* tests adapted — all pass
- [x] ElementFilter group_by interaction tests adapted — all pass
- [x] New GroupBY.CompositeKeySealedIndex — dual-field (int32+int64)
sealed+HNSW, strict_group_size=true, ground truth verification via
seg_offset -> raw_data lookup, exact group count and per-group size
assertions
- [x] New GroupBY.CompositeKeyGrowing — dual-field (int8+int32) growing
segment, exact 6 groups x group_size=3, every group fully filled
- [x] New GroupBY.CompositeKeyReduce — dual-field cross-segment C-API
reduce via ReduceSearchResultsAndFillData, PK dedup + composite
group_size validation
- [x] New GroupBY.SingleFieldViaCompositeProtoPath — single field via
new group_by_field_ids proto, backward compat with exact 7 groups x
group_size=3 = 20 results
- [x] **All 68 guarded tests passing**, clang-format clean
issue: #49046
design doc:
https://github.com/milvus-io/milvus-design-docs/blob/main/design_docs/20260413-search_embedded_agg.md
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two bugs in the Go-layer query reduce pipeline caused panics and wrong
ValidData output for nullable vector fields:
Bug 1: buildCompactIndices inferred nullable from ValidData presence.
When segcore returns empty ValidData + empty Contents (e.g. index not
ready via fill_with_empty), the compact mapping was not built, causing
getVecDataIdx to return rowIdx directly and Contents[rowIdx] to panic.
Bug 2: buildMergedFieldData set validData[i]=true when ValidData was
absent for a nullable field, incorrectly marking null rows as valid.
Fix: use schema (map[fieldID]bool) as the source of truth for
nullability. Empty ValidData for a nullable field now means "all rows
are null" rather than "non-nullable". All QN-side and proxy-side
operators (ReduceByPKTS, DeduplicatePK, ReduceByPK, ConcatAndCheckPK)
receive the nullable map built from schema at construction time.
Also add bounds checks to buildMergedScalarField for all scalar types to
guard against nullable fields with empty Data arrays.
related: #48700
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary
Remove the entire SuffixSnapshot time-travel layer from RootCoord
metadata storage, replacing it with plain KV operations.
Replace SnapShotKV with TxnKV in Catalog (~30 call sites)
Delete suffix_snapshot.go (~750 lines) and tests (~917 lines)
Remove SnapShotKV interface from pkg/kv/kv.go
Remove snapshot GC config params (ttl, reserveTime)
Keep IsTombstone/ComposeSnapshotKey constants for migration tool
compatibility
Why: Investigation confirmed snapshot keys are write-only artifacts that
nobody reads:
No client/proxy sends non-zero timestamp for time-travel metadata
queries
Tombstone sweeper calls RemoveCollection(ts=0), bypassing snapshot
tombstone writes entirely
Snapshot keys accumulate as orphans — the old GC couldn't fully clean
them because ts=0 deletion path never wrote tombstone markers
Net reduction: -2180 lines
related: #48697
## Test plan
- [x] Unit tests for GC batch logic (expiry, cursor progress,
latest-per-group protection, tombstone cleanup)
- [x] Unit tests for Save/Load with time-travel verification
- [x] `go vet` passes on all changed packages
- [ ] CI passes
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary
- Add `GetCollectionName()` method to 12 RESTful API request types that
were missing it
- Ensures `ProxyFunctionCall` metrics report the correct collection name
label for all RESTful endpoints
- Affected types: `RenameCollectionReq`, `QueryReqV2`,
`CollectionIDReq`, `CollectionFilterReq`, `CollectionDataReq`,
`SearchReqV2`, `HybridSearchReq`, `PartitionsReq`, `GrantV2Req`,
`IndexParamReq`, `CollectionReq`, `RunAnalyzerReq`
issue: #48461
## Test plan
- [ ] Verify existing unit tests pass
- [ ] Confirm RESTful API metrics now include collection name for
search/query/hybrid_search endpoints
🤖 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.6 (1M context) <noreply@anthropic.com>
## Summary
- HashTable now dynamically rehashes (doubles capacity) when load factor
exceeds 7/8, fixing crash when GROUP BY cardinality > ~1792
- Added configurable `queryNode.segcore.maxGroupByGroups` (default 100K)
to cap total groups and prevent OOM on both C++ (per-segment HashTable)
and Go (cross-segment agg reducer) layers
- Added 4 C++ unit tests covering rehash basic/correctness, max groups
limit, and multiple rehash rounds
issue: #47569
## Test plan
- [ ] C++ unit tests: `--gtest_filter="*HashTableRehash*:*MaxGroups*"`
- [ ] E2E: GROUP BY aggregation with >2K unique values should succeed
- [ ] E2E: Set `queryNode.segcore.maxGroupByGroups` to small value,
verify clear error message
🤖 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.6 <noreply@anthropic.com>
## Summary
- Add `QueryOrderByNode` operator with `SortBuffer` for heap-based
partial sort in segcore
- Add `FillOrderByResult` path in `SegmentInterface` that bypasses
`find_first` for ORDER BY queries
- Parse `order_by_fields` from query plan proto into segcore plan nodes
- Proto changes: add `OrderByField` message and `order_by_fields` field
in `QueryPlanNode`
issue: #41675
## Test plan
- [x] C++ unit test `test_sort_buffer.cpp` covers SortBuffer with
various data types, null handling, multi-key sort
- [ ] Full E2E coverage in the follow-up Go-layer PR
design doc:
https://github.com/milvus-io/milvus-design-docs/blob/main/design_docs/20260203-query-orderby.md
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary
Fixes#47316
- **GroupingSet::outputRowCount()**: Add null check for `lookup_` to
prevent SIGSEGV when filter matches zero rows and no input data arrives
- **ExecPlanNodeVisitor::setupRetrieveResult()**: When query result is
nullptr, build empty field_data arrays with correct schema (0 rows, N
columns) so downstream receives proper column structure instead of
missing columns
- Enable previously skipped E2E test `test_empty_result_aggregation`
## Test plan
- [x] E2E test `test_empty_result_aggregation` passes (GROUP BY +
aggregation with filter matching 0 rows returns empty result `[]`)
- [ ] Existing aggregation E2E tests still pass
- [ ] CI green
🤖 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.6 <noreply@anthropic.com>
related: #36380
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
- Core invariant: aggregation is centralized and schema-aware — all
aggregate functions are created via the exec Aggregate registry
(milvus::exec::Aggregate) and validated by ValidateAggFieldType, use a
single in-memory accumulator layout (Accumulator/RowContainer) and
grouping primitives (GroupingSet, HashTable, VectorHasher), ensuring
consistent typing, null semantics and offsets across planner → exec →
reducer conversion paths (toAggregateInfo, Aggregate::create,
GroupingSet, AggResult converters).
- Removed / simplified logic: removed ad‑hoc count/group-by and reducer
code (CountNode/PhyCountNode, GroupByNode/PhyGroupByNode, cntReducer and
its tests) and consolidated into a unified AggregationNode →
PhyAggregationNode + GroupingSet + HashTable execution path and
centralized reducers (MilvusAggReducer, InternalAggReducer,
SegcoreAggReducer). AVG now implemented compositionally (SUM + COUNT)
rather than a bespoke operator, eliminating duplicate implementations.
- Why this does NOT cause data loss or regressions: existing data-access
and serialization paths are preserved and explicitly validated —
bulk_subscript / bulk_script_field_data and FieldData creation are used
for output materialization; converters (InternalResult2AggResult ↔
AggResult2internalResult, SegcoreResults2AggResult ↔
AggResult2segcoreResult) enforce shape/type/row-count validation; proxy
and plan-level checks (MatchAggregationExpression,
translateOutputFields, ValidateAggFieldType, translateGroupByFieldIds)
reject unsupported inputs (ARRAY/JSON, unsupported datatypes) early.
Empty-result generation and explicit error returns guard against silent
corruption.
- New capability and scope: end-to-end GROUP BY and aggregation support
added across the stack — proto (plan.proto, RetrieveRequest fields
group_by_field_ids/aggregates), planner nodes (AggregationNode,
ProjectNode, SearchGroupByNode), exec operators (PhyAggregationNode,
PhyProjectNode) and aggregation core (Aggregate implementations:
Sum/Count/Min/Max, SimpleNumericAggregate, RowContainer, GroupingSet,
HashTable) plus proxy/querynode reducers and tests — enabling grouped
and global aggregation (sum, count, min, max, avg via sum+count) with
schema-aware validation and reduction.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
related: #46649
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
- Core invariant: STS IAM credential providers for Aliyun, Tencent
Cloud, and Huawei Cloud are global, stateless resources that must be
instantiated once and reused across all ChunkManager instances
(singleton), rather than created per-manager.
- Logic removed/simplified: Removed per-instance Aws::MakeShared
instantiation of STSAssumeRoleWebIdentityCredentialsProvider inside
Aliyun/Tencent/Huawei ChunkManager constructors and replaced them with
public static Get...CredentialsProvider() methods that return a
thread-safe, lazily-initialized shared_ptr singleton (static local
variable). This eliminates duplicate provider construction and
header/signal dependency usages tied to per-constructor instantiation.
- Why this does NOT introduce data loss or behavior regression:
Credential acquisition and usage paths are unchanged — callers still
call provider->GetAWSCredentials() and use the returned AWSCredentials
to construct Aws::S3::S3Client. The singleton returns the same provider
object but the provider is stateless with respect to per-manager data
(it only reads environment/platform credentials and produces
AWSCredentials). C++11+ static local initialization provides atomic,
thread-safe construction, so first-access semantics and validation
checks (AssertInfo on access key/secret/token) remain intact.
- PR type (Enhancement/Refactor): Improves credential management by
centralizing provider lifecycle, removing redundant allocations and
header dependencies, and enforcing a single shared provider per cloud
vendor where IAM is used.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
related: #46296
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
- Core invariant: expiration comparisons use
Aws::Utils::DateTime::Now().count() which returns milliseconds; any
expiration grace period must be expressed in milliseconds and compared
via (GetExpiration() - Now()).count() in ExpiresSoon() (Huawei and
Tencent providers).
- Root cause and fix: the grace period constant was authored as 7200
(seconds) but used against millisecond counts, causing premature
refreshes. The PR changes
STS_CREDENTIAL_PROVIDER_EXPIRATION_GRACE_PERIOD to 180 * 1000 (180000
ms) in HuaweiCloudCredentialsProvider.cpp and
TencentCloudCredentialsProvider.cpp to align units and stop unnecessary
refreshes.
- Removed/replaced redundant/incorrect behavior: the PR does not add new
control flow but corrects unit mismatch and simplifies logging/STS
request handling — HuaweiCloudSTSClient now explicitly requests a
7200-second token by adding "token": {"duration_seconds": 7200} to the
JSON body and uses JsonValue(...).View() for parsing; Huawei logging
level raised from TRACE to DEBUG and now logs expiration_count_diff_ms
for clarity. These changes remove ambiguity about requested token
lifetime and improve diagnostic output.
- No data loss or regression: credential contents and assignment are
unchanged — Reload()/RefreshIfExpired()/ExpiresSoon() still populate
m_credentials from STS responses and return them via
GetAWSCredentials(); only the grace-period unit and the Huawei STS
request body/parsing/logging were adjusted. Code paths affected are
ExpiresSoon()/RefreshIfExpired()/Reload() in both providers and
HuaweiCloudSTSCredentialsClient::callHuaweiCloudSTS; since credentials
are still read from the same response fields (access, secret,
securitytoken, expires_at) and assigned to result.creds, there is no
data loss or altered persistence/authorization semantics beyond aligning
requested token duration and correct refresh timing.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
