feat(session): add per-message created_at timestamps#2788
feat(session): add per-message created_at timestamps#2788LiusCraft wants to merge 9 commits intosipeed:mainfrom
Conversation
| return nil | ||
| } | ||
| if msg.CreatedAt.IsZero() { | ||
| msg.CreatedAt = time.Now() |
There was a problem hiding this comment.
It's not optimal to assigns time.Now() before acquiring the per-session lock. Two concurrent writers can therefore persist messages in an order that does not match their timestamps, which breaks chronology and any merge/sort logic that trusts created_at. Capture one now only after the lock is held, then use it for both msg.CreatedAt and meta.UpdatedAt.
| type Message struct { | ||
| Role string `json:"role"` | ||
| Content string `json:"content"` | ||
| CreatedAt time.Time `json:"created_at,omitempty"` |
There was a problem hiding this comment.
Using omitempty on a time.Time field does not omit zero values with Go's encoding/json. Any uninitialized message written through SessionManager or rewriteJSONL will be serialized as 0001-01-01T00:00:00Z, which is garbage data masquerading as a real timestamp. Use *time.Time for an optional field, or guarantee population on every marshal path and add raw-JSON tests to prove zero timestamps are never written.
…ondition - Change CreatedAt from time.Time to *time.Time so omitempty works correctly with encoding/json (zero time.Time struct is never omitted) - Move time.Now() inside per-session lock in addMsg to ensure timestamp ordering matches write order - Use the same now value for msg.CreatedAt and meta timestamps
|
Thanks for the review! I've addressed all the feedback — @afjcjsbx could you take another look when you get a chance? |
|
|
||
| now := time.Now() | ||
| if msg.CreatedAt == nil { | ||
| msg.CreatedAt = &now |
There was a problem hiding this comment.
When using the JSONL backend, this timestamp is added only to the local copy passed into addMsg; callers such as turnState.recordPersistedMessage still keep the same message with CreatedAt == nil. After a mid-turn restore-point refresh (for example context-window retry compression), refreshRestorePointFromSession reads the persisted messages back with non-nil CreatedAt and compares them to the recorded messages with reflect.DeepEqual, so the current turn tail no longer matches and gets included in the rollback snapshot; aborting that turn can then leave partially persisted current-turn messages in history.
There was a problem hiding this comment.
@alexhoshina The CreatedAt normalization (setting to nil on both sides before DeepEqual) in refreshRestorePointFromSession fixes the specific regression this PR introduced. However, after deeper analysis, there is a pre-existing issue with the same tail-matching logic: fields tagged with json:"-" — such as ToolCall.Name, ToolCall.Arguments, and Message.PromptLayer — are also lost during the JSONL roundtrip, so reflect.DeepEqual would still mismatch when comparing an in-memory persisted message (which has these fields populated) with a deserialized history message (which does not).
This means the tail-matching heuristic is fundamentally fragile — it breaks on any field that differs between the in-memory and serialized representations, not just CreatedAt.
I think the proper long-term fix is to introduce a unique MessageID so we can use set-subtraction instead of content-based DeepEqual matching. For now, the CreatedAt nil normalization is sufficient to fix this PR's regression. The json:"-" issue is pre-existing and unrelated to this PR — I'll file a separate PR to address it.
…romSession The CreatedAt field on messages persisted via JSONL loses its monotonic clock portion during roundtrip, causing reflect.DeepEqual to fail when matchingTurnMessageTail compares in-memory persistedMessages against deserialized history. Normalize both sides to nil before comparing. Also remove the addMsg-level CreatedAt fallback since every caller now explicitly sets CreatedAt on the messages it creates.
…reatedAt The previous approach mutated CreatedAt=nil on input slices before calling reflect.DeepEqual, which could pollute shared state. Replace with a comparison function that copies structs locally and nils CreatedAt on the copies, making the comparison side-effect-free.
|
Thanks for the reviews! Both issues from the initial review have been addressed in commit
Regarding the DeepEqual tail-matching regression raised by @alexhoshina — @afjcjsbx @alexhoshina could you take another look when you get a chance? The blocking |
3 participants
Background
Session API (
GET /api/sessions/{id}) returns messages without individual timestamps. The frontend has to use the session-levelupdatedtime for all messages, which is inaccurate when messages span different times.Changes
Backend
pkg/providers/protocoltypes/types.go— AddedCreatedAt time.Timefield toMessagestruct withjson:"created_at,omitempty"for backward compatibilitypkg/memory/jsonl.go— SetCreatedAt = time.Now()inaddMsgwhen the field is zero (after transient message check), so every stored message gets a timestampweb/backend/api/session.go— Exposecreated_atinsessionChatMessageAPI response; pass through fromproviders.Messageto all transcript message types (user, assistant, thought, tool_calls, visible tool messages); fallback to sessionupdatedtime for legacy messages with zeroCreatedAtpkg/memory/jsonl_test.go— AddedTestAddMessage_SetsCreatedAtverifying timestamps are set on writeFrontend
src/api/sessions.ts— Addedcreated_at?: stringtoSessionDetail.messagestypesrc/features/chat/history.ts— Usemessage.created_atwhen available (message.created_at ?? detail.updated)src/components/chat/user-message.tsx— Addedtimestampprop and displays formatted time below message bubblesrc/components/chat/assistant-message.tsx— Show timestamp in collapsed block header (thought, tool_calls) — previously only shown for normal assistant messagessrc/components/chat/chat-page.tsx— PasstimestamptoUserMessageBehavior Impact
created_attimestamps in API responsescreated_at) will use the sessionupdatedtime as fallback — no more0001-01-01T00:00:00ZMessageis never directly serialized to provider APIs)Risks and Rollback
omitempty,IsZero()guard, and backend fallback tosession.updatedVerification
go test ./pkg/memory/— 44/44 PASSgo test ./web/backend/api/— all PASSgo build ./pkg/providers/protocoltypes/ ./pkg/memory/ ./web/backend/api/— all passpnpm build:backendpassesNotes
Fixes #2787