Session manager by DOsinga · Pull Request #4648 · block/goose

DOsinga · 2025-09-15T23:01:54Z

Use SQLite for Session manager

Don't implement your own database

Copilot

Pull Request Overview

This pull request implements a comprehensive migration from a file-based session storage system to an SQLite database for session management. The primary purpose is to replace the custom JSONL file-based session storage with a more robust, standardized database approach.

Key changes:

Replace custom JSONL session storage with SQLite database using SessionManager
Migrate all session-related APIs to use the new database schema
Remove deprecated file-based session functions and replace with database operations

Reviewed Changes

Copilot reviewed 40 out of 42 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
ui/desktop/src/sessions.ts	Removes custom session management functions (updateSessionMetadata, deleteSession)
ui/desktop/src/goosed.ts	Adds error logging and removes server status check for external backend
ui/desktop/src/components/sessions/SessionsInsights.tsx	Updates to use new API session insights instead of custom fetch
ui/desktop/src/components/sessions/SessionListView.tsx	Migrates to use API functions for session operations
ui/desktop/src/api/types.gen.ts	Adds new API types for session insights and metadata operations
ui/desktop/src/api/sdk.gen.ts	Adds generated API functions for session management
crates/goose/src/session/session_manager.rs	New SQLite-based session manager implementation
crates/goose/src/session/legacy.rs	Legacy session file reader for migration purposes
crates/goose/src/agents/agent.rs	Updates agent code to use SessionManager instead of file operations
crates/goose-server/src/routes/session.rs	Updates server routes to use SessionManager
crates/goose-cli/src/session/mod.rs	Updates CLI session handling to use SessionManager

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

ui/desktop/src/components/sessions/SessionsInsights.tsx

crates/goose/src/session/session_manager.rs

crates/goose/src/session/legacy.rs

crates/goose-server/src/routes/session.rs

crates/goose-cli/src/commands/web.rs

jamadeo · 2025-09-22T13:34:38Z

crates/goose/src/session/session_manager.rs

+                let storage = Arc::new(SessionStorage::new().await?);
+                match SESSION_STORAGE.set(storage.clone()) {
+                    Ok(_) => Ok(storage),
+                    Err(_) => Ok(SESSION_STORAGE.get().unwrap().clone()),


I think you do want to check the variants: https://docs.rs/tokio/latest/tokio/sync/enum.SetError.html#variants

the LLMs said:

async fn instance() -> Result<Arc> {
let storage = Arc::new(SessionStorage::new().await?);
match SESSION_STORAGE.set(storage) {
Ok() => Ok(SESSION_STORAGE.get().unwrap().clone()),
Err() => Ok(SESSION_STORAGE.get().unwrap().clone()),
}
}

should do the trick, since failure means we already have the thing

I think that's true for a regular OnceCell, but this is a tokio::OnceCell, which can fail to set if it's being concurrently set

If the OnceCell is empty, but some other task is currently trying to set the value, this call will fail with SetError::InitializingError.

crates/goose/src/session/session_manager.rs

crates/goose-cli/src/commands/web.rs

jamadeo · 2025-09-23T01:30:16Z

crates/goose-cli/src/commands/web.rs

-                        {
-                            error!("Failed to persist compacted messages: {}", e);
-                        }
+                        tracing::info!("History replaced, compacting happened in reply");


not in scope here, but have we considered how compaction should work with session history? seems like we should store the both the old and compacted state of things

I think the idea is that we keep all messages around but annotate them with visibility. Hmm, is this already in /cc @katzdave

Yes we have message metadata tagging messages appropriately as agent vs user visible.

I could see benefits of even more metadata tags like 'summary' being relevant.

I'm pushing this domain actively both in the UI for making the post/pre-summary messages more clear, and I have some other ideas around improving the summaries with with keeping around more user messages + messages at the end of tool call chains.

jamadeo · 2025-09-23T01:37:16Z

crates/goose-server/src/routes/agent.rs

+
+        session = SessionManager::get_session(&session.id, false)
+            .await
+            .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;


can we do something other than throw away the error here? maybe at minimum a log

why particularly here? this should never fail I think - we're just refreshing the session after updating it

maybe we leave it for another error handling PR. I just think returning 500s without error messages isn't great because there's always an extra step when debugging

jamadeo · 2025-09-23T01:41:08Z

crates/goose/src/conversation/mod.rs

+                .0
+                .iter()
+                .zip(&other.0)
+                .all(|(a, b)| a.role == b.role && a.created == b.created && a.content == b.content)


should we not defer to equivalence of the Messages ?

crates/goose-server/src/openapi.rs

ui/desktop/src/api/types.gen.ts

Copilot

Pull Request Overview

Copilot reviewed 41 out of 43 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

crates/goose/src/agents/agent.rs:1

This TODO indicates that database updates are missing when handling interruptions. This could lead to session state inconsistencies and should be addressed.

use std::collections::HashMap;

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

crates/goose/src/agents/agent.rs

crates/goose-cli/src/session/mod.rs

crates/goose-cli/src/commands/web.rs

ui/desktop/src/goosed.ts

crates/goose-cli/src/commands/bench.rs

jamadeo · 2025-09-26T01:55:07Z

crates/goose-cli/src/commands/web.rs

+    match SessionManager::create_session(
+        std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")),
+        "Web session".to_string(),
+    )


suggest something like

let session = SessionManager::create_session(..).await.map_err(..)?; Redirect::permanent(..)

hmm, also shouldn't be a permanent redirect I think

crates/goose-cli/src/commands/web.rs

jamadeo · 2025-09-26T02:19:41Z

crates/goose-cli/src/session/builder.rs

+        )
+        .await
+        .unwrap();
+        Some(session.id)


