update java sdk

[ZhaoQing7892]

No description provided.

[kruise-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[furykerry]

Code Review: Java SDK Refactoring

Overview

This PR is a major refactoring of the Java SDK, splitting it into two architectural layers:

• E2B layer (client/e2b/): Control plane — ConnectionConfig, SandboxApi, Sandbox
• Runtime layer (client/runtime/): Data plane — RuntimeClient, RuntimeConfig, Commands, Filesystem

The key architectural change is migrating from gRPC to OkHttp + Connect Protocol for runtime communication, removing the gRPC dependency. ~100 files changed, ~19,800 lines added. The bulk (~80%) is auto-generated OpenAPI code.

---

Critical Issues (Must Fix)

1. OkHttp Response body leak — resource/connection leak

• Location: Commands.java (list(), sendInput(), sendSignal(), closeStdin()) and Filesystem.java (listDir(), makeDir(), move(), remove(), exists(), getInfo())
• Issue: OkHttp Response objects are never closed. The response body is consumed via response.body().charStream() or response.body().string(), but response.close() is never called, nor is it used in a try-with-resources.
• Why: OkHttp's connection pool will leak sockets if response bodies are not closed. This leads to connection exhaustion under load.
• Fix: Use try-with-resources for every Response:

try (Response response = httpClient.newCall(request).execute()) {
    if (!response.isSuccessful()) {
        throw new IOException("...");
    }
    Reader reader = response.body().charStream();
    // ...
}

2. RuntimeClient.close() is empty — OkHttpClient never shut down

• Location: RuntimeClient.java
• Issue: close() is a no-op. The OkHttpClient instances created in RuntimeConfig (with thread pools and connection pools) are never shut down.
• Why: Leaks threads (Dispatcher, ConnectionPool) on every RuntimeClient lifecycle. Particularly bad in Sandbox.close() → runtimeClient.close() which does nothing.
• Fix: At minimum, shut down the OkHttpClient's dispatcher and connection pool:

@Override
public void close() {
    // If RuntimeConfig owns the clients, expose a shutdown method
    // or call httpClient.dispatcher().executorService().shutdown()
}

3. Wrong error message in Filesystem.getInfo() — copy-paste bug

• Location: Filesystem.java — getInfo() catch block
• Issue: throw new RuntimeException("Failed to Remove", e); — should say "Failed to getInfo"
• Fix: throw new RuntimeException("Failed to get file info: " + path, e);

---

Warnings (Should Fix)

4. Java naming convention violation — public uppercase fields

• Location: Sandbox.java, RuntimeClient.java
• Issue: public final Commands Commands and public final Filesystem Files use PascalCase for field names, violating Java conventions (should be camelCase: commands, files).
• Recommendation: Rename to commands and files. If the uppercase style is intentional to match the E2B Python SDK convention, document this decision.

5. Sandbox.close() catches only ApiException — breaks try-with-resources on unexpected errors

• Location: Sandbox.java
• Issue: If kill() throws a RuntimeException (e.g., from network issues), it will propagate out of close(), breaking try-with-resources.
• Recommendation: Catch Exception instead of ApiException:

} catch (Exception e) {
    LOG.log(Level.WARNING, "Failed to kill sandbox " + sandboxID + " on close()", e);
}

6. Unused imports in SandboxNotFoundException

• Location: SandboxNotFoundException.java
• Issue: import java.util.List; import java.util.Map; are unused. Remove them.

7. K8sHelper.getRuntimeToken() throws raw Exception and generic RuntimeException

• Location: K8sHelper.java
• Recommendation: Use a more specific exception type and narrow the throws clause.

8. ConnectResponseAdapter extends ConnectStreamReader with a dummy ByteArrayInputStream

• Location: Commands.java inner class ConnectResponseAdapter
• Issue: super(new ByteArrayInputStream(new byte[0]), ...) is a hack. The parent's readNextFrame() is never called, but this is fragile.
• Recommendation: Extract a common interface (e.g., MessageStream<T>) instead of inheritance with a dummy stream.

9. E2bRuntimeConfig.getSandboxURL() returns incomplete URL in PRIVATE mode

