[PR #126] [CLOSED] fix: prevent goroutine leak and data race in terminal exec timeout #175

New Issue

2026-06-06T22:09:33-04:00

yindo commented

2026-06-06 22:09:33 -04:00

📋 Pull Request Information

Original PR: https://github.com/vxcontrol/pentagi/pull/126
Author: @mason5052
Created: 2/22/2026
Status: ❌ Closed

Base: master ← Head: fix/terminal-exec-goroutine-leak

📝 Commits (1)

2c6b7a6 fix: prevent goroutine leak and data race in terminal exec timeout

📊 Changes

1 file changed (+11 additions, -1 deletions)

View changed files

📝 backend/pkg/tools/terminal.go (+11 -1)

📄 Description

Description of the Change

Problem

In getExecResult() (terminal.go), when command execution times out, three issues occur:

Goroutine leak: The goroutine running io.Copy(&dst, resp.Reader) remains blocked on the reader indefinitely. While defer resp.Close() eventually runs, there is a window where the goroutine is still blocked between the timeout and function return. Under sustained load with many timeouts, this leads to goroutine accumulation and memory bloat.
Data race on buffer: On timeout, dst.String() is called (line 223) while the goroutine may still be writing to dst via io.Copy — a concurrent read/write on bytes.Buffer.
Data race on err variable: The goroutine writes to the shared err variable (line 216), while the main goroutine also writes to err in the timeout case (line 224) — a classic data race.

Solution

Close resp immediately on timeout to unblock io.Copy — prevents goroutine leak
Wait for goroutine to finish (with 5s safety timeout) before reading dst.String() — eliminates buffer data race
Use separate copyErr variable for the goroutine, assigned to err only after goroutine completes — eliminates err variable data race

The existing defer resp.Close() serves as a safety net for the normal (non-timeout) path. Double-close on HijackedResponse is safe.

Closes #108

Type of Change

🐛 Bug fix (non-breaking change which fixes an issue)
🛡️ Security update

Areas Affected

Security Tools Integration

Testing and Verification

Test Steps

Build the backend: cd backend && go build ./...
Run a long-running command in a terminal container with a short timeout
Monitor goroutine count before and after timeout (e.g., via pprof or runtime.NumGoroutine())
Verify goroutine count returns to baseline after timeout

Security Considerations

Goroutine leaks can lead to resource exhaustion (memory, file descriptors) during sustained penetration testing operations. Fixing this prevents potential DoS conditions when many commands time out simultaneously.

Performance Impact

Positive impact: eliminates goroutine leaks that would otherwise accumulate during long-running pentest sessions with command timeouts.

Checklist

Code Quality

My code follows the project's coding standards
All new and existing tests pass
I have run go fmt and go vet (for Go code)

Security

I have considered security implications
Changes maintain or improve the security model

Compatibility

Changes are backward compatible

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/vxcontrol/pentagi/pull/126 **Author:** [@mason5052](https://github.com/mason5052) **Created:** 2/22/2026 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix/terminal-exec-goroutine-leak` --- ### 📝 Commits (1) - [`2c6b7a6`](https://github.com/vxcontrol/pentagi/commit/2c6b7a668990af6d7a080367d8cf2f0c67c86e19) fix: prevent goroutine leak and data race in terminal exec timeout ### 📊 Changes **1 file changed** (+11 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `backend/pkg/tools/terminal.go` (+11 -1) </details> ### 📄 Description ### Description of the Change #### Problem In `getExecResult()` (`terminal.go`), when command execution times out, three issues occur: 1. **Goroutine leak**: The goroutine running `io.Copy(&dst, resp.Reader)` remains blocked on the reader indefinitely. While `defer resp.Close()` eventually runs, there is a window where the goroutine is still blocked between the timeout and function return. Under sustained load with many timeouts, this leads to goroutine accumulation and memory bloat. 2. **Data race on buffer**: On timeout, `dst.String()` is called (line 223) while the goroutine may still be writing to `dst` via `io.Copy` — a concurrent read/write on `bytes.Buffer`. 3. **Data race on err variable**: The goroutine writes to the shared `err` variable (line 216), while the main goroutine also writes to `err` in the timeout case (line 224) — a classic data race. #### Solution - **Close `resp` immediately** on timeout to unblock `io.Copy` — prevents goroutine leak - **Wait for goroutine to finish** (with 5s safety timeout) before reading `dst.String()` — eliminates buffer data race - **Use separate `copyErr` variable** for the goroutine, assigned to `err` only after goroutine completes — eliminates err variable data race The existing `defer resp.Close()` serves as a safety net for the normal (non-timeout) path. Double-close on `HijackedResponse` is safe. Closes #108 ### Type of Change - [x] 🐛 Bug fix (non-breaking change which fixes an issue) - [x] 🛡️ Security update ### Areas Affected - [x] Security Tools Integration ### Testing and Verification #### Test Steps 1. Build the backend: `cd backend && go build ./...` 2. Run a long-running command in a terminal container with a short timeout 3. Monitor goroutine count before and after timeout (e.g., via pprof or `runtime.NumGoroutine()`) 4. Verify goroutine count returns to baseline after timeout ### Security Considerations Goroutine leaks can lead to resource exhaustion (memory, file descriptors) during sustained penetration testing operations. Fixing this prevents potential DoS conditions when many commands time out simultaneously. ### Performance Impact Positive impact: eliminates goroutine leaks that would otherwise accumulate during long-running pentest sessions with command timeouts. ### Checklist #### Code Quality - [x] My code follows the project's coding standards - [x] All new and existing tests pass - [x] I have run `go fmt` and `go vet` (for Go code) #### Security - [x] I have considered security implications - [x] Changes maintain or improve the security model #### Compatibility - [x] Changes are backward compatible --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

yindo added the pull-request label 2026-06-06 22:09:33 -04:00

yindo closed this issue

2026-06-06 22:09:34 -04:00

yindo referenced this issue

2026-06-06 22:09:44 -04:00

[PR #178] [MERGED] fix: prevent infinite loop in performAgentChain on repeating tool calls #213

yindo referenced this issue

2026-06-06 22:09:44 -04:00

[PR #180] [MERGED] test: add unit tests for repeatingDetector and clearCallArguments #217

yindo referenced this issue

2026-06-06 22:09:46 -04:00

[PR #194] [CLOSED] fix(providers): add iteration cap to performAgentChain to prevent infinite loops #223