Merge pull request #1263 from cluesmith/builder/bugfix-1261

[Bugfix #1261] Hold Tower API requests until boot wiring completes

Author: waleedkadous

codev/projects/bugfix-1261-tower-serves-api-requests-befo/status.yaml

@@ -0,0 +1,17 @@
+id: bugfix-1261
+title: tower-serves-api-requests-befo
+protocol: bugfix
+phase: pr
+plan_phases: []
+current_plan_phase: null
+gates:
+  pr:
+    status: approved
+    requested_at: '2026-07-26T21:23:32.875Z'
+    approved_at: '2026-07-27T05:25:11.442Z'
+iteration: 1
+build_complete: false
+history: []
+started_at: '2026-07-26T21:08:00.200Z'
+updated_at: '2026-07-27T05:25:11.443Z'
+pr_ready_for_human: false

codev/resources/arch.md

@@ -240,6 +240,22 @@ Tower binds its port and starts serving immediately, but `reconcileTerminalSessi
 
 **Invariant for new Tower-startup work**: any endpoint that reads `workspaceTerminals` to build a response should route through `getRehydratedTerminalsEntry` so it inherits the gate, rather than reading the map directly.
 
+#### Boot Readiness Gate (#1261)
+
+The #997 barrier gates one dependency (reconcile's output) for the readers that name it. Every *other* boot dependency was still unguarded, and the largest of them was `initInstances()` — which sets `tower-instances.ts`'s `_deps` and was the last step of the boot sequence. Requests landing before it got whatever a half-wired Tower could produce: `DELETE /api/terminals/:id` returned **404 for a terminal that existed**, because `killTerminalWithShellper()` returns a bare `false` when `_deps` is null and the route reads that as "not found". The window scaled with disk state — the #1227 husk sweep and #1238 log sweep ran inside it — so a log-heavy machine failed deterministically while CI stayed green.
+
+The fix inverts the default from "serve whatever we have" to "serve nothing until wired":
+
+- The port still binds first. It is the single-Tower mutex (a second `afx tower start` needs `EADDRINUSE`) and what every readiness probe connects to.
+- `tower-server.ts` holds each request in `http.createServer` until `bootSequence()` calls `markBootComplete()`; held requests get 503 + `Retry-After` if boot exceeds `BOOT_READY_TIMEOUT_MS` (20s), so a hung boot fails loud instead of hanging clients forever.
+- `markBootComplete()` fires as soon as the *dependencies* are wired (through `initCron()`). Maintenance and background services — husk sweep, log-retention sweep, `initTunnel()` — deliberately run **after** it. Gating readiness on `initTunnel()` in particular would make an unreachable cloud endpoint look like a broken local Tower.
+
+This also makes `afx tower start`'s readiness signal honest: it polls `/api/status` for a 200, which the pre-fix Tower returned during the window (`getInstances()` returns `[]` when `_deps` is null).
+
+**Invariant for new Tower-startup work**: anything a request handler depends on must be wired *before* `markBootComplete()`; anything else (sweeps, timers, remote connections) goes after it. Do not add work between `server.listen()` and the gate.
+
+**Second line of defence**: `instancesReady()` lets routes distinguish "not wired yet" from "no such thing" — the `_deps`-dependent terminal-delete paths answer 503 rather than 404 or a lying 204. Unreachable while the gate holds, and deliberately so: the failure mode if it is ever bypassed should be honest.
+
 #### Wire Protocol
 
 Binary frame format: `[1-byte type] [4-byte big-endian length] [payload]`
@@ -1713,20 +1729,23 @@ The startup ordering is critical — race conditions have caused real bugs when
 
 | Step | Operation | Why this order |
 |------|-----------|----------------|
-| 1 | HTTP server binds to `localhost:port` | Must be listening before anything registers routes |
+| 1 | HTTP server binds to `localhost:port` | Single-Tower mutex + what readiness probes connect to. **Requests are held, not served, until step 9** (#1261) |
 | 2 | SessionManager init + stale socket cleanup | Prepares shellper infrastructure |
 | 3 | `initTerminals()` | Terminal management module ready |
 | 4 | `startSendBuffer()` | Typing-aware message delivery ready |
 | 5 | **`reconcileTerminalSessions()`** | **MUST run before step 7** — reconnects shellper sessions from previous run |
 | 6 | `killOrphanedShellpers()` | **MUST run after step 5** — avoids killing sessions that were just reconnected |
 | 7 | `initInstances()` | Enables workspace API handlers — triggers dashboard polling |
 | 8 | `initCron()` | Scheduler starts after instances ready |
-| 9 | `initTunnel()` | Cloud tunnel connects last |
-| 10 | WebSocket upgrade handler installed | Terminal connections accepted |
+| 9 | **`markBootComplete()`** | **Readiness gate opens** — held requests are released and Tower starts serving (#1261) |
+| 10 | Husk sweep (#1227) + session-log sweep (#1238) | Maintenance: scales with disk state, so it must not gate the API |
+| 11 | `initTunnel()` | Cloud tunnel connects last — a remote endpoint must never gate local readiness |
+| 12 | WebSocket upgrade handler installed | Terminal connections accepted (installed at module load; not gated — see #997 barrier) |
 
 **Known ordering bugs**:
 - **Bugfix #274**: `initInstances()` before `reconcileTerminalSessions()` allowed dashboard polls to race with reconciliation, corrupting shellper sessions
 - **Bugfix #341**: Killing orphaned shellpers before reconciliation killed sessions that were about to be reconnected
+- **Bugfix #1261**: `initInstances()` ran last, *after* the two disk-scaling sweeps, so every `_deps`-dependent route was broken for as long as those scans took — `DELETE /api/terminals/:id` 404'd for a terminal that existed. Fixed by moving the sweeps after readiness and holding requests until step 9
 
 **Defense in depth**: During startup, `getTerminalsForWorkspace()` skips on-the-fly shellper reconnection (via `_reconciling` guard) to prevent races through alternate code paths.
 

codev/state/bugfix-1261_thread.md

@@ -0,0 +1,111 @@
+# bugfix-1261 — Tower serves API before internal wiring completes
+
+## Investigate
+
+Issue: `DELETE /api/terminals/:id` 404s for an existing terminal during Tower's
+startup window, because the whole boot sequence lives inside the
+`server.listen()` callback and `initInstances()` (which sets `_deps`) is the
+*last* async step. `killTerminalWithShellper()` bails with `false` when
+`_deps` is null; the DELETE route maps that to 404.
+
+Confirmed by reading:
+- `tower-server.ts:375` — `server.listen(...)` callback holds the entire boot.
+- `tower-server.ts:575` — `initInstances()` is last, after stale-socket cleanup,
+  consolidation, `reconcileTerminalSessions()`, `killOrphanedShellpers()`,
+  the #1227 husk sweep, and the #1238 session-log retention scan.
+- `tower-instances.ts:800` — `killTerminalWithShellper` → `if (!_deps) return false`.
+- `tower-routes.ts:842` — DELETE translates that `false` into 404 NOT_FOUND.
+
+Extra finding not in the issue: `afx tower start` treats a 200 from
+`/api/status` as readiness (`commands/tower.ts:110`), and `/api/status`
+answers 200 during the window (`getInstances()` returns `[]` when `_deps` is
+null). So the CLI's "Tower started" is a *false* readiness signal — that is
+what makes `afx tower start && <immediate afx cmd>` racy.
+
+### Reproduced
+
+Wrote a scratchpad repro that tight-loop-connects to the port and fires
+create+DELETE the instant it binds (the e2e helper's 200ms poll adds slack that
+usually hides it). On this machine, with an *empty* log dir:
+
+```
+POST   /api/terminals      -> 201 (269ms after port open)
+DELETE /api/terminals/:id  -> 404 {"error":"NOT_FOUND", ...}
+GET    /api/terminals/:id  -> 200   <-- terminal exists; the 404 was spurious
+```
+
+Deterministic, 100% of runs. So the window is not exotic — it just needs a
+client that doesn't sleep before its first request.
+
+Planned fix (three parts, all small):
+1. Readiness gate in `tower-server.ts`: `server.listen()` still binds first
+   (preserves EADDRINUSE detection + the single-Tower mutex), but
+   `handleRequest` awaits a boot-complete promise before dispatching, with a
+   bounded timeout → 503 + `Retry-After`.
+2. Reorder boot: move `initInstances()` up to right after the ordering-
+   constrained steps; push the disk-scaling husk sweep + log retention sweep
+   after it. Shrinks the window from O(disk) to O(process scan).
+3. Defense in depth: DELETE (and any `_deps`-dependent route) returns 503
+   "Tower is starting up" instead of 404, generalizing what `stopInstance`
+   already does.
+
+## Fix
+
+All three parts implemented.
+
+- `tower-server.ts`: readiness gate (`bootComplete` + `whenBootComplete`) in
+  the `http.createServer` handler; the listen callback now just logs and kicks
+  off a named `bootSequence()`. Boot-throw still exits the process (was an
+  unhandled rejection before, same net effect). Held requests get 503 +
+  `Retry-After` if boot exceeds 20s.
+- `tower-server.ts`: `initInstances()` + `initCron()` moved up to right after
+  `killOrphanedShellpers()`; the #1227 husk sweep, the #1238 log-retention
+  sweep, and `initTunnel()` now run *after* the gate opens. Tunnel especially:
+  gating local readiness on a remote connect would make an unreachable cloud
+  endpoint look like a broken Tower. Measured effect on this machine: boot
+  reaches ready at ~90ms instead of running the disk scans first.
+- `tower-instances.ts`: new `instancesReady()`; `tower-routes.ts`: DELETE
+  `/api/terminals/:id` and the workspace tab-delete path return 503 instead of
+  404 / a lying 204 when the module isn't wired.
+- Test hook `AF_TEST_BOOT_DELAY_MS` widens the window deterministically —
+  without it the race depends on this machine's process table and log volume,
+  which is exactly why CI never saw it.
+
+### Verification
+
+Scratchpad repro, same invocation as before the fix:
+
+```
+POST   /api/terminals      -> 201
+DELETE /api/terminals/:id  -> 204   (was 404)
+GET    /api/terminals/:id  -> 404   (was 200 — i.e. it really is gone now)
+```
+
+New `tower-startup-readiness.e2e.test.ts` (2 tests) passes. Confirmed it
+*fails* with the gate disabled in dist: DELETE 503 (the second-line guard
+firing) and the status-hold assertion 2ms vs the required ≥1200ms.
+
+Known adjacent case left alone: WebSocket upgrades bypass the gate
+(`setupUpgradeHandler` attaches its own listener). Not reachable in practice —
+every client does an HTTP call first — so out of BUGFIX scope; noted in the PR.
+
+## PR
+
+PR #1263 — "[Bugfix #1261] Hold Tower API requests until boot wiring completes".
+
+CMAP (3-way, `--issue 1261 --project-id bugfix-1261`; note the bare
+`consult --protocol bugfix --type pr` form bails with "Multiple projects
+found" in this repo — it needs the issue/project flags):
+
+| Model  | Verdict | Confidence | Key issues |
+|--------|---------|------------|------------|
+| gemini | APPROVE | HIGH       | None |
+| codex  | APPROVE | HIGH       | None |
+| claude | APPROVE | HIGH       | None |
+
+No REQUEST_CHANGES, so nothing to address or rebut. Claude raised two
+non-blocking observations and dismissed both itself: the `waitForPortImmediate`
+name, and whether the `elapsed >= BOOT_DELAY_MS * 0.8` assertion is CI-timing
+sensitive (it isn't — a slow machine only makes `>=` more true).
+
+Awaiting the human `pr` gate.

packages/codev/src/agent-farm/__tests__/helpers/tower-test-utils.ts

@@ -80,6 +80,23 @@ export async function waitForPort(port: number, timeoutMs: number): Promise<bool
   return false;
 }
 
+/**
+ * Wait for a port to start listening, retrying as tightly as the event loop
+ * allows.
+ *
+ * Issue #1261: waitForPort's 200ms cadence usually lands well after Tower's
+ * boot sequence finishes, which is why the startup-window race stayed
+ * invisible to the test suite. Tests that need to hit the instant of the bind
+ * — before anything else has had a chance to run — use this instead.
+ */
+export async function waitForPortImmediate(port: number, timeoutMs: number): Promise<boolean> {
+  const start = Date.now();
+  while (Date.now() - start < timeoutMs) {
+    if (await isPortListening(port)) return true;
+  }
+  return false;
+}
+
 /**
  * Find an available port starting from the given port
  */
@@ -91,11 +108,23 @@ async function findAvailablePort(startPort: number): Promise<number> {
   throw new Error(`No available port found starting from ${startPort}`);
 }
 
+export interface StartTowerOptions {
+  /**
+   * Return the moment the port accepts a connection rather than on the next
+   * 200ms poll tick. Issue #1261 tests need to race the bind itself.
+   */
+  returnAtBind?: boolean;
+}
+
 /**
  * Start the tower server for testing.
  * Creates an isolated socket directory for shellper sessions (Spec 0116).
  */
-export async function startTower(port?: number, extraEnv?: Record<string, string>): Promise<TowerHandle> {
+export async function startTower(
+  port?: number,
+  extraEnv?: Record<string, string>,
+  opts?: StartTowerOptions,
+): Promise<TowerHandle> {
   const actualPort = port ?? (await findAvailablePort(14100));
 
   // Spec 0116: Create isolated socket dir so tests don't pollute ~/.codev/run/
@@ -120,7 +149,9 @@ export async function startTower(port?: number, extraEnv?: Record<string, string
   proc.stderr?.on('data', (d) => (stderr += d.toString()));
 
   // Wait for tower to start
-  const started = await waitForPort(actualPort, TOWER_START_TIMEOUT);
+  const started = opts?.returnAtBind
+    ? await waitForPortImmediate(actualPort, TOWER_START_TIMEOUT)
+    : await waitForPort(actualPort, TOWER_START_TIMEOUT);
   if (!started) {
     proc.kill();
     try { rmSync(socketDir, { recursive: true, force: true }); } catch { /* ignore */ }

packages/codev/src/agent-farm/__tests__/tower-routes.test.ts

@@ -63,6 +63,9 @@ vi.mock('../servers/tower-instances.js', () => ({
   getDirectorySuggestions: vi.fn(async () => []),
   launchInstance: vi.fn(async () => ({ success: true })),
   killTerminalWithShellper: vi.fn(async () => true),
+  // Issue #1261: routes that need the instances module ask this first, so a
+  // wired-up Tower is the default for every route test here.
+  instancesReady: vi.fn(() => true),
   stopInstance: vi.fn(async () => ({ ok: true })),
   addArchitect: vi.fn(async () => ({ success: true, name: 'sibling', terminalId: 'term-arch-sibling' })),
   removeArchitect: vi.fn(async () => ({ success: true })),
@@ -978,6 +981,24 @@ describe('tower-routes', () => {
       expect(deleteTerminalSession).not.toHaveBeenCalled();
       expect(removeTerminalFromRegistry).not.toHaveBeenCalled();
     });
+
+    // Issue #1261: "Tower isn't wired up yet" is not "no such terminal".
+    // Answering 404 sent callers off hunting for a terminal that was there all
+    // along; 503 + Retry-After tells them to try again instead.
+    it('returns 503 rather than 404 when the instances module is not wired yet', async () => {
+      const { instancesReady, killTerminalWithShellper } = await import('../servers/tower-instances.js');
+      (instancesReady as any).mockReturnValueOnce(false);
+
+      const req = makeReq('DELETE', `/api/terminals/${terminalId}`);
+      const { res, statusCode, body, headers } = makeRes();
+      await handleRequest(req, res, makeCtx());
+
+      expect(statusCode()).toBe(503);
+      expect(headers()['Retry-After']).toBe('1');
+      expect(JSON.parse(body()).error).toBe('STARTING_UP');
+      // And it must not have tried to kill anything on the way out.
+      expect(killTerminalWithShellper).not.toHaveBeenCalled();
+    });
   });
 
   // =========================================================================

packages/codev/src/agent-farm/__tests__/tower-startup-readiness.e2e.test.ts

@@ -0,0 +1,92 @@
+/**
+ * Issue #1261: Tower must not serve API requests before its internal
+ * dependency wiring is complete.
+ *
+ * Tower binds its port first — the port is the single-Tower mutex and the
+ * thing every readiness probe checks — but binding used to mean "serving".
+ * Requests landing between the bind and `initInstances()` were answered by a
+ * half-wired Tower: `DELETE /api/terminals/:id` returned 404 for a terminal
+ * that existed, because `killTerminalWithShellper()` bails when `_deps` is
+ * null and the route reads that `false` as "not found".
+ *
+ * These tests widen that window to a known duration with
+ * AF_TEST_BOOT_DELAY_MS, so the race is deterministic rather than a function
+ * of how long the boot's disk work takes on the machine running the suite.
+ * That is why the bug hid from CI: fresh runners boot fast enough that the
+ * window closed before the test's first request arrived.
+ */
+
+import { describe, it, expect, afterEach } from 'vitest';
+import {
+  startTower,
+  cleanupAllTerminals,
+  cleanupTestDb,
+  type TowerHandle,
+} from './helpers/tower-test-utils.js';
+
+const TEST_TOWER_PORT = 14107;
+
+// Long enough that no plausible scheduling delay lets a request slip in after
+// wiring completes; short enough to keep the suite quick.
+const BOOT_DELAY_MS = 1500;
+
+let tower: TowerHandle | null = null;
+
+afterEach(async () => {
+  if (tower) {
+    await cleanupAllTerminals(tower.port);
+    await tower.stop();
+    cleanupTestDb(tower.port);
+    tower = null;
+  }
+});
+
+describe('Tower startup readiness (Issue #1261)', () => {
+  it('does not answer DELETE /api/terminals/:id with a spurious 404 during startup', async () => {
+    tower = await startTower(
+      TEST_TOWER_PORT,
+      { AF_TEST_BOOT_DELAY_MS: String(BOOT_DELAY_MS) },
+      { returnAtBind: true },
+    );
+
+    // Both requests are issued inside the delayed window. Before the fix the
+    // DELETE returned 404 while the terminal was demonstrably still there.
+    const createRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/api/terminals`, {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ label: 'readiness-1261' }),
+    });
+    expect(createRes.status).toBe(201);
+    const created = await createRes.json();
+
+    const deleteRes = await fetch(
+      `http://localhost:${TEST_TOWER_PORT}/api/terminals/${created.id}`,
+      { method: 'DELETE' },
+    );
+    expect(deleteRes.status).toBe(204);
+
+    // And the delete really happened — a 204 that killed nothing would be a
+    // different lie with the same status code.
+    const getRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/api/terminals/${created.id}`);
+    expect(getRes.status).toBe(404);
+  }, 30_000);
+
+  it('holds requests issued at bind time until wiring completes', async () => {
+    tower = await startTower(
+      TEST_TOWER_PORT,
+      { AF_TEST_BOOT_DELAY_MS: String(BOOT_DELAY_MS) },
+      { returnAtBind: true },
+    );
+
+    // `afx tower start` polls /api/status and treats 200 as "Tower is up".
+    // That signal was dishonest: /api/status answered 200 during the window
+    // because getInstances() returns [] when the module isn't wired. The gate
+    // makes the first 200 mean what the CLI assumes it means.
+    const started = Date.now();
+    const statusRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/api/status`);
+    const elapsed = Date.now() - started;
+
+    expect(statusRes.status).toBe(200);
+    expect(elapsed).toBeGreaterThanOrEqual(BOOT_DELAY_MS * 0.8);
+  }, 30_000);
+});

packages/codev/src/agent-farm/servers/tower-instances.ts

@@ -168,6 +168,21 @@ export function shutdownInstances(): void {
   _deps = null;
 }
 
+/**
+ * Whether the module has been wired up.
+ *
+ * Issue #1261: routes that call into this module need to tell "not wired yet"
+ * apart from "no such thing", because those deserve different answers — 503
+ * "try again" versus 404 "it isn't here". `killTerminalWithShellper()` returns
+ * a bare boolean and cannot express the difference, so the DELETE route lands
+ * on 404 for a terminal that exists. Tower's readiness gate now holds requests
+ * until boot completes, so this should never be false at request time; it is
+ * kept as a second line of defence.
+ */
+export function instancesReady(): boolean {
+  return _deps !== null;
+}
+
 // ============================================================================
 // Known workspace registration
 // ============================================================================