common icon indicating copy to clipboard operation
common copied to clipboard

Add configurable upload timeout for parcel uploads

Open Copilot opened this issue 4 months ago • 1 comments

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: https://github.com/storj/common/blob/b9952c61d22febe124585cb2529231854d493a61/accesslogs/uploader.go#L195-L197

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:

https://github.com/storj/common/blob/b9952c61d22febe124585cb2529231854d493a61/accesslogs/uploader.go#L195-L197

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 avatar Oct 21 '25 16:10 Copilot

@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 avatar Oct 21 '25 17:10 Copilot