Skip to content

Commit 7afe33a

Browse files
committed
fix: address multi-model code review feedback
- Remove dead else branch in `ENGINE_INSTALL_SPEC` conditional (constant is always truthy) - Add `extras` field to `engine_started` telemetry event for debugging - Update `ENGINE_INSTALL_SPEC` comment to reflect production usage - Tighten concurrent startup test assertion to verify mutex coalescing (`ensureEngineCalls <= 2` instead of just `>= 1`)
1 parent f55858e commit 7afe33a

3 files changed

Lines changed: 9 additions & 6 deletions

File tree

packages/opencode/src/altimate/bridge/engine.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ declare const OPENCODE_VERSION: string
2525
// Mutex to prevent concurrent ensureEngine/ensureUv calls from corrupting state
2626
let pendingEnsure: Promise<void> | null = null
2727

28-
/** The pip install spec used by ensureEngine — exported for tests. */
28+
/** Pip extras spec for altimate-engine (e.g. "warehouses" → altimate-engine[warehouses]).
29+
* Used in ensureEngine install command and recorded in manifest for upgrade detection. */
2930
export const ENGINE_INSTALL_SPEC = "warehouses"
3031

3132
interface Manifest {
@@ -199,9 +200,7 @@ async function ensureEngineImpl(): Promise<void> {
199200
const pythonPath = enginePythonPath()
200201
Log.Default.info("installing altimate-engine", { version: ALTIMATE_ENGINE_VERSION })
201202
try {
202-
const spec = ENGINE_INSTALL_SPEC
203-
? `altimate-engine[${ENGINE_INSTALL_SPEC}]==${ALTIMATE_ENGINE_VERSION}`
204-
: `altimate-engine==${ALTIMATE_ENGINE_VERSION}`
203+
const spec = `altimate-engine[${ENGINE_INSTALL_SPEC}]==${ALTIMATE_ENGINE_VERSION}`
205204
execFileSync(uv, ["pip", "install", "--python", pythonPath, spec], { stdio: "pipe" })
206205
} catch (e: any) {
207206
Telemetry.track({
@@ -234,6 +233,7 @@ async function ensureEngineImpl(): Promise<void> {
234233
session_id: Telemetry.getContext().sessionId,
235234
engine_version: ALTIMATE_ENGINE_VERSION,
236235
python_version: pyVersion,
236+
extras: ENGINE_INSTALL_SPEC,
237237
status: isUpgrade ? "upgraded" : "started",
238238
duration_ms: Date.now() - startTime,
239239
})

packages/opencode/src/altimate/telemetry/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ export namespace Telemetry {
153153
session_id: string
154154
engine_version: string
155155
python_version: string
156+
extras?: string
156157
status: "started" | "restarted" | "upgraded"
157158
duration_ms: number
158159
}

packages/opencode/test/bridge/client.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -406,9 +406,11 @@ describe("Bridge.start integration", () => {
406406
expect(r.status).toBe("rejected")
407407
}
408408

409-
// ensureEngine should have been called, but not 3 times
410-
// (mutex coalesces concurrent calls)
409+
// The startup mutex should coalesce concurrent calls into a single
410+
// ensureEngine invocation. In JS's single-threaded model, the first
411+
// call sets pendingStart before any await, so subsequent calls join it.
411412
expect(ensureEngineCalls).toBeGreaterThanOrEqual(1)
413+
expect(ensureEngineCalls).toBeLessThanOrEqual(2)
412414
Bridge.stop()
413415
})
414416
})

0 commit comments

Comments
 (0)