[codex] Fix process list count limit#1865
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f79f0897
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Safety cap: head -n 2000 prevents maxBuffer/timeout on process-dense hosts. | ||
| // This is NOT a functional limit — monitored processes still show accurate metrics. | ||
| "ps -eo pid= -o ppid= -o user= -o stat= -o pcpu= -o pmem= -o rss= -o vsz= -o etime= -o args= 2>/dev/null | head -n 2000", | ||
| "ps -eo pid= -o ppid= -o user= -o stat= -o pcpu= -o pmem= -o rss= -o vsz= -o etime= -o args= 2>/dev/null", |
There was a problem hiding this comment.
When this runs for a local Linux/macOS session with enough processes or long command lines, removing the row cap lets ps output exceed execOnLocalMachine's 10 MiB execFile maxBuffer in electron/bridges/systemManager/execOnSession.cjs; that path resolves as success if any stdout was captured, so parseProcessLines can silently show only the truncated prefix rather than all processes. ET sessions have an even smaller default execFile buffer, so this change can also turn large process lists into load failures. The cap removal should be paired with streaming/parsing or an explicit truncation/error path so dense hosts are not still capped by the transport buffer.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6346754ad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await execOnSession(event, sessionId, PROCESS_LIST_SCRIPT_POSIX, 12000, { | ||
| maxBuffer: PROCESS_LIST_MAX_BUFFER, | ||
| }); |
There was a problem hiding this comment.
For normal SSH sessions, this maxBuffer option is never enforced: execOnSession reaches execOnConnection, whose stream handler just appends every stdout chunk and ignores execOptions.maxBuffer (only local execFile and ET honor it). After removing the head -n 2000 cap, opening the Processes tab against a host with very many processes or long command lines can stream unbounded data into the Electron main process and spike memory or hang instead of failing cleanly at the intended 64 MiB limit.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5f2bdd7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await execOnLocalMachine( | ||
| "Get-CimInstance Win32_Process | Sort-Object KernelModeTime -Descending | Select-Object -First 2000 ProcessId,ParentProcessId,Name,WorkingSetSize | ConvertTo-Json -Compress", | ||
| "Get-CimInstance Win32_Process | Sort-Object KernelModeTime -Descending | Select-Object ProcessId,ParentProcessId,Name,WorkingSetSize | ConvertTo-Json -Compress", | ||
| 10000, | ||
| ); |
There was a problem hiding this comment.
On local Windows sessions this command now removes the -First 2000 cap, but the execOnLocalMachine call still omits maxBuffer, so it falls back to the 10 MiB default in execOnSession.cjs. On process-dense Windows hosts where the full JSON exceeds that default, listProcesses will now fail with a maxBuffer error instead of showing the uncapped list; this path should use the same larger process-list buffer as the POSIX path.
Useful? React with 👍 / 👎.
| maxBuffer: Number.isFinite(Number(execOpts.maxBuffer)) && Number(execOpts.maxBuffer) > 0 | ||
| ? Math.floor(Number(execOpts.maxBuffer)) | ||
| : undefined, |
There was a problem hiding this comment.
When execOpts.maxBuffer is omitted, this now passes maxBuffer: undefined to child_process.execFile; in Node this is not the same as omitting the option and leaves ET execs without the default output cap. ET operations that do not supply a maxBuffer, such as capability probes or tmux/docker commands, can therefore buffer unbounded stdout/stderr until the timeout if the remote command is noisy; only include this property for valid overrides or set an explicit default.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Fixes #1862.
Validation
node --test electron/bridges/systemManagerBridge.processes.test.cjsnpm run lintnpm test