Skip to content

fix: session Update clobbers user_turns from stale in-memory structs#766

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-fdf904aa
Closed

fix: session Update clobbers user_turns from stale in-memory structs#766
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-fdf904aa

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Removed user_turns from SQLiteStore.Update so metadata-only updates no longer write a caller-supplied turn count back into sessions.
  • Updated the method comment to treat user_turns like the other counters that must be mutated through atomic store paths.
  • Added TestUpdateDoesNotClobberUserTurns to cover the failure mode where IncrementUserTurns is followed by Update on a stale in-memory session struct.

Why this is high-value

user_turns is a durable counter used for user-visible state and downstream logic. In the current code, IncrementUserTurns atomically increments the database value, but a later Update call can overwrite that increment with a stale in-memory value. The Telegram path already does exactly that when it increments the counter and then persists the first-message summary without first updating sess.meta.UserTurns. This fix removes the clobber path entirely and keeps turn-count mutations on the atomic update path.

Validation

  • Verified the issue is real in current code by inspecting internal/session/sqlite.go and the Telegram call path in internal/serve/telegram.go.
  • Added and ran targeted tests:
    • go test ./internal/session -run 'TestUpdateDoesNotClobber(TokenMetrics|UserTurns)$'
  • Ran full validation:
    • go build ./...
    • go test ./...
  • Ran diff hygiene checks:
    • git diff --check

@SamSaffron

Copy link
Copy Markdown
Owner

Implemented the important fixes directly in the main worktree; closing this PR.

@SamSaffron SamSaffron closed this Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants