Skip to content

Commit 6e45353

Browse files
starbopsclaude
starbops
and
claude
committed
fix: address PR review comments and resolve lint issues
- Fix container registration to use actual container ID after creation - Implement updateExecutionStatus method with proper repository access - Add proper cancellation logic in Executor.Cancel method - Fix code formatting issues that caused lint CI failure - Add container lifecycle tracking and cleanup manager integration - Ensure proper resource cleanup and container status management Addresses review comments from PR #45: - Container registration with empty ID (task_executor_service.go:87) - No-op status update method (task_executor_service.go:140) - Incomplete cancellation implementation (executor.go:256) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 24fba6a commit 6e45353

File tree

11 files changed

+127
-102
lines changed

11 files changed

+127
-102
lines changed

‎internal/executor/cleanup.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (cm *CleanupManager) RegisterContainer(containerID string, taskID, executio
6161
Image: image,
6262
}
6363

64-
cm.logger.Debug("registered container for tracking",
64+
cm.logger.Debug("registered container for tracking",
6565
"container_id", containerID[:12],
6666
"task_id", taskID.String(),
6767
"execution_id", executionID.String())
@@ -87,7 +87,7 @@ func (cm *CleanupManager) MarkContainerCompleted(containerID string, status stri
8787

8888
if info, exists := cm.containers[containerID]; exists {
8989
info.Status = status
90-
cm.logger.Debug("marked container as completed",
90+
cm.logger.Debug("marked container as completed",
9191
"container_id", containerID[:12],
9292
"status", status)
9393
}
@@ -152,15 +152,15 @@ func (cm *CleanupManager) CleanupExecution(ctx context.Context, executionID uuid
152152
return nil
153153
}
154154

155-
cm.logger.Info("cleaning up containers for execution",
155+
cm.logger.Info("cleaning up containers for execution",
156156
"execution_id", executionID.String(),
157157
"container_count", len(containersToCleanup))
158158

159159
var lastErr error
160160
for _, containerID := range containersToCleanup {
161161
if err := cm.CleanupContainer(ctx, containerID, true); err != nil {
162162
lastErr = err
163-
cm.logger.Error("failed to cleanup container in execution cleanup",
163+
cm.logger.Error("failed to cleanup container in execution cleanup",
164164
"container_id", containerID[:12],
165165
"execution_id", executionID.String(),
166166
"error", err)
@@ -186,15 +186,15 @@ func (cm *CleanupManager) CleanupTask(ctx context.Context, taskID uuid.UUID) err
186186
return nil
187187
}
188188

189-
cm.logger.Info("cleaning up containers for task",
189+
cm.logger.Info("cleaning up containers for task",
190190
"task_id", taskID.String(),
191191
"container_count", len(containersToCleanup))
192192

193193
var lastErr error
194194
for _, containerID := range containersToCleanup {
195195
if err := cm.CleanupContainer(ctx, containerID, true); err != nil {
196196
lastErr = err
197-
cm.logger.Error("failed to cleanup container in task cleanup",
197+
cm.logger.Error("failed to cleanup container in task cleanup",
198198
"container_id", containerID[:12],
199199
"task_id", taskID.String(),
200200
"error", err)
@@ -222,7 +222,7 @@ func (cm *CleanupManager) CleanupStaleContainers(ctx context.Context, maxAge tim
222222
return nil
223223
}
224224

225-
cm.logger.Info("cleaning up stale containers",
225+
cm.logger.Info("cleaning up stale containers",
226226
"count", len(staleContainers),
227227
"max_age", maxAge.String())
228228

@@ -336,7 +336,7 @@ func (cm *CleanupManager) startPeriodicCleanup() {
336336

337337
for range ticker.C {
338338
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
339-
339+
340340
// Cleanup containers older than 1 hour
341341
if err := cm.CleanupStaleContainers(ctx, 1*time.Hour); err != nil {
342342
cm.logger.Error("periodic stale container cleanup failed", "error", err)
@@ -380,7 +380,7 @@ func (cm *CleanupManager) ForceCleanupOrphanedContainers(ctx context.Context) er
380380
if len(name) > 0 && name[0] == '/' {
381381
name = name[1:] // Remove leading slash
382382
}
383-
383+
384384
if len(name) > 10 && name[:10] == "voidrunner" {
385385
// Check if we're tracking this container
386386
cm.mu.RLock()
@@ -406,11 +406,11 @@ func (cm *CleanupManager) ForceCleanupOrphanedContainers(ctx context.Context) er
406406
for _, containerID := range orphanedContainers {
407407
if err := cm.CleanupContainer(ctx, containerID, true); err != nil {
408408
lastErr = err
409-
cm.logger.Error("failed to cleanup orphaned container",
409+
cm.logger.Error("failed to cleanup orphaned container",
410410
"container_id", containerID[:12],
411411
"error", err)
412412
}
413413
}
414414

415415
return lastErr
416-
}
416+
}

‎internal/executor/config.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ func NewDefaultConfig() *Config {
6666
DockerEndpoint: "unix:///var/run/docker.sock",
6767
DefaultResourceLimits: ResourceLimits{
6868
MemoryLimitBytes: 128 * 1024 * 1024, // 128MB
69-
CPUQuota: 50000, // 0.5 CPU cores
70-
PidsLimit: 128, // Max 128 processes
71-
TimeoutSeconds: 300, // 5 minutes
69+
CPUQuota: 50000, // 0.5 CPU cores
70+
PidsLimit: 128, // Max 128 processes
71+
TimeoutSeconds: 300, // 5 minutes
7272
},
7373
DefaultTimeoutSeconds: 300,
7474
Images: ImageConfig{
@@ -162,13 +162,13 @@ func (c *Config) GetSecurityConfigForTask(task *models.Task) SecurityConfig {
162162
}
163163

164164
return SecurityConfig{
165-
User: c.Security.ExecutionUser,
166-
NoNewPrivileges: true,
167-
ReadOnlyRootfs: true,
168-
NetworkDisabled: true,
169-
SecurityOpts: securityOpts,
165+
User: c.Security.ExecutionUser,
166+
NoNewPrivileges: true,
167+
ReadOnlyRootfs: true,
168+
NetworkDisabled: true,
169+
SecurityOpts: securityOpts,
170170
TmpfsMounts: map[string]string{
171-
"/tmp": "rw,noexec,nosuid,size=100m",
171+
"/tmp": "rw,noexec,nosuid,size=100m",
172172
"/var/tmp": "rw,noexec,nosuid,size=10m",
173173
},
174174
DropAllCapabilities: true,
@@ -210,4 +210,4 @@ func (c *Config) Validate() error {
210210
}
211211

212212
return nil
213-
}
213+
}

‎internal/executor/config_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,11 @@ func TestConfig_GetResourceLimitsForTask(t *testing.T) {
7171
config := NewDefaultConfig()
7272

7373
tests := []struct {
74-
name string
75-
task *models.Task
76-
expectedMemory int64
77-
expectedCPU int64
78-
expectedTimeout int
74+
name string
75+
task *models.Task
76+
expectedMemory int64
77+
expectedCPU int64
78+
expectedTimeout int
7979
}{
8080
{
8181
name: "Low priority task",
@@ -84,8 +84,8 @@ func TestConfig_GetResourceLimitsForTask(t *testing.T) {
8484
TimeoutSeconds: 0, // Use default
8585
},
8686
expectedMemory: 64 * 1024 * 1024, // Half of default
87-
expectedCPU: 25000, // Half of default
88-
expectedTimeout: 300, // Default
87+
expectedCPU: 25000, // Half of default
88+
expectedTimeout: 300, // Default
8989
},
9090
{
9191
name: "Normal priority task",
@@ -94,8 +94,8 @@ func TestConfig_GetResourceLimitsForTask(t *testing.T) {
9494
TimeoutSeconds: 600,
9595
},
9696
expectedMemory: 128 * 1024 * 1024, // Default
97-
expectedCPU: 50000, // Default
98-
expectedTimeout: 600, // Custom
97+
expectedCPU: 50000, // Default
98+
expectedTimeout: 600, // Custom
9999
},
100100
{
101101
name: "High priority task",
@@ -104,8 +104,8 @@ func TestConfig_GetResourceLimitsForTask(t *testing.T) {
104104
TimeoutSeconds: 0,
105105
},
106106
expectedMemory: 256 * 1024 * 1024, // Double default
107-
expectedCPU: 100000, // Double default
108-
expectedTimeout: 300, // Default
107+
expectedCPU: 100000, // Double default
108+
expectedTimeout: 300, // Default
109109
},
110110
{
111111
name: "Critical priority task",
@@ -114,8 +114,8 @@ func TestConfig_GetResourceLimitsForTask(t *testing.T) {
114114
TimeoutSeconds: 1800,
115115
},
116116
expectedMemory: 512 * 1024 * 1024, // Quadruple default
117-
expectedCPU: 100000, // Double default (capped)
118-
expectedTimeout: 1800, // Custom
117+
expectedCPU: 100000, // Double default (capped)
118+
expectedTimeout: 1800, // Custom
119119
},
120120
}
121121

@@ -313,4 +313,4 @@ func TestConfig_Validate(t *testing.T) {
313313
}
314314
})
315315
}
316-
}
316+
}

‎internal/executor/docker_client.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,4 +394,4 @@ func (dc *DockerClient) GetDockerVersion(ctx context.Context) (interface{}, erro
394394
}
395395

396396
return version, nil
397-
}
397+
}

‎internal/executor/errors.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,4 +186,4 @@ func IsSecurityError(err error) bool {
186186
func IsConfigError(err error) bool {
187187
var confErr *ConfigError
188188
return errors.As(err, &confErr)
189-
}
189+
}

‎internal/executor/errors_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ func TestErrorTypeCheckers(t *testing.T) {
213213

214214
func TestErrorWrapping(t *testing.T) {
215215
baseErr := errors.New("base error")
216-
216+
217217
t.Run("ExecutorError wrapping", func(t *testing.T) {
218218
wrappedErr := NewExecutorError("test", "wrapped error", baseErr)
219219
assert.True(t, errors.Is(wrappedErr, baseErr))
@@ -228,4 +228,4 @@ func TestErrorWrapping(t *testing.T) {
228228
wrappedErr := NewSecurityError("test", "wrapped error", baseErr)
229229
assert.True(t, errors.Is(wrappedErr, baseErr))
230230
})
231-
}
231+
}

0 commit comments

Comments
 (0)