Skip to content

Commit 9f36e90

Browse files
authored
Fixed racy mutation that prevented file restoration in edge case, better test, fixed spelling (#262)
* Fix racy mutation, spelling * Use timeout instead of fixed time, change delay to internally use ms instead of s
1 parent 5d15344 commit 9f36e90

File tree

4 files changed

+47
-20
lines changed

4 files changed

+47
-20
lines changed

internal/storage/FileServing.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -810,30 +810,32 @@ func DeleteFile(fileId string, deleteSource bool) bool {
810810

811811
// DeleteFileSchedule schedules a file for deletion after a specified delay and optionally deletes its source.
812812
// Returns true if scheduling is successful, false otherwise.
813-
func DeleteFileSchedule(fileId string, delaySeconds int, deleteSource bool) bool {
813+
func DeleteFileSchedule(fileId string, delayMs int, deleteSource bool) bool {
814814
if fileId == "" {
815815
return false
816816
}
817817
file, ok := database.GetMetaDataById(fileId)
818818
if !ok {
819819
return false
820820
}
821-
deletionTime := time.Now().Add(time.Duration(delaySeconds) * time.Second).Unix()
821+
deletionTime := time.Now().Add(time.Duration(delayMs) * time.Millisecond).Unix()
822822
file.PendingDeletion = deletionTime
823823
database.SaveMetaData(file)
824-
go func() {
824+
// Explicit parameter to avoid accidental changes
825+
go func(id string, timestamp int64) {
825826
select {
826-
case <-time.After(time.Duration(delaySeconds) * time.Second):
827+
case <-time.After(time.Duration(delayMs) * time.Millisecond):
827828
}
828-
file, ok = database.GetMetaDataById(fileId)
829-
if !ok {
829+
// A new models.File needs to be assigned to avoid a racy mutation
830+
retrievedFile, exists := database.GetMetaDataById(id)
831+
if !exists {
830832
return
831833
}
832834
// To prevent race conditions, it is checked if the deletion time is the same time that was originally set
833-
if file.PendingDeletion == deletionTime {
834-
DeleteFile(fileId, deleteSource)
835+
if retrievedFile.PendingDeletion == timestamp {
836+
DeleteFile(id, deleteSource)
835837
}
836-
}()
838+
}(fileId, deletionTime)
837839
return true
838840
}
839841

internal/storage/chunking/Chunking.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,11 @@ func ParseChunkInfo(r *http.Request, isApiCall bool) (ChunkInfo, error) {
6868
if len(info.UUID) < 10 {
6969
return ChunkInfo{}, errors.New("invalid uuid submitted, needs to be at least 10 characters long")
7070
}
71-
info.UUID = sanitseUuid(info.UUID)
71+
info.UUID = sanitiseUuid(info.UUID)
7272
return info, nil
7373
}
7474

75-
func sanitseUuid(input string) string {
75+
func sanitiseUuid(input string) string {
7676
reg, err := regexp.Compile("[^a-zA-Z0-9-]")
7777
helper.Check(err)
7878
return reg.ReplaceAllString(input, "_")

internal/webserver/api/Api.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ func apiDeleteFile(w http.ResponseWriter, r requestParser, user models.User) {
345345
if request.DelaySeconds == 0 {
346346
_ = storage.DeleteFile(request.Id, true)
347347
} else {
348-
_ = storage.DeleteFileSchedule(request.Id, request.DelaySeconds, true)
348+
_ = storage.DeleteFileSchedule(request.Id, request.DelaySeconds*1000, true)
349349
}
350350
}
351351

internal/webserver/api/Api_test.go

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,10 +1227,9 @@ func TestRestoreFile(t *testing.T) {
12271227
testRestoreFileCall(t, apiKey.Id, fileUser.Id, 200, fileUser.ToJsonResult(config.ServerUrl, config.IncludeFilename))
12281228
testRestoreFileCall(t, apiKey.Id, fileAdmin.Id, 401, `{"Result":"error","ErrorMessage":"No permission to restore this file"}`)
12291229

1230-
storage.DeleteFileSchedule(fileUser.Id, 1, true)
1231-
storage.DeleteFileSchedule(fileAdmin.Id, 1, true)
1230+
storage.DeleteFileSchedule(fileUser.Id, 500, true)
1231+
storage.DeleteFileSchedule(fileAdmin.Id, 500, true)
12321232

1233-
time.Sleep(400 * time.Millisecond)
12341233
file, ok := database.GetMetaDataById(fileUser.Id)
12351234
test.IsEqualBool(t, ok, true)
12361235
test.IsEqualBool(t, file.PendingDeletion != 0, true)
@@ -1248,12 +1247,38 @@ func TestRestoreFile(t *testing.T) {
12481247
test.IsEqualBool(t, ok, true)
12491248
test.IsEqualBool(t, file.PendingDeletion != 0, true)
12501249

1251-
time.Sleep(600 * time.Millisecond)
1252-
file, ok = database.GetMetaDataById(fileUser.Id)
1253-
test.IsEqualBool(t, ok, true)
1250+
startTime := time.Now()
1251+
for {
1252+
if time.Since(startTime) > 10*time.Second {
1253+
t.Errorf("Timeout waiting for file to be deleted")
1254+
break
1255+
}
1256+
_, ok = database.GetMetaDataById(fileAdmin.Id)
1257+
if !ok {
1258+
break
1259+
} else {
1260+
time.Sleep(50 * time.Millisecond)
1261+
}
1262+
}
1263+
1264+
startTime = time.Now()
1265+
for {
1266+
if time.Since(startTime) > 10*time.Second {
1267+
t.Errorf("Timeout waiting for file to be restored")
1268+
break
1269+
}
1270+
file, ok = database.GetMetaDataById(fileUser.Id)
1271+
if !ok {
1272+
t.Errorf("File was deleted")
1273+
break
1274+
}
1275+
if file.PendingDeletion == 0 {
1276+
break
1277+
} else {
1278+
time.Sleep(50 * time.Millisecond)
1279+
}
1280+
}
12541281
test.IsEqualInt64(t, file.PendingDeletion, 0)
1255-
_, ok = database.GetMetaDataById(fileAdmin.Id)
1256-
test.IsEqualBool(t, ok, false)
12571282
storage.DeleteFile(fileUser.Id, true)
12581283
}
12591284

0 commit comments

Comments
 (0)