Skip to content

test(debuginfo): add unit tests for debug info store and upload reader#4896

Merged
korniltsev-grafanista merged 7 commits into
grafana:debuginfo-uploadfrom
korniltsev-grafanista-yolo-vibecoder239:vk/e00a-add-unit-tests-c
Mar 13, 2026
Merged

test(debuginfo): add unit tests for debug info store and upload reader#4896
korniltsev-grafanista merged 7 commits into
grafana:debuginfo-uploadfrom
korniltsev-grafanista-yolo-vibecoder239:vk/e00a-add-unit-tests-c

Conversation

@korniltsev-grafanista-yolo-vibecoder239
Copy link
Copy Markdown
Contributor

@korniltsev-grafanista-yolo-vibecoder239 korniltsev-grafanista-yolo-vibecoder239 commented Mar 13, 2026

Summary

Adds comprehensive unit tests for the new debug info service (pkg/debuginfo/), which previously had zero test coverage.

  • pkg/debuginfo/store_test.go — 9 test functions (~30 subtests) covering the store logic
  • pkg/debuginfo/reader/reader_test.go — 4 test functions (~13 subtests) covering the chunked upload reader

What's Tested

Store (store_test.go)

  • ValidateGnuBuildID — regex boundary validation (min/max length, valid/invalid hex chars)
  • validateInit — init message validation (nil checks, debuginfo type validation, build ID propagation)
  • NewStore — constructor validation (bucket required when enabled, optional when disabled)
  • ObjectPath / MetadataObjectPath — path construction for object storage
  • shouldInitiateUpload — full state machine coverage (first-time, stale upload retry, in-progress, already exists, unknown state error)
  • uploadIsStale — staleness threshold boundary cases (startedAt + MaxUploadDuration + 2min)
  • fetchMetadata — bucket reads (not-found → nil, valid protojson, corrupt data → error)
  • writeMetadata — bucket writes and write-then-fetch round-trip

Upload Reader (reader/reader_test.go)

  • Read — single/multiple chunks, empty streams, small buffers, error propagation
  • Size — incremental byte tracking
  • Context cancellation — before first read and between chunks
  • io.ReadAll integration

Approach

  • Uses memory.NewInMemBucket() (in-memory object storage) for real bucket behavior rather than mocks
  • All tests use t.Parallel() and table-driven patterns per project conventions
  • The Upload() streaming handler is not directly tested since it takes a concrete *connect.BidiStream type; instead, all its constituent helper functions are tested individually

🤖 Generated with Claude Code


Note

Medium Risk
Mostly adds tests, but it also changes the Store.Upload path (error type, proto field names, and explicitly marking metadata STATE_UPLOADED), which could affect client-visible behavior and upload state transitions.

Overview
Adds extensive new test coverage for the debug info subsystem, including unit tests for the chunked UploadReader (chunk boundaries, buffering, size tracking, and context cancellation) and for Store helpers (build-id/init validation, staleness decisions, object paths, and metadata read/write).

Updates Store.Upload to use the renamed proto fields/enums (GnuBuildId, FileMetadata_TYPE_*), return connect.NewError(connect.CodeInternal, ...) instead of gRPC status errors, and to set uploaded metadata to STATE_UPLOADED after a successful bucket upload.

Written by Cursor Bugbot for commit df0e8eb. This will update automatically on new commits. Configure here.

Cover the store logic (ValidateGnuBuildID, validateInit, NewStore,
shouldInitiateUpload, uploadIsStale, fetchMetadata, writeMetadata,
ObjectPath, MetadataObjectPath) and the chunked upload reader
(Read, Size, context cancellation) with ~43 test cases across
two new test files.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@korniltsev-grafanista-yolo-vibecoder239 korniltsev-grafanista-yolo-vibecoder239 changed the title add unit tests covering the functionality of the new debug info service (vibe-kanban) test(debuginfo): add unit tests for debug info store and upload reader Mar 13, 2026
The base branch (debuginfo-upload) uses GnuBuildId and
FileMetadata_TYPE_* naming in the generated protobuf, while
this worktree had a divergent proto with GNU and
DEBUGINFO_TYPE_* names. Align the test file with the base
branch so CI compiles correctly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align struct field formatting to pass gofmt check in CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add TestUploadE2E with three subtests that exercise the full Upload
bidi streaming RPC over a real HTTP/2 test server with connect client:

- full upload flow: init -> chunks -> verify stored data and metadata
- already uploaded: pre-populate metadata, verify service declines upload
- disabled service: verify service returns ReasonDisabled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BidiStream requires HTTP/2. Switch from h2c (cleartext) to TLS via
StartTLS() which negotiates HTTP/2 through ALPN. Remove unused
h2c and http2 imports.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.

Comment thread pkg/debuginfo/store_test.go
…d state bug

- Align store.go with base branch protobuf field names (GnuBuildId,
  TYPE_EXECUTABLE_FULL/NO_TEXT) and use connect.NewError instead of
  grpc status.Error
- Add missing md.State = STATE_UPLOADED after successful upload
- Fix e2e test to wait for server handler completion by calling
  Receive() after CloseRequest() before checking bucket contents

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@korniltsev-grafanista korniltsev-grafanista merged commit afe536a into grafana:debuginfo-upload Mar 13, 2026
22 of 24 checks passed
korniltsev-grafanista added a commit that referenced this pull request Mar 13, 2026
* parca symbol upload

* test: add symbolizer test for parca bucket debuginfo source

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* Add debug logging to debuginfo upload

Add debug-level logging to help troubleshoot debuginfo upload issues:
- Log ShouldInitiateUpload results (build_id, decision, reason)
- Log Upload gRPC results (success with size, or failure with error)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* Remove debuginfod support from parca/debuginfo package

