Skip to content

fix(ts-sdk): fix nohup process completion detection and OSS download path#1224

Merged
Timandes merged 3 commits into
alibaba:masterfrom
xdlkc:fix/ts-sdk-nohup-process-completion
Jul 13, 2026
Merged

fix(ts-sdk): fix nohup process completion detection and OSS download path#1224
Timandes merged 3 commits into
alibaba:masterfrom
xdlkc:fix/ts-sdk-nohup-process-completion

Conversation

@xdlkc

@xdlkc xdlkc commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes three bugs in the TS SDK's nohup/OSS download path that caused large file downloads to fail:

  • waitForProcessCompletion misinterpreted "process exited" exceptions as failures (divergence from Python SDK behavior)
  • ensureOssutil used normal mode which hits the 85s server-side run_in_session timeout
  • oss_client.ts wget/ossutil cp used normal mode instead of nohup for long-running transfers

Changes

File Change
client.ts Distinguish timeout vs command-failure exceptions in waitForProcessCompletion
file_system.ts ensureOssutil writes script to file + runs via nohup (matches Python process.execute_script)
oss_client.ts Revert wget/ossutil cp to nohup mode with waitTimeout: 600
http.ts Relax PID extraction regex (don't require PID_SUFFIX)

Test Plan

  • 50MB file download via auto mode — MD5 verified
  • 50MB file download via explicit --download-mode oss — MD5 verified
  • Small file download via direct mode — content intact
  • All 861 unit tests pass (npm run test:unit)

Fixes #1223

🤖 Generated with Claude Code

…path

The TS SDK's waitForProcessCompletion incorrectly treats all exceptions
from `kill -0 PID` as failures. When the process exits, the server
returns status=Failed for the kill check, but this should be interpreted
as "process completed" (matching Python SDK behavior).

Also fixes ensureOssutil and oss_client commands that were using normal
mode (85s server-side hard timeout) instead of nohup mode for long-running
operations like ossutil install, wget, and ossutil cp.

Fixes alibaba#1223

Co-Authored-By: Claude Code <noreply@anthropic.com>
AI-Model: claude-opus-4-6
AI-Contributed/Feature: 92/92
AI-Contributed/UT: 0/0
@Timandes

Timandes commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Review note:

[P1] rock/ts-sdk/src/sandbox/oss_client.ts around the sandbox-side wget path: after switching wget to mode: 'nohup', sandbox.arun() only reports that the monitored process completed; it does not prove the wrapped wget command exited successfully. The current follow-up check only verifies that targetPath exists, so a failed or interrupted wget that leaves an empty/partial file can still make uploadViaOss() return success.

Please add a stronger verification step before returning success, for example compare stat -c %s '${targetPath}' with the local fileSize, or make the nohup wrapper persist and check the real command exit code. This matters because the signed URL download is now long-running and precisely the path where partial files are most likely to appear.

Reject incomplete sandbox downloads by comparing the remote file size with the local source size after wget exits in nohup mode. Add regression coverage for partial, unverifiable, and complete transfers.

Co-Authored-By: Codex <noreply@openai.com>

AI-Model: gpt-5

AI-Contributed/Feature: 20/20

AI-Contributed/UT: 65/65
@Timandes

Copy link
Copy Markdown
Collaborator

Review note:

[P1] rock/ts-sdk/src/sandbox/file_system.ts in ensureOssutil() now does a quick check with this.sandbox.execute({ command: ['ossutil', 'version'], timeout: 60 }). When ossutil is not installed yet, the server executes this as subprocess.run([...]) with shell=false, so the missing executable raises FileNotFoundError. That gets wrapped as status=Failed, and the TS SDK execute() throws before this method can fall through to the install script.

This breaks the first-time install path that the PR is trying to fix. Please either remove the quick check, or make it non-throwing when the command is missing, e.g. run a shell/session check like command -v ossutil >/dev/null 2>&1 && ossutil version and catch failures so the install script still runs.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: GPT-5
AI-Contributed/Feature: 12/12
AI-Contributed/UT: 25/25
@Timandes
Timandes merged commit d4a516e into alibaba:master Jul 13, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] TS SDK nohup process completion misdetects finished processes as failures

2 participants