-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(goose-acp): enable parallel sessions with isolated agent state #6363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(goose-acp): enable parallel sessions with isolated agent state #6363
Conversation
c617a9e to
c3f5398
Compare
e65da8c to
160d5ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables parallel ACP agent sessions with isolated state, eliminating global singleton dependencies. The main achievement is enabling safe concurrent test execution, improving test performance by 55x (42.22s → 0.77s for goose-acp tests).
Key Changes:
- Introduces
goose-acpcrate with isolated ACP agent implementation - Refactors
PermissionInspectorto passpermission_modeper-call instead of storing in shared mutable state - Converts
SessionManagerandPermissionManagerto instance-based with thread-safe internals - Built-in extensions now run in-process via duplex streams instead of subprocess
- Consolidates workspace dependencies to enforce consistency
Reviewed changes
Copilot reviewed 79 out of 88 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/goose/src/agents/agent.rs |
Introduces AgentConfig struct to thread session/permission managers through agent |
crates/goose/src/permission/permission_inspector.rs |
Removes shared mode state; goose_mode passed per inspect() call |
crates/goose/src/session/session_manager.rs |
Refactors from static methods to instance-based with lazy pool initialization |
crates/goose/src/config/permission.rs |
Adds RwLock for thread-safe permission map access |
crates/goose/src/agents/mcp_client.rs |
Adds session_id parameter to call_tool trait method |
crates/goose/src/agents/extension_manager.rs |
Changes built-in extensions to run in-process via duplex streams |
crates/goose-acp/* |
New crate for isolated ACP server implementation |
crates/goose-mcp/src/lib.rs |
Adds BUILTIN_EXTENSIONS map for in-process spawning |
| Test files | Removes serial test constraints, adds per-test temp directories |
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
160d5ad to
0fc93f2
Compare
codefromthecrypt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notes on the important things
| STDIN: {"jsonrpc":"2.0","method":"notifications/initialized"} | ||
| STDERR: time=2025-12-11T17:58:47.642-05:00 level=INFO msg="session initialized" | ||
| STDIN: {"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"_meta":{"progressToken":0},"name":"get_file_contents","arguments":{"owner":"block","path":"README.md","repo":"goose","sha":"ab62b863c1666232a67048b6c4e10007a2a5b83c"}}} | ||
| STDIN: {"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"_meta":{"goose-session-id":"test-session-id","progressToken":0},"name":"get_file_contents","arguments":{"owner":"block","path":"README.md","repo":"goose","sha":"ab62b863c1666232a67048b6c4e10007a2a5b83c"}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that session is propagated via parameter in the case we always know it, the expectation file bumped for good reason.
| } | ||
| } | ||
|
|
||
| fn build_and_get_binary_path() -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that builtin extensions no longer require being launched as subprocesses (mcp xxx args), ACP doesn't need a build to run tests. This leaves only one path where we launch processes in tests, the old one for MCP replay, only used when recording. This is put back to exactly the same code from before we needed to extract a common cargo build utility.
| ) -> Result<Option<(String, DateTime<Utc>)>, SchedulerError>; | ||
| } | ||
|
|
||
| const UNAVAILABLE_MESSAGE: &str = "Scheduler not available. This tool only works in server mode."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we had a single call site we noticed scheduler in use and disabled it for CLI sessions. This extracts so it isn't optional. Unless there's a good reason to have CLI sessions fail on scheduler, I recommend in this PR or the next just letting them work and removing this workaround and/or only using it in tests
|
|
||
| static SESSION_STORAGE: OnceCell<Arc<SessionStorage>> = OnceCell::const_new(); | ||
| static SESSION_STORAGE: LazyLock<Arc<SessionStorage>> = | ||
| LazyLock::new(|| Arc::new(SessionStorage::new(Paths::data_dir()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to propagate and also still have a singleton I looked around at about 10 rust projects and this lazy lock pattern seemed the best, as old sites still get a global singleton until/unless updated, and new can propagate through in all cases the actual connect stuff is deferred to first.
| AllowOnce, | ||
| Cancel, | ||
| DenyOnce, | ||
| AlwaysDeny, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were missing this even though our permissions system allows persistence of deny. This is needed for ACP and the motivation was testing permissions which had a bug and they now are testable without clobbering or ENV
| .unwrap(); | ||
| } | ||
|
|
||
| #[test_case(Some(PermissionOptionKind::AllowAlways), ToolCallStatus::Completed, "user:\n always_allow:\n - lookup__get_code\n ask_before: []\n never_allow: []\n"; "allow_always")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the new test and notice no ACP test requires serial anymore, and none use ENV or clobber eachother's database or permissions files
|
|
||
| const PERMISSION_FILE: &str = "permission.yaml"; | ||
|
|
||
| static PERMISSION_MANAGER: LazyLock<Arc<PermissionManager>> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permission manager needs to be a singleton where used, so I used exact same pattern as session storage. before depending on call site reads might come from memory which could have problems when not shared. now it is always shared and pushed down consistently
|
can't change the head branch without a new PR so re-opened here so it is a branch off this repo instead of my fork #6392 |
Summary
This PR makes it possible to run ACP agents and sessions in parallel, with critical state isolated to each agent instance.
Before, we were using global singletons for most state, which made it impossible to change configuration without ENV or subprocess isolation. Moreover, unlike platform, built-in extensions launched subprocesses which implied you need a binary entrypoint to run them.
This PR extracts the most critical state into structs and ensures consistent threading to call sites. It also reconfigures built-in extensions to run in-process, via duplex streams.
Legacy entrypoints retain global singletons despite ACP entrypoints being isolated. This was done to allow progress, particularly to implement goose-server with ACP. This is also due to the pending PR to replace the CLI. We don't want to do too much work for code that is going to be replaced.
The most important impact is the new goose-acp crate which contains isolated code for critical paths even though the CLI entrypoint that uses it will act the same.
Later in the tests section you will see how the combination of these changes allow more ACP integration tests than before to complete in less than a second.
Type of Change
AI Assistance
Testing
GooseAcpAgentto accept variables viaAgentConfig, tests don't rely on ENV variables.Above and other changes allow us to safely run tests in parallel, removing env-lock, serial and reset patterns.
This means despite having more ACP integration tests, our performance is incredibly faster.
Works cited
What this PR does is a step towards a more modular codebase which avoids global singletons.
Some projects I studied for reference were:
Related Issues