Debuginfod fetching is now handled in the symbolizer instead of in the
parca debuginfo store. This removes the debuginfod client implementation,
related tests, and test fixtures from the parca/debuginfo package.

Also removes tenant ID handling from the debuginfo store since it's not
needed for this use case.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* test: use HTTP/2 with AuthenticateUser middleware in debuginfo store test

Refactor the store test to use an HTTP/2 server with h2c and the
util.AuthenticateUser middleware instead of a direct gRPC server.
This better reflects the production setup where requests go through
HTTP middleware for tenant authentication.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* make go/mod

* make fmt

* Update pkg/parca/debuginfo/metadata.go

Co-authored-by: Marc Sanmiquel <marcsanmiquel@gmail.com>

* remove writepath options from the new debuginfo handlers

* level.Error

* cleanup

* remove cache

* init

* add gopclntab

* integrate lidia

* add test for gopclntab-only symbol resolution

Test creates a lidia table from /proc/self/exe using only gopclntab
(symtab disabled), then resolves the test function's own address and
verifies the name matches.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* lint

* disable unused linter for pclntab.go

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* new file name for lidia files with gopclntab support

* improve error logging

* increase upload size

* dont use parca

* dont use parca

* generate

* generate

* revert changes

* revert changes

* revert changes

* revert changes

* todo

* revert change

* fix symbolizer tests after New() signature change

Remove parcaBucket parameter from test helper and all callers
after the symbolizer was refactored to use a single bucket for
both lidia and parca debug info storage.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* rename debuginfo API package to v1alpha1 to mark as experimental

Move proto package from debuginfo.v1 to debuginfo.v1alpha1 and update
all generated code and consumer imports accordingly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* revert lidia change

* address PR review comments: proto naming, state bug, error type, rename

- Rename proto fields to snake_case (gnu_build_id, go_build_id, otel_file_id)
- Rename enum DebuginfoType → Type, add TYPE_UNSPECIFIED, fix prefix
- Fix bug: set STATE_UPLOADED after upload completes
- Replace gRPC status.Error with connect.NewError for consistency
- Rename fetchFromParca → fetchFromUploadedDebugInfo
- Revert ProcessELFData back to private method on Symbolizer

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* cleanup

* test(debuginfo): add unit tests for debug info store and upload reader (#4896)

* add unit tests for debuginfo service

Cover the store logic (ValidateGnuBuildID, validateInit, NewStore,
shouldInitiateUpload, uploadIsStale, fetchMetadata, writeMetadata,
ObjectPath, MetadataObjectPath) and the chunked upload reader
(Read, Size, context cancellation) with ~43 test cases across
two new test files.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix store_test.go to use base branch protobuf field names

The base branch (debuginfo-upload) uses GnuBuildId and
FileMetadata_TYPE_* naming in the generated protobuf, while
this worktree had a divergent proto with GNU and
DEBUGINFO_TYPE_* names. Align the test file with the base
branch so CI compiles correctly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix gofmt alignment in store_test.go

Align struct field formatting to pass gofmt check in CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* add end-to-end connect client tests for debuginfo Upload RPC

Add TestUploadE2E with three subtests that exercise the full Upload
bidi streaming RPC over a real HTTP/2 test server with connect client:

- full upload flow: init -> chunks -> verify stored data and metadata
- already uploaded: pre-populate metadata, verify service declines upload
- disabled service: verify service returns ReasonDisabled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* remove unused net/http import from store_test.go

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix e2e test to use TLS for HTTP/2 bidi streaming

BidiStream requires HTTP/2. Switch from h2c (cleartext) to TLS via
StartTLS() which negotiates HTTP/2 through ALPN. Remove unused
h2c and http2 imports.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix store.go and e2e test: align with base branch protobuf, fix upload state bug

- Align store.go with base branch protobuf field names (GnuBuildId,
  TYPE_EXECUTABLE_FULL/NO_TEXT) and use connect.NewError instead of
  grpc status.Error
- Add missing md.State = STATE_UPLOADED after successful upload
- Fix e2e test to wait for server handler completion by calling
  Receive() after CloseRequest() before checking bucket contents

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix proto formatting (buf format)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address remaining PR review comments

- Fix wrong error messages: "init.FileData" → "init.File" to match actual field name
- Enforce MaxUploadSize in UploadReader to prevent unlimited chunked uploads
- Tenant-isolate lidia cache paths to prevent cross-tenant cache poisoning

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address Cursor BugBot review comments

- Use path.Join instead of filepath.Join in lidiaObjectPath for
  OS-independent object storage keys
- Restore tenant-scoped not-found cache in getLidiaBytes to reduce
  redundant object storage lookups for unknown build IDs
- Extract MaxSizeReader from UploadReader into a composable wrapper
- Handle simultaneous data+EOF in UploadReader.Read per io.Reader contract
- Revert debug-specific IDE run configuration changes

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove MaxSizeReader, enforce size limits at upload and read sites

- Remove MaxSizeReader wrapper; check r.Size() after upload and delete
  oversized objects in store.go
- Remove duplicate not-found cache in symbolizer (debuginfod client
  already caches not-found build IDs)
- Bound io.ReadAll in fetchLidiaFromDebuginfod with readAllWithLimit
  to enforce SymbolizerMaxSymbolSizeBytes for uploaded debug-info files

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Marc Sanmiquel <marcsanmiquel@gmail.com>
Co-authored-by: Tolya Korniltsev YOLO vibecoder <anatoly.korniltsev+githubvibecoder@grafana.com>
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