fix(runtime): report an inconclusive port probe as unknown, not as exit

packages/runtime/src/agent/server.test.ts

@@ -1,9 +1,10 @@
 import type {Logger} from '../shared/logger.js'
+import type {QuiescenceProbeSocket} from './server.js'
 import net from 'node:net'
 import process from 'node:process'
 import {createOpencode} from '@opencode-ai/sdk'
 import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'
-import {bootstrapOpenCodeServer, waitForServerQuiescence} from './server.js'
+import {bootstrapOpenCodeServer, isPortOpen, waitForServerQuiescence} from './server.js'
 
 vi.mock('@opencode-ai/sdk', () => ({
   createOpencode: vi.fn(),
@@ -387,4 +388,141 @@ describe('waitForServerQuiescence', () => {
     // #then the failure is reported as an unconfirmed quiescence, not an exception
     expect(result).toEqual({quiesced: false})
   })
+
+  // This is the end-to-end counterpart to isPortOpen's own polarity test below: that test
+  // pins isPortOpen's *return value* (true) for an inconclusive probe, but nothing pinned
+  // that waitForServerQuiescence's do/while loop actually treats that `true` as "keep
+  // polling" rather than "the child exited". A one-line call-site regression --
+  // `if (stillOpen) return {quiesced: true}` -- inverts that read, passes every other test
+  // in this file (isPortOpen's own polarity test doesn't touch this call site, and the
+  // real-listener tests above never exercise the timeout branch at all), and silently
+  // reintroduces the bug this PR fixed one line away from the fix. Threading the same
+  // optional `connect` injector through to this level is what makes the loop's own read of
+  // that return value directly assertable without depending on real network timing.
+  it('never reports quiesced: true from repeated inconclusive probes -- only a genuinely refused connection, or the deadline, may produce a result', async () => {
+    // #given a connect() that always times out inconclusively (fires isPortOpen's own
+    // setTimeout branch, never 'connect' or 'error') -- standing in for a firewalled port
+    // or a host silently dropping SYNs on every single poll attempt
+    let connectCalls = 0
+    const alwaysInconclusive = (): QuiescenceProbeSocket => {
+      connectCalls++
+      const fake = createFakeSocket()
+      queueMicrotask(fake.fireTimeout)
+      return fake.socket
+    }
+
+    // #when waiting for quiescence with a budget that allows many poll cycles. The budget is
+    // deliberately ~100x the poll interval rather than ~6x: the do/while checks the deadline
+    // only after the first iteration, so a budget close to the interval lets a single stalled
+    // delay() on a contended runner exit after one probe and fail the connectCalls assertion.
+    // The quiesced: false assertion below is stall-immune either way -- a stalled loop still
+    // cannot produce quiesced: true -- so the headroom protects against a spurious red, never
+    // against a wrong green.
+    const result = await waitForServerQuiescence('http://127.0.0.1:4096', 500, 5, alwaysInconclusive)
+
+    // #then the deadline is what ends the wait, reporting the honest unconfirmed state --
+    // never quiesced: true, which would mean an inconclusive probe was misread as "the
+    // child exited"
+    expect(result).toEqual({quiesced: false})
+    expect(connectCalls).toBeGreaterThan(1)
+  })
+})
+
+// isPortOpen's own socket-timeout branch is otherwise unreachable from a deterministic
+// test: a real socket only reaches it by actually hanging for the full timeout budget,
+// which depends on real network/OS behavior a CI sandbox cannot guarantee (a black-holed
+// address may instead fail fast with ECONNREFUSED/ENETUNREACH, silently skipping the
+// branch entirely). Injecting a fake QuiescenceProbeSocket makes the branch, and the
+// polarity it resolves to, directly and deterministically assertable.
+function createFakeSocket(): {
+  readonly socket: QuiescenceProbeSocket
+  readonly fireTimeout: () => void
+  readonly fireConnect: () => void
+  readonly fireError: () => void
+  readonly calls: string[]
+} {
+  const calls: string[] = []
+  let timeoutCallback: (() => void) | undefined
+  let connectListener: (() => void) | undefined
+  let errorListener: (() => void) | undefined
+
+  const socket: QuiescenceProbeSocket = {
+    once: (event, listener) => {
+      if (event === 'connect') connectListener = listener
+      if (event === 'error') errorListener = listener
+    },
+    setTimeout: (_ms, onTimeout) => {
+      timeoutCallback = onTimeout
+    },
+    destroy: () => calls.push('destroy'),
+    removeAllListeners: () => calls.push('removeAllListeners'),
+  }
+
+  return {
+    socket,
+    fireTimeout: () => timeoutCallback?.(),
+    fireConnect: () => connectListener?.(),
+    fireError: () => errorListener?.(),
+    calls,
+  }
+}
+
+describe('isPortOpen', () => {
+  it('resolves true when the socket times out without connect or error ever firing (the pessimistic default this fix exists to pin)', async () => {
+    // #given a connection attempt that neither succeeds nor is refused within the budget
+    const fake = createFakeSocket()
+    const connect = vi.fn(() => fake.socket)
+
+    // #when the probe's own timeout fires
+    const resultPromise = isPortOpen('127.0.0.1', 4096, 50, connect)
+    fake.fireTimeout()
+
+    // #then it resolves true ("still open as far as this attempt could tell"), NOT false --
+    // an inconclusive attempt must not be read as "the child exited". Every other unknown
+    // in this change resolves the same way (verifyDatabaseUsable: usable: true by default;
+    // isStructuralCorruptionError: false unless SQLite positively says otherwise).
+    await expect(resultPromise).resolves.toBe(true)
+    expect(connect).toHaveBeenCalledWith('127.0.0.1', 4096)
+  })
+
+  it('resolves false when the connection is refused (error fires, no timeout)', async () => {
+    // #given a connection that is actively refused -- the real "child has exited" signal
+    const fake = createFakeSocket()
+    const connect = vi.fn(() => fake.socket)
+
+    const resultPromise = isPortOpen('127.0.0.1', 4096, 50, connect)
+    fake.fireError()
+
+    await expect(resultPromise).resolves.toBe(false)
+  })
+
+  it('resolves true when the connection succeeds (the port is still held by a live process)', async () => {
+    const fake = createFakeSocket()
+    const connect = vi.fn(() => fake.socket)
+
+    const resultPromise = isPortOpen('127.0.0.1', 4096, 50, connect)
+    fake.fireConnect()
+
+    await expect(resultPromise).resolves.toBe(true)
+  })
+
+  it('destroys the socket before removing its listeners, and never settles twice when a stale event fires after the outcome is already decided', async () => {
+    // #given a socket whose timeout fires first
+    const fake = createFakeSocket()
+    const connect = vi.fn(() => fake.socket)
+
+    const resultPromise = isPortOpen('127.0.0.1', 4096, 50, connect)
+    fake.fireTimeout()
+    // #and a stale 'error' event arrives afterward, as destroying a mid-connect socket can
+    // trigger -- this must not flip an already-decided true result to false
+    fake.fireError()
+
+    const result = await resultPromise
+
+    // #then the first-decided outcome wins, and teardown ran destroy() before
+    // removeAllListeners() so removing the error handler can never itself be the cause of
+    // an unhandled error from a socket still mid-connect
+    expect(result).toBe(true)
+    expect(fake.calls).toEqual(['destroy', 'removeAllListeners'])
+  })
 })

packages/runtime/src/agent/server.ts

@@ -20,6 +20,24 @@ async function delay(ms: number): Promise<void> {
   })
 }
 
