From cd781b58df56301fecb5165497dd3f464218e0d0 Mon Sep 17 00:00:00 2001 From: Fabrizio Chignoli Date: Thu, 25 Jun 2026 10:57:12 +0200 Subject: [PATCH] fix(statedb): preserve detected session-id keys on stale full-table saves A freshly-launched session's claude_session_id (and the gemini/opencode/codex equivalents) could be silently wiped from state.db, surfacing as "no session ID detected" on a session whose transcript is actively being written. Root cause: the per-tool conversation ids are detected asynchronously and persisted shortly after launch, but any concurrent full-table save (SaveWithGroups -> SaveInstances) whose in-memory snapshot has not yet observed the id rebuilds tool_data with the id omitted (omitempty zero-value). MergeToolDataExtras treated every typed/known key as authoritative "including absence", so the omitted id cleared the previously-persisted value instead of being preserved. The rename hook (hook_name_sync) is the hottest such writer; it fires on every Claude hook. Fix: treat the session-id keys ({claude,gemini,opencode,codex}_session_id and their *_detected_at) as sticky in MergeToolDataExtras. When the new blob OMITS a sticky key but the old row has it, carry the old value forward. A non-empty new value still wins (a real resume/fork that changes the id), and an explicit empty present in the new blob is honored as an intentional clear -- only outright omission preserves. Extends tool_data_extras_test.go: repoints the typed-absence test to a non-sticky key (latest_prompt) and adds sticky coverage for preserve-on- omit, non-empty-new-wins, and explicit-empty-clears. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Wm3xPWp2Wm5B5QjSHkEnXD --- internal/statedb/tool_data_extras.go | 42 +++++++++++- internal/statedb/tool_data_extras_test.go | 80 +++++++++++++++++++++-- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/internal/statedb/tool_data_extras.go b/internal/statedb/tool_data_extras.go index 0568a42f8..85324b10d 100644 --- a/internal/statedb/tool_data_extras.go +++ b/internal/statedb/tool_data_extras.go @@ -24,6 +24,19 @@ import ( // setting a key wins over the old value (no silent override of intended // updates). Only keys that are completely unknown to the typed schema AND // absent from the new blob are carried forward. +// +// EXCEPTION — sticky detected session-id keys (stickyToolDataKeys): the +// per-tool conversation ids (claude_session_id / gemini_session_id / +// opencode_session_id / codex_session_id and their *_detected_at) are +// write-once-per-conversation identity, populated asynchronously after a +// session starts. They are treated like extras for the ABSENCE case: when the +// new blob OMITS them (omitempty zero-value) but the old row HAS them, the old +// value is carried forward, so an unrelated full-table save whose in-memory +// snapshot simply has not detected the id yet can no longer silently wipe a +// live session's mapping (t-0133). A NON-EMPTY new value still wins (a real +// resume/fork that changes the id), and an EXPLICIT empty value present in the +// new blob (`"claude_session_id":""`) is honored as an intentional clear — only +// outright OMISSION is treated as "unaware writer, preserve". func MergeToolDataExtras(oldToolData, newToolData json.RawMessage) json.RawMessage { if len(oldToolData) == 0 { return newToolData @@ -49,13 +62,14 @@ func MergeToolDataExtras(oldToolData, newToolData json.RawMessage) json.RawMessa } known := toolDataKnownKeys() + sticky := stickyToolDataKeys() merged := false for k, v := range oldMap { - if known[k] { - continue // typed schema is authoritative + if known[k] && !sticky[k] { + continue // typed schema is authoritative (except sticky identity keys) } if _, exists := newMap[k]; exists { - continue // new explicitly set this key + continue // new explicitly set this key (incl. explicit empty = intentional clear) } newMap[k] = v merged = true @@ -71,6 +85,28 @@ func MergeToolDataExtras(oldToolData, newToolData json.RawMessage) json.RawMessa return out } +// stickyToolDataKeys returns the typed tool_data keys that MergeToolDataExtras +// preserves from the old row when the new blob OMITS them (rather than letting +// omitempty-absence clear them). These are the per-tool conversation ids and +// their detection timestamps: identity that is detected asynchronously and is +// write-once for the life of a conversation. Keeping them sticky prevents a +// concurrent full-table save (e.g. the rename hook's LoadWithGroups + +// SaveWithGroups) whose snapshot has not yet observed the id from wiping a live +// session's session-id mapping (t-0133). A non-empty new value, or an explicit +// empty present in the new blob, still wins — only outright omission preserves. +func stickyToolDataKeys() map[string]bool { + return map[string]bool{ + "claude_session_id": true, + "claude_detected_at": true, + "gemini_session_id": true, + "gemini_detected_at": true, + "opencode_session_id": true, + "opencode_detected_at": true, + "codex_session_id": true, + "codex_detected_at": true, + } +} + // toolDataKnownKeys returns the set of JSON keys that toolDataBlob explicitly // models. Used by MergeToolDataExtras to distinguish agent-deck's // authoritative schema from externally-managed extras. diff --git a/internal/statedb/tool_data_extras_test.go b/internal/statedb/tool_data_extras_test.go index 38479647a..b4119d024 100644 --- a/internal/statedb/tool_data_extras_test.go +++ b/internal/statedb/tool_data_extras_test.go @@ -43,18 +43,86 @@ func TestMergeToolDataExtras_NewExplicitWinsOverOldUnknown(t *testing.T) { } func TestMergeToolDataExtras_TypedKeyAbsenceRespected(t *testing.T) { - // When the new tool_data omits a typed key (e.g., omitempty zero-value), - // the merge must NOT carry the old value forward. The typed schema is - // authoritative for typed fields. This protects intentional clears. - old := json.RawMessage(`{"claude_session_id":"abc"}`) + // When the new tool_data omits a NON-STICKY typed key (e.g., omitempty + // zero-value), the merge must NOT carry the old value forward. The typed + // schema is authoritative for those fields. (Sticky session-id keys are the + // deliberate exception, covered by the sticky tests below.) + old := json.RawMessage(`{"latest_prompt":"hello"}`) new_ := json.RawMessage(`{}`) merged := MergeToolDataExtras(old, new_) var got map[string]json.RawMessage _ = json.Unmarshal(merged, &got) - if _, present := got["claude_session_id"]; present { - t.Errorf("claude_session_id should be absent in merged when new omits it; got %s", got["claude_session_id"]) + if _, present := got["latest_prompt"]; present { + t.Errorf("latest_prompt should be absent in merged when new omits it; got %s", got["latest_prompt"]) + } +} + +func TestMergeToolDataExtras_StickySessionIDPreservedWhenNewOmits(t *testing.T) { + // t-0133: a session-id key present in the old row but OMITTED by the new + // blob (omitempty zero-value, e.g. a concurrent full-table save whose + // in-memory snapshot has not yet detected the id) must be carried forward, + // not wiped. Covers all four tools + a *_detected_at companion. + old := json.RawMessage(`{` + + `"claude_session_id":"c-uuid","claude_detected_at":111,` + + `"gemini_session_id":"g-uuid",` + + `"opencode_session_id":"o-uuid",` + + `"codex_session_id":"x-uuid"}`) + new_ := json.RawMessage(`{"latest_prompt":"hi"}`) + + merged := MergeToolDataExtras(old, new_) + + var got map[string]json.RawMessage + if err := json.Unmarshal(merged, &got); err != nil { + t.Fatalf("merged JSON does not parse: %v", err) + } + for k, want := range map[string]string{ + "claude_session_id": `"c-uuid"`, + "claude_detected_at": `111`, + "gemini_session_id": `"g-uuid"`, + "opencode_session_id": `"o-uuid"`, + "codex_session_id": `"x-uuid"`, + } { + if string(got[k]) != want { + t.Errorf("%s = %s, want %s (sticky: preserved from old when new omits)", k, got[k], want) + } + } + // The new blob's own key still lands. + if string(got["latest_prompt"]) != `"hi"` { + t.Errorf("latest_prompt = %s, want \"hi\"", got["latest_prompt"]) + } +} + +func TestMergeToolDataExtras_StickyNonEmptyNewWins(t *testing.T) { + // A real resume/fork writes a NEW non-empty session id; that must win over + // the old value (sticky only protects the omission case, never overrides an + // explicit update). + old := json.RawMessage(`{"claude_session_id":"old-uuid"}`) + new_ := json.RawMessage(`{"claude_session_id":"new-uuid"}`) + + merged := MergeToolDataExtras(old, new_) + + var got map[string]json.RawMessage + _ = json.Unmarshal(merged, &got) + if string(got["claude_session_id"]) != `"new-uuid"` { + t.Errorf("claude_session_id = %s, want \"new-uuid\" (non-empty new wins)", got["claude_session_id"]) + } +} + +func TestMergeToolDataExtras_StickyExplicitEmptyIsIntentionalClear(t *testing.T) { + // An EXPLICIT empty value present in the new blob (not omitted) is honored + // as an intentional clear — the escape hatch for a deliberate reset/dedup + // that wants the id gone. Only outright omission preserves. + old := json.RawMessage(`{"claude_session_id":"old-uuid"}`) + new_ := json.RawMessage(`{"claude_session_id":""}`) + + merged := MergeToolDataExtras(old, new_) + + var got map[string]json.RawMessage + _ = json.Unmarshal(merged, &got) + if string(got["claude_session_id"]) != `""` { + t.Errorf("claude_session_id = %s, want \"\" (explicit empty = intentional clear)", got["claude_session_id"]) } }