cockroachdb / cockroachdb/cockroach

util/span: frontier entry is returned to the pool before it is unlinked

Open
#174,948 2 comments 0 reactions 0 assignees View on GitHub
A-cdc C-bug O-agent T-cdc
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Summary:**

`span.btreeFrontier.deleteEntry` registers `defer putFrontierEntry(e)` *before* removing `e` from the heap and the tree. If anything between the defer and `f.tree.Delete(e)` fails — an `expensiveChecksEnabled()` error return, or a panic in `heap.Remove` on a stale `heapIdx` — the entry is returned to the package-global pool while still linked into the tree. `btreeFrontier.Release` then walks the tree and pools every entry it finds, returning the same pointer a second time.

Two unrelated frontiers then alias one entry. A frontier gates emitted resolved timestamps, so the observable failure is a resolved timestamp ahead of unemitted data, for changefeeds, PCR and LDR. There is no error, no log and no metric on this path.

The error-return case does not require a panic at all and reproduces in `crdb_test` builds today. Part of #174944.

**Findings:**

- `span.btreeFrontier.deleteEntry`: `e` (`*btreeFrontierEntry`) is pooled before it is unlinked, and `btreeFrontier.Release` pools it again.
- No item pool in the package has a double-`Put` guard, which is why this class is undetectable in the field: a reissued pooled object surfaces only as wrong answers, arbitrarily later, in unrelated work.

**Next Steps:**

- [ ] Move `putFrontierEntry(e)` after `f.tree.Delete(e)` and after the `checkSpan` error return
- [ ] Guard `heap.Remove` on `e.heapIdx >= 0`
- [ ] Add a `pooled bool` double-`Put` assertion to `btreeFrontierEntry` under `crdb_test`

Epic: none

Jira issue: CRDB-68119

Contributor guide

Open the contributing guide

Research direction

Start at span.btreeFrontier.deleteEntry and btreeFrontier.Release, tracing the ordering around checkSpan, heap.Remove, f.tree.Delete(e), and putFrontierEntry(e). Reproduce the error-return path in a crdb_test build and exercise stale heapIdx handling. Done means entries are unlinked before pooling and the pooled bool assertion catches any double Put.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.