Skip to content

Commit 45fc744

Browse files
anandgupta42claude
anandgupta42
and
claude
committed
fix: only count real external clients in client_connected
Code review found the serve-process trace consumer trips the metric. On `altimate serve`, `subscribeTraceConsumer` immediately opens an in-process GET /event subscription (trace-consumer.ts), which hit the client_connected middleware and consumed the once-per-process latch before any IDE client attached. The event would fire on ~100% of serve launches and measure the internal subscription, not a real client. - The internal subscription now sets `x-altimate-trace-consumer`; the hook skips requests carrying it. - The hook emits after `next()` and only on a 2xx response, so a request that fails before the event stream is established no longer burns the latch (also addresses the "record after the stream starts" review note). - Wiring test covers the trace-consumer skip and uses a bounded poll instead of a fixed sleep (removes the flagged timing race). Tests: telemetry, server-wiring and trace-consumer suites pass; typecheck clean. Co-Authored-By: Claude Opus 4.8 <[email protected]>
1 parent a91fbe4 commit 45fc744

3 files changed

Lines changed: 68 additions & 33 deletions

File tree

‎packages/opencode/src/altimate/observability/trace-consumer.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,9 @@ export function subscribeTraceConsumer(
388388
(() => {
389389
const fetchFn = (async (fetchInput: RequestInfo | URL, init?: RequestInit) => {
390390
const request = new Request(fetchInput, init)
391+
// Mark this as the in-process observability subscription so the client_connected telemetry
392+
// hook in Server.createApp does not count it as a real external client attaching.
393+
request.headers.set("x-altimate-trace-consumer", "1")
391394
const password = Flag.OPENCODE_SERVER_PASSWORD
392395
if (password) {
393396
const username = Flag.OPENCODE_SERVER_USERNAME ?? "opencode"

‎packages/opencode/src/server/server.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -312,12 +312,21 @@ export namespace Server {
312312
// altimate_change end
313313
return basicAuth({ username, password })(c, next)
314314
})
315-
// altimate_change start — telemetry: first authenticated event-stream subscription = a client is really attached
315+
// altimate_change start — telemetry: a real external client attached to the event stream.
316+
// Emit after next() and only on a 2xx, so a request that fails before the stream is
317+
// established doesn't burn the once-per-process latch. Skip the in-process trace consumer
318+
// (serve.ts subscribes to /event for observability via x-altimate-trace-consumer): it is not
319+
// a real client and would otherwise trip the latch on every `serve` launch before the IDE
320+
// attaches.
316321
.use(async (c, next) => {
317-
if (c.req.method === "GET" && (c.req.path === "/event" || c.req.path === "/global/event")) {
322+
const isClientEventStream =
323+
c.req.method === "GET" &&
324+
(c.req.path === "/event" || c.req.path === "/global/event") &&
325+
!c.req.header("x-altimate-trace-consumer")
326+
await next()
327+
if (isClientEventStream && c.res.ok) {
318328
void import("../altimate/telemetry").then((m) => m.Telemetry.clientConnected()).catch(() => {})
319329
}
320-
return next()
321330
})
322331
// altimate_change end
323332
.use(async (c, next) => {

‎packages/opencode/test/server/altimate-client-connected-wiring.test.ts‎

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,48 @@
11
// altimate_change — regression test for the server-side wiring that produces `client_connected`.
22
//
33
// The telemetry unit tests (test/altimate/telemetry/first-run-health.test.ts) prove the
4-
// `Telemetry.clientConnected()` latch, but nothing exercised the `.use` middleware in
5-
// `Server.createApp` that actually calls it. Without this test, deleting the middleware, changing
6-
// the `/event` / `/global/event` path literals, dropping the `GET` guard, or moving the hook ahead
7-
// of basicAuth would all leave the suite green while silently killing the IDE-funnel signal.
4+
// `Telemetry.clientConnected()` latch; this file proves the `.use` middleware in `Server.createApp`
5+
// that calls it. Without it, deleting the middleware, changing the `/event` / `/global/event` path
6+
// literals, dropping the `GET` guard, failing to skip the in-process trace consumer, or emitting on
7+
// a non-2xx would all leave the suite green while breaking (or corrupting) the IDE-funnel signal.
88
//
9-
// The middleware fires `void import("../altimate/telemetry").then((m) => m.Telemetry.clientConnected())`
10-
// after next() is set up, so the call lands on a later microtask; each case waits a tick before
11-
// asserting. We spy on `Telemetry.clientConnected` itself (not `track`) so the assertion is purely
12-
// about the middleware deciding to call it — the once-per-process latch lives inside that function
13-
// and is covered elsewhere.
9+
// The middleware emits AFTER next() and only when the event route returned a 2xx, via
10+
// `void import("../altimate/telemetry").then((m) => m.Telemetry.clientConnected())`. The call lands
11+
// on a later microtask, so positive cases poll with a bounded timeout rather than sleeping a fixed
12+
// amount. We spy on `Telemetry.clientConnected` itself so the assertion is purely about the
13+
// middleware deciding to call it; the once-per-process latch lives inside that function and is
14+
// covered elsewhere.
1415
//
15-
// These cases catch deleting the middleware, changing the `/event` / `/global/event` path literals,
16-
// and dropping the `GET` guard. The remaining regression the reviewer named — moving the hook above
17-
// basicAuth so unauthenticated probes count — is not exercised here: auth is enforced by a separate
18-
// `basicAuth` middleware gated on `Flag.OPENCODE_SERVER_PASSWORD`, which is a module-level const
19-
// frozen at load (flag.ts), and CI runs the package in one Bun process, so a password set at test
20-
// time can't drive basicAuth to 401. The ordering is enforced structurally: this hook is registered
21-
// immediately after the basicAuth `.use` in `Server.createApp`.
16+
// Not exercised here: the "hook moved above basic-auth" case. Auth is a separate `basicAuth`
17+
// middleware gated on `Flag.OPENCODE_SERVER_PASSWORD`, a module-level const frozen at load, and CI
18+
// runs the package in one Bun process, so a password set at test time can't drive a 401. The order
19+
// is enforced structurally: the hook is registered immediately after the basic-auth `.use`, and it
20+
// now only counts a 2xx response, so an auth rejection (non-2xx) cannot trip it.
2221
import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"
2322
import { Server } from "../../src/server/server"
2423
import { Telemetry } from "../../src/altimate/telemetry"
2524
import { resetDatabase } from "./db"
26-
import { disposeAllInstances } from "../fixture/fixture"
27-
28-
const tick = () => new Promise((r) => setTimeout(r, 15))
25+
import { disposeAllInstances, tmpdir } from "../fixture/fixture"
2926

3027
async function cancelBody(response: Response) {
3128
await response.body?.cancel().catch(() => {})
3229
}
3330

31+
// The emit is async (dynamic import + .then). Poll for it instead of sleeping a fixed amount.
32+
async function waitForCall(spy: { mock: { calls: unknown[] } }, timeoutMs = 1000): Promise<boolean> {
33+
const start = Date.now()
34+
while (Date.now() - start < timeoutMs) {
35+
if (spy.mock.calls.length > 0) return true
36+
await new Promise((r) => setTimeout(r, 5))
37+
}
38+
return false
39+
}
40+
41+
// A negative can't be proven instantly; give the microtask/import a real chance to fire first.
42+
async function settle() {
43+
await new Promise((r) => setTimeout(r, 50))
44+
}
45+
3446
let spy: ReturnType<typeof spyOn<typeof Telemetry, "clientConnected">>
3547

3648
beforeEach(() => {
@@ -44,31 +56,42 @@ afterEach(async () => {
4456
})
4557

4658
describe("client_connected server wiring", () => {
47-
test("GET /event invokes Telemetry.clientConnected", async () => {
48-
const response = await Server.Default().request("/event")
59+
test("GET /global/event (2xx) invokes Telemetry.clientConnected", async () => {
60+
const response = await Server.Default().request("/global/event")
61+
expect(response.ok).toBe(true)
4962
await cancelBody(response)
50-
await tick()
51-
expect(spy).toHaveBeenCalled()
63+
expect(await waitForCall(spy)).toBe(true)
5264
})
5365

54-
test("GET /global/event invokes Telemetry.clientConnected", async () => {
55-
const response = await Server.Default().request("/global/event")
66+
test("GET /event (2xx, resolved instance) invokes Telemetry.clientConnected", async () => {
67+
await using tmp = await tmpdir({ git: true, config: { formatter: false, lsp: false } })
68+
const response = await Server.Default().request("/event", { headers: { "x-opencode-directory": tmp.path } })
69+
expect(response.ok).toBe(true)
5670
await cancelBody(response)
57-
await tick()
58-
expect(spy).toHaveBeenCalled()
71+
expect(await waitForCall(spy)).toBe(true)
72+
})
73+
74+
test("the in-process trace consumer (x-altimate-trace-consumer) is NOT counted", async () => {
75+
const response = await Server.Default().request("/global/event", {
76+
headers: { "x-altimate-trace-consumer": "1" },
77+
})
78+
expect(response.ok).toBe(true)
79+
await cancelBody(response)
80+
await settle()
81+
expect(spy).not.toHaveBeenCalled()
5982
})
6083

6184
test("a non-event path does not invoke Telemetry.clientConnected", async () => {
62-
const response = await Server.Default().request("/config")
85+
const response = await Server.Default().request("/global/config")
6386
await cancelBody(response)
64-
await tick()
87+
await settle()
6588
expect(spy).not.toHaveBeenCalled()
6689
})
6790

6891
test("a non-GET request to /event does not invoke Telemetry.clientConnected", async () => {
6992
const response = await Server.Default().request("/event", { method: "POST" })
7093
await cancelBody(response)
71-
await tick()
94+
await settle()
7295
expect(spy).not.toHaveBeenCalled()
7396
})
7497
})

0 commit comments

Comments
 (0)