Skip to content

fix: surface ConnectRPC errors instead of silently swallowing them#14

Open
tumberger wants to merge 1 commit intomainfrom
04-08-fix_surface_connectrpc_errors
Open

fix: surface ConnectRPC errors instead of silently swallowing them#14
tumberger wants to merge 1 commit intomainfrom
04-08-fix_surface_connectrpc_errors

Conversation

@tumberger
Copy link
Copy Markdown
Contributor

@tumberger tumberger commented Apr 8, 2026

Summary

  • backend.IngestEvent: propagate Receive() errors instead of discarding the response
  • sidecar.ingestEvent: surface first failure + periodic reminders to stderr so the user sees when telemetry is broken
  • sidecar.heartbeatLoop: surface persistent heartbeat failures with escalating severity
  • run.Start teardown: report EndSession failure instead of printing "session ended" unconditionally

Context

Sessions show up in the dashboard but events/traces don't appear. The entire hook → sidecar → ConnectRPC → API pipeline was silently swallowing all errors, making it impossible to diagnose where events were being lost.

Test plan

  • Run kontext start --agent claude, trigger tool calls, verify events appear in traces
  • Kill the API server mid-session, verify stderr shows the ingestion failure message
  • Let a session idle past 30s, verify heartbeat errors surface if backend is unreachable

🤖 Generated with Claude Code


Open with Devin

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

- backend.IngestEvent: propagate Receive() errors instead of discarding
- sidecar.ingestEvent: surface first failure and periodic reminders to
  stderr so the user knows telemetry is broken
- sidecar.heartbeatLoop: surface persistent heartbeat failures
- run.Start teardown: report EndSession failure instead of printing
  success unconditionally

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tumberger tumberger force-pushed the 04-08-fix_surface_connectrpc_errors branch from f35ceb2 to 7383268 Compare April 8, 2026 11:07
@tumberger tumberger marked this pull request as ready for review April 8, 2026 12:35
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment thread internal/run/run.go
// Use /tmp (not $TMPDIR) with a short ID to keep the Unix socket path
// under macOS's 104-byte sun_path limit. $TMPDIR on macOS is a long
// path like /var/folders/.../T/ which pushes the socket path over.
sessionDir := filepath.Join("/tmp", "kontext", truncateID(sessionID))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Session directory uses truncated 8-char ID, creating collision risk between concurrent sessions

The session directory is now derived from truncateID(sessionID) which uses only the first 8 characters of the session ID (internal/run/run.go:100). If two concurrent kontext start instances receive session IDs sharing the same 8-character prefix, they will share the same directory. This causes:

  1. The second session's sidecar.Start() removes the first session's socket (internal/sidecar/sidecar.go:49)
  2. Either session ending calls os.RemoveAll(sessionDir) (internal/run/run.go:138), deleting the other's socket and settings mid-operation

Critically, the truncation is unnecessary for the stated goal of staying under macOS's 104-byte sun_path limit. Switching from os.TempDir() to /tmp already saves ~35+ characters. Using the full session ID gives a path of ~62 bytes (/tmp/kontext/<UUID>/kontext.sock), well under the 104-byte limit. The old code used the full sessionID, which guaranteed uniqueness.

Suggested change
sessionDir := filepath.Join("/tmp", "kontext", truncateID(sessionID))
sessionDir := filepath.Join("/tmp", "kontext", sessionID)
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

1 participant