Fix DeletePartialMatch deleting unrelated metrics on hash collision

[pujitha24]

Motivation:
Issue #1810 reports metrics not being freed as expected after calling
DeletePartialMatch(). While investigating, a distinct, reproducible bug
was found in metricMap.deleteByLabels (the function backing
MetricVec.DeletePartialMatch): when two or more differently-labeled
metrics land in the same hash bucket (a collision), and only one of
them matches the partial-label deletion criteria, the old code deleted
the entire bucket via delete(m.metrics, h) instead of only the
matching entry - silently discarding unrelated metrics that were never
meant to be deleted. Delete() and DeleteLabelValues() already handled
this correctly elsewhere in the same file (splicing out only the
matching slice element); DeletePartialMatch did not.

Additionally, the old single-match code path did not stop after
removing one bucket, and any collision-safe removal needs to clear the
vacated slice slot so it does not keep referencing a Metric object
through the backing array's spare capacity, which would prevent that
Metric from being garbage collected.

This bug is a plausible contributor to reports like #1810, but the
original reporter's case was never profiled, so it is not confirmed as
the root cause. Hash collisions on the label-value hash are rare in
practice, so this alone may not fully explain a leak; regardless, the
incorrect bucket-wide deletion is a real correctness bug worth fixing
on its own.

Approach:
Rewrite deleteByLabels to filter each hash bucket's slice in place,
removing only the entries that actually match the partial-label
criteria (there can be more than one match per bucket), and explicitly
zero the vacated tail slots of the reused backing array so deleted
Metric objects don't stay reachable. Removed findMetricWithPartialLabels,
which became unused once its only call site was replaced.

Validation:
Added TestDeletePartialMatchWithCollisions, following the existing
TestDeleteWithCollisions / TestDeleteLabelValuesWithCollisions pattern:
forces all label combinations into a single hash bucket via a
stubbed-out hashAdd/hashAddByte, then reuses the existing
testDeletePartialMatch test body. Confirmed this test fails against the
pre-fix code (unrelated collided metrics vanish) and passes after the
fix.

go build ./...
go test ./prometheus/... -run 'TestDelete|TestMetricVec|TestCurryVec' -race -v

All of the above passed. (The full go test ./prometheus/... has two
pre-existing, unrelated failures on this checkout's Go toolchain, caused
by a godebug runtime metric mismatch in go_collector_latest_test.go;
they reproduce identically on main without this change and are
unaffected by it.)

Fixes #1810

Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pujitha24]

Just checking in on this — it's still rebased and green, happy to make any changes that would help move review along.

[bwplotka]

Again, hash collision is not really a common occurence, unless you we can prove otherwise

Can you provide a repro test that confirms that e.g. constructs two series that collide?

[pujitha24]

TestDeletePartialMatchWithCollisions (already in this PR, vec_test.go) does exactly that: it overrides hashAdd/hashAddByte to force distinct label combinations into the same bucket, then runs testDeletePartialMatch against it. That's the same technique TestDeleteWithCollisions and TestDeleteLabelValuesWithCollisions already use elsewhere in this file for Delete/DeleteLabelValues — constructing an actual 64-bit hash collision by picking label values isn't really feasible, so the existing tests fake it by stubbing the hash functions. I just re-verified locally: reverting the deleteByLabels fix and running go test -run TestDeletePartialMatchWithCollisions ./prometheus/ fails (got 1, want 2, unrelated metrics vanish from the bucket), and it passes with the fix applied. Let me know if you had something else in mind by "repro."

[lowang-bh]

recently, when I use AI review code, it report a bug about this same issue:

1. 数据结构是多值桶（vec.go）：

  m.metrics[hash] = append(m.metrics[hash], metricWithLabelValues{...})

  插入用 append 到 slice，说明设计上明确允许同一 hash 挂多个 metric（哈希冲突）。

  2. 冲突场景存在于所有精确操作的处理逻辑里：

  // deleteByHashWithLabelValues / getMetricWithHashAndLabelValues 都有：
  i := findMetricWithLabelValues(metrics, lvs, curry)  // 桶内逐个比对 label values

  即：其他所有操作（Get/Delete/CurryWith）都遵循"hash 定位桶 → 桶内比对完整 label 值"的两段式，说明作者知道桶里可能有多个不相关的 metric。

  3. 唯独 deleteByLabels 不匹配这个模式：

  for h, metrics := range m.metrics {
      i := findMetricWithPartialLabels(...)  // 只找第一个匹配的
      if i >= len(metrics) { continue }
      delete(m.metrics, h)                   // 删整个桶（slice 全丢）
  }

  - 找到 1 个匹配就删掉整个 slice——桶里其他不匹配的 metric 跟着没了
  - 桶内第一个不匹配、后面有匹配 → 漏删

[pujitha24]

Thanks for double-checking with an AI reviewer. The pattern it describes is exactly the bug this PR fixes — that write-up matches the old, pre-fix deleteByLabels (find one match, then delete(m.metrics, h) for the whole bucket). The current code here already splices out only the matching entries per bucket (and handles more than one match per bucket), and TestDeletePartialMatchWithCollisions exercises that exact collision scenario. Let me know if you're seeing something different in the current diff.

[bwplotka]

— constructing an actual 64-bit hash collision by picking label values isn't really feasible, so the existing tests fake it by stubbing the hash functions.

That's my exact point. If it's not feasible, it means it's unlikely to happen on production, then why fixing?