[GH-ISSUE #108] [Bug] Goroutine and Resource Leak in Terminal Exec Timeout Handling #55

New Issue

Closed

opened 2026-06-06 22:08:50 -04:00 by yindo · 5 comments

yindo commented 2026-06-06 22:08:50 -04:00

Owner

Copy Link

Copy Source

Originally created by @zesty-clawd on GitHub (Feb 21, 2026).
Original GitHub issue: https://github.com/vxcontrol/pentagi/issues/108

Goroutine and Resource Leak in Terminal Tool Exec Timeout Handling

Summary

The getExecResult method in backend/pkg/tools/terminal.go contains a goroutine leak when command execution times out. When the context expires, the function returns early but leaves a goroutine blocked on io.Copy, which continues reading from resp.Reader indefinitely.

Location

File: backend/pkg/tools/terminal.go
Method: func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error)
Lines: Approximately 220-260

The Bug

func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error) {
    ctx, cancel := context.WithTimeout(ctx, timeout)
    defer cancel()

    resp, err := t.dockerClient.ContainerExecAttach(ctx, id, container.ExecAttachOptions{
        Tty: true,
    })
    if err != nil {
        return "", fmt.Errorf("failed to attach to exec process: %w", err)
    }
    defer resp.Close()

    dst := bytes.Buffer{}
    done := make(chan struct{})
    go func() {
        _, err = io.Copy(&dst, resp.Reader)  // ← This goroutine can leak
        close(done)
    }()

    select {
    case <-done:
    case <-ctx.Done():  // ← When this happens, we return but goroutine keeps running
        result := fmt.Sprintf("temporary output: %s", dst.String())
        err = fmt.Errorf("timeout value is too low, use greater value if you need so: %w: %s", ctx.Err(), result)
    }
    // ...
    return results, nil
}

Problem Analysis

When ctx.Done() is triggered (timeout), the function:

1. Returns immediately with a timeout error
2. Executes defer resp.Close() which closes the connection
3. BUT the goroutine running io.Copy(&dst, resp.Reader) may still be blocked on the read

Why This Is Problematic

1. Goroutine Leak: The goroutine never terminates because:

   • io.Copy may be blocked waiting for data
   • Even after resp.Close(), the read might not immediately unblock depending on the underlying implementation
   • The goroutine will remain in memory until the process terminates

2. Resource Accumulation: In a long-running PentAGI system executing many commands:

   • Each timeout creates an orphaned goroutine
   • Accumulation of these goroutines leads to memory bloat
   • System performance degrades over time

3. Unpredictable Behavior:

   • The leaked goroutine may complete later and write to closed channels or buffers
   • Error handling becomes unreliable since errors from the goroutine are discarded

Impact

• Severity: Medium to High
• Affected Scenarios:
  • Commands that frequently timeout
  • Long-running penetration testing tasks with many tool invocations
  • Systems with strict resource limits
• Observable Symptoms:
  • Gradual memory increase over time
  • Growing number of goroutines (visible via runtime.NumGoroutine())
  • Potential Docker connection pool exhaustion

Reproduction Steps

1. Execute a command that will timeout:

./pentagi terminal execute --command "sleep 600" --timeout 5

2. Monitor goroutine count before and after multiple timeout events:

fmt.Printf("Goroutines before: %d\n", runtime.NumGoroutine())
// Execute multiple commands with timeouts
fmt.Printf("Goroutines after: %d\n", runtime.NumGoroutine())

3. Observe that goroutine count increases with each timeout

Recommended Fix

Use a context-aware copy operation or ensure the goroutine can be interrupted:

Option 1: Context-Aware Copy (Preferred)

