Skip to content

Conversation

Copy link

Copilot AI commented Oct 21, 2025

Problem

The access logs uploader was using context.TODO() when uploading parcels to object storage, which meant there was no timeout for upload operations. This could cause uploads to hang indefinitely if the storage backend became unresponsive or experienced network issues.

The issue was identified at:

// TODO(artur): we need to figure out what context we want
// to pass here. WithTimeout(Background, …)?
if err := up.store.Put(context.TODO(), up.bucket, up.key, up.body); err != nil {

Solution

This PR adds a configurable UploadTimeout option that sets a maximum duration for each individual parcel upload operation:

  • New configuration field: UploadTimeout in Options.UploadingOptions with a default of 30 seconds
  • Context with timeout: Replaced context.TODO() with context.WithTimeout() in the upload loop
  • Per-parcel timeout: Each upload carries its own timeout value, allowing for future per-parcel configuration
  • User-configurable: Users can adjust the timeout via the config struct to match their storage backend characteristics

Changes

  1. Added uploadTimeout field to upload struct to make timeout scoped to individual parcels
  2. Added uploadTimeout field to sequentialUploaderOptions and sequentialUploader structs
  3. Updated Options.UploadingOptions to include UploadTimeout time.Duration with a default of 30 seconds
  4. Modified sequentialUploader.run() to use the timeout from each individual upload
  5. Updated both queueUpload and queueUploadWithoutQueueLimit to pass the timeout when creating uploads
  6. Updated all existing tests to include the new timeout parameter
  7. Added TestUploadTimeout to verify that uploads properly respect the configured timeout

Testing

All existing tests pass, and a new test specifically validates timeout behavior by:

  • Creating a storage mock that delays longer than the configured timeout
  • Verifying the upload is cancelled with context.DeadlineExceeded
  • Confirming the retry mechanism handles timeouts appropriately

Example usage:

processor := accesslogs.NewProcessor(log, accesslogs.Options{
    UploadingOptions: struct {
        UploadTimeout: 2 * time.Minute, // Custom timeout
    },
})

Fixes the TODO comment and provides robust timeout handling for production deployments.

Fixes storj/edge#555

Original prompt

This section details on the original issue you should resolve

<issue_title>add timeouts for the parcel uploads</issue_title>
<issue_description>In the current server access logging implementation, we haven't set a robust timeout for the parcel uploads:

// TODO(artur): we need to figure out what context we want
// to pass here. WithTimeout(Background, …)?
if err := up.store.Put(context.TODO(), up.bucket, up.key, up.body); err != nil {

We need to add one; perhaps make it configurable.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes storj/edge#555


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI changed the title [WIP] Add timeouts for the parcel uploads Add configurable upload timeout for parcel uploads Oct 21, 2025
Copilot AI requested a review from amwolff October 21, 2025 16:11
Copilot finished work on behalf of amwolff October 21, 2025 16:11
Copy link
Member

@amwolff amwolff left a comment

Choose a reason for hiding this comment

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

@copilot could you make the timeout not be global but scoped to an individual parcel?

opts.UploadingOptions.ShutdownTimeout = time.Minute
}
if opts.UploadingOptions.UploadTimeout <= 0 {
opts.UploadingOptions.UploadTimeout = 5 * time.Minute
Copy link
Member

Choose a reason for hiding this comment

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

@copilot a 5-minute default is a bit too much. Let's go with a slightly lower value, like 30 seconds.

Copy link
Author

Choose a reason for hiding this comment

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

Changed the default upload timeout from 5 minutes to 30 seconds in commit 9ef9a2c.

- Changed default upload timeout from 5 minutes to 30 seconds
- Made timeout scoped to individual parcels instead of global
- Added uploadTimeout field to upload struct
- Each upload now carries its own timeout value

Co-authored-by: amwolff <[email protected]>
Copy link
Author

Copilot AI commented Oct 21, 2025

@copilot could you make the timeout not be global but scoped to an individual parcel?

Made the timeout scoped to individual parcels in commit 9ef9a2c. Each upload struct now carries its own uploadTimeout field, and the timeout is read from the upload when creating the context in the run loop.

Copilot AI requested a review from amwolff October 21, 2025 17:03
Copilot finished work on behalf of amwolff October 21, 2025 17:03
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.

add timeouts for the parcel uploads

2 participants