Skip to content

Log session_id on engine session close - #293

Open
leifj wants to merge 1 commit into
mainfrom
fix/engine-session-eviction
Open

Log session_id on engine session close#293
leifj wants to merge 1 commit into
mainfrom
fix/engine-session-eviction

Conversation

@leifj

@leifj leifj commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Live triage of the Geneva 2026 test event (see sirosid-dev/gdc) reported apparent continuous engine-session flapping - sessions established then immediately closed - which looked like a server-side session-map bug.
  • Reading internal/engine/session.go closely: registerSession/unregisterSession's map handling is correct (the current == session pointer-equality guard in unregisterSession already prevents an evicted connection's cleanup from deleting the replacement session's entry). The real defect is purely in logging: session.logger's bound "session" field is set to the user's short ID (not the session's own) whenever authenticated, specifically so log lines from the same user's repeated reconnects grep together - but this means two rapid reconnects for one user produce two "Session closed" lines with no way to tell which session actually closed, unlike "Session established" which already logs an explicit session_id.
  • This PR adds the same explicit session_id field to the "Session closed" log line, so a real reconnect storm (driven by a duplicate-client bug on the SDK side, tracked separately) can actually be distinguished from a server-side eviction bug in logs going forward.

Test plan

  • GOWORK=off go build ./internal/engine/...
  • GOWORK=off go test ./internal/engine/... - passing
  • Pre-commit hooks (gofmt, golangci-lint) passing

🤖 Generated with Claude Code

https://claude.ai/code/session_014CUkUYCxTRRRrvVWSGS5dj

"Session closed" only logged session.logger's bound "session" field, which
is the user's short ID (not this session's own) whenever authenticated -
deliberately shared across every reconnect for that user so log lines
group together by grep. Without an explicit session_id (unlike "Session
established", which already logs one), two rapid reconnects for the same
user produced two indistinguishable "Session closed" lines, which read as
a session-eviction/map-corruption bug in a live triage when the actual
cause was simply frequent reconnects from a duplicate-client bug elsewhere.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014CUkUYCxTRRRrvVWSGS5dj
Copilot AI lite review requested due to automatic review settings August 30, 2026 08:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves observability for WebSocket engine sessions by including an explicit session_id field in the "Session closed" log line, matching the existing "Session established" logging and making rapid reconnect/close sequences distinguishable in production logs.

Changes:

  • Add zap.String("session_id", session.ID) to the "Session closed" log entry in Manager.unregisterSession.
  • Add an explanatory comment clarifying why session.logger’s bound "session" field can’t be relied on to uniquely identify a specific session instance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/engine/session.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@leifj leifj left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a config gate and should only be logged in debug-mode to avoid leaking privacy sensitive data

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