During index building in ComputeSyncActions, indexing errors are collected in shared slices:
var sourceIndexErrs, destinationIndexErrs []error
These slices are appended to from multiple goroutines:
sourceIndexErrs = append(sourceIndexErrs, sourceIndexErr)
destinationIndexErrs = append(destinationIndexErrs, destinationIndexErr)
However, slice appends are not thread-safe. Multiple goroutines may append to the slice simultaneously since indexing runs in parallel:
go func(index int) {
...
}()
Impact
This issue may only appear on larger datasets or systems with many CPU cores.
While rare, this bug can cause:
- data races (detectable with go test -race)
- corrupted slice state
- lost indexing errors
- undefined behavior or potential runtime panic under heavy concurrency
Fixing it improves correctness without changing behavior.
Proposed Fix
Protect the slice with a mutex or use a channel.
Minimal fix (mutex)
Apply the fix inside the goroutine (shown below) and do the same for destination errors.
var sourceIndexErrs, destinationIndexErrs []error
var errMutex sync.Mutex
if sourceIndexErr != nil {
errMutex.Lock()
sourceIndexErrs = append(sourceIndexErrs, sourceIndexErr)
errMutex.Unlock()
}
Alternative (channel-based)
errChan := make(chan error, parallelismForSource+parallelismForDestination)
Inside goroutines:
if sourceIndexErr != nil {
errChan <- sourceIndexErr
}
After wg.Wait():
close(errChan)
for err := range errChan {
sourceIndexErrs = append(sourceIndexErrs, err)
}
Test to reproduce the data races (using "go test -race")
package service
import (
"testing"
"github.com/m-manu/rsync-sidekick/entity"
)
func TestComputeSyncActions_RaceOnErrorCollection(t *testing.T) {
sourceFiles := map[string]entity.FileMeta{}
destFiles := map[string]entity.FileMeta{}
// create many fake files to force parallel indexing
orphans := []string{}
candidates := []string{}
for i := 0; i < 200; i++ {
name := "file" + string(rune(i))
sourceFiles[name] = entity.FileMeta{
Size: 100,
ModifiedTimestamp: int64(i),
}
destFiles[name] = entity.FileMeta{
Size: 100,
ModifiedTimestamp: int64(i),
}
orphans = append(orphans, name)
candidates = append(candidates, name)
}
var srcCounter int32
var dstCounter int32
// paths don't exist -> buildIndex will generate errors
ComputeSyncActions(
"/nonexistent/source",
sourceFiles,
orphans,
"/nonexistent/dest",
destFiles,
candidates,
&srcCounter,
&dstCounter,
false,
)
}
During index building in ComputeSyncActions, indexing errors are collected in shared slices:
var sourceIndexErrs, destinationIndexErrs []error
These slices are appended to from multiple goroutines:
sourceIndexErrs = append(sourceIndexErrs, sourceIndexErr)
destinationIndexErrs = append(destinationIndexErrs, destinationIndexErr)
However, slice appends are not thread-safe. Multiple goroutines may append to the slice simultaneously since indexing runs in parallel:
go func(index int) {
...
}()
Impact
This issue may only appear on larger datasets or systems with many CPU cores.
While rare, this bug can cause:
Fixing it improves correctness without changing behavior.
Proposed Fix
Protect the slice with a mutex or use a channel.
Minimal fix (mutex)
Apply the fix inside the goroutine (shown below) and do the same for destination errors.
var sourceIndexErrs, destinationIndexErrs []error
var errMutex sync.Mutex
if sourceIndexErr != nil {
errMutex.Lock()
sourceIndexErrs = append(sourceIndexErrs, sourceIndexErr)
errMutex.Unlock()
}
Alternative (channel-based)
errChan := make(chan error, parallelismForSource+parallelismForDestination)
Inside goroutines:
if sourceIndexErr != nil {
errChan <- sourceIndexErr
}
After wg.Wait():
close(errChan)
for err := range errChan {
sourceIndexErrs = append(sourceIndexErrs, err)
}
Test to reproduce the data races (using "go test -race")
package service
import (
"testing"
"github.com/m-manu/rsync-sidekick/entity"
)
func TestComputeSyncActions_RaceOnErrorCollection(t *testing.T) {
sourceFiles := map[string]entity.FileMeta{}
destFiles := map[string]entity.FileMeta{}
// create many fake files to force parallel indexing
orphans := []string{}
candidates := []string{}
for i := 0; i < 200; i++ {
name := "file" + string(rune(i))
sourceFiles[name] = entity.FileMeta{
Size: 100,
ModifiedTimestamp: int64(i),
}
destFiles[name] = entity.FileMeta{
Size: 100,
ModifiedTimestamp: int64(i),
}
orphans = append(orphans, name)
candidates = append(candidates, name)
}
var srcCounter int32
var dstCounter int32
// paths don't exist -> buildIndex will generate errors
ComputeSyncActions(
"/nonexistent/source",
sourceFiles,
orphans,
"/nonexistent/dest",
destFiles,
candidates,
&srcCounter,
&dstCounter,
false,
)
}