Skip to content

Commit

Permalink
filesessions: continue processing uploads on error (#46128)
Browse files Browse the repository at this point in the history
If we encounter a BadParameter error, assume something is wrong
with the upload, mark it as corrupted, and move on to the next one.

This prevents one bad file from preventing other legitimate uploads
from completing.
  • Loading branch information
zmb3 authored Sep 3, 2024
1 parent 3c827ac commit 453ab6e
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
2 changes: 1 addition & 1 deletion lib/events/filesessions/fileasync.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ func (u *Uploader) Scan(ctx context.Context) (*ScanStats, error) {
u.log.Debugf("Recording %v was uploaded by another process.", fi.Name())
continue
}
if isSessionError(err) {
if isSessionError(err) || trace.IsBadParameter(err) {
u.log.WithError(err).Warningf("Skipped session recording %v.", fi.Name())
stats.Corrupted++
continue
Expand Down
18 changes: 13 additions & 5 deletions lib/events/filesessions/fileasync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,17 +138,23 @@ func TestMovesCorruptedUploads(t *testing.T) {
sessionID := session.NewID()
uploadPath := filepath.Join(scanDir, sessionID.String()+".tar")
errorPath := filepath.Join(scanDir, sessionID.String()+".error")
badFilePath := filepath.Join(scanDir, "not-a-uuid.tar")

// create a "corrupted" upload and error file in the scan dir
b := make([]byte, 4096)
rand.Read(b)
require.NoError(t, os.WriteFile(uploadPath, b, 0o600))
require.NoError(t, uploader.writeSessionError(sessionID, errors.New("this is a corrupted upload")))

// create a file with an invalid name (not a session ID)
badFile, err := os.Create(badFilePath)
require.NoError(t, err)
require.NoError(t, badFile.Close())

stats, err := uploader.Scan(context.Background())
require.NoError(t, err)
require.Equal(t, 1, stats.Scanned)
require.Equal(t, 1, stats.Corrupted)
require.Equal(t, 2, stats.Scanned)
require.Equal(t, 2, stats.Corrupted)
require.Equal(t, 0, stats.Started)

require.NoFileExists(t, uploadPath)
Expand All @@ -157,11 +163,13 @@ func TestMovesCorruptedUploads(t *testing.T) {
require.FileExists(t, filepath.Join(corruptedDir, filepath.Base(uploadPath)))
require.FileExists(t, filepath.Join(corruptedDir, filepath.Base(errorPath)))

// run a second scan to verify that the file is no longer processed
// run a second scan to verify that:
// 1. the corrupted file is no longer processed
// 2. the file with the bad name was still flagged as corrupted
stats, err = uploader.Scan(context.Background())
require.NoError(t, err)
require.Equal(t, 0, stats.Scanned)
require.Equal(t, 0, stats.Corrupted)
require.Equal(t, 1, stats.Scanned)
require.Equal(t, 1, stats.Corrupted)
require.Equal(t, 0, stats.Started)
}

Expand Down

0 comments on commit 453ab6e

Please sign in to comment.