Skip to content

fix(host-service): unstrand tunnel reconnect; wire relay Sentry #4537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
saddlepaddle merged 3 commits into from
May 14, 2026
+32 −1
Merged

fix(host-service): unstrand tunnel reconnect; wire relay Sentry #4537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7436166
chore(relay): wire Sentry logs and uncaught-throw safety net
saddlepaddle May 14, 2026
6bdd3ce
fix(host-service): use valid close code in tunnel cleanupChannels
saddlepaddle May 14, 2026
9827893
review: drop duplicate Sentry capture; log cleanupChannels failures
saddlepaddle May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions apps/relay/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ const redactingLogger = logger((message, ...rest) => {

initSentry();

process.on("uncaughtException", (err) => {
console.error("[relay] uncaughtException (suppressed)", err);
});
process.on("unhandledRejection", (reason) => {
console.error("[relay] unhandledRejection (suppressed)", reason);
});

type AppContext = {
Variables: {
auth: AuthContext;
Expand All @@ -43,6 +50,14 @@ const { injectWebSocket, upgradeWebSocket } = createNodeWebSocket({ app });
app.use("*", redactingLogger);
app.use("*", cors());

app.onError((err, c) => {
captureSentryException(err, {
op: "hono.onError",
path: new URL(c.req.url).pathname,
});
return c.json({ error: "Internal server error" }, 500);
});

app.get("/health", (c) => c.json({ ok: true, region: env.FLY_REGION }));

// ── Auth ────────────────────────────────────────────────────────────
Expand Down
9 changes: 9 additions & 0 deletions apps/relay/src/sentry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ export function initSentry(): void {
release: process.env.FLY_IMAGE_REF,
environment: process.env.FLY_APP_NAME ?? "relay-local",
tracesSampleRate: 0,
_experiments: { enableLogs: true },
integrations: [
Sentry.consoleLoggingIntegration({
levels: ["log", "info", "warn", "error"],
}),
Sentry.onUncaughtExceptionIntegration({
exitEvenIfOtherHandlersAreRegistered: false,
}),
],
Comment on lines +15 to +22
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 14, 2026

Choose a reason for hiding this comment

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

P1 Both onUncaughtExceptionIntegration and onUnhandledRejectionIntegration are auto-enabled by default in @sentry/node. Passing them in the integrations: [...] array form adds a second copy on top of the defaults rather than replacing them. The default copy has exitEvenIfOtherHandlersAreRegistered: true, so it will still call process.exit() unconditionally — directly defeating the intent of this PR. Every uncaught exception will also be captured twice by Sentry (once by each instance). Use the function form to filter out the defaults first, then supply the customised replacements.

Suggested change
integrations: [
Sentry.consoleLoggingIntegration({
levels: ["log", "info", "warn", "error"],
}),
Sentry.onUncaughtExceptionIntegration({
exitEvenIfOtherHandlersAreRegistered: false,
}),
],
integrations: (defaults) => [
...defaults.filter(
(i) =>
i.name !== "OnUncaughtException" &&
i.name !== "OnUnhandledRejection",
),
Sentry.consoleLoggingIntegration({
levels: ["log", "info", "warn", "error"],
}),
Sentry.onUncaughtExceptionIntegration({
exitEvenIfOtherHandlersAreRegistered: false,
}),
Sentry.onUnhandledRejectionIntegration({
mode: "none",
}),
],
Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/relay/src/sentry.ts
Line: 15-22

Comment:
Both `onUncaughtExceptionIntegration` and `onUnhandledRejectionIntegration` are **auto-enabled by default** in `@sentry/node`. Passing them in the `integrations: [...]` array form adds a second copy on top of the defaults rather than replacing them. The default copy has `exitEvenIfOtherHandlersAreRegistered: true`, so it will still call `process.exit()` unconditionally — directly defeating the intent of this PR. Every uncaught exception will also be captured twice by Sentry (once by each instance). Use the function form to filter out the defaults first, then supply the customised replacements.

```suggestion
		integrations: (defaults) => [
			...defaults.filter(
				(i) =>
					i.name !== "OnUncaughtException" &&
					i.name !== "OnUnhandledRejection",
			),
			Sentry.consoleLoggingIntegration({
				levels: ["log", "info", "warn", "error"],
			}),
			Sentry.onUncaughtExceptionIntegration({
				exitEvenIfOtherHandlersAreRegistered: false,
			}),
			Sentry.onUnhandledRejectionIntegration({
				mode: "none",
			}),
		],
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Collaborator Author

@saddlepaddle saddlepaddle May 14, 2026

Choose a reason for hiding this comment

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

Verified against @sentry/core@10.46.0 source (integration.js: filterDuplicates + getIntegrationsToSetup): the array form does dedupe by name, and a user instance replaces a default of the same name (defaults are tagged isDefaultInstance = true; the merge rule "never want a default instance to overwrite an existing user instance" runs in our favor since user integrations come after defaults in the merged array).

Also verified @sentry/node-core@9.47.1 onUncaughtExceptionIntegration source: its built-in default for exitEvenIfOtherHandlersAreRegistered is false, not true. So even the default wouldn't unconditionally exit.

So the array form is fine here — single instance with our config, no exit unless no other handlers are attached.

You did sniff out a real adjacent issue though: Sentry's integration unconditionally calls captureException itself, and our process.on('uncaughtException'|'unhandledRejection') handlers were also calling captureSentryException — double captures. Fixed in 9827893 by slimming the handlers to just log (they still need to exist so Sentry's "other handlers attached" check stays true, keeping the process alive).

initialScope: {
tags: {
region: env.FLY_REGION,
Expand Down
9 changes: 8 additions & 1 deletion packages/host-service/src/tunnel/tunnel-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,14 @@ export class TunnelClient {

private cleanupChannels(): void {
for (const channel of this.localChannels.values()) {
channel.ws.close(1001, "Tunnel disconnected");
try {
channel.ws.close(1000, "Tunnel disconnected");
} catch (err) {
console.warn(
"[host-service:tunnel] error closing local channel ws",
err,
);
}
}
this.localChannels.clear();
}
Comment on lines 303 to 315
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 14, 2026

Choose a reason for hiding this comment

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

P2 The empty catch {} silently discards any error that isn't the now-fixed 1001 code issue. Since the primary fix is the 1000 code change, the try/catch is purely defensive, but a silent swallow means future ws.close() problems on cleanup will be invisible in logs.

Suggested change
private cleanupChannels(): void {
for (const channel of this.localChannels.values()) {
channel.ws.close(1001, "Tunnel disconnected");
try {
channel.ws.close(1000, "Tunnel disconnected");
} catch {}
}
this.localChannels.clear();
}
private cleanupChannels(): void {
for (const channel of this.localChannels.values()) {
try {
channel.ws.close(1000, "Tunnel disconnected");
} catch (err) {
console.warn("[host-service:tunnel] error closing local channel ws", err);
}
}
this.localChannels.clear();
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/host-service/src/tunnel/tunnel-client.ts
Line: 303-310

Comment:
The empty `catch {}` silently discards any error that isn't the now-fixed 1001 code issue. Since the primary fix is the `1000` code change, the try/catch is purely defensive, but a silent swallow means future `ws.close()` problems on cleanup will be invisible in logs.

```suggestion
	private cleanupChannels(): void {
		for (const channel of this.localChannels.values()) {
			try {
				channel.ws.close(1000, "Tunnel disconnected");
			} catch (err) {
				console.warn("[host-service:tunnel] error closing local channel ws", err);
			}
		}
		this.localChannels.clear();
	}
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Collaborator Author

@saddlepaddle saddlepaddle May 14, 2026

Choose a reason for hiding this comment

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

Good catch — applied in 9827893. With consoleLoggingIntegration wired, that console.warn flows to Sentry Logs automatically.

Expand Down
Loading