func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error) {
    ctx, cancel := context.WithTimeout(ctx, timeout)
    defer cancel()

    resp, err := t.dockerClient.ContainerExecAttach(ctx, id, container.ExecAttachOptions{
        Tty: true,
    })
    if err != nil {
        return "", fmt.Errorf("failed to attach to exec process: %w", err)
    }
    defer resp.Close()

    dst := bytes.Buffer{}
    errChan := make(chan error, 1)
    
    go func() {
        // Use context-aware copy
        _, err := io.Copy(&dst, resp.Reader)
        errChan <- err
    }()

    select {
    case err := <-errChan:
        if err != nil && err != io.EOF {
            return "", fmt.Errorf("failed to copy output: %w", err)
        }
    case <-ctx.Done():
        // Close the connection to interrupt the io.Copy
        resp.Close()
        
        // Wait a short time for goroutine to exit gracefully
        select {
        case <-errChan:
            // Goroutine completed
        case <-time.After(100 * time.Millisecond):
            // Goroutine still blocked, but we've done our best
        }
        
        result := fmt.Sprintf("temporary output: %s", dst.String())
        return "", fmt.Errorf("timeout value is too low, use greater value if you need so: %w: %s", ctx.Err(), result)
    }

    // Continue with rest of function...
}

Option 2: Use io.CopyN or Buffered Reading

Alternatively, use buffered reads with context checking in a loop instead of io.Copy, allowing interruption between reads.

Additional Notes

• This pattern appears in other similar codebases and is a common mistake when dealing with context cancellation and I/O operations
• Consider adding goroutine leak detection in tests using tools like goleak
• The fix should ensure backward compatibility with existing timeout behavior

Environment

• Project: PentAGI
• Component: Terminal Tool Executor
• Language: Go
• Docker SDK: github.com/docker/docker

---

This bug represents a fundamental issue with resource management in concurrent systems and should be addressed to ensure system stability during extended penetration testing operations.

