fix: prevent premature process termination and optimize sandbox timeouts #111
Conversation
… support Critical bug fix: - Fixed bug where processes were killed after 60s stream timeout - Processes now correctly continue running in background when stream times out - Only kill processes on user abort or 10min max execution timeout New feature - tail --pid support: - Added support for 'tail --pid=<pid> -f /dev/null' to wait for long-running processes - Extended timeout to 10 minutes for tail --pid commands (vs 60s for regular commands) - Removed custom wait_pid script in favor of standard Unix utility - Added user-friendly messages: 'Waiting for process X to complete...' Improvements: - Enhanced timeout messages to include PID when available - Updated handler.getResult() to accept optional PID parameter - Added command chaining and pipes best practices to prompt - Added guideline to never run inline code (python3 -c, node -e) - Clarified that file-based execution is required This fixes the workflow where AI can now: 1. Start long scan in background (or let it timeout) 2. Use tail --pid to wait for completion (up to 10min) 3. Retrieve results without premature termination
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds PID-aware handling for "tail --pid" wait commands, tightens max command execution time, propagates processId through handler.getResult/timeouts/abort paths, updates TIMEOUT_MESSAGE signature, defaults S3 upload MIME to application/octet-stream when missing, and removes a success log/assert for S3 cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Caller
participant Runner as run-terminal-cmd
participant Exec as TerminalExecutor
participant Handler as createTerminalHandler
User->>Runner: execute(command)
alt command matches "tail --pid <N>"
Runner->>Exec: start stream (streamTimeout = MAX_COMMAND_EXECUTION_TIME)
note right of Exec `#DDEEFF`: extract waiting PID N and emit "Waiting for process N..."
Exec->>Handler: stream events...
par process ends or timeout
Exec-->>Handler: on process exit -> emit "Process N completed"
Handler-->>User: getResult(processId=N) -> final result
and timeout handling
alt short background continuation
Handler-->>User: getResult(processId=N) with background TIMEOUT_MESSAGE(N)
else long MAX timeout
Handler-->>User: getResult(processId=N) with external termination message
end
end
else non-tail command
Runner->>Exec: start stream (streamTimeout = SHORT_STREAM_TIMEOUT)
Exec-->>Handler: normal completion or timeout (may discover pid)
Handler-->>User: getResult(pid if known)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/ai/tools/run-terminal-cmd.ts (1)
201-218: Consider edge cases in tail --pid detection.Minor edge case: Line 213 uses
parseIntwithout validation. If the regex somehow captures non-numeric content,waitingForPidcould beNaN. While the regex\d+should only match digits, consider adding a validation:if (pidMatch && !isNaN(parseInt(pidMatch[1], 10))).Inconsistency with full paths: Line 203 uses
startsWith("tail --pid"), which won't detect/usr/bin/tail --pid=123. However, line 211's regex would extract the PID from such commands. This could lead to:
- Using
STREAM_TIMEOUT_SECONDSinstead ofMAX_COMMAND_EXECUTION_TIME- Not printing the "Waiting for process..." message
If users might use full paths, consider:
command.trim().match(/(?:^|\/)tail\s+--pid/)for detection.-const isWaitCommand = command.trim().startsWith("tail --pid"); +const isWaitCommand = /(?:^|\/)tail\s+--pid/.test(command.trim());And add NaN check:
const pidMatch = command.match(/tail\s+--pid[=\s]+(\d+)/); if (pidMatch) { - waitingForPid = parseInt(pidMatch[1], 10); + const parsed = parseInt(pidMatch[1], 10); + if (!isNaN(parsed)) { + waitingForPid = parsed; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/ai/tools/run-terminal-cmd.ts(6 hunks)lib/api/chat-handler.ts(1 hunks)lib/token-utils.ts(1 hunks)lib/utils/terminal-executor.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/ai/tools/run-terminal-cmd.ts (3)
lib/utils/terminal-executor.ts (1)
createTerminalHandler(21-102)lib/ai/tools/utils/pid-discovery.ts (1)
findProcessPid(11-80)lib/token-utils.ts (1)
TIMEOUT_MESSAGE(31-34)
lib/utils/terminal-executor.ts (1)
lib/token-utils.ts (1)
TIMEOUT_MESSAGE(31-34)
🔇 Additional comments (5)
lib/token-utils.ts (1)
31-34: LGTM! Clean backward-compatible enhancement.The optional
pidparameter is properly typed, the ternary logic is clear, and including the PID in timeout messages when available significantly improves debuggability for users tracking long-running processes.lib/utils/terminal-executor.ts (1)
81-84: LGTM! Proper integration with updated TIMEOUT_MESSAGE.The optional
pidparameter is correctly threaded through toTIMEOUT_MESSAGE, the fallbacktimeoutSeconds || 0prevents undefined values, and backward compatibility is maintained for callers that don't provide a PID.lib/ai/tools/run-terminal-cmd.ts (3)
378-383: LGTM! Clear user feedback for process completion.The completion message provides helpful feedback when a
tail --pidwait command finishes successfully. The placement after cleanup and before resolution is correct, and the message clearly indicates which process completed.
241-255: No actionable changes required—the assumption is verified as correct.E2B automatically enforces timeoutMs: the sandbox and running processes are killed when timeoutMs expires. This confirms that line 252's comment about e2b killing the process at
MAX_COMMAND_EXECUTION_TIMEis accurate, and the timeout logic at lines 241-244 is sound. The code correctly relies on e2b's automatic termination behavior.
25-45:tail --pidis fully supported—no changes needed.Verification confirms the sandbox environment has GNU coreutils 9.1, which supports the
--pidflag (available since version 8.28, released in 2017). Direct testing shows the flag is recognized without errors. The documented guidance to usetail --pid=<pid> -f /dev/nullfor waiting on background processes is correct and will function properly in the sandbox environment.
| // Current ask model doesn't support sequential tool calls | ||
| // ...(mode === "ask" && { parallel_tool_calls: false }), |
There was a problem hiding this comment.
Clarify the comment or verify the intended behavior.
The comment states the model "doesn't support sequential tool calls," but the removed code was setting parallel_tool_calls: false, which disables parallel execution and forces sequential tool calls. This appears contradictory.
If the model doesn't support sequential calls, the code removal is correct, but the comment should say "doesn't support parallel tool calls" (or better yet, "now supports parallel tool calls"). Alternatively, if the comment is correct as-is, please clarify the reasoning.
🤖 Prompt for AI Agents
In lib/api/chat-handler.ts around lines 341-342, the inline comment contradicts
the removed code: it says the model "doesn't support sequential tool calls"
while the removed line set parallel_tool_calls: false (which forces sequential
calls). Verify the model's actual capability against documentation or tests; if
the model cannot perform sequential tool calls, restore the conditional setting
(mode === "ask" && { parallel_tool_calls: false }) so behavior matches the
comment; otherwise, update the comment to correctly state that the model doesn't
support parallel tool calls (or that it now supports parallel calls) to reflect
the actual behavior.
- Add fallback to 'application/octet-stream' when file.type is empty - Prevents 'Invalid contentType: contentType cannot be empty' error - Occurs when browser cannot determine MIME type for certain files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/hooks/useFileUpload.ts (1)
139-149: Robust content-type fallback; consider factoring into a local constantThe fallback to
"application/octet-stream"whenfile.typeis falsy in both the presign call and the PUT headers is a solid robustness improvement and keeps those two call sites in sync.To avoid future drift and slightly improve readability, you could derive the content type once and reuse it:
const contentType = file.type || "application/octet-stream"; // Step 1 const { uploadUrl, s3Key } = await generateS3UploadUrlAction({ fileName: file.name, contentType, }); // Step 2 const uploadResponse = await fetch(uploadUrl, { method: "PUT", body: file, headers: { "Content-Type": contentType }, });Optionally, you might also pass this
contentTypeintosaveFileasmediaTypeif you want stored metadata to match what was actually used on upload; current behavior leavesmediaTypeas the rawfile.type, which may be empty for some files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/hooks/useFileUpload.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:23:01.117Z
Learnt from: fkesheh
Repo: hackerai-tech/hackerai PR: 92
File: lib/ai/tools/utils/sandbox-file-uploader.ts:10-10
Timestamp: 2025-11-12T15:23:01.117Z
Learning: In the hackerai codebase, NEXT_PUBLIC_USE_S3_STORAGE is intentionally used in both client and server code (including server-only files like lib/ai/tools/utils/sandbox-file-uploader.ts) to ensure the frontend and backend stay in sync regarding which storage backend (S3 vs Convex) is being used. This consistency is important for file upload flows that span both environments.
Applied to files:
app/hooks/useFileUpload.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/__tests__/s3Cleanup.test.ts (1)
36-54: Update the test name to reflect the removed assertion.The test is named "should successfully delete S3 object and log success" but no longer verifies the logging behavior. This creates misleading documentation.
Apply this diff to update the test name:
- it("should successfully delete S3 object and log success", async () => { + it("should successfully delete S3 object", async () => {Alternatively, if the logging assertion was intentionally removed due to a production change, consider adding a brief comment explaining why the expectation is commented out rather than deleted.
🧹 Nitpick comments (1)
convex/__tests__/s3Cleanup.test.ts (1)
51-53: Consider removing the commented-out code.Commented-out code adds clutter and can be confusing. Since this assertion no longer applies (per the production change that removed the success log), it should be removed entirely rather than left as a comment.
Apply this diff to clean up the commented code:
expect(mockDeleteS3Object).toHaveBeenCalledWith(args.s3Key); - // expect(console.log).toHaveBeenCalledWith( - // `Successfully deleted S3 object: ${args.s3Key}`, - // ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
convex/__tests__/s3Cleanup.test.ts(1 hunks)convex/s3Cleanup.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- convex/s3Cleanup.ts
- Update BASH_SANDBOX_AUTOPAUSE_TIMEOUT from 15min to 7min for cost savings - Keep BASH_SANDBOX_TIMEOUT at 15min for reliable connections - MAX_COMMAND_EXECUTION_TIME already set to 7min - ~53% reduction in idle sandbox runtime costs
rossmanko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ai/tools/run-terminal-cmd.ts (1)
173-188: Fix race condition in onTimeout and remove contradictory timeout messages for long-running waitsVerification confirms the identified issues:
Race condition: The
onTimeoutcallback checksresolvedat line 225 before theawait findProcessPid()at line 238. A competing path (abort or normal completion) can setresolved = truewhile the await is in flight, butonTimeoutcontinues and resolves again at line 263, writing duplicate timeout messages.Contradictory messages for tail --pid: When
isContinuingInBackground = false(fortail --pid),onTimeoutwrites "was terminated" at line 253, buthandler.getResult()at line 259 appendsTIMEOUT_MESSAGE(from terminal-executor.ts line 82-84) which states "continues in background"—contradictory for a wait command whose process is being killed by e2b.Apply the suggested fix: add a
resolvedguard after thefindProcessPidawait, checkresolvedbefore writing timeout messages, and strip the duplicate "continues in background" suffix from the final output for the non-background case.Lines 224–267 in lib/ai/tools/run-terminal-cmd.ts require these changes.
🧹 Nitpick comments (1)
lib/ai/tools/run-terminal-cmd.ts (1)
25-45: Prompt guidance updates for background jobs andtail --pidlook good (minor nits only)The expanded instructions around command chaining, pipes, avoiding inline interpreters, and the recommended
is_background+tail --pid=<pid> -f /dev/nullworkflow make the tool behavior much clearer and align with the new timeout semantics.Only minor polish you might consider (optional):
- Fix the small typo “Dont include any newlines” → “Don’t include any newlines”.
- Note that the
tail --piddetection logic assumes the command starts withtail --pid(no wrapping shell), which matches our intended usage but is worth keeping in mind.No functional issues here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/ai/tools/run-terminal-cmd.ts(7 hunks)lib/ai/tools/utils/sandbox.ts(2 hunks)lib/api/chat-handler.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/api/chat-handler.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/ai/tools/run-terminal-cmd.ts (3)
lib/utils/terminal-executor.ts (1)
createTerminalHandler(21-102)lib/ai/tools/utils/pid-discovery.ts (1)
findProcessPid(11-80)lib/token-utils.ts (1)
TIMEOUT_MESSAGE(31-34)
🔇 Additional comments (2)
lib/ai/tools/run-terminal-cmd.ts (2)
348-395: PID-awaretail --pidwait workflow is coherent and user-friendlyThe
isWaitCommand/waitingForPidhandling plus:
- Initial “Waiting for process to complete...” message, and
- Final “Process completed” message in the success path,
combined with tracking
processIdfor background processes and registering them inbackgroundProcessTracker, gives a clear, PID-centric story for long‑running workflows.Assuming the timeout behavior above is adjusted, this flow looks solid.
Also applies to: 378-383
173-176: Propagating PID intogetResultacross abort/timeout/success/error pathsPassing
processId ?? undefinedintohandler.getResultin:
- The abort path,
- The stream timeout path,
- The normal completion path, and
- The
CommandExitErrorpath,nicely aligns with the updated
TIMEOUT_MESSAGE(seconds, pid?)helper so that any “continues in background” messaging can include a concrete PID when we know it.Once the timeout/
tail --pidcontradictions are resolved as above, this PID propagation should make background behavior much easier for users to reason about.Also applies to: 258-260, 361-362, 407-409
| import { waitForSandboxReady } from "./utils/sandbox-health"; | ||
|
|
||
| const MAX_COMMAND_EXECUTION_TIME = 10 * 60 * 1000; // 10 minutes | ||
| const MAX_COMMAND_EXECUTION_TIME = 7 * 60 * 1000; // 7 minutes |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm 7‑minute MAX_COMMAND_EXECUTION_TIME vs 10‑minute expectations
MAX_COMMAND_EXECUTION_TIME is now 7 * 60 * 1000 (7 minutes), and the guidance in point 10 also tells the model to keep scans under 7 minutes. The PR description, however, mentions a 10‑minute maximum execution timeout / extended wait for tail --pid.
Please double‑check that 7 minutes is the intended system cap (for both regular commands and tail --pid waits) and, if so, update the PR description and any external docs to avoid confusion. If you did intend 10 minutes specifically for tail --pid, you’ll need a separate constant for that and to decouple it from MAX_COMMAND_EXECUTION_TIME.
🏁 Script executed:
#!/bin/bash
# Examine the terminal command file for timeout constants and guidance text
cat lib/ai/tools/run-terminal-cmd.ts | head -150Length of output: 9203
🏁 Script executed:
#!/bin/bash
# Search for "10 minute" or "10-minute" references in the codebase
rg -i "10\s*minute" --type ts --type js --type mdLength of output: 430
🏁 Script executed:
#!/bin/bash
# Search for timeout-related constants and references
rg -i "timeout|MAX.*TIME" lib/ai/tools/run-terminal-cmd.ts -n -A2 -B2Length of output: 4969
🏁 Script executed:
#!/bin/bash
# Look for PR description or changelog files
fd -i "readme|pr|changelog|description" --type f | head -20Length of output: 592
🏁 Script executed:
#!/bin/bash
# Get context around line 242 where the 10-minute comment appears
sed -n '235,260p' lib/ai/tools/run-terminal-cmd.tsLength of output: 1301
🏁 Script executed:
#!/bin/bash
# Search for any "10" references with timeout context
rg "10.*min|timeout.*10" lib/ai/tools/run-terminal-cmd.ts -n -B3 -A3Length of output: 1838
🏁 Script executed:
#!/bin/bash
# Check git history or comments for any mention of 10-minute intent
rg "10.*minute|10.*min|extended.*wait" lib/ai/tools/run-terminal-cmd.ts -n -B2 -A2Length of output: 1407
Fix incorrect comment on line 242 stating "(10min)" when MAX_COMMAND_EXECUTION_TIME is 7 minutes
The code is internally consistent: MAX_COMMAND_EXECUTION_TIME is 7 * 60 * 1000 (7 minutes) on line 14, and guidance point 10 correctly instructs to keep execution under 7 minutes. However, the comment on line 242 incorrectly states "(10min)" when referencing MAX_COMMAND_EXECUTION_TIME. Update the comment from:
// For MAX_COMMAND_EXECUTION_TIME (10min), the process is killed by e2b
to:
// For MAX_COMMAND_EXECUTION_TIME (7 minutes), the process is killed by e2b
🤖 Prompt for AI Agents
In lib/ai/tools/run-terminal-cmd.ts around lines 14 and 242, update the
incorrect explanatory comment that currently says "(10min)" to reflect the
actual constant value: change the comment to read "// For
MAX_COMMAND_EXECUTION_TIME (7 minutes), the process is killed by e2b" so the
comment matches MAX_COMMAND_EXECUTION_TIME = 7 * 60 * 1000; and any surrounding
context that references the duration should consistently state 7 minutes.
| const BASH_SANDBOX_TIMEOUT = 15 * 60 * 1000; // 15 minutes connection timeout | ||
| const BASH_SANDBOX_AUTOPAUSE_TIMEOUT = 7 * 60 * 1000; // 7 minutes auto-pause inactivity timeout | ||
|
|
There was a problem hiding this comment.
Align auto‑pause documentation with new 7‑minute timeout constant
BASH_SANDBOX_AUTOPAUSE_TIMEOUT is set to 7 minutes and used as the timeoutMs for betaCreate with autoPause: true, but the flow comment still says auto‑pause happens after 15 minutes. That’s now misleading.
Recommend updating step 6 to either reference the constant (e.g., “after BASH_SANDBOX_AUTOPAUSE_TIMEOUT of inactivity”) or explicitly say 7 minutes, to keep behavior and docs in sync.
Also applies to: 25-32
🤖 Prompt for AI Agents
In lib/ai/tools/utils/sandbox.ts around lines 5-7 (and also update lines 25-32),
the inline comment describing the auto-pause timeout is incorrect (mentions 15
minutes) while BASH_SANDBOX_AUTOPAUSE_TIMEOUT is set to 7 minutes; update the
comment in step 6 to reference the constant or state “7 minutes” (e.g.,
“auto-pause after BASH_SANDBOX_AUTOPAUSE_TIMEOUT (7 minutes) of inactivity”) so
the documentation matches the actual timeout value used by betaCreate with
autoPause: true.
Critical bug fix:
New feature - tail --pid support:
Improvements:
This fixes the workflow where AI can now:
Summary by CodeRabbit
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.