From 884a7372e2c46a0a8b26b489ac75cee6de8125ae Mon Sep 17 00:00:00 2001 From: QuentinBisson Date: Tue, 18 Aug 2026 13:11:24 +0200 Subject: [PATCH 1/2] fix: reject session create on a soft-deleted id Creating a session with the id of a soft-deleted one matched the tombstone through ON CONFLICT and updated it with deleted_at still set, so the row was invisible to every read path, including the create path's own reload, which then failed with an internal error on every retry. The caller was told the request failed while a write had happened. UpsertSession now guards its DO UPDATE with deleted_at IS NULL and returns no rows when the write is rejected, the same shape UpsertTask already uses, so a retired id is reported as a conflict and nothing is written. The tombstone and the events and tasks it owns are left exactly as they are. Fixes #2279 Signed-off-by: QuentinBisson --- go/api/database/client.go | 5 +++ go/core/internal/database/client_postgres.go | 12 +++++- go/core/internal/database/client_test.go | 42 +++++++++++++++++++ go/core/internal/database/gen/querier.go | 7 +++- go/core/internal/database/gen/sessions.sql.go | 17 ++++++-- .../internal/database/queries/sessions.sql | 11 ++++- .../internal/grpcserver/session_task_test.go | 19 +++++++++ go/core/internal/service/session/service.go | 6 +++ .../internal/service/session/service_test.go | 19 +++++++++ 9 files changed, 130 insertions(+), 8 deletions(-) diff --git a/go/api/database/client.go b/go/api/database/client.go index 8732fb3c7..4853b5e34 100644 --- a/go/api/database/client.go +++ b/go/api/database/client.go @@ -15,6 +15,11 @@ import ( // different user. var ErrTaskOwnedByAnotherUser = errors.New("task id owned by another user") +// ErrSessionIDRetired means the session id belongs to a soft-deleted session. +// The id is not reusable: the tombstone and the events and tasks it owns are +// left as they are. +var ErrSessionIDRetired = errors.New("session id belongs to a deleted session") + var ErrIdempotencyConflict = errors.New("request id was already used with different parameters") var ErrAgentInstanceConflict = errors.New("AgentInstance lifecycle operation conflicts with its current state") diff --git a/go/core/internal/database/client_postgres.go b/go/core/internal/database/client_postgres.go index 2a384873a..c6f4ecef3 100644 --- a/go/core/internal/database/client_postgres.go +++ b/go/core/internal/database/client_postgres.go @@ -109,7 +109,17 @@ func (c *postgresClient) StoreSession(ctx context.Context, session *dbpkg.Sessio src := string(*session.Source) params.Source = &src } - return q.UpsertSession(ctx, params) + // UpsertSession returns no rows when the write was rejected: the id + // belongs to a soft-deleted session (deleted ids stay burned). Under + // READ COMMITTED the guard is evaluated against the latest committed + // row, so a delete committing mid-statement still rejects the write. + if _, err := q.UpsertSession(ctx, params); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return dbpkg.ErrSessionIDRetired + } + return fmt.Errorf("failed to store session %s: %w", session.ID, err) + } + return nil }) } diff --git a/go/core/internal/database/client_test.go b/go/core/internal/database/client_test.go index 15c6cbb88..6df29c188 100644 --- a/go/core/internal/database/client_test.go +++ b/go/core/internal/database/client_test.go @@ -240,6 +240,48 @@ func TestStoreSessionIdempotence(t *testing.T) { require.Error(t, err, "another user's session must not be readable") } +// A soft-deleted session keeps its (id, user_id) row, so its id is burned: +// recreating it must fail loudly instead of updating a row that stays deleted +// and that no read path can see. +func TestDeletedSessionIdCannotBeReused(t *testing.T) { + db := setupTestDB(t) + client := NewClient(db) + ctx := context.Background() + + name := "First incarnation" + session := &dbpkg.Session{ID: "s-dead", UserID: "alice", Name: &name} + require.NoError(t, client.StoreSession(ctx, session)) + require.NoError(t, client.DeleteSession(ctx, "s-dead", "alice")) + + tombstone := func() (createdAt, deletedAt *time.Time, storedName *string) { + require.NoError(t, db.QueryRow(ctx, + "SELECT created_at, deleted_at, name FROM session WHERE id = $1 AND user_id = $2", + "s-dead", "alice").Scan(&createdAt, &deletedAt, &storedName)) + return + } + createdBefore, deletedBefore, _ := tombstone() + require.NotNil(t, deletedBefore, "delete must leave a tombstone") + + renamed := "Second incarnation" + err := client.StoreSession(ctx, &dbpkg.Session{ID: "s-dead", UserID: "alice", Name: &renamed}) + require.ErrorIs(t, err, dbpkg.ErrSessionIDRetired, "the owner must not silently resurrect a deleted id") + + createdAfter, deletedAfter, storedName := tombstone() + assert.Equal(t, createdBefore, createdAfter, "the tombstone's created_at must not move") + assert.Equal(t, deletedBefore, deletedAfter, "the tombstone must stay deleted") + assert.Equal(t, name, *storedName, "the rejected write must not have touched the tombstone") + + _, err = client.GetSession(ctx, "s-dead", "alice") + require.Error(t, err, "the session must stay deleted") + + // A tombstone is not a live row, so the same id under another user does not + // collide with it. + require.NoError(t, client.StoreSession(ctx, &dbpkg.Session{ID: "s-dead", UserID: "bob", Name: &renamed})) + bobs, err := client.GetSession(ctx, "s-dead", "bob") + require.NoError(t, err) + assert.Equal(t, renamed, *bobs.Name) +} + func TestListSessionsOrdersByRecentActivity(t *testing.T) { db := setupTestDB(t) client := NewClient(db) diff --git a/go/core/internal/database/gen/querier.go b/go/core/internal/database/gen/querier.go index 27982c941..babcff1b8 100644 --- a/go/core/internal/database/gen/querier.go +++ b/go/core/internal/database/gen/querier.go @@ -139,7 +139,12 @@ type Querier interface { UpsertCrewAIMemory(ctx context.Context, arg UpsertCrewAIMemoryParams) error UpsertPushNotification(ctx context.Context, arg UpsertPushNotificationParams) error UpsertRuntimeRevision(ctx context.Context, arg UpsertRuntimeRevisionParams) error - UpsertSession(ctx context.Context, arg UpsertSessionParams) error + // UpsertSession returns the upserted id, or no rows when the write was + // rejected: the id belongs to a soft-deleted session. A deleted id is never + // updated or resurrected, it stays burned, so the tombstone and the events and + // tasks it owns are left exactly as they are. Callers map "no rows" to a + // conflict error. + UpsertSession(ctx context.Context, arg UpsertSessionParams) (string, error) UpsertShareAccess(ctx context.Context, arg UpsertShareAccessParams) error // UpsertTask returns the upserted id, or no rows when the write was rejected: // the id belongs to another user, or it belongs to a soft-deleted task (a diff --git a/go/core/internal/database/gen/sessions.sql.go b/go/core/internal/database/gen/sessions.sql.go index e2f225a24..d025541a7 100644 --- a/go/core/internal/database/gen/sessions.sql.go +++ b/go/core/internal/database/gen/sessions.sql.go @@ -267,7 +267,7 @@ func (q *Queries) SoftDeleteSession(ctx context.Context, arg SoftDeleteSessionPa return err } -const upsertSession = `-- name: UpsertSession :exec +const upsertSession = `-- name: UpsertSession :one INSERT INTO session (id, user_id, name, agent_id, source, created_at, updated_at) VALUES ($1, $2, $3, $4, $5, NOW(), NOW()) ON CONFLICT (id, user_id) DO UPDATE SET @@ -275,6 +275,8 @@ ON CONFLICT (id, user_id) DO UPDATE SET agent_id = EXCLUDED.agent_id, source = EXCLUDED.source, updated_at = NOW() +WHERE session.deleted_at IS NULL +RETURNING id ` type UpsertSessionParams struct { @@ -285,13 +287,20 @@ type UpsertSessionParams struct { Source *string } -func (q *Queries) UpsertSession(ctx context.Context, arg UpsertSessionParams) error { - _, err := q.db.Exec(ctx, upsertSession, +// UpsertSession returns the upserted id, or no rows when the write was +// rejected: the id belongs to a soft-deleted session. A deleted id is never +// updated or resurrected, it stays burned, so the tombstone and the events and +// tasks it owns are left exactly as they are. Callers map "no rows" to a +// conflict error. +func (q *Queries) UpsertSession(ctx context.Context, arg UpsertSessionParams) (string, error) { + row := q.db.QueryRow(ctx, upsertSession, arg.ID, arg.UserID, arg.Name, arg.AgentID, arg.Source, ) - return err + var id string + err := row.Scan(&id) + return id, err } diff --git a/go/core/internal/database/queries/sessions.sql b/go/core/internal/database/queries/sessions.sql index 9900723ca..ec7f7f2bc 100644 --- a/go/core/internal/database/queries/sessions.sql +++ b/go/core/internal/database/queries/sessions.sql @@ -32,14 +32,21 @@ WHERE agent_id = $1 AND deleted_at IS NULL AND (source IS NULL OR source != 'agent') ORDER BY updated_at DESC, created_at DESC; --- name: UpsertSession :exec +-- UpsertSession returns the upserted id, or no rows when the write was +-- rejected: the id belongs to a soft-deleted session. A deleted id is never +-- updated or resurrected, it stays burned, so the tombstone and the events and +-- tasks it owns are left exactly as they are. Callers map "no rows" to a +-- conflict error. +-- name: UpsertSession :one INSERT INTO session (id, user_id, name, agent_id, source, created_at, updated_at) VALUES ($1, $2, $3, $4, $5, NOW(), NOW()) ON CONFLICT (id, user_id) DO UPDATE SET name = EXCLUDED.name, agent_id = EXCLUDED.agent_id, source = EXCLUDED.source, - updated_at = NOW(); + updated_at = NOW() +WHERE session.deleted_at IS NULL +RETURNING id; -- name: SoftDeleteSession :exec UPDATE session SET deleted_at = NOW() diff --git a/go/core/internal/grpcserver/session_task_test.go b/go/core/internal/grpcserver/session_task_test.go index 173298585..ead3353bc 100644 --- a/go/core/internal/grpcserver/session_task_test.go +++ b/go/core/internal/grpcserver/session_task_test.go @@ -40,6 +40,9 @@ type generatedClientSessionTaskStore struct { lastTaskListUserID string recordedShareUserID string recordedShareID int64 + // retired maps a deleted session id to its owner: a soft delete keeps the + // row, so the id stays burned for that user. + retired map[string]string } func newGeneratedClientSessionTaskStore() *generatedClientSessionTaskStore { @@ -50,10 +53,14 @@ func newGeneratedClientSessionTaskStore() *generatedClientSessionTaskStore { shares: make(map[string]*database.SessionShare), tasks: make(map[string]*a2a.Task), taskOwners: make(map[string]string), + retired: make(map[string]string), } } func (s *generatedClientSessionTaskStore) StoreSession(_ context.Context, value *database.Session) error { + if owner, ok := s.retired[value.ID]; ok && owner == value.UserID { + return database.ErrSessionIDRetired + } copy := *value if copy.CreatedAt.IsZero() { copy.CreatedAt = time.Date(2026, time.August, 2, 10, 0, 0, 0, time.UTC) @@ -111,6 +118,7 @@ func (s *generatedClientSessionTaskStore) DeleteSession(_ context.Context, id, u return database.ErrNotFound } delete(s.sessions, id) + s.retired[id] = userID return nil } @@ -411,6 +419,17 @@ func TestSessionAndTaskGeneratedClients(t *testing.T) { if _, err := sessionClient.DeleteSession(userContext, &apiv1alpha1.DeleteSessionRequest{SessionId: sessionID}); err != nil { t.Fatalf("DeleteSession() error = %v", err) } + + // Recreating a deleted id must conflict rather than report a server fault, + // so a client can tell a retired id from an unhealthy server. + _, err = sessionClient.CreateSession(userContext, &apiv1alpha1.CreateSessionRequest{ + Id: &sessionID, + AgentRef: "default/agent", + Name: &name, + }) + if status.Code(err) != codes.AlreadyExists { + t.Fatalf("CreateSession() on a deleted id error = %v, want already exists", err) + } } func assertTaskObject(t *testing.T, object *a2apb.Task, taskID, contextID string) { diff --git a/go/core/internal/service/session/service.go b/go/core/internal/service/session/service.go index 6ae275dd1..f740909d0 100644 --- a/go/core/internal/service/session/service.go +++ b/go/core/internal/service/session/service.go @@ -178,6 +178,9 @@ func (s *Service) Create(ctx context.Context, request CreateRequest) (*database. Source: request.Source, } if err := s.store.StoreSession(ctx, value); err != nil { + if errors.Is(err, database.ErrSessionIDRetired) { + return nil, serviceerrors.NewAlreadyExists("Session ID belongs to a deleted session", err) + } return nil, serviceerrors.NewInternal("Failed to create session", err) } stored, err := s.store.GetSession(ctx, id, userID) @@ -245,6 +248,9 @@ func (s *Service) Update(ctx context.Context, request UpdateRequest) (*database. session.AgentID = &agent.ID } if err := s.store.StoreSession(ctx, session); err != nil { + if errors.Is(err, database.ErrSessionIDRetired) { + return nil, serviceerrors.NewNotFound("Session not found", err) + } return nil, serviceerrors.NewInternal("Failed to update session", err) } return session, nil diff --git a/go/core/internal/service/session/service_test.go b/go/core/internal/service/session/service_test.go index 8795bb55f..7ddf59c45 100644 --- a/go/core/internal/service/session/service_test.go +++ b/go/core/internal/service/session/service_test.go @@ -275,3 +275,22 @@ func TestMissingAuthenticationAndStoreErrorsAreCanonical(t *testing.T) { t.Fatalf("Get() error = %v, want internal", err) } } + +func TestCreateAndUpdateMapRetiredSessionID(t *testing.T) { + store := newSessionTestStore() + store.agents["default__NS__agent"] = &database.Agent{ID: "default__NS__agent"} + store.storeSessionError = database.ErrSessionIDRetired + id := "retired-session" + + _, err := NewService(store).Create(sessionContext("user-a", ""), CreateRequest{ID: &id, AgentRef: "default/agent"}) + if !serviceerrors.IsCode(err, serviceerrors.CodeAlreadyExists) { + t.Fatalf("Create() error = %v, want already exists", err) + } + + store.sessions[id] = &database.Session{ID: id, UserID: "user-a"} + name := "renamed" + _, err = NewService(store).Update(sessionContext("user-a", ""), UpdateRequest{SessionID: id, Name: &name}) + if !serviceerrors.IsCode(err, serviceerrors.CodeNotFound) { + t.Fatalf("Update() error = %v, want not found", err) + } +} From 00978409c142ac9d9264fb59a47a3cfa08fca3b3 Mon Sep 17 00:00:00 2001 From: QuentinBisson Date: Wed, 19 Aug 2026 01:48:10 +0200 Subject: [PATCH 2/2] docs: clarify that a retired session id frees up after retention Signed-off-by: QuentinBisson --- go/api/database/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/go/api/database/client.go b/go/api/database/client.go index 4853b5e34..1eb2cb45d 100644 --- a/go/api/database/client.go +++ b/go/api/database/client.go @@ -16,8 +16,9 @@ import ( var ErrTaskOwnedByAnotherUser = errors.New("task id owned by another user") // ErrSessionIDRetired means the session id belongs to a soft-deleted session. -// The id is not reusable: the tombstone and the events and tasks it owns are -// left as they are. +// The id stays unusable for as long as the tombstone exists, and the tombstone +// and the events and tasks it owns are left as they are. Once retention +// hard-deletes the tombstone the id inserts normally again. var ErrSessionIDRetired = errors.New("session id belongs to a deleted session") var ErrIdempotencyConflict = errors.New("request id was already used with different parameters")