Skip to content

cache: mark cache panic messages as redact.Safe#5829

Merged
annrpom merged 1 commit intocockroachdb:masterfrom
annrpom:fix-cache-panic-redaction
Mar 9, 2026
Merged

cache: mark cache panic messages as redact.Safe#5829
annrpom merged 1 commit intocockroachdb:masterfrom
annrpom:fix-cache-panic-redaction

Conversation

@annrpom
Copy link
Contributor

@annrpom annrpom commented Mar 3, 2026

Previously, panic messages in the cache package containing ref counts were being redacted when surfaced in CockroachDB logs, making it impossible to diagnose cache value ref counting issues.

Wrap the panic format strings with redact.Safe() so the full message (including the ref count) is visible in redacted logs.

Informs: cockroachdb/cockroach#164740.

Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@annrpom
Copy link
Contributor Author

annrpom commented Mar 3, 2026

would we be interested in getting claude to audit our pebble repo for this pattern?

this PR was motivated by the tagged sentry report - was just trying to triage them in passing

Previously, panic messages in the cache package containing ref counts
were being redacted when surfaced in CockroachDB logs, making it
impossible to diagnose cache value ref counting issues.

Wrap the panic format strings with errors.AssertionFailedf() so the
full message (including the ref count) is visible in redacted logs.

Informs: cockroachdb/cockroach#164740.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@annrpom annrpom force-pushed the fix-cache-panic-redaction branch from 9750e37 to c459dd0 Compare March 4, 2026 17:00
@annrpom annrpom marked this pull request as ready for review March 9, 2026 14:16
@annrpom annrpom requested a review from a team as a code owner March 9, 2026 14:16
@annrpom annrpom requested a review from RaduBerinde March 9, 2026 14:16
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We could add a linter that disallows panic(fmt.Sprintf())

@annrpom
Copy link
Contributor Author

annrpom commented Mar 9, 2026

TFTR! ('-')7

sgtm - i'll get claude to cook that up during some downtime

@annrpom annrpom merged commit 0e1a672 into cockroachdb:master Mar 9, 2026
10 checks passed
@annrpom annrpom deleted the fix-cache-panic-redaction branch March 9, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants