Skip to content

feat(profiling): Add 30-second timeout for chunk downloads #664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sentry wants to merge 1 commit into from
Closed

feat(profiling): Add 30-second timeout for chunk downloads #664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion cmd/vroom/chunk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package main

import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"strconv"
"time"

"github.com/getsentry/sentry-go"
"github.com/google/uuid"
Expand Down Expand Up @@ -43,6 +45,8 @@ type postProfileFromChunkIDsResponse struct {
// body request, we use a POST method instead (similarly to the flamegraph endpoint).
func (env *environment) postProfileFromChunkIDs(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
downloadContext, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
hub := sentry.GetHubFromContext(ctx)
ps := httprouter.ParamsFromContext(ctx)
rawOrganizationID := ps.ByName("organization_id")
Expand Down Expand Up @@ -87,7 +91,7 @@ func (env *environment) postProfileFromChunkIDs(w http.ResponseWriter, r *http.R
go func() {
for _, ID := range requestBody.ChunkIDs {
readJobs <- chunk.ReadJob{
Ctx: ctx,
Ctx: downloadContext,
Storage: env.storage,
OrganizationID: organizationID,
ProjectID: projectID,
Expand Down Expand Up @@ -127,6 +131,10 @@ func (env *environment) postProfileFromChunkIDs(w http.ResponseWriter, r *http.R
w.WriteHeader(http.StatusNotFound)
return
}
if errors.Is(err, context.Canceled) {
w.WriteHeader(http.StatusServiceUnavailable)
return
}
Copy link
Copy Markdown

@cursor cursor bot Mar 20, 2026

Choose a reason for hiding this comment

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

Timeout produces DeadlineExceeded, not Canceled error

High Severity

The downloadContext is created with context.WithTimeout, which produces context.DeadlineExceeded when the 30-second timeout fires. However, the error check only matches context.Canceled. This means timeout errors will bypass the 503 handler and fall through to the generic 500 path, getting reported to Sentry — the exact problem this PR aims to fix. The existing flamegraph code correctly checks for context.DeadlineExceeded in the same scenario.

Fix in Cursor Fix in Web

Comment on lines +134 to +137
Copy link
Copy Markdown
Author

@sentry sentry bot Mar 20, 2026

Choose a reason for hiding this comment

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

Bug: The new timeout error handling only checks for context.Canceled and misses context.DeadlineExceeded, causing timeouts to return an incorrect HTTP 500 error instead of 503.
Severity: MEDIUM

Suggested Fix

Add a check for context.DeadlineExceeded alongside the existing check for context.Canceled. If the error is context.DeadlineExceeded, write the http.StatusServiceUnavailable header and return, mirroring the behavior for a canceled context. This should be applied in both postProfileFromChunkIDs() and getRawChunk().

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: cmd/vroom/chunk.go#L134-L137

Potential issue: The error handling for the newly introduced 30-second chunk download
timeout is incomplete. When the timeout is exceeded, `context.WithTimeout` generates a
`context.DeadlineExceeded` error. However, the code only checks for `context.Canceled`.
This causes the timeout error to be missed, leading the request to fall through to the
generic error handler which returns an HTTP 500 Internal Server Error instead of the
intended HTTP 503 Service Unavailable. This same logical error exists in both
`postProfileFromChunkIDs()` and `getRawChunk()`.

Did we get this right? 👍 / 👎 to inform future reviews.

var e *googleapi.Error
if ok := errors.As(err, &e); ok {
hub.Scope().SetContext("Google Cloud Storage Error", map[string]interface{}{
Expand Down Expand Up @@ -292,6 +300,10 @@ func (env *environment) getRawChunk(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotFound)
return
}
if errors.Is(err, context.Canceled) {
w.WriteHeader(http.StatusServiceUnavailable)
return
}
var e *googleapi.Error
if ok := errors.As(err, &e); ok {
hub.Scope().SetContext("Google Cloud Storage Error", map[string]interface{}{
Expand Down
Loading