Skip to content

fix(sessions): use UPSERT in Save() to persist first-run cron sessions#553

Open
duhd-vnpay wants to merge 1 commit intonextlevelbuilder:mainfrom
duhd-vnpay:fix/session-save-upsert
Open

fix(sessions): use UPSERT in Save() to persist first-run cron sessions#553
duhd-vnpay wants to merge 1 commit intonextlevelbuilder:mainfrom
duhd-vnpay:fix/session-save-upsert

Conversation

@duhd-vnpay
Copy link
Copy Markdown

Fixes #379

Problem

PGSessionStore.Save() uses UPDATE sessions ... WHERE session_key = $19. When a cron job runs for the first time, the session row doesn't exist in DB yet. The UPDATE affects 0 rows and returns nilsilently losing the session.

This causes two cascading failures:

  1. Cron sessions never persist — exist only in memory, lost on restart
  2. Stale cache on retryExecuteWithRetry reads leftover messages from the failed attempt's in-memory cache, polluting retries with empty user messages + "..." assistant fallback. The stale messages cause LLM 400 errors (messages.0.content.0.text.text: Field required), and all retry attempts fail with the same error.

Fix

Change Save() from UPDATE to INSERT ... ON CONFLICT (tenant_id, session_key) DO UPDATE SET ....

Before:

UPDATE sessions SET messages = $1, ... WHERE session_key = $19 AND tenant_id = $20
-- 0 rows affected → silent data loss

After:

INSERT INTO sessions (id, session_key, tenant_id, messages, ...)
VALUES ($1, $2, $3, $4, ...)
ON CONFLICT (tenant_id, session_key) DO UPDATE SET
  messages = EXCLUDED.messages, ...
-- Always persists: INSERT on first run, UPDATE on subsequent saves

Why UPSERT over alternatives?

Option Pro Con
A: UPSERT (this PR) Single atomic operation, no conditional logic Generates new UUID on every save (ignored on conflict)
B: Check + INSERT Two queries Race condition between check and insert
C: Ensure row before agent loop Requires caller discipline Misses edge cases

The UUID generation cost on conflict is negligible — uuid.NewV7() is ~50ns.

Changes

File Lines Change
internal/store/pg/sessions_list.go +32, -9 UPDATEINSERT ... ON CONFLICT ... DO UPDATE

Verification

go build ./...                   # ✅ PG build
go build -tags sqliteonly ./...  # ✅ SQLite build
go vet ./...                     # ✅ Clean

The fix uses the same ON CONFLICT (tenant_id, session_key) unique index already used by GetOrCreate() (line 146 of sessions.go), confirming the constraint exists.

🤖 Generated with Claude Code

Save() used UPDATE-only, which silently affected 0 rows when the session
didn't exist in DB yet (first-run cron jobs). This caused two problems:

1. Cron sessions never persisted — lost on restart
2. Stale in-memory cache polluted retry attempts with leftover messages

Change UPDATE to INSERT...ON CONFLICT (tenant_id, session_key) DO UPDATE
so first-run sessions are created atomically and subsequent saves update
the existing row.

Fixes nextlevelbuilder#379

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
duhd-vnpay added a commit to duhd-vnpay/goclaw that referenced this pull request Mar 30, 2026
Key changes (21 commits):
- fix(security): harden env file permissions, block NUL byte injection
- fix(agent): panic recovery, unblock stuck /stop, graceful shutdown
- fix(providers): prevent crash on cancel, nil guard, thinking signature
- refactor(cron): normalize payload columns into dedicated DB fields (000033)
- feat(cron): add stateless mode + promote payload fields
- fix(store): session Save() UPSERT fallback (upstream merged our PR nextlevelbuilder#553)
- fix(config): MCP env resolution, channel field filter, orphan provider
- fix(agent): group session unresponsive during team task
- fix(tools): delivered media tracker prevents duplicate delivery
- fix(channels): display name in [From:] for Telegram/WhatsApp
- feat(ui): KG graph depth visualization, perf 808ms→<100ms

Local patches preserved:
- Project-as-a-channel (ConsumerDeps, MCP project overrides)
- Security audit logs (memory.go)
- Zalo OA bot fixes

Migration collision fixed:
- 000033_system_configs → 000037 (was duplicate with cron_payload_columns)
- RequiredSchemaVersion bumped to 37

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

Session Save() uses UPDATE-only — cron job sessions never persist, retry reads stale cache

1 participant