Originally created by @zesty-clawd on GitHub (Feb 21, 2026). Original GitHub issue: https://github.com/vxcontrol/pentagi/issues/108 # Goroutine and Resource Leak in Terminal Tool Exec Timeout Handling ## Summary The `getExecResult` method in `backend/pkg/tools/terminal.go` contains a goroutine leak when command execution times out. When the context expires, the function returns early but leaves a goroutine blocked on `io.Copy`, which continues reading from `resp.Reader` indefinitely. ## Location **File:** `backend/pkg/tools/terminal.go` **Method:** `func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error)` **Lines:** Approximately 220-260 ## The Bug ```go func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error) { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() resp, err := t.dockerClient.ContainerExecAttach(ctx, id, container.ExecAttachOptions{ Tty: true, }) if err != nil { return "", fmt.Errorf("failed to attach to exec process: %w", err) } defer resp.Close() dst := bytes.Buffer{} done := make(chan struct{}) go func() { _, err = io.Copy(&dst, resp.Reader) // ← This goroutine can leak close(done) }() select { case <-done: case <-ctx.Done(): // ← When this happens, we return but goroutine keeps running result := fmt.Sprintf("temporary output: %s", dst.String()) err = fmt.Errorf("timeout value is too low, use greater value if you need so: %w: %s", ctx.Err(), result) } // ... return results, nil } ``` ## Problem Analysis When `ctx.Done()` is triggered (timeout), the function: 1. Returns immediately with a timeout error 2. Executes `defer resp.Close()` which closes the connection 3. **BUT** the goroutine running `io.Copy(&dst, resp.Reader)` may still be blocked on the read ### Why This Is Problematic 1. **Goroutine Leak**: The goroutine never terminates because: - `io.Copy` may be blocked waiting for data - Even after `resp.Close()`, the read might not immediately unblock depending on the underlying implementation - The goroutine will remain in memory until the process terminates 2. **Resource Accumulation**: In a long-running PentAGI system executing many commands: - Each timeout creates an orphaned goroutine - Accumulation of these goroutines leads to memory bloat - System performance degrades over time 3. **Unpredictable Behavior**: - The leaked goroutine may complete later and write to closed channels or buffers - Error handling becomes unreliable since errors from the goroutine are discarded ## Impact - **Severity**: Medium to High - **Affected Scenarios**: - Commands that frequently timeout - Long-running penetration testing tasks with many tool invocations - Systems with strict resource limits - **Observable Symptoms**: - Gradual memory increase over time - Growing number of goroutines (visible via `runtime.NumGoroutine()`) - Potential Docker connection pool exhaustion ## Reproduction Steps 1. Execute a command that will timeout: ```bash ./pentagi terminal execute --command "sleep 600" --timeout 5 ``` 2. Monitor goroutine count before and after multiple timeout events: ```go fmt.Printf("Goroutines before: %d\n", runtime.NumGoroutine()) // Execute multiple commands with timeouts fmt.Printf("Goroutines after: %d\n", runtime.NumGoroutine()) ``` 3. Observe that goroutine count increases with each timeout ## Recommended Fix Use a context-aware copy operation or ensure the goroutine can be interrupted: ### Option 1: Context-Aware Copy (Preferred) ```go func (t *terminal) getExecResult(ctx context.Context, id string, timeout time.Duration) (string, error) { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() resp, err := t.dockerClient.ContainerExecAttach(ctx, id, container.ExecAttachOptions{ Tty: true, }) if err != nil { return "", fmt.Errorf("failed to attach to exec process: %w", err) } defer resp.Close() dst := bytes.Buffer{} errChan := make(chan error, 1) go func() { // Use context-aware copy _, err := io.Copy(&dst, resp.Reader) errChan <- err }() select { case err := <-errChan: if err != nil && err != io.EOF { return "", fmt.Errorf("failed to copy output: %w", err) } case <-ctx.Done(): // Close the connection to interrupt the io.Copy resp.Close() // Wait a short time for goroutine to exit gracefully select { case <-errChan: // Goroutine completed case <-time.After(100 * time.Millisecond): // Goroutine still blocked, but we've done our best } result := fmt.Sprintf("temporary output: %s", dst.String()) return "", fmt.Errorf("timeout value is too low, use greater value if you need so: %w: %s", ctx.Err(), result) } // Continue with rest of function... } ``` ### Option 2: Use io.CopyN or Buffered Reading Alternatively, use buffered reads with context checking in a loop instead of `io.Copy`, allowing interruption between reads. ## Additional Notes - This pattern appears in other similar codebases and is a common mistake when dealing with context cancellation and I/O operations - Consider adding goroutine leak detection in tests using tools like `goleak` - The fix should ensure backward compatibility with existing timeout behavior ## Environment - Project: PentAGI - Component: Terminal Tool Executor - Language: Go - Docker SDK: github.com/docker/docker --- This bug represents a fundamental issue with resource management in concurrent systems and should be addressed to ensure system stability during extended penetration testing operations.

yindo closed this issue 2026-06-06 22:08:50 -04:00

yindo commented 2026-06-06 22:08:51 -04:00

Author

Owner

Copy Link

Copy Source

@HassanSystems commented on GitHub (Feb 21, 2026):

Good catch — this is a real goroutine leak.

The root issue is that io.Copy runs in a goroutine that is not guaranteed to exit when the context times out. Returning on ctx.Done() without synchronizing on that goroutine leaves it blocked on resp.Reader.

The fix needs to:

Force-unblock the reader (resp.Close())

Synchronously wait for the copy goroutine to exit

Ensure the goroutine can never block on channel send

Example pattern:

errChan := make(chan error, 1)
go func() {
_, err := io.Copy(&dst, resp.Reader)
select {
case errChan <- err:
default:
}
}()

select {
case err := <-errChan:
// normal completion
case <-ctx.Done():
_ = resp.Close()
<-errChan
return "", ctx.Err()
}

This guarantees deterministic goroutine shutdown and avoids resource leakage under repeated timeouts.