• Location: E2bRuntimeConfig.java
• Issue: In PRIVATE mode, returns sandboxBaseURL (<scheme>://<domain>) without sandbox ID or port. But ConnectionConfig.getSandboxURL() in PRIVATE mode returns <scheme>://<domain>/kruise/<sandboxID>/<port>. These are inconsistent.
• Recommendation: Align URL construction between the two classes.

10. Potential NPE in SandboxApi.create(NewSandbox)

• Location: SandboxApi.java
• Issue: body.setTimeout(ConnectionConfig.DEFAULT_SANDBOX_TIMEOUT) passes an int to a setter that may expect Integer. Verify this compiles correctly with the generated model.

---

Suggestions (Consider Improving)

11. No unit tests

• ~19,800 lines of code with zero unit tests. The hand-written classes (ConnectionConfig, SandboxApi, RuntimeConfig, Commands, Filesystem, ConnectStreamReader, SandboxInfo) should have unit tests — especially URL construction logic, ConnectStreamReader frame parsing, error handling paths (404, 409, timeouts), and Builder pattern validation.

12. Documentation is Chinese-only

• The README files are only in Chinese. Since this is a CNCF subproject with international users, consider adding English documentation.

13. SandboxInfo has setters despite being conceptually read-only

• SandboxInfo is returned from list() and getInfo(), but exposes public setters for all fields. Consider making it immutable.

14. write() in Filesystem silently swallows JSON parse errors

• The catch (Exception ignored) block silently falls back to basic info. Consider logging at DEBUG level.

---

Positive Findings

1. Clean architecture split — E2B (control plane) and Runtime (data plane) separation is well-designed.
2. Double-checked locking done correctly — volatile + synchronized pattern in ConnectionConfig and RuntimeConfig.
3. Good error mapping in SandboxApi — 404 → SandboxNotFoundException, 409 → idempotent success for pause.
4. Connect Protocol migration — Simplifies dependency tree while maintaining streaming semantics. ConnectStreamReader is clean and well-documented.
5. AutoCloseable support — Both Sandbox and RuntimeClient implement AutoCloseable.
6. EnvdMethods constants class — Centralizes service/method name strings, avoiding magic string duplication.
7. Comprehensive examples — Four well-structured example files covering lifecycle, commands, files, and K8s direct connect.

[furykerry]

Follow-up Review (commits 9c5ccd3 + 6511b43)

Thanks for addressing the feedback! Here's a summary of what was fixed and what remains.

---

Addressed Issues ✅

#	Original Issue	Fix Applied
1	OkHttp Response body leak	✅ All unary calls now use try (Response response = ...). Streaming calls store Response in CommandHandle/WatchHandle and close it on close()/stop(). Error paths also close the response.
2	RuntimeClient.close() is empty	✅ Now calls config.shutdown(), which shuts down Dispatcher executor services and evicts ConnectionPools.
3	Java naming convention (Commands/Files)	✅ Renamed to commands/files in Sandbox, RuntimeClient, and all examples.
4	Sandbox.close() catches only ApiException	✅ Removed kill() from close() entirely — design change (see below).
5	Unused imports in SandboxNotFoundException	✅ List and Map imports removed.
6	K8sHelper throws raw Exception/RuntimeException	✅ New K8sOperationException class. K8sHelper and RuntimeClient.newFromK8s() now throw this specific type.
7	ConnectResponseAdapter extends with dummy stream	✅ New MessageStream<T> interface extracted. CommandHandle uses MessageStream instead of ConnectStreamReader.
8	Chinese-only docs	✅ English README.md added for both e2b and runtime modules.
9	SandboxInfo has setters / not immutable	✅ All fields now final, setters removed, Builder pattern with unmodifiable collections.

---

Remaining Issues

Critical #3: Wrong error message in Filesystem.getInfo() — still needs verification

• Original issue: throw new RuntimeException("Failed to Remove", e); should say "Failed to getInfo"
• The Filesystem.java diff shows refactoring to use try (Response response = ...) but the truncated patch makes it hard to confirm the error message was fixed. Please verify this copy-paste bug is corrected.

Warning #9: E2bRuntimeConfig.getSandboxURL() returns incomplete URL in PRIVATE mode

• Not addressed. In PRIVATE mode, getSandboxURL() returns sandboxBaseURL without appending /<sandboxID>/<port>, while ConnectionConfig.getSandboxURL() does append them. These URLs should be consistent.

Warning #10: Potential NPE in SandboxApi.create()

• Not addressed. body.setTimeout(ConnectionConfig.DEFAULT_SANDBOX_TIMEOUT) passes an int to a setter that may expect Integer. Please verify the generated model's setter signature.

Suggestion #11: No unit tests

• Still no unit tests for any of the hand-written classes.

---

New Observations from the Delta

N1. Sandbox.close() no longer kills the sandbox — intentional breaking change

• Impact: Previously try (Sandbox sandbox = api.create("...")) would auto-kill. Now users must explicitly call api.kill(sandbox.getSandboxID()).
• The README examples correctly show the new pattern, which is good.
• Question: Is this intentional? The E2B Python SDK auto-kills on close. This is a significant behavioral change that may surprise users migrating from the previous SDK version.

N2. RuntimeConfig.shutdown() — minor TOCTOU concern

• shutdown is volatile (good for visibility), but getOrCreateHttpClient() checks shutdown outside the lock. In theory, a thread could read shutdown == false, then another thread calls shutdown(), and the first thread proceeds to create a new client after shutdown. This is a minor race that's unlikely in practice since shutdown is called after all clients are done, but worth noting.

N3. WatchHandle.stop() still uses System.err.println instead of a Logger

• Filesystem now has a Logger, but WatchHandle.stop() still uses System.err.println. Please use the Logger consistently.

N4. CommandHandle.close() — streamingResponse.close() not guarded by try-catch

• streamReader.close() is wrapped in try-catch (good), but streamingResponse.close() is called directly afterward without a try-catch. If it throws, it will propagate. Recommend wrapping in try-catch:

if (streamingResponse != null) {
    try {
        streamingResponse.close();
    } catch (Exception ignored) {
        // Resource cleanup, ignore close exceptions
    }
}

---

Summary

The author addressed 9 out of 14 issues from the original review — excellent turnaround. The remaining items are mostly minor (error message verification, PRIVATE URL consistency, unit tests). The one item worth discussing with the team is N1 — the behavioral change in Sandbox.close() from auto-kill to explicit-kill.

[furykerry]

Code Review Summary

Files reviewed: ~100 files, ~19,800 lines. Hand-written: ConnectionConfig, E2bRuntimeConfig, Sandbox, SandboxApi, SandboxInfo, SandboxNotFoundException, RuntimeClient, RuntimeConfig, K8sHelper, EnvdMethods, Commands, CommandHandle, Filesystem, WatchHandle, WatchDirResponseObserver, ConnectStreamReader, MessageStream, K8sOperationException, SandboxException. Auto-generated (package rename only): envd/ protobuf code. Auto-generated (new): e2b/api/ OpenAPI client.

Note: This review builds on the two prior review rounds by @furykerry. Issues already addressed are not re-listed; only new or still-open findings are reported.

---

Critical

No new critical issues found. The previously identified critical issues (#1 OkHttp response leak, #2 RuntimeClient.close() empty, #3 Filesystem.getInfo() copy-paste error) have all been addressed in the follow-up commits. The getInfo() error message now correctly reads "Failed to get file info: " + path.

---

High

• CommandHandle.close() — streamingResponse double-close will throw — CommandHandle.java

  close() calls streamReader.close() (wrapped in try-catch, good) then streamingResponse.close() (NOT wrapped). For connect() calls, the streamReader is a ConnectResponseAdapter whose close() already calls response.close(). So CommandHandle.close() will call response.close() twice — the second call on an already-closed OkHttp Response can throw IllegalStateException. Fix: wrap streamingResponse.close() in try-catch:

  if (streamingResponse != null) {
      try { streamingResponse.close(); } catch (Exception ignored) {}
  }

• E2bRuntimeConfig.getSandboxURL() inconsistent with ConnectionConfig in PRIVATE mode — E2bRuntimeConfig.java

  In PRIVATE mode, E2bRuntimeConfig.getSandboxURL() returns just sandboxBaseURL (scheme://domain), while ConnectionConfig.getSandboxURL() returns scheme://domain/kruise/<sandboxID>/<port>. The runtime layer uses getSandboxURL() to build request URLs like {url}/{service}/{method}, so in PRIVATE mode the sandbox ID and port are missing from the URL path. This is likely a routing bug unless the gateway resolves it via headers — needs verification or alignment.

• RuntimeConfig TOCTOU race on shutdown flag — RuntimeConfig.java

  getOrCreateHttpClient() checks if (shutdown) outside the httpClientLock. A thread can read shutdown == false, then another thread calls shutdown(), and the first thread proceeds to create a new client on a shut-down config. While unlikely in practice, the check should be inside the synchronized block for correctness:

  public OkHttpClient getOrCreateHttpClient() {
      if (sharedHttpClient == null) {
          synchronized (httpClientLock) {
              if (shutdown) throw new IllegalStateException("...");
              if (sharedHttpClient == null) { ... }
          }
      }
      return sharedHttpClient;
  }

---

Medium

• WatchHandle.stop() uses System.err.println instead of Logger — WatchHandle.java — Filesystem has a proper Logger LOG, but WatchHandle.stop() still writes to System.err.println. Use the Logger consistently.

• WatchDirResponseObserver.onError() uses System.err + printStackTrace — WatchDirResponseObserver.java — System.err.println(...) and t.printStackTrace() should be replaced with a proper Logger call.

• ConnectResponseAdapter.close() doesn't protect response.close() — Commands.java (inner class) — response.close() in the adapter's close() is not wrapped in try-catch. If it throws, it will propagate out of close(), which violates the Closeable contract.

• Commands.sendSignal() error message says "Failed to send kill" — Commands.java — The error message "Failed to send kill" is misleading — should be "Failed to send signal" since the method sends any signal, not just SIGKILL.

• Filesystem.watchDir() background thread not interruptible on shutdown — Filesystem.java — The watch thread is a daemon (good), but calling RuntimeConfig.shutdown() doesn't close the streaming Response held by WatchHandle. If the user shuts down the config without calling watchHandle.stop(), the thread may linger.

---

Low

• Filesystem.urlEncode() uses deprecated "UTF-8" string — Use StandardCharsets.UTF_8 instead to avoid the unnecessary try-catch.

• RuntimeConfig.DEFAULT_AUTH_HEADER hardcoded base64 credential — "Basic cm9vdDo=" decodes to root:. Not a leaked secret, but a weak default that could silently authenticate requests. Consider documenting prominently.

• No unit tests — ~19,800 lines of code with zero unit tests. Hand-written classes like ConnectStreamReader, RuntimeConfig, E2bRuntimeConfig, and SandboxApi should have focused unit tests.

• MessageStream.java missing newline at end of file

---

Positive Findings

• Clean architecture split — E2B (control plane) and Runtime (data plane) separation is well-designed with clear package boundaries.
• All previously identified critical and high issues were addressed in follow-up commits.
• Double-checked locking done correctly in ConnectionConfig and RuntimeConfig (volatile + synchronized).
• ConnectStreamReader is well-implemented — proper frame length validation, trailer error parsing, max frame size guard.
• Good error mapping in SandboxApi — 404 → SandboxNotFoundException, 409 → idempotent success for pause.
• MessageStream<T> interface extraction is clean, enabling CommandHandle to work with both stream types uniformly.
• Proper resource cleanup in error paths — runBackground() and connect() close both streamReader and response on all error paths.
• Bilingual documentation (English + Chinese READMEs) for both modules.

---

Verdict

REQUEST CHANGES — The double-close bug in CommandHandle.close() is a correctness issue that should be fixed before merge. The PRIVATE mode URL inconsistency needs verification. The remaining items are improvements that would strengthen reliability but don't block merge.