Repository navigation
Conversation
✅ Deploy Preview for tarko ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
yang0228
left a comment
yang0228
left a comment
There was a problem hiding this comment.
Reviewed commit 5f2e8f8.
The immediate release fixes the original startup-failure leak, but I found a regression when another streaming request targets a session that is already executing: the rejected request clears the slot still owned by the active run. This affects both server implementations; details are in the inline comment.
Validation: an isolated Node source harness reproduced the original leak on the base commit, passed 12 lifecycle checks on this head across both implementations, and reproduced the overlapping-request regression in both (base remains busy; head becomes available while the first stream continues). The harness loads the session implementations and server slot methods, with a fake agent modeling the existing Agent.run() executing guard. The project Vitest suites and builds were not run because workspace dependencies are not installed.
There was a problem hiding this comment.
[P2] Preserve the active run's slot when a duplicate query is rejected
A rejected agent.run() does not necessarily mean this session has no active stream. Start a streaming query for session S, keep it running, then send another /api/v1/sessions/query/stream request for S. These query routes only use session-restore middleware, so the second request reaches Agent.run(), whose isExecuting() guard rejects it. This new unconditional clear then removes S's slot even though the first run is still executing. canAcceptNewRequest() becomes true, allowing session creation (and legacy one-shot execution) through the exclusive-mode gate. The base commit retains S's slot in this scenario; I reproduced the change in both implementations.
Please reject overlapping runs before changing slot ownership, or track ownership per run so this catch only releases its own acquisition. Add a regression test that keeps the first stream active while the second call rejects, and apply the same protection to agent-server-next at its matching catch block.
There was a problem hiding this comment.
Addressed in ab82586. Both implementations capture whether the agent was already executing and avoid clearing the active run's slot when an overlapping request is rejected. The regression keeps the first stream active while the second request rejects.
Follow-up validation recorded on 2026-10-17: all 6 lifecycle tests passed across both implementations; the non-SQLite run passed 6 files / 47 tests; both agent-server builds passed, including declarations. The fix was already present on this head; no duplicate implementation was added.
SQLite remains excluded from this result; the historical Windows SQLite failures are not marked as resolved. The secretlint run on 2026-09-25 ended with startup_failure and 0 jobs, so it provides no completed scan result:
https://github.com/bytedance/UI-TARS-desktop/actions/runs/36132548639
Summary
Closes #1951
In exclusive mode, both Agent Server implementations mark a session as running before awaiting
agent.run(). If that promise rejects before returning anAsyncIterable, the catch path returns a synthetic error stream while leaving the exclusive slot occupied indefinitely.This change releases the slot immediately in that startup-failure path for both
@tarko/agent-serverand@tarko/agent-server-next. A successfully created stream still owns the slot until iteration finishes, preserving the existing lifecycle for active streams.The regression tests exercise both server generations and verify that:
Verification
pnpm exec vitest run tests/core/agent-session-exclusive-mode.test.ts: 4 tests pass.pnpm exec vitest run --exclude tests/SQLiteStorageProvider.test.ts: 6 suites, 45 tests pass.pnpm --filter @tarko/agent-server build: passes.pnpm --filter @tarko/agent-server-next build: passes.pnpm exec prettier --check tarko/agent-server/tests/core/agent-session-exclusive-mode.test.ts: passes.@tarko/agent-serversuite: 45 tests pass; the same 19 pre-existing SQLite tests fail before and after this change on Windows because the test setup constructs paths such asD:\tmp\C:\Users\....Checklist