this function should probably not panic or exit(1) but that's too much yak shaving

this is the cli though so it is not the end of the world (well, for the cli it is) - but I agree since who knows who might thing it is a good idea to call this builder. pre-existing ...

jamadeo · 2025-09-26T02:26:13Z

crates/goose-cli/src/session/mod.rs

+                        .await
+                        .map(|session| session.conversation.unwrap_or_default())
+                        .unwrap_or_else(|e| {
+                            eprintln!("Warning: Failed to load message history: {}", e);


I guess this isn't new but seems weird that this would just warn and continue

so now you do want to panic? :)

crates/goose-cli/src/session/mod.rs

jamadeo · 2025-09-26T03:04:28Z

crates/goose/src/session/session_manager.rs

+                let storage = Arc::new(SessionStorage::new().await?);
+                match SESSION_STORAGE.set(storage.clone()) {
+                    Ok(_) => Ok(storage),
+                    Err(_) => Ok(SESSION_STORAGE.get().unwrap().clone()),


I think that's true for a regular OnceCell, but this is a tokio::OnceCell, which can fail to set if it's being concurrently set

If the OnceCell is empty, but some other task is currently trying to set the value, this call will fail with SetError::InitializingError.

alexhancock · 2025-09-26T14:59:38Z

crates/goose/src/session/session_manager.rs

+        let current_version = self.get_schema_version().await?;
+
+        if current_version < CURRENT_SCHEMA_VERSION {
+            println!(


Perhaps we configure the logging utility so all output from these modules goes somewhere like:

/Users/alexhancock/.local/state/goose/logs/session-db

alongside the others?

replaced the println!

alexhancock · 2025-09-26T15:01:38Z

crates/goose/src/session/session_manager.rs

+            println!("Creating new session database...");
+            let storage = Self::create(&db_path).await?;
+
+            if let Err(e) = storage.import_legacy(&session_dir).await {


Would be good to test this with a few real session dirs from the wild to ensure no issues before launch

I imported 8000 from my folder...

alexhancock · 2025-09-26T15:05:38Z

crates/goose/src/session/session_manager.rs

+        .await?
+        .unwrap_or(0);
+
+        let session_id = format!("{}_{}", today, max_idx + 1);


Do we ever use the fact that the ids have these dates in them? If not, could just be an autoincrementing int in sqlite.

alexhancock · 2025-09-26T15:12:19Z

crates/goose/src/session/legacy.rs

+    let modified_time = file_metadata.modified().unwrap_or(SystemTime::now());
+    let created_time = file_metadata
+        .created()
+        .unwrap_or_else(|_| parse_session_timestamp(session_name).unwrap_or(modified_time));


were the old session names human readable title strings? or is this a different field from the metadata and called session_name in this code

nah, we just called it that. they were date strings. description is the human readable version

alexhancock · 2025-09-26T15:15:14Z

ui/desktop/src/components/sessions/SessionListView.tsx

-          s.id === sessionId
-            ? { ...s, metadata: { ...s.metadata, description: newDescription } }
-            : s
+          s.id === sessionId ? { ...s, metadata: { ...s, description: newDescription } } : s


do we need all the props at both root and metadata of this object? it seems like how we've flattened out the data model a lot this object could become flattened as well

oops. there is no more metadata, this is just a remnant that will set this useless property that we then don't display until we get confirmation from the server. nice catch!

alexhancock

Looks like some conflicts but once resolved 🚢

DOsinga · 2025-09-26T15:38:36Z

Looks like some conflicts but once resolved 🚢

yeah, with this type of thing, I'd rather let the conflicts vester than keep updating them

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: HikaruEgashira <[email protected]>

r0x0d · 2025-10-07T16:52:39Z

crates/goose-cli/src/session/mod.rs

+        let messages = if let Some(session_id) = &session_id {
+            tokio::task::block_in_place(|| {
+                tokio::runtime::Handle::current().block_on(async {
+                    SessionManager::get_session(session_id, true)
+                        .await
+                        .map(|session| session.conversation.unwrap_or_default())
+                        .unwrap()
+                })


Hi, @DOsinga! Since this got merged, I started seeing this error:

thread 'main' panicked at crates/goose-cli/src/session/mod.rs:136:26: called `Result::unwrap()` on an `Err` value: Session not found note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Not sure if a good patch, but this fixed the issue for me locally: #5055

This bug got introduced in when merging dc29288 (block#4648). The problem was that the code was trying to load a session id from the database that does not exist.

This bug got introduced in when merging dc29288 (block#4648). The problem was that the code was trying to load a session id from the database that does not exist. Signed-off-by: Rodolfo Olivieri <[email protected]>

Douwe Osinga added 8 commits September 14, 2025 10:00

Step 1

f8c44da

Speed up insights

af727bb

Session manager

e3e6f12

Some more

5dfb4d3

WIP

e7d76d1

SLUG

5e9742c

Now it compiles!

4f6961c

Refactor again

79b4b25

jamadeo requested a review from Copilot September 22, 2025 13:21

Copilot AI reviewed Sep 22, 2025

View reviewed changes

WIP

d2cc1d1

jamadeo reviewed Sep 23, 2025

View reviewed changes

Douwe Osinga added 3 commits September 23, 2025 11:02

Make it compile. Again

a82fe2d

Build the db correctly

decaa9d

Fix the thing!

54f8d2a

DOsinga requested a review from Copilot September 23, 2025 21:20

Copilot AI reviewed Sep 23, 2025

View reviewed changes

crates/goose/src/agents/agent.rs Show resolved Hide resolved

crates/goose-cli/src/session/mod.rs Show resolved Hide resolved

crates/goose-cli/src/commands/web.rs Outdated Show resolved Hide resolved

ui/desktop/src/goosed.ts Show resolved Hide resolved

Douwe Osinga added 11 commits September 24, 2025 14:42

More

a98c9b7

update description

064debb

Make web work

e473cd4

Small changes

1eca68f

Check point

58c727f

Also the client

6b27bb6

yeah remove

5a1bad3

Get rid of session history

ba34320

Session insights

1289021

Fix a test

e141e4a

What jack said

e4b3288

jamadeo approved these changes Sep 26, 2025

View reviewed changes

jamadeo mentioned this pull request Sep 26, 2025

feat: Easy session failure reporting #4183

Closed

6 tasks

alexhancock reviewed Sep 26, 2025

View reviewed changes

alexhancock self-requested a review September 26, 2025 15:25

alexhancock reviewed Sep 26, 2025

View reviewed changes

Douwe Osinga added 9 commits September 26, 2025 11:59

Various review things

53b9d19

Merge branch 'main' into session-manager

ed3cccc

Either one!

e1aa27f

Fix more issues

4b9111e

Merge branch 'main' into session-manager

a1262a0

Remove unused

9d53bb7

HUh they unflushed

705384c

Hmm, this is getting weird

d86d820

Jack's clippy

5cd2744

DOsinga merged commit dc29288 into main Sep 26, 2025
10 checks passed

DOsinga deleted the session-manager branch September 26, 2025 18:56

jsibbison-square mentioned this pull request Sep 30, 2025

Cant name a session in cli #4893

Closed

HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 2025

Session manager (block#4648)

c7d773b

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: HikaruEgashira <[email protected]>

wpfleger96 mentioned this pull request Oct 6, 2025

Using --name Without --resume No Longer Works As Expected #5044

Closed

r0x0d reviewed Oct 7, 2025

View reviewed changes

r0x0d mentioned this pull request Oct 7, 2025

handle non-existent sessions gracefully instead of panicking #5055

Closed

9 tasks

r0x0d mentioned this pull request Oct 7, 2025

history.txt should be stored in ~/.local/share (XDG_DATA_HOME or XDG_STATE_HOME) #2493

Closed

This was referenced Oct 8, 2025

chore(release): release version 1.10.0 #5060

Closed

release/1.10.0 #5101

Closed

coderabbitai bot mentioned this pull request Mar 26, 2026

refactor: eval quality fixes — dedup, types, docs, tests soilSpoon/opengoose#359

Merged

4 tasks

Conversation

DOsinga commented Sep 15, 2025

Use SQLite for Session manager

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key changes:

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!