+// The subset of net.Socket that isPortOpen depends on. Narrowed to an interface (rather
+// than importing net.Socket directly into the signature) so tests can inject a fake that
+// deterministically exercises the setTimeout branch -- a real socket only reaches that
+// branch by actually hanging for `timeoutMs`, which depends on real network/OS behavior
+// (a black-holed address may instead fail fast with ECONNREFUSED/ENETUNREACH in a
+// sandboxed CI network, silently skipping the very branch a test aims to pin) and is slow
+// even when it works.
+export interface QuiescenceProbeSocket {
+  readonly once: (event: 'connect' | 'error', listener: (error?: Error) => void) => void
+  readonly setTimeout: (ms: number, onTimeout: () => void) => void
+  readonly destroy: () => void
+  readonly removeAllListeners: () => void
+}
+
+function defaultConnect(hostname: string, port: number): QuiescenceProbeSocket {
+  return net.connect({host: hostname, port})
+}
+
 // Best-effort liveness probe for the OpenCode child, used only by shutdown() below.
 // Resolves true while something answers a TCP connect on the given host/port, false the
 // instant the connection is refused (or otherwise errors) -- which, for a port this
@@ -30,21 +48,42 @@ async function delay(ms: number): Promise<void> {
 // relying solely on `connect`/`error` firing: a connection attempt that neither succeeds
 // nor is refused -- a firewalled port, a host that silently drops SYNs -- would otherwise
 // never settle this promise, which would stall waitForServerQuiescence's do/while loop
-// forever despite its own `timeoutMs` parameter. `finish` is guarded against running twice
-// so a `timeout` that fires and a subsequent `error` from the torn-down socket cannot both
-// resolve the same promise.
-async function isPortOpen(hostname: string, port: number, timeoutMs: number): Promise<boolean> {
+// forever despite its own `timeoutMs` parameter. A timeout here resolves `true` ("still
+// open as far as this attempt could tell"), NOT `false`: every other unknown this change
+// introduces resolves pessimistically (verifyDatabaseUsable defaults to usable: true only
+// for a *recognized-safe* throw shape; isStructuralCorruptionError defaults to false unless
+// SQLite positively says otherwise), and "connection attempt inconclusive" must default to
+// "cannot confirm the child exited", not to a manufactured `quiesced: true`. Resolving
+// `true` here means the do/while loop simply keeps polling on the next interval; the outer
+// `waitForServerQuiescence` deadline is what eventually produces an honest `quiesced:
+// false` if the port genuinely never stops answering -- the cost of that correctness is
+// that a genuinely inconclusive probe now rides out the full outer timeout budget
+// (`DEFAULT_SHUTDOWN_QUIESCE_TIMEOUT_MS`, 5000ms as of writing) instead of returning after
+// a single poll interval; this is one server per run and bounded, so it is an acceptable,
+// intended trade, not a regression, but a future change to either constant should account
+// for it deliberately rather than rediscovering it. `finish` is guarded against running
+// twice so a `timeout` that fires and a subsequent `error` from the torn-down socket cannot
+// both resolve the same promise. `destroy()` runs before `removeAllListeners()`, not after:
+// removing the `error` listener first would leave a socket that is mid-connect (and may
+// still emit `error` as a side effect of being destroyed) with no listener attached, which
+// Node treats as an uncaught exception.
+export async function isPortOpen(
+  hostname: string,
+  port: number,
+  timeoutMs: number,
+  connect: (hostname: string, port: number) => QuiescenceProbeSocket = defaultConnect,
+): Promise<boolean> {
   return new Promise(resolve => {
     let settled = false
-    const socket = net.connect({host: hostname, port})
+    const socket = connect(hostname, port)
     const finish = (result: boolean): void => {
       if (settled) return
       settled = true
-      socket.removeAllListeners()
       socket.destroy()
+      socket.removeAllListeners()
       resolve(result)
     }
-    socket.setTimeout(timeoutMs, () => finish(false))
+    socket.setTimeout(timeoutMs, () => finish(true))
     socket.once('connect', () => finish(true))
     socket.once('error', () => finish(false))
   })
@@ -70,6 +109,7 @@ export async function waitForServerQuiescence(
   url: string,
   timeoutMs: number = DEFAULT_SHUTDOWN_QUIESCE_TIMEOUT_MS,
   pollIntervalMs: number = DEFAULT_SHUTDOWN_QUIESCE_POLL_INTERVAL_MS,
+  connect: (hostname: string, port: number) => QuiescenceProbeSocket = defaultConnect,
 ): Promise<ShutdownResult> {
   let hostname: string
   let port: number
@@ -96,7 +136,7 @@ export async function waitForServerQuiescence(
 
   const deadline = Date.now() + timeoutMs
   do {
-    const stillOpen = await isPortOpen(hostname, port, pollIntervalMs)
+    const stillOpen = await isPortOpen(hostname, port, pollIntervalMs, connect)
     if (!stillOpen) {
       return {quiesced: true}
     }

packages/runtime/src/object-store/s3-adapter.test.ts

@@ -1,5 +1,6 @@
 import type {Logger} from '../shared/logger.js'
 import type {ObjectStoreConfig} from './types.js'
+import {Buffer} from 'node:buffer'
 import * as fs from 'node:fs/promises'
 import * as os from 'node:os'
 import * as path from 'node:path'
@@ -74,6 +75,15 @@ function createLogger(): Logger {
   }
 }
 
+// Yields a real chunk before the stream itself errors, so pipeline() has already opened
+// the destination via createWriteStream and written that chunk by the time this rejects --
+// mirroring a real network failure partway through an S3 GetObject transfer rather than a
+// failure before any byte was ever written.
+async function* partialThenFail(): AsyncGenerator<Buffer> {
+  yield Buffer.from('partial-bytes-before-mid-transfer-failure')
+  throw new Error('simulated mid-transfer network failure')
+}
+
 function getCommandInput(callIndex: number): Record<string, unknown> {
   const command = sentCommands[callIndex]
 
@@ -301,6 +311,42 @@ describe('createS3Adapter', () => {
     await expect(fs.readFile(localPath, 'utf8')).resolves.toBe('downloaded bytes')
   })
 
+  it('removes the partial file left by a mid-transfer download failure, rather than leaving a truncated file on disk', async () => {
+    // #given a response body that yields real bytes before the stream itself errors
+    sendMock.mockResolvedValue({Body: Readable.from(partialThenFail())})
+    const logger = createLogger()
+    const adapter = createS3Adapter(baseConfig, logger)
+    const localPath = path.join(tempDir, 'download', 'opencode.db')
+
+    // #when downloading
+    const result = await adapter.download('fro-bot-state/github/owner/repo/sessions/opencode.db', localPath)
+
+    // #then the download reports failure, and — the property this test exists to pin — no
+    // partial file is left behind for a caller to later mistake for a usable database
+    expect(result.success).toBe(false)
+    await expect(fs.access(localPath)).rejects.toThrow()
+  })
+
+  it('leaves a pre-existing local file at the download target path untouched when the GetObjectCommand call itself rejects before any write begins', async () => {
+    // #given the S3 call fails before a response (and therefore before createWriteStream)
+    // is ever reached -- the counterpart case to the mid-transfer test above, pinning that
+    // cleanup is scoped to files this exact pipeline() call opened, not applied broadly
+    sendMock.mockRejectedValue(new Error('ECONNREFUSED'))
+    const logger = createLogger()
+    const adapter = createS3Adapter(baseConfig, logger)
+    const localPath = path.join(tempDir, 'download', 'opencode.db')
+    await fs.mkdir(path.dirname(localPath), {recursive: true})
+    await fs.writeFile(localPath, 'pre-existing-legitimate-local-database')
+
+    // #when downloading
+    const result = await adapter.download('fro-bot-state/github/owner/repo/sessions/opencode.db', localPath)
+
+    // #then the download reports failure, and the pre-existing local file this attempt
+    // never got far enough to write to is left completely untouched
+    expect(result.success).toBe(false)
+    await expect(fs.readFile(localPath, 'utf8')).resolves.toBe('pre-existing-legitimate-local-database')
+  })
+
   it('conditionally uploads object data and returns the etag on success', async () => {
     // #given
     sendMock.mockResolvedValue({ETag: 'etag-123'})