agentscope-ai / agentscope-ai/agentscope-java

[Bug]: SessionTree mirror NPEs when it outlives the sandbox — PinnedSandboxFilesystem cannot know the pinned sandbox is closed (regression from #2946, v2.0.3)

未关闭
#3,036 2 条评论 1 个 reaction 已指派 0 人 在 GitHub 查看
area/extensions area/harness bug
主要语言
Java
星标
5.6k
派生
1.3k
平均合并
4 天 12 小时
30 天内合并 PR
77

描述

**Versions:** `agentscope-harness:2.0.3`, `agentscope-extensions-sandbox-kubernetes:2.0.3`
**Regressed by:** #2946 (merged 2026-09-06, released in v2.0.3 on 2026-09-07)
**Same root cause as:** #2147 (still open) — different teardown point
**Context:** #2490 / #2675 (per-call sandbox binding), #2771 (per-call K8s pod termination), #2777 / #2935 (fire-and-forget flush)

### Summary

`SessionTree` mirrors the session log to the agent's filesystem fire-and-forget. When that filesystem is a sandbox, the queued mirror routinely runs *after* the sandbox has been shut down and dies with a `NullPointerException` rather than any meaningful error.

Before #2946 this same race degraded to a `No active sandbox` warning, because the async mirror resolved the sandbox lazily through the shared slot, which release had already cleared. #2946 pinned the `Sandbox` instance to fix *which* sandbox the mirror targets — correct in itself — but the pinned reference is now a hard one, so the mirror walks into a dead sandbox instead of failing soft.

### Observed

```
WARN session-tree-mirror i.a.h.a.filesystem.sandbox.SandboxBackedFilesystem
[sandbox-fs] native upload failed for path: agents//sessions/.log.jsonl
java.lang.NullPointerException: Cannot invoke
"io.agentscope.extensions.sandbox.kubernetes.client.CommandExecutor.run(String)"
because the return value of
"io.agentscope.extensions.sandbox.kubernetes.client.Sandbox.commands()" is null
at io.agentscope.extensions.sandbox.kubernetes.KubernetesSandbox.uploadFile(KubernetesSandbox.java:272)
at io.agentscope.harness.agent.filesystem.sandbox.SandboxBackedFilesystem.uploadFiles(...)
at io.agentscope.harness.agent.memory.session.SessionTree.mirrorToFilesystem(SessionTree.java:733)
at io.agentscope.harness.agent.memory.session.SessionTree.lambda$scheduleMirror$6(SessionTree.java:593)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
```

One run, three log lines 78 ms apart: the trace middleware logs POST_CALL, then `[sandbox-client] Connection to sandbox claim '' has been closed.`, then the mirror fails.

> The `SandboxBackedFilesystem` frame's line number is omitted because we shadow that class locally for an unrelated reason. The frame is the unmodified native-transfer branch of `uploadFiles` that calls `SandboxFileTransfer.uploadFile`.

### Root cause

1. `SessionTree.flush()` → `scheduleMirror()` wraps the filesystem via `pinIfSandbox(...)` and hands the work to the static single-threaded `MIRROR_EXECUTOR` (thread `session-tree-mirror`). `flush()` returns as soon as the task is queued.
2. `PinnedSandboxFilesystem` pins the concrete `Sandbox` in its constructor (`setSandbox(sandbox)`) **and overrides `clearSandboxIfCurrent` to a no-op**. The override is deliberate — it stops release from un-pinning the instance — but `clearSandboxIfCurrent` is also the only channel through which this object could learn the sandbox was released (#2675). It therefore holds a hard reference to a sandbox it has no way of knowing is dead.
3. `SandboxLifecycleMiddleware.releaseForCall` → `SandboxManager.release` calls `stop()` then `shutdown()` for every sandbox with `isSelfManaged() == true`, at the end of *every* call.
4. `KubernetesSandbox.shutdown()` reaches the SDK's `Sandbox.closeConnection()`, which nulls `commands` and `files` and sets `closed = true`.
5. The queued mirror then calls `KubernetesSandbox.uploadFile`, which does `sdkSandbox.commands().run("mkdir -p ...")` with no liveness check → NPE.

Steps 1 and 3 are not synchronised in any way.

### This is not limited to user-managed sandboxes

#2771 documents the same step 3 for the stock client: *"`SandboxLifecycleMiddleware` … releases the sandbox after **every** call: `SandboxManager.release()` (self-managed path) → `stop()` → `shutdown()`. `KubernetesSandbox.shutdown()` sees `claimOwned == true` and calls `terminate()`, deleting the claim and the pod — every call."* #2771 was closed on the acquire/resume side; `release` still shuts the sandbox down per call in 2.0.3. So a plain framework-managed Kubernetes sandbox hits this race too.

### Why the existing quiescence API doesn't cover it

`SessionTree.awaitMirrorQuiescence` exists, but across the whole harness jar it is referenced only by `SessionTree` itself and `HarnessAgent.close()` — i.e. container shutdown. Nothing on the sandbox lifecycle path drains the queue, and there is no way to wait for one session's mirror; the executor is static and global, so the only available barrier is "wait for every session".

That is consistent with how the API arrived: #2777 made the flush fire-and-forget on purpose, to unblock conversation completion, and #2935 describes `awaitMirrorQuiescence` as something `HarnessAgent#close()` already did, reusing it test-side to stop `@TempDir` teardown flakiness. It was never introduced as a lifecycle contract for resource owners.

**#2147 is the same root cause at a different teardown point** and has been open since 2026-07-12: there the late `session-tree-mirror` write lands on a closed Jedis pool (`Pool not open`) after `HarnessAgent.close()` returned. Its "Expected" list — stop accepting new mirror jobs on close, drain or cancel accepted ones, close the executor before returning — is exactly what the sandbox path needs as well. Filing separately because the fix location differs (pinned filesystem / sandbox release rather than agent close), but the two should probably be solved together.

### Secondary defects

- **`KubernetesSandbox.uploadFile` / `downloadFile` don't check liveness.** The SDK's `Sandbox` already exposes `isActive()`. A closed sandbox should produce a `SandboxException` naming the cause, not an NPE from dereferencing a nulled field.
- **`isRunning()` lies after shutdown.** `KubernetesSandbox.shutdown()` overrides the interface default and only calls `terminate()` / `closeConnection()`; it never clears `AbstractBaseSandbox`'s `running` flag, so `Sandbox.isRunning()` still returns `true` on a shut-down sandbox. Callers wanting to guard defensively have no reliable check on the `Sandbox` interface and must downcast to `KubernetesSandbox` for `getSdkSandbox().isActive()`.

### Suggested fixes, roughly in order of value

1. Have `PinnedSandboxFilesystem` (or `SessionTree.mirrorToFilesystem`) skip the write and log at debug when the pinned sandbox is no longer alive — the mirror is best-effort by design.
2. Make `KubernetesSandbox.uploadFile` / `downloadFile` fail with a `SandboxException` on a closed sandbox instead of an NPE.
3. Clear the `running` flag in `KubernetesSandbox.shutdown()` so `isRunning()` is trustworthy.
4. Drain the mirror queue in `SandboxManager.release` before `shutdown()` for self-managed sandboxes, or expose a per-session `awaitMirror(sessionId, timeout)` so lifecycle owners can order the two correctly. This would also address #2147.

`SessionTreeMirrorTest`, added by #2946, is the natural home for a regression test: schedule a mirror against a sandbox that is shut down before the executor picks the task up.

### Workaround

We own our sandbox lifecycle (`SandboxContext.externalSandbox`), so we call `SessionTree.awaitMirrorQuiescence(5, TimeUnit.SECONDS)` immediately before shutting the sandbox down. It covers the normal path deterministically — `TranscriptMiddleware` triggers the flush inside the middleware chain, which completes before the `Flux.using` cleanup that releases the sandbox — but it relies on internal ordering rather than a contract, it blocks on unrelated sessions queued on the shared executor, and it isn't available at all to anyone using a framework-managed sandbox, since that shutdown happens inside `SandboxManager.release`.

贡献指南

打开贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。