Skip to content

roachvet: add swallowed error detection analyzers#162155

Draft
ajstorm wants to merge 1 commit intocockroachdb:masterfrom
ajstorm:ajstorm-error-linter
Draft

roachvet: add swallowed error detection analyzers#162155
ajstorm wants to merge 1 commit intocockroachdb:masterfrom
ajstorm:ajstorm-error-linter

Conversation

@ajstorm
Copy link
Copy Markdown
Collaborator

@ajstorm ajstorm commented Feb 1, 2026

Summary

This PR adds 5 new static analysis passes to roachvet for detecting swallowed errors that could cause silent data loss or corruption:

  • defererr: Detects errors in defer blocks that are logged but not propagated
  • iteratorerr: Detects iterator loops where the error variable may not be checked after the loop exits
  • errdelegate: Detects Err() methods that don't delegate to embedded types
  • logerr: Detects log-and-swallow patterns
  • txnerr: Detects swallowed errors in transaction closures

Current Findings

Running these analyzers on the codebase finds 10 issues (3 iteratorerr, 7 defererr).

Click to expand full findings

iteratorerr (3 issues) - Likely Real Bugs

These are cases where iterator loop errors are not checked after the loop exits:

File Line Issue
pkg/server/index_usage_stats.go 272 Iterator loop returns nil instead of err after loop
pkg/server/index_usage_stats.go 418 Iterator loop returns nil instead of err after loop
pkg/sql/scheduledlogging/captured_index_usage_stats.go 295 Iterator loop returns nil instead of err after loop

defererr (7 issues) - Mostly Intentional Patterns

These are cases where errors in defer blocks are logged but not propagated:

File Line Assessment
pkg/backup/backup_processor.go 442 Likely safe - progress reported after Finish() succeeds
pkg/ccl/cmdccl/clusterrepl/main.go 103 Intentional - best-effort cleanup on shutdown
pkg/ccl/sqlproxyccl/conn_migration.go 195 Intentional - recovery pattern with Close() fallback
pkg/cmd/dev/build.go 136 Dev tooling - less critical
pkg/cmd/dev/build.go 138 Dev tooling - less critical
pkg/cmd/roachprod-centralized/.../migrator.go 215 Roachprod tooling
pkg/roachprod/vm/gce/dns.go 469 Roachprod tooling

Test plan

  • Unit tests for each analyzer in testdata/ directories
  • Analyzers are integrated into roachvet and run via nogo during bazel builds

🤖 Generated with Claude Code

Release note: none

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Feb 1, 2026

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Add new lint passes for detecting swallowed errors:
- defererr: detects errors ignored in defer statements
- iteratorerr: detects unchecked iterator errors
- errdelegate: detects errors not properly delegated
- logerr: detects errors logged but not returned
- txnerr: detects transaction error handling issues

Epic: None
Release Notes: None

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ajstorm ajstorm force-pushed the ajstorm-error-linter branch from c49dd14 to 7e1bbaf Compare February 1, 2026 19:52
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.

2 participants