Fix race conditions and improve etcd timeout handling

This commit addresses several critical race conditions and improves the reliability
of etcd operations through better timeout and retry handling.

## Race Condition Fixes

1. **Task Resource Management Bug**
   - Fixed incorrect variable usage in task/list.go:78
   - Was using completed task's resources instead of idle task's resources
   - This caused resource conflicts and potential deadlocks

2. **Database Channel Initialization**
   - Added sync.Once pattern to ensure thread-safe channel initialization
   - Prevents panic from concurrent access during startup
   - Created initDBRequests() function for safe initialization

3. **Published Storage Double-Checked Locking**
   - Implemented double-checked locking pattern in GetPublishedStorage
   - Reduces lock contention while preventing concurrent initialization
   - Improves performance for frequently accessed storage

4. **File Operation Synchronization**
   - Created FileLockRegistry in utils/filelock.go
   - Prevents concurrent file operations (create, rename, delete, link)
   - Implements deadlock prevention for multi-file operations
   - Critical for preventing file corruption during parallel publishes

5. **WaitGroup Miscount Prevention**
   - Added defer pattern to ensure Done() is always called
   - Protects against panics during task execution
   - Prevents "negative WaitGroup counter" errors

## etcd Improvements

1. **Timeout Protection**
   - Replaced global context.TODO() with per-operation timeout contexts
   - Default timeout: 60 seconds (configurable)
   - Prevents indefinite hangs when etcd is unresponsive

2. **Environment Variable Configuration**
   - APTLY_ETCD_TIMEOUT: Operation timeout (default: 60s)
   - APTLY_ETCD_DIAL_TIMEOUT: Connection timeout (default: 60s)
   - APTLY_ETCD_KEEPALIVE: Keep-alive timeout (default: 7200s)
   - APTLY_ETCD_MAX_MSG_SIZE: Max message size (default: 50MB)

3. **Retry Logic for Read Operations**
   - Get operations retry up to 3 times with exponential backoff
   - Only retries on temporary/network errors
   - Improves reliability without risking data inconsistency

4. **Enhanced Error Logging**
   - All etcd errors now logged with operation context
   - Replaces silent failures with actionable error messages
   - Improves debugging and monitoring capabilities

5. **Increased Message Size Limits**
   - Default increased from 10MB to 50MB
   - Configurable via environment variable
   - Prevents "message too large" errors for large operations

## Testing

- Added comprehensive tests for etcd timeout functionality
- Tests verify context timeout, retry logic, and configuration
- All existing tests pass with the new implementation

## Documentation

- Updated README.rst with etcd configuration section
- Documented all environment variables and their defaults
- Added examples and feature descriptions

These changes significantly improve the reliability and debuggability of aptly
when using etcd as the database backend, while also fixing critical race
conditions that could cause data corruption or service crashes.
This commit is contained in:
Nick Bozhenko
2025-07-10 10:05:49 -04:00
parent 660cee2ce3
commit 463c34a38e
12 changed files with 483 additions and 51 deletions
+17 -7
View File
@@ -7,6 +7,7 @@ import (
"sort"
"strconv"
"strings"
"sync"
"sync/atomic"
"github.com/aptly-dev/aptly/aptly"
@@ -100,7 +101,18 @@ type dbRequest struct {
err chan<- error
}
var dbRequests chan dbRequest
var (
dbRequests chan dbRequest
dbRequestsOnce sync.Once
)
// initDBRequests initializes the database request channel in a thread-safe manner
func initDBRequests() {
dbRequestsOnce.Do(func() {
dbRequests = make(chan dbRequest, 1)
go acquireDatabase()
})
}
// Acquire database lock and release it when not needed anymore.
//
@@ -139,9 +151,8 @@ func acquireDatabase() {
// runTaskInBackground to run a task which accquire database.
// Important do not forget to defer to releaseDatabaseConnection
func acquireDatabaseConnection() error {
if dbRequests == nil {
return nil
}
// Ensure channel is initialized
initDBRequests()
errCh := make(chan error)
dbRequests <- dbRequest{acquiredb, errCh}
@@ -151,9 +162,8 @@ func acquireDatabaseConnection() error {
// Release database connection when not needed anymore
func releaseDatabaseConnection() error {
if dbRequests == nil {
return nil
}
// Ensure channel is initialized
initDBRequests()
errCh := make(chan error)
dbRequests <- dbRequest{releasedb, errCh}
+3 -11
View File
@@ -86,25 +86,17 @@ func Router(c *ctx.AptlyContext) http.Handler {
// We use a goroutine to count the number of
// concurrent requests. When no more requests are
// running, we close the database to free the lock.
dbRequests = make(chan dbRequest)
go acquireDatabase()
initDBRequests()
api.Use(func(c *gin.Context) {
var err error
errCh := make(chan error)
dbRequests <- dbRequest{acquiredb, errCh}
err = <-errCh
err := acquireDatabaseConnection()
if err != nil {
AbortWithJSONError(c, 500, err)
return
}
defer func() {
dbRequests <- dbRequest{releasedb, errCh}
err = <-errCh
err := releaseDatabaseConnection()
if err != nil {
AbortWithJSONError(c, 500, err)
}