<!-- gh-comment-id:3938551769 --> @HassanSystems commented on GitHub (Feb 21, 2026): Good catch — this is a real goroutine leak. The root issue is that io.Copy runs in a goroutine that is not guaranteed to exit when the context times out. Returning on ctx.Done() without synchronizing on that goroutine leaves it blocked on resp.Reader. The fix needs to: Force-unblock the reader (resp.Close()) Synchronously wait for the copy goroutine to exit Ensure the goroutine can never block on channel send Example pattern: errChan := make(chan error, 1) go func() { _, err := io.Copy(&dst, resp.Reader) select { case errChan <- err: default: } }() select { case err := <-errChan: // normal completion case <-ctx.Done(): _ = resp.Close() <-errChan return "", ctx.Err() } This guarantees deterministic goroutine shutdown and avoids resource leakage under repeated timeouts.

yindo commented 2026-06-06 22:08:51 -04:00

Author

Owner

Copy Link

Copy Source

@ChiragHS-MYS commented on GitHub (Feb 22, 2026):

Assign it to me and I can fix it for you sir

<!-- gh-comment-id:3940189515 --> @ChiragHS-MYS commented on GitHub (Feb 22, 2026): Assign it to me and I can fix it for you sir

yindo commented 2026-06-06 22:08:51 -04:00

Author

Owner

Copy Link

Copy Source

@Muitamax commented on GitHub (Feb 23, 2026):

Advanced hardening options:
Use errgroup.WithContext
Wrap reader in io.LimitedReader
Add goleak.VerifyNone(t) in tests
Add Docker client timeout tuning
Add integration test simulating hung container

<!-- gh-comment-id:3943010560 --> @Muitamax commented on GitHub (Feb 23, 2026): Advanced hardening options: Use errgroup.WithContext Wrap reader in io.LimitedReader Add goleak.VerifyNone(t) in tests Add Docker client timeout tuning Add integration test simulating hung container

yindo commented 2026-06-06 22:08:51 -04:00

Author

Owner

Copy Link

Copy Source

@Vaibhavee89 commented on GitHub (Feb 23, 2026):

Fix Implemented

I've created PR #134 that implements the fix for this goroutine leak issue.

Implementation Details

The fix follows the recommended Option 1 from this issue with the following approach:

Key Changes:

1. Error Channel: Replaced the done channel with a buffered error channel (errChan) that captures the io.Copy result
2. Graceful Shutdown: On timeout, uses a nested select to wait up to defaultExtraExecTimeout (5 seconds) for the goroutine to complete
3. Proper Cleanup: The existing defer resp.Close() ensures the connection is closed, which helps interrupt the blocked io.Copy

Why This Works

Normal Execution:

• Goroutine completes io.Copy and sends error (or nil) to errChan
• Main function receives it and handles appropriately

Timeout Scenario:

• Context expires, triggering ctx.Done() case
• Nested select waits up to 5 seconds for goroutine to finish gracefully
• If goroutine completes in time, we capture its error
• If still blocked after 5 seconds, we continue (but the goroutine will eventually exit when the deferred resp.Close() takes effect)
• Return timeout error with partial output

Advantages Over Issue Recommendation

Our implementation uses defaultExtraExecTimeout (5 seconds) instead of 100ms for the grace period, which:

• Provides more time for Docker I/O operations to complete cleanly
• Reduces the chance of orphaned goroutines in slow environments
• Is consistent with the existing timeout constants in the codebase

The fix prevents the goroutine leak while maintaining backward compatibility with existing timeout behavior.

Please review PR #134 when you have a chance. The CI tests should run automatically to verify the fix doesn't break existing functionality.

<!-- gh-comment-id:3943826802 --> @Vaibhavee89 commented on GitHub (Feb 23, 2026): ## Fix Implemented I've created PR #134 that implements the fix for this goroutine leak issue. ### Implementation Details The fix follows the recommended Option 1 from this issue with the following approach: **Key Changes:** 1. **Error Channel**: Replaced the `done` channel with a buffered error channel (`errChan`) that captures the `io.Copy` result 2. **Graceful Shutdown**: On timeout, uses a nested `select` to wait up to `defaultExtraExecTimeout` (5 seconds) for the goroutine to complete 3. **Proper Cleanup**: The existing `defer resp.Close()` ensures the connection is closed, which helps interrupt the blocked `io.Copy` ### Why This Works **Normal Execution:** - Goroutine completes `io.Copy` and sends error (or nil) to `errChan` - Main function receives it and handles appropriately **Timeout Scenario:** - Context expires, triggering `ctx.Done()` case - Nested `select` waits up to 5 seconds for goroutine to finish gracefully - If goroutine completes in time, we capture its error - If still blocked after 5 seconds, we continue (but the goroutine will eventually exit when the deferred `resp.Close()` takes effect) - Return timeout error with partial output ### Advantages Over Issue Recommendation Our implementation uses `defaultExtraExecTimeout` (5 seconds) instead of 100ms for the grace period, which: - Provides more time for Docker I/O operations to complete cleanly - Reduces the chance of orphaned goroutines in slow environments - Is consistent with the existing timeout constants in the codebase The fix prevents the goroutine leak while maintaining backward compatibility with existing timeout behavior. Please review PR #134 when you have a chance. The CI tests should run automatically to verify the fix doesn't break existing functionality.

yindo commented 2026-06-06 22:08:52 -04:00

Author

Owner

Copy Link

Copy Source

@Muitamax commented on GitHub (Feb 23, 2026):

Fix (PR #134)
The fix uses Option 1: context-aware copy with buffered error channel:
Key points:
Buffered Error Channel

errChan := make(chan error, 1)
go func() {
_, err := io.Copy(&dst, resp.Reader)
errChan <- err // non-blocking send because channel is buffered
}()

Graceful Shutdown on Timeout

select {
case err := <-errChan:
// normal completion
case <-ctx.Done():
resp.Close() // force unblock io.Copy
// wait up to 5 seconds for goroutine to exit
select {
case <-errChan:
case <-time.After(defaultExtraExecTimeout):
}
return "", fmt.Errorf("timeout: %w, partial output: %s", ctx.Err(), dst.String())
}

defaultExtraExecTimeout (5s) is used instead of 100ms to allow Docker I/O to finish cleanly.
Ensures no orphaned goroutines remain.
Proper Cleanup
defer resp.Close() ensures the Docker connection is closed.
errChan ensures goroutine errors are captured safely.

<!-- gh-comment-id:3944063581 --> @Muitamax commented on GitHub (Feb 23, 2026): Fix (PR #134) The fix uses Option 1: context-aware copy with buffered error channel: Key points: Buffered Error Channel errChan := make(chan error, 1) go func() { _, err := io.Copy(&dst, resp.Reader) errChan <- err // non-blocking send because channel is buffered }() Graceful Shutdown on Timeout select { case err := <-errChan: // normal completion case <-ctx.Done(): resp.Close() // force unblock io.Copy // wait up to 5 seconds for goroutine to exit select { case <-errChan: case <-time.After(defaultExtraExecTimeout): } return "", fmt.Errorf("timeout: %w, partial output: %s", ctx.Err(), dst.String()) } defaultExtraExecTimeout (5s) is used instead of 100ms to allow Docker I/O to finish cleanly. Ensures no orphaned goroutines remain. Proper Cleanup defer resp.Close() ensures the Docker connection is closed. errChan ensures goroutine errors are captured safely.

yindo referenced this issue 2026-06-06 22:09:22 -04:00

[PR #55] [MERGED] chore(deps): bump axios from 1.9.0 to 1.12.0 in /frontend in the npm_and_yarn group across 1 directory #139

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feature/frontend

ui/stage-1-non-flows

fix/sandbox-port-collision

feature/next-release

fix/flow-terminal-actions-db-fallback-signed

integrate/open-prs-v2

integrate/open-prs

dependabot/go_modules/backend/go_modules-356acce74b

fix/ram-consumption

dependabot/npm_and_yarn/frontend/npm_and_yarn-bed0206944

v2.1.0

v2.0.0

v1.2.0

v1.1.0

v1.0.1

v1.0.0

v0.3.0

v0.2.0

v0.1.0

Labels

Clear labels

bug

enhancement

help wanted

pull-request

Mirrored from GitHub Pull Request

question

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

yindo

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vxcontrol/pentagi#55