diff --git a/codev/evidence/1365-dev-approval-transcript.txt b/codev/evidence/1365-dev-approval-transcript.txt new file mode 100644 index 000000000..7cfbbf07f --- /dev/null +++ b/codev/evidence/1365-dev-approval-transcript.txt @@ -0,0 +1,80 @@ +Issue #1365 — dev-approval evidence +isolated Tower on port 14650 (NOT 4100 — the live Tower is untouched) +tower build: /home/user/code/codev_root/codev/.builders/pir-1365/packages/codev/dist/agent-farm/servers/tower-server.js +started: 2026-08-18T00:08:35.776Z + +workspace: /home/user/.agent-farm/test-workspaces/pir1365-rTSkWJ +terminal: 7dbbf04f-cc9d-4444-a11a-b863edf2bb7a (real shellper-backed PTY, echo oracle) + +============================================================================== +SCENARIO 1 — 10x long multi-line send + concurrent --interrupt +============================================================================== + PASS run 0: body never fragmented on the wire (whole=1 starts=1) + PASS run 0: the body was not duplicated — saw 1 + PASS run 0: reported delivered → the body IS on the wire, whole + PASS run 0: the interrupt itself landed + PASS run 0: the interrupt's ^C landed + PASS run 1: body never fragmented on the wire (whole=1 starts=1) + PASS run 1: the body was not duplicated — saw 1 + PASS run 1: reported delivered → the body IS on the wire, whole + PASS run 1: the interrupt itself landed + PASS run 1: the interrupt's ^C landed + … [runs 2-8 identical — 35 further checks, all PASS; trimmed for length] + PASS run 9: body never fragmented on the wire (whole=1 starts=1) + PASS run 9: the body was not duplicated — saw 1 + PASS run 9: reported delivered → the body IS on the wire, whole + PASS run 9: the interrupt itself landed + PASS run 9: the interrupt's ^C landed + + at request time: delivered=10 held=0 + bodies that reached the wire: 10/10 — every one of them WHOLE + (holding is a correct outcome; fragmenting or lying about delivery is not) + rows still held after scenario 1: 0 + +============================================================================== +SCENARIO 2 — --delay 5 --interrupt against a MID-TURN agent +============================================================================== + PASS the send is SCHEDULED, not written at request time + PASS no body on the wire at request time + PASS the ^C fired at due time + PASS the body did NOT land mid-turn (the screen is still a draft) + PASS the body landed EXACTLY once on the clean prompt — saw 1 + PASS the ^C preceded the body + +============================================================================== +SCENARIO 3 — --interrupt against a busy line returns within the 2000ms ceiling +============================================================================== + interrupt round-trip: 2156ms (ceiling 2000ms) + PASS the interrupt returned near the ceiling, not after the whole write — 2156ms + PASS the interrupt still succeeded (the escape hatch works) + delivery outcome: delivered=false held=true reason=busy + tower: [2026-08-18T00:09:14.349Z] [WARN] interrupt → pir-1365-probe (terminal 7dbbf04f...) waited 2000ms for an in-flight write and proceeded UNSERIALIZED — it may interleave with that write. A message long enough to hold the line this long is the usual cause. + tower: [2026-08-18T00:09:16.139Z] [INFO] [mailbox] write to pir-1365-probe @ pir1365-rTSkWJ was raced by an unserialized operator write — holding 1ca72147… for redelivery rather than reporting it delivered + PASS Tower logged the ceiling degradation loudly (WARN) + PASS the raced delivery reported PREEMPTED and held its row for redelivery + PASS the raced delivery did NOT claim delivered — the false-delivered failure cannot recur here + +============================================================================== +SCENARIO 4 — --escape unchanged; a non-writable terminal is refused +============================================================================== + PASS escape returns 200 ok + PASS the ESC byte reached the PTY + PASS its trailing Enter reached the PTY + escape to a killed terminal → 404 NOT_FOUND + PASS an operator action to a dead terminal is REFUSED, never silently dropped + normal send to the same dead agent → 404 held=undefined reason=undefined + PASS an unknown-after-death agent 404s (registry seam needs a real builder — see note) + + NOT SCRIPTED HERE, and why: + 503 TERMINAL_NOT_WRITABLE needs a shellper socket that has DIED while the + session still reports status=running (#1198). That state cannot be forced + from the public API without killing the shellper out of band, which would + be staging the result rather than observing it. It is covered by + tower-routes.test.ts:1560 against the real route, and this change does not + touch that branch — only the lock the branch returns before reaching. + +============================================================================== +RESULT +============================================================================== + 66/66 checks passed + ALL SCENARIOS PASSED diff --git a/codev/plans/1365-serializer-convergence-route-m.md b/codev/plans/1365-serializer-convergence-route-m.md new file mode 100644 index 000000000..dde7583b8 --- /dev/null +++ b/codev/plans/1365-serializer-convergence-route-m.md @@ -0,0 +1,525 @@ +# PIR Plan: Serializer convergence — route the mailbox write edge through `submitToSession` + +Issue #1365. Refs #1313 / PR #1330 / #1480 (absorbed) / #1481 (interlock, sequenced after this) / #1473 (echo-lag residual). + +**Revision 2** — incorporates the architect's 3-way plan review (gemini APPROVE, codex + +claude REQUEST_CHANGES; both REQUEST_CHANGES reviews ratify Part 1). All five blocking items +are addressed in Part 2; item 4 (`--escape`) also changed Part 1, which now treats escape as +a first-class instance of the bug. Changes from revision 1 are summarised at the end. + +**This plan leads with the evaluation the issue's "Evaluate first" section demands.** The +four failure questions are answered end-to-end from the code, *before* any convergence +design. The evaluation's conclusion (converge) is what the `plan-approval` gate is being +asked to ratify; the implementation section is contingent on that ratification. + +--- + +## Part 1 — Evaluation: how the write paths actually compose + +### The paths, as they exist today + +| # | Path | Lock it takes | Where | +|---|---|---|---| +| A1 | Immediate `--interrupt` | **per-terminal** `submitToSession(terminalId, …)` | `servers/tower-routes.ts:2023` | +| A2 | Immediate `--escape` | **per-terminal** `submitToSession(terminalId, …)` | `servers/tower-routes.ts:1963` | +| B | Gated mailbox delivery (every normal send, cron, held-drain, owner notice) | **per-agent** `KeyedSerializer` keyed by `agentKey(workspacePath, toAgent)` | `servers/mailbox-delivery.ts:490-500` → `servers/mailbox-wiring.ts:215` → `writeMessagePaced` (`servers/message-write.ts:133`) | +| C | Delayed `--interrupt` | **per-terminal** for the bare `^C` only; the body then rides path B | `servers/tower-routes.ts:1724-1752` | + +A1/A2/C take the same lock. B takes a disjoint one. The two lock spaces never intersect, so +**A/C vs B is unserialized** — the boundary this issue names. + +Four facts bound the analysis, each verified in code rather than assumed: + +1. `PtySession.write()` (`terminal/pty-session.ts:549`) is what *both* families call, and it + emits no delivery signal — only `handleUserInput` (`terminal/pty-session.ts:802`, human + keystrokes) emits `'submit'`; `'quiescence'` comes off an output timer + (`terminal/pty-session.ts:503-508`). No write from inside a lock can synchronously + re-enter the other lock. +2. `writeMessagePaced` (path B) returns `false` only when a **PTY write is dropped** (#1198, + dead shellper socket). It cannot detect *semantic* loss — bytes the PTY accepted but the + TUI then discarded. +3. The render gate's TOCTOU re-validation (`mailbox-delivery.ts:397`) closes the window + between *classify* and *write start*. Nothing covers write-start through the trailing + Enter — 50 ms (short message) to `(lines−1)×10+80` ms (paced multi-line). +4. `ringToken` is built from `bytesWritten`, a cumulative **output** counter. Input that has + not yet been echoed is invisible to it. This is the echo-lag residual (#1473) and it + bounds what any pre-write check can promise. + +### Q1 — Can a gated delivery interleave between a Ctrl+C and the post-interrupt clean prompt? + +**Yes, in both orderings — and `--escape` is a third instance of the same class.** + +The immediate-interrupt critical section is `^C` → 100 ms settle → text → Enter, all inside +one `submitToSession` acquisition (`tower-routes.ts:2023-2026`). Path B holds none of that +lock. + +**Ordering 1 — the mailbox write lands inside the interrupt's settle window.** The drainer +classifies clean at t₀, re-validates the ring token, starts `writeMessagePaced`. The +interrupt's `^C` fires at t₀+ε and its own text is scheduled for `^C`+100 ms. Both bodies now +occupy one composer and a single Enter submits the fusion — the `w1a` blob shape Spec 1313 +exists to make impossible. Narrow (the `^C` must land after the token re-check), but real. + +**Ordering 2 — the `^C` lands inside the delivery's own text→Enter window, and the row is +still marked `delivered`.** The delivery writes text at t₀ and schedules Enter at t₀+50 ms +(short) or +80 ms after the last line. A `^C` in that window clears the composer; at t₀+50 ms +the delivery's Enter fires into an empty prompt. `writeMessagePaced` returns `true` — every +byte *was* accepted by the PTY (fact 2) — so `markDelivered` transitions the row +(`mailbox-delivery.ts:463`) and the delivery is broadcast. + +That is **silent message loss with a false `delivered` audit record**, the one outcome Spec +1313's architecture exists to exclude. The gate cannot help: it proved the prompt empty +*before* the write, and nothing re-checks after. + +**Ordering 3 — `--escape` truncates an in-flight multi-line delivery** (raised by claude in +review; folded in here as first-class). `writeEscapeToSession` +(`servers/message-write.ts:53`) writes a bare ESC, then Enter 50 ms later. A multi-line +delivery writes its lines 10 ms apart, so an ESC arriving mid-sequence discards what has been +typed so far, the delivery's remaining lines land on the now-cleared composer, and either +Enter (escape's at +50 ms, or the delivery's own) submits a **truncated body**. The row is +marked `delivered`. Escape is not a milder cousin of interrupt here — for multi-line +deliveries it is the more likely trigger, because the delivery's exposed window is longest +exactly when the body is long. + +Note the asymmetry with the interrupt path's own claim-first tradeoff +(`tower-routes.ts:1997-2008`): there, a lost message is a *documented, deliberate* choice by +an operator taking an explicit bypass. Here it is an autonomous background delivery losing a +message it reports as delivered, with no operator present. + +### Q2 — Does the delayed-interrupt reshape leave a window where the body lands mid-turn? + +**No.** The reshape (`tower-routes.ts:1715-1753`) fires only the `^C` on the timer; the body +is a persisted `not_before` row that can leave the mailbox only through `deliverAgentMail`, +which requires a render-verified empty prompt. There is no code path from the timer to a body +write. A mid-turn screen classifies not-clean → held. The reshape is sound and needs no +change. + +Two true-but-benign residuals, recorded because #1481 inherits them: + +- **`^C`→body is not atomic.** The `^C` callback returns `0`, so the terminal lock releases + immediately; `scheduleDrain` (line 1751) then runs the gate. Anything may occupy the + terminal in between. Correct by design — the gate re-decides — but the delayed interrupt + guarantees "the turn was ended," never "this body is next." +- **A no-op `^C` is indistinguishable from success to the caller.** Liveness is re-checked + inside the lock (lines 1733-1735); a dead/unwritable session logs and drops the nudge. + +Under Ordering 2, the delayed path is *also* an exposed writer: its `^C` can clear another +delivery's half-written composer. It fires **unattended**, on a timer — so the "a human is +standing at this terminal" premise that makes the immediate path's race acceptable does not +apply to it at all. + +### Q3 — Is escalation/held state consistent if an interrupt tears the session mid-write? + +**Bookkeeping stays internally consistent; its correspondence to reality does not.** + +- **Session torn down mid-write** (socket death, not interrupt): `writeMessagePaced` returns + `false` → `hold('no-live-pty')` (`mailbox-delivery.ts:458`); row stays held, escalation + clock keeps running. **Correct.** +- **Interrupt clears the composer mid-write** (Ordering 2) **or escape truncates it** + (Ordering 3): row → `delivered`, leaves the held set, `onHeldStateChange` fires, held count + drops, any `escalated` flag becomes moot. Every derived indicator agrees with the DB, and + the DB is **wrong**. Nothing after the write re-examines the screen, so there is no + detector. **This is the inconsistency** — invisible rather than noisy. Ordering 3 is the + worse variant: the agent receives a *partial* message, so the failure can propagate as + acted-upon-but-wrong rather than merely absent. +- **Immediate interrupt's own row**: claimed `delivered` before the write by design + (`tower-routes.ts:1997-2008`) — consistent with its documented tradeoff; unchanged here. +- **Escalation/liveness telemetry**: unaffected either way. `isClassifierStuck` + (`mailbox-delivery.ts:242`) escalates only `no-profile` / `no-region-end` / + `no-composer-marker`, so neither `busy` nor `no-live-pty` holds can false-alarm. + +### Q4 — Should the disjoint-lock boundary stay accepted, or converge? + +**Converge.** In order of weight: + +1. **The failure is silent loss (or silent truncation) with a false `delivered`, not a + garbled composer.** The accepted-boundary argument covers the fusion case (Ordering 1) but + not Orderings 2 and 3, which were not separately reasoned about when the boundary was + accepted. +2. **The "an operator is present" premise is false for the delayed path.** Path C fires on a + timer with nobody watching, and #1481 (`--interrupt-after`) turns that from a rarity into + a routine, *scheduled* co-occurrence of an interrupt and a pending gated delivery to the + same terminal. The boundary gets more load-bearing exactly where the workstream is heading. +3. **The fix is cheap and cycle-free.** The per-terminal lock is acquired as a leaf inside the + per-agent serializer, so the order is always agent → terminal, never the reverse; fact 1 + rules out re-entrancy. + +**What convergence buys, stated precisely** (revised per review item 3 — revision 1 overclaimed +here): + +> **Serialization is the structural guarantee.** After this change, no lock-taking writer — +> gated delivery, `--interrupt`, `--escape`, delayed `^C` — can put bytes on a terminal while +> another lock-taking writer's submission is in flight. That is what closes Orderings 1, 2 +> and 3, and it is a property of the lock, not of any check. +> +> **The in-lock precheck narrows, but does not close, the echo-lag residual.** Re-validating +> the gate verdict inside the lock is *necessary* — without it, a delivery that classified +> clean and then waited behind an interrupt would write onto a screen the interrupt just +> changed, which is worse than today. But `ringToken` tracks output (fact 4), so input from a +> writer that does **not** take the lock — the raw `POST /api/terminals/:id/write` +> passthrough, and human keystrokes over the WebSocket — can sit on the line un-echoed and +> defeat it. Those writers stay deliberately uncovered (a human owns their own composer), so +> that residual survives this change by design. It is **#1473's territory**, and the boundary +> comment must say so rather than implying the race is gone. + +The lock must also stay a **leaf around the write only**, never widened to cover the async +classify: `--interrupt` is the human's escape hatch for a wedged agent, and making it queue +behind a gate classification would be a real regression for the one action that must always +get through fast. + +**Interlock note for #1481** (`--interrupt-after`): after this change, "interrupt, then +deliver this body" is expressible as *ordered acquisitions of one lock* rather than a race +between two. #1481 should build on that and must not re-introduce a body write outside the +gate. The Q2 residuals (the `^C`→body gap is gate-mediated, not atomic; a no-op `^C` is only +logged) are the two things it must design against explicitly. + +--- + +## Part 2 — Proposed change (contingent on the gate ratifying Part 1) + +Route path B's write edge through `submitToSession`, keyed by the terminal id, as a leaf +inside the existing per-agent serializer, with the delivery's preconditions re-validated +inside the lock — and with the delivery side using a **non-blocking** acquisition so the +drainer can never stall. + +### D1. The delivery path learns the terminal id, and the id is guarded at runtime + +`DeliverySession` (`mailbox-delivery.ts:52`) gains `readonly id: string`. `PtySession` +already has it (`terminal/pty-session.ts:118`), so the live binding is free. + +**Runtime guard (review item 5).** A missing id must not silently become a global lock. +`tower-routes.test.ts:221`'s `gateSession()` is an un-annotated object literal with **no +`id`** that reaches the *real* `mailbox-wiring` binding — verified. Structural typing means +such a fake can compile while keying every lock on `undefined`, collapsing per-terminal +serialization into one global lock without a single failing assertion. So `submitMessagePaced` +throws on a non-string/empty id. A throw there propagates out of `writeMessage`, past the +`finally` that invalidates the memo, through the per-agent serializer to the drainer's +per-agent `try/catch` (`mailbox-delivery.ts:661`) — logged, row stays **held**, never marked +delivered. Fail-loud and fail-safe. `gateSession()` gains a real `id` and is added to the +change list. + +### D2. Asymmetric acquisition — deliveries try, operators block (review item 2) + +The review is right that a symmetric blocking lock is a liveness regression: +`MailboxDrainer.tick` awaits agents **sequentially** (`mailbox-delivery.ts:644-664`), so one +agent blocked on a terminal lock stalls every other agent's delivery, plus that tick's +escalation, owner-notice and prune passes. + +`session-submit.ts` therefore gains a non-blocking sibling: + +```ts +/** True while a submission is in flight for this session. */ +export function isSubmissionInFlight(sessionId: string): boolean; + +/** submitToSession, but abandons instead of queueing when the session is contended. + * Resolves `false` (nothing written) if another submission holds the session. */ +export function trySubmitToSession(sessionId, write, clock?): Promise; +``` + +The check-then-install is race-free without ceremony: JS is single-threaded and `chains.has` +→ `chains.set` has no await between them, so no second caller can interleave. + +- **Delivery (path B) uses `trySubmitToSession`.** Contended → write nothing, return + `aborted:'busy'`, row stays held, backstop retries in ≤1.5 s. This costs nothing real: a + contended terminal means another writer is mid-submission, so the in-lock precheck would + have aborted the delivery anyway. The drainer never blocks, so head-of-line blocking is + gone — a *stronger* liveness property than today, where the drainer awaits a full paced + write per agent. +- **`--interrupt` / `--escape` / delayed `^C` keep `submitToSession`** and block. They are + operator actions that must land. + +Starvation is not a concern in the other direction: interrupts are human-rate, deliveries +retry every 1.5 s. + +### D3. Interrupt latency has a new worst case, and needs a ceiling (new finding) + +Not in the review, and it changes the shape of D2's operator side. Today `--interrupt` never +waits. After convergence it waits for any in-flight delivery write, whose duration is +`(lines−1)×10+80` ms — and body size is bounded only by `parseJsonBody`'s **1 MiB** default +(`agent-farm/utils/server-utils.ts:47`). A 48 KB `--file` attachment of short lines is ~48k +lines ≈ **8 minutes**; a realistic 500-line paste is ~5 s. So "the escape hatch stalls behind +a long message" is an ordinary case, not a pathological one, and an unbounded block on +`afx interrupt` would be a worse regression than the bug being fixed. + +**Remedy: a bounded wait with explicit degradation.** `submitToSession` gains an optional +`waitCeilingMs` (default off; set for the interrupt/escape call sites, proposed **2000 ms**). +If the lock is not acquired within the ceiling, the operator write proceeds **unserialized** +and logs loudly at WARN with the session id and the wait. + +This is strictly better than today at every point: below the ceiling we get the full +guarantee; at or above it we degrade to exactly today's unserialized behaviour, which is the +status quo — never worse — and we now say so in the log instead of never knowing. The +alternative (unbounded block) trades a rare silent corruption for a routine visible hang on +the one action that exists to rescue a wedged agent. + +The ceiling value is a judgment call and is flagged for the reviewer. Consider it settled +only if the gate says so; the fallback is unbounded blocking plus a documented latency +tradeoff. + +### D4. The in-lock precheck (including the row-status re-check, review item 1) + +The `writeMessage` port cannot express "aborted before writing anything," so it changes shape: + +```ts +/** Why a gated write abandoned inside the lock. */ +export type WriteAbort = + | { kind: 'hold'; reason: MailboxReason } // re-hold: busy (screen moved / contended) or no-live-pty + | { kind: 'row-resolved' }; // dismissed/superseded under us — terminal state, no hold + +export type WriteResult = + | { status: 'written' } // every byte landed, Enter included + | { status: 'dropped' } // #1198 partial/dropped → hold no-live-pty + | { status: 'aborted'; abort: WriteAbort }; + +writeMessage( + session: DeliverySession, + formattedMessage: string, + noEnter: boolean, + precheck: () => WriteAbort | null, // null = proceed; runs INSIDE the lock +): WriteResult | Promise; +``` + +`deliverAgentMail` supplies the precheck, so all reason authority stays in the delivery +module. It re-runs, at the write instant, the three checks the code already performs before +the lock: + +```ts +() => { + if (!session.writable) return { kind: 'hold', reason: 'no-live-pty' }; // mirrors :424 + if (ringToken(session, profile) !== tokenBefore) // mirrors :397 + return { kind: 'hold', reason: 'busy' }; + const now = getById(db, row.id); // mirrors :411 — review item 1 + if (!now || now.status !== 'held') return { kind: 'row-resolved' }; + return null; +} +``` + +The row-status re-check is load-bearing, not defensive tidiness: without it, this change +would **widen** the dismiss→bytes-on-wire window from ~zero to the whole lock wait. Dismiss +and supersede are independent synchronous DB writes not routed through the delivery +serializer (as `mailbox-delivery.ts:404-410` already explains), and `better-sqlite3` is +synchronous, so this re-read sees anything committed up to the write instant. The pre-lock +checks stay as cheap fast-paths that avoid a pointless acquisition. + +Outcome mapping in `deliverAgentMail`, replacing `if (!written)` at `:458`: + +| result | action | +|---|---| +| `written` | `markDelivered` + broadcast, as today | +| `dropped` | `hold('no-live-pty')`, as today | +| `aborted / hold` | `hold(reason)` | +| `aborted / row-resolved` | `ports.onHeldStateChange()` + `{ delivered: [], reason: null }` — exactly the existing `:411-416` branch | + +The unconditional `finally { memo?.delete(cacheKey) }` (`:447`) stays. An aborted write puts +no bytes on the wire, but discarding a verdict we just proved stale is right anyway, and +keeping it unconditional preserves the rejection-safety its comment argues for. + +### D5. `submitMessagePaced` — the new leaf + +In `message-write.ts` (which then imports `session-submit.ts`; `session-submit.ts` imports +nothing, so no cycle): + +```ts +export async function submitMessagePaced( + session: WritableSession & { id: string }, message: string, noEnter: boolean, + precheck: () => WriteAbort | null, +): Promise +``` + +It guards the id (D1), acquires via `trySubmitToSession` (D2) — contended → return +`aborted:{kind:'hold',reason:'busy'}` — runs `precheck()` first thing inside the callback +(abort → return offset `0`, nothing written), otherwise runs the existing tracked paced write +and returns its completion offset so the lock is held through the trailing Enter. + +Enter-before-resolve ordering is preserved: `submitToSession` registers its `sleep` timer +*after* `write()` returns — i.e. after the Enter's `setTimeout` was registered at the same +offset — so the Enter still executes first, exactly as `writeMessagePaced` documents +(`message-write.ts:124-127`). This is asserted with fake timers (see Test Plan). + +`writeMessagePaced`'s only live caller is `mailbox-wiring.ts:215`; after this it is +test-only. It will be removed and `spec-1313-paced-write-drop.test.ts` re-pointed at +`submitMessagePaced`, unless review prefers keeping the unlocked primitive exported. + +### D6. Wiring + +`mailbox-wiring.ts:215` becomes +`writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck)`. + +### Lock-order / deadlock safety (must hold, and does) + +- Order is always **per-agent → per-terminal**; nothing acquires them in reverse. Paths + A1/A2/C take only the per-terminal lock and never enter the per-agent serializer. +- No synchronous re-entry (fact 1). Even if a `scheduleDrain` were raised from inside a lock + callback it is `void`-ed onto a microtask and never awaited. +- With D2 the delivery side never blocks at all, so the only wait in the system is an + operator waiting on a delivery — bounded by D3. + +### Files to change + +- `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` — `DeliverySession.id`; + `WriteAbort`/`WriteResult`; `writeMessage` port signature + doc; precheck construction and + outcome mapping around `:397-:466`. +- `packages/codev/src/agent-farm/servers/session-submit.ts` — add `isSubmissionInFlight`, + `trySubmitToSession`, `waitCeilingMs`; **rewrite the "Exactly what it covers" boundary + comment** (`:42-69`): it now covers gated mailbox deliveries; state the per-agent → + per-terminal order, the asymmetric acquisition and why, the D3 ceiling and its degradation, + why the lock is a leaf around the write and not the classify, that **serialization is the + structural guarantee while the precheck only narrows the echo-lag residual (#1473)**, and + what stays deliberately uncovered (`POST /api/terminals/:id/write`, WS keystrokes). + *Deliverable in either outcome.* +- `packages/codev/src/agent-farm/servers/message-write.ts` — add `submitMessagePaced`; retire + `writeMessagePaced`. +- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts:215` — bind the new port shape. +- `packages/codev/src/agent-farm/servers/tower-routes.ts` — replace the "flagged, not done + here" scope paragraph (`:2017-2022`) with the converged guarantee; pass `waitCeilingMs` at + the interrupt (`:2023`), escape (`:1963`) and delayed-`^C` (`:1728`) call sites. +- `codev/resources/arch.md` §7 item 5 (~line 1798) — replace the "disjoint lock … accepted, + documented boundary" sentence with the converged model: one lock at the write edge, the + agent→terminal order, the delivery-tries/operator-blocks asymmetry, the delayed-interrupt + sequencing (`^C` on the timer, body through the gate, not atomic by design), and the + #1473 residual stated honestly. *Deliverable in either outcome.* +- `codev/resources/arch-critical.md` — the review suggests a hot-tier fact for the + agent→terminal lock-order invariant. The hot tier is **at its 10-fact cap**, so this + requires demoting an existing fact. Proposed in the review phase as an explicit + displacement recommendation for the maintainer, not applied unilaterally. +- Tests: new `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts`; + fake/port updates in `tower-routes.test.ts` (`gateSession` at `:221` — add `id`), + `send-delivery.test.ts` (fake session **plus** the inline `ports.writeMessage` overrides at + `:422`, `:604`, `:618` — note `:604` currently returns `undefined` and relies on falsy⇒hold, + which the typed result makes explicit), `send-mailbox-repro.test.ts`, + `cron-delivery.test.ts`, `send-architect-identity.test.ts`; + `spec-1313-paced-write-drop.test.ts` re-pointed. + +No skeleton mirror: this is `packages/codev` source plus our own `codev/resources/`, not +framework template content. + +### Phasing (git commits inside one PR) + +1. **Lock primitives** — `isSubmissionInFlight`, `trySubmitToSession`, `waitCeilingMs`, with + their own tests (including the D3 ceiling's degradation path). +2. **Convergence + delivery tests** — the port reshape, precheck, `submitMessagePaced`, + wiring, fake updates; red-to-green on the race tests. +3. **Boundary model documentation** — `session-submit.ts`, `tower-routes.ts`, `arch.md`, + written against what actually landed. +4. **CMAP consultation** (implementation + tests) and fixes; review doc, incl. the + `arch-critical.md` displacement proposal. + +## Risks & Alternatives Considered + +- **Risk — the lock moves the race instead of closing it.** A delivery that classified clean + before queuing would write onto a screen the writer ahead of it just changed. *Mitigation*: + the in-lock precheck (D4) is mandatory and is what the new tests assert; with D2 the + delivery does not queue at all. Dropping the precheck turns this change into a regression. +- **Risk — interrupt latency (D3).** Bounded by the ceiling, degrading to today's behaviour + with a loud WARN. Unbounded blocking is the rejected alternative. +- **Risk — head-of-line blocking in the sequential drainer.** Removed by D2; the drainer + never waits on a terminal lock. +- **Risk — port signature churn breaks fakes.** Six test files. Compile-time, except the + `undefined`-returning override at `send-delivery.test.ts:604`, which is called out + explicitly. +- **Risk — more `busy` holds under contention.** Intended (those writes would have landed on + a moved screen), and the backstop re-delivers within 1.5 s. +- **Risk — a silently global lock from a missing id.** Closed by D1's runtime guard plus the + different-terminals test. +- **Alternative — accept the boundary (wontfix).** Rejected on Orderings 2 and 3: a false + `delivered` on a lost or truncated message is not a robustness nicety, and #1481 removes the + "operator is present" premise. Had the analysis found only Ordering 1, wontfix would have + been defensible. +- **Alternative — symmetric blocking lock.** Rejected per review item 2: stalls the + sequential drainer for every other agent. +- **Alternative — widen `submitToSession` to hold across gate + write.** Rejected: queues the + human's escape hatch behind an async classification and buys nothing the precheck does not. +- **Alternative — keep `boolean` and map an aborted write to `false`.** Simpler, no port + churn, but reports `no-live-pty` for a `busy` abort and cannot express `row-resolved` at + all. Rejected; noted as the fallback if review wants minimal surface. +- **Alternative — post-write verification (re-classify after Enter, re-hold on mismatch).** + Rejected: detect-and-repair is the architecture Spec 1313 replaced, and a re-hold risks + double delivery. + +## Test Plan + +**Unit** — new `spec-1365-serializer-convergence.test.ts`, fake sessions with an ordered +write log, `vi.useFakeTimers()` (the paced writer uses raw `setTimeout`, so an injected clock +alone cannot assert ordering — review's non-blocking note): + +*Serialization (the structural guarantee)* +- A gated delivery and a concurrent immediate `--interrupt` to the same terminal **never + interleave**: the `^C` appears wholly before the delivery's text or wholly after its Enter. + (Fails on `main`.) +- Same for `--escape` (Ordering 3): a multi-line delivery is **never truncated** by an ESC + landing between its lines. (Fails on `main`.) +- **Ordering 2/3 regression**: an interrupt or escape racing an in-flight delivery no longer + leaves the row `delivered` — the delivery either completes intact or aborts with the row + still `held`. (Fails on `main`: today the row reads `delivered` with nothing, or a + fragment, on the agent's screen.) + +*In-lock precheck* +- Screen moved while queued → zero bytes written, hold `busy`, `markDelivered` never runs. +- Session became unwritable → zero bytes, hold `no-live-pty`. +- **Row dismissed/superseded during the lock wait → zero bytes, no hold, `onHeldStateChange` + fires, outcome `{delivered: [], reason: null}`** (review item 1). + +*Asymmetric acquisition (D2)* +- Contended terminal → delivery returns `aborted:'busy'` **immediately** (no await on the + holder) and the row stays held. +- **The drainer does not stall**: with agent A's terminal held by a long operator submission, + agent B's delivery in the same `tick()` still completes, and escalation/prune still run. +- Interrupt/escape still **block** and land after the in-flight write. + +*Ceiling (D3)* +- An operator submission waiting past `waitCeilingMs` proceeds unserialized and logs WARN; + below the ceiling it serializes normally. + +*Key hygiene (review item 5)* +- **Deliveries to two different terminals do not serialize** (proves the key is the real id). +- A session with a missing/empty `id` throws from `submitMessagePaced`, the row stays + **held**, and nothing is written. + +*Preserved semantics* +- Dropped write (#1198) still holds `no-live-pty`; two deliveries to one agent still cannot + interleave; the returned promise still resolves **after** the Enter; `noEnter` staging + unchanged. + +*Hygiene* +- After mixed delivery/interrupt/escape traffic settles, `pendingSubmissionSessions() === 0`, + the per-agent serializer is inactive, and every promise settles (deadlock-freedom). + +**Regression suites**: `spec-1273-submission-lock`, `spec-1273-interrupt`, `write-queue`, +`mailbox`, `send-delivery`, `send-mailbox-repro`, `send`, `spec-1313-paced-write-drop`, +`spec-1307-send-delay`, `cron-delivery`, `tower-routes`, plus a full +`pnpm --filter @cluesmith/codev test` and `build`. + +**Manual (for the `dev-approval` gate)** — live Tower, two agents: + +1. `afx send ""` and, within the same second, + `afx interrupt ` from a second shell. Repeat ~10×. Expect: never a fused composer, + and **never** a row in `afx inbox show` reading `delivered` whose text did not appear + (whole and intact) on the agent's screen. +2. Same race with `afx send --escape` — expect no truncated body. +3. `afx send --delay 5 --interrupt ""` against a mid-turn agent: `^C` at due + time, body lands only once the prompt is clean, exactly one copy. +4. **Liveness**: with a large body mid-delivery to agent A, confirm agent B's held mail still + delivers promptly (D2), and that `afx interrupt ` returns within the ceiling (D3), + logging the WARN if it degrades. +5. A not-writable target still 503s `TERMINAL_NOT_WRITABLE`. + +**Cross-platform**: none — server-side Node only, no UI surface. + +--- + +## Changes from revision 1 (for the reviewer) + +| Review item | Where addressed | +|---|---| +| 1 — row-status re-check in the precheck | D4 (`getById` in the precheck; `row-resolved` in `WriteAbort`; outcome-mapping row; dedicated test) | +| 2 — head-of-line blocking / asymmetric try-lock | **Adopted** — D2 (`trySubmitToSession`, delivery fail-fasts, operators block) + drainer-does-not-stall test | +| 3 — echo-lag residual overclaimed | Q4's "What convergence buys, stated precisely"; fact 4 in Part 1; carried into the `session-submit.ts` and arch.md deliverables | +| 4 — `--escape` is a second instance | Part 1 Q1 Ordering 3 + Q3; first-class in the test matrix and manual steps | +| 5 — test-fake id hazard | D1 runtime guard; `gateSession` fix; different-terminals + missing-id tests | +| non-blocking — clock/fake timers | Test Plan preamble | +| non-blocking — file paths, `send-delivery.ts` overrides | Files to change (paths corrected; `:422`/`:604`/`:618` listed, `:604`'s `undefined` return called out) | +| non-blocking — `arch-critical.md` hot fact | Files to change — flagged as requiring displacement at the 10-fact cap; proposed in review, not applied unilaterally | + +**New finding not in the review**: D3 — interrupt latency. Body size is capped only by +`parseJsonBody`'s 1 MiB, so a blocking operator acquisition can wait minutes on a realistic +`--file`-sized body. Proposed remedy is a bounded wait that degrades to today's behaviour with +a loud WARN. The ceiling value (2000 ms) is a judgment call flagged for the gate. diff --git a/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md b/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md new file mode 100644 index 000000000..312b2f690 --- /dev/null +++ b/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md @@ -0,0 +1,124 @@ +# Review iteration 1 — dispositions + +| Lane | Verdict | Action | +|---|---|---| +| gemini | APPROVE, no key issues | none required | +| codex | APPROVE, no key issues | none required | +| claude | **REQUEST_CHANGES** | all six points addressed below | + +A second, independent CMAP was run by the architect against PR #1492. Its **codex** lane +returned REQUEST_CHANGES and converged on the *same* first finding as this lane's claude — +which is why an APPROVE here was not treated as settling the matter. Every finding was +verified against the code before being accepted or refused; none was dismissed because some +other lane approved. + +--- + +## 1. Operator-vs-operator ceiling regression (blocking) — ACCEPTED, fixed + +**Finding.** `bounded` keyed only off "is anything in flight" and never asked what *kind* of +writer was ahead, so `afx send --interrupt <48 KB --file>` followed by a second `--interrupt` +let the second bypass the first after 2 s. Operator-vs-operator was **always** fully serialized +before #1365 — `submitToSession` had no ceiling at all; that pair is Spec 1273's `/clear` +fusion bug. My ceiling therefore made one pair strictly *worse* than the status quo, and +falsified the review doc's "never worse" claim. + +**Verified.** Real. Reproduced by reading the `bounded` expression at HEAD: nothing in it +distinguished a delivery holder from an operator holder. + +**Fix** (`ece06a5e`). Chain entries carry a `SubmissionKind`; a `pendingOperators` per-session +count tracks operators **queued as well as in flight**; `bounded` gains `&& !behindOperator`. +Self-counted *after* the check (so a submission does not read its own presence as a reason to +block) and *before* any await (so a later operator sees it while merely queued). Queued has to +count: bypassing an operator that has not started is the same violation as bypassing one +mid-write. + +**Pinned by** `operator vs operator NEVER degrades — the wait stays unbounded, as before #1365` +and `a THIRD operator does not bypass a QUEUED one`. The pre-existing ceiling test needed its +holder changed from an operator to a delivery — that fixture change **is** the behaviour +change, not a workaround. + +## 2. A ceiling-degraded `--interrupt` reported `delivered: true` (blocking) — ACCEPTED, fixed + +**Finding.** The row is claimed `delivered` before the write, so a degraded operator write +returned unqualified success with only a Tower-side WARN — the same false-success class this +issue exists to remove, relocated from the delivery path to the operator path. + +**Verified.** Real, and squarely against this PR's own thesis: a success signal must not lie. + +**Fix** (`ece06a5e`). Claim-first is **kept** — un-claiming reopens the double-delivery hole +reasoned through at implement-phase CMAP round 3 — and the truth is surfaced instead: +`/api/send` returns `degraded: true` + `degradedReason: 'submit-wait-ceiling-expired'`, +threaded through `packages/sdk/src/tower-client.ts` and warned about by `afx send`. An +indicator nobody surfaces is half a fix. + +**Pinned by** `a body-bearing interrupt that crosses the wait ceiling reports degraded` +(`tower-routes.test.ts`), whose holder is a delivery — the only thing the ceiling may bypass. + +## 3. Review doc stale relative to the worktree — ACCEPTED, fixed + +Files Changed, the commit list and the test figure are refreshed (**4885 passed / 0 failed / +48 skipped**, 246 files), and the "never worse than the old status quo" wording in *Things to +Look At* item 2 is rewritten to state the guarantee **per pair**. A full `never worse` / +`status quo` / `never waited` grep across `packages/codev/src/agent-farm/` and +`codev/resources/` confirmed no residual claim-site still overstates it. + +## 4. Two files outside the stated 21-file scope — ACCEPTED, disclosed + +`packages/codev/src/agent-farm/commands/send.ts` and `packages/sdk/src/tower-client.ts`. They +are the minimum needed to make a degraded write visible to the *sender* rather than only to +the Tower log, which requires crossing the server→client boundary. Now called out explicitly +in the review doc's Files Changed section so the human meets no surprise at the diff. The +architectural boundary itself is respected: `codev-sdk` still imports only `codev-types`. + +## 5. Nits + +| Nit | Disposition | +|---|---| +| Stale `{@link writeMessagePaced}` in `message-write.ts` | **Fixed** — repointed at `submitMessagePaced` | +| `unserializedWrites` entries never pruned | **Fixed** — see below | +| `DEGRADED_SUBMIT_REASON` orphaning `logCeilingExpired`'s JSDoc | **Fixed** — moved above it | +| Ceiling timer not cancelled when the predecessor wins | **Not taken — flagged** | +| `waited < 100` / `tickMs < 250` timing-sensitive | **Not taken — flagged** | + +**The counter-eviction fix, and why it is not a two-line delete.** `unserializedWrites` cannot +self-delete on drain the way `chains` and `pendingOperators` do: it must **outlive** the +submission whose watcher is about to compare against it, and a reset landing between a +watcher's two reads would read as "nobody raced me" — the exact false `delivered` this issue +exists to eliminate. So eviction is interlocked with an explicit `watchBypasses(sessionId)` +window (held by `submitMessagePaced` across its write, released in a `finally`): refused while +any watch is open, and attempted from **both** the chain's drain cleanup and the last watch's +release, so whichever runs second evicts and no ordering leaks. This needs no session-teardown +hook, and therefore no `terminal/` → `agent-farm/` layer crossing. Pinned by `the +degraded-write counter is evicted once the session goes idle` and `eviction cannot land inside +a watcher window and mask a race`. + +**Also fixed, from the architect's relay of the same lane:** the bypass counter was bumped on +ceiling expiry *regardless of whether bytes went out*. The delayed `^C` re-checks liveness +inside the lock and can write nothing; that no-op forced a concurrent delivery into a spurious +`preempted` re-delivery. `SubmitOptions.wroteBytes` is now consulted straight after the write +callback — no `await` in between, so the ordering guarantee the old placement provided is +unchanged. Pinned by `a degraded write that writes NOTHING is not counted as a bypass`. + +**Why the two refusals.** Cancelling the ceiling timer means adding abort semantics to the +injected `SubmitClock` interface that every test double implements — a broader change than the +cost of one short-lived timer per *contended* operator submission, late in a review round; a +reviewer who disagrees should say so, it is small, just not free. The timing assertions are +deliberate: they are what make "the drainer does not stall" and "the escape hatch stays +responsive" testable claims rather than prose, and both carry ≥2.5× headroom over the +behaviour they exclude. + +## 6. Process note: hot-tier `arch-critical.md` — DISCLOSED, human's call + +The plan said the hot-tier change would be *proposed*; I appended the clause directly. The +reviewer's own read is that it is defensible (it extends the existing mailbox-first fact rather +than adding an eleventh, so the 10-fact cap holds with no displacement) but that ratifying it +is the human's call. Agreed on both counts — it is disclosed in the review doc, and reverting +it is a one-line edit. + +--- + +**Result:** both blocking findings accepted and fixed, three of five nits taken, two refused +with reasons, one process note escalated to the human. `pnpm --filter @cluesmith/codev build` +and `pnpm --filter @cluesmith/codev-sdk build` clean; full suite **4885 passed / 0 failed / +48 skipped**. diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml new file mode 100644 index 000000000..70eba4f45 --- /dev/null +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -0,0 +1,30 @@ +id: '1365' +title: serializer-convergence-route-m +protocol: pir +phase: verified +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-08-17T23:31:21.297Z' + approved_at: '2026-08-17T23:46:57.233Z' + dev-approval: + status: approved + requested_at: '2026-08-18T00:01:11.748Z' + approved_at: '2026-08-18T00:12:08.059Z' + pr: + status: approved + requested_at: '2026-08-18T03:05:41.820Z' + approved_at: '2026-08-18T03:12:26.436Z' +iteration: 1 +build_complete: true +history: [] +started_at: '2026-08-17T23:25:51.772Z' +updated_at: '2026-08-18T03:12:30.821Z' +pr_history: + - phase: review + pr_number: 1492 + branch: builder/pir-1365 + created_at: '2026-08-18T00:14:35.869Z' +pr_ready_for_human: false diff --git a/codev/resources/arch-critical.md b/codev/resources/arch-critical.md index 17bfac744..97d79af83 100644 --- a/codev/resources/arch-critical.md +++ b/codev/resources/arch-critical.md @@ -14,7 +14,7 @@ and keeps the map in sync with arch.md's top-level sections. See codev/resources - State lives in a single user-global ~/.agent-farm/global.db (Issue #1118 retired the per-workspace state.db; architect/builders keyed by workspace_path); one Tower on port 4100. Never modify state by hand. - Worktrees in .builders/ are Agent-Farm-managed — never delete manually (use afx cleanup); run afx from the main workspace root only. - Server/client isolation (#1189): codev-core (server) and codev-sdk (client) never import each other; both import only codev-types. The sdk is environment-agnostic (no node:*/vscode/direct fetch outside its /node adapter; zero runtime deps) — boundary tests on both sides enforce this in CI. -- `afx send` is mailbox-first (Spec 1313): persist to global.db first, then deliver only onto a render-gate-verified empty prompt. Any new message writer routes through the mailbox+gate — never write a PTY directly, never force-inject. Response: `delivered` | `held`+reason. +- `afx send` is mailbox-first (Spec 1313): persist to global.db first, then deliver only onto a render-gate-verified empty prompt. Any new message writer routes through the mailbox+gate — never write a PTY directly, never force-inject. Response: `delivered` | `held`+reason. Every message writer also takes the per-terminal `submitToSession` lock (#1365); lock order is per-agent → per-terminal, never the reverse. - Two human gates (spec-approval, plan-approval) plus the pr gate; only humans transition conceived→specified and committed→integrated. ## Map of arch.md (consult when…) diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 3b891c319..fef005efb 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -1797,7 +1797,15 @@ Spec 1313 replaced Spec 403's in-memory, timer-based, force-flushing `SendBuffer 2. **Gate before every write.** The render-gate (`render-gate.ts`) classifies the session's **persistent bounded headless-screen mirror** (`SessionScreen`, fed the session's output byte-for-byte from birth on the live path — its current viewport IS the live screen) and applies a per-app classifier profile (`gate-profiles.ts`: **claude** & **codex** use a dim-placeholder rule; **agy** uses a color-keyed `placeholderFgPalette` rule — see below). Clean (marker present AND a *positively-bounded* composer region — a rule/status line below the marker, never a scan to the screen bottom — with zero normal-intensity cells) → deliver; else → keep holding. One measured exemption (Spec 1313, 2026-08-06): claude paints a **suggested-command ghost** into an *idle* composer when its own last reply mentioned a runnable command, and the ghost's first char doubles as the software block cursor — SGR-7 **inverse at normal intensity** over a SGR-2-dim tail. The classifier exempts exactly that one cell (inverse + non-dim, at the headless cursor, with a **non-empty** dim tail as positive ghost evidence — `isGhostCursorCell`), so an idle ghost no longer classifies `busy` forever and strands mail to an unattended agent; it is deliberately not a blanket inverse skip (an inverse selection over a real draft keeps every other cell counted), and a real draft never trips it because claude never inverse-renders typed text (the block cursor rests on trailing whitespace) and a lone inverse cell with an empty tail — a 1-char draft with the cursor on its only char — stays `busy` (fail-toward-hold). codex renders its own ghost wholly dim, so the dim rule already covers it. **Why a persistent mirror, not a whole-ring re-render** (Spec 1313 round 2): the gate originally rebuilt the screen each check by replaying the whole output ring through a throwaway terminal. But #1205 caps the ring's newline-free `partial` at 2 MiB (`trimPartial` halves to ~1 MiB), and a claude/codex alt-screen frame is one giant partial — so once a busy long-lived agent's frame crossed the cap the ring handed the gate a **torn** front (dropped composer marker/rule → permanent `busy`), resurrecting the over-ceiling outage for exactly the busiest agents. A bounded terminal mirror needs only the live byte stream, so the cap is irrelevant, the live-ring tear is gone, each classify is O(viewport) not O(ring size), and the whole-render era's unbounded-`partial` OOM risk (#1047) is closed. The mirror is fed at `PtySession`'s single output chokepoint (`onPtyData` + the `attachShellper` replay seed), in lockstep with `RingBuffer.bytesWritten` — the **monotone** gate change-token (cumulative output bytes; the old `currentSeq:partialBytes` pair fell on a trim and could alias a stale verdict). The drainer still memoizes each verdict against the live session + that token so a static held screen skips re-classify; the cost-aware backstop backoff the whole-render era needed is retired (a viewport classify is cheap regardless of history). The throwaway-terminal `classifyScreen(snapshot)` path survives only for the fixture suite. (Caveat: on adopt/reconnect after a Tower restart the mirror is seeded from the FULL shellper replay — ≤8 MB wire cap; only the *ring* seed is `capRingSeed`-trimmed to 1 MiB (PIR #1354) — so a long-lived alt-screen frame is born torn only when its coherent start predates the shellper's whole retention; that residual case classifies not-clean → the gate HOLDS, fail-safe, and self-heals on the next repaint. Startup cost is bounded by draining each >1 MiB seed's parse before adopting the next session, measured ~58 ms/session at 8 MB. Residual tracked as #1361.) 3. **Honest response vocabulary.** `delivered` (gate passed, write completed) or `held` + row id + **why-held reason** ∈ {`busy` (draft/menu/mode), `no-profile` (unknown app), `no-live-pty` (no live terminal)}. Additive over the old shape (`ok`/`terminalId`/`deferred` retained for old binaries; a `held` outcome is still `ok:true`). Surfaced to senders through `packages/core/src/tower-client.ts` and `commands/send.ts` (both single-send and `--all` aggregation). 4. **Delivery moments** (each runs the gate; the gate decides): enqueue-time, **user-submit** trigger, **output-quiescence** trigger (Spec 467 `lastDataAt`), and a **poll backstop** (`DEFAULT_BACKSTOP_INTERVAL_MS = 1500`). Submit/quiescence come from `PtySession`'s single `handleUserInput` chokepoint. A missed trigger only delays to the next backstop — it can't corrupt anything (triggers *schedule*; they never authorize). -5. **Per-agent write serialization.** `write-queue.ts` chains writes for one agent on completion (keyed by `agentKey` — *per-agent*, not per-PTY; a message's text and its Enter are one unit), so concurrent gated deliveries to one agent can't interleave/blob. This is a **disjoint** lock from the per-terminal submission lock that `escape`/`interrupt` take (`session-submit.ts`): a gated delivery is *not* serialized against a concurrent interrupt/escape — an accepted, documented boundary (a gated delivery only ever writes onto a render-verified empty prompt, and `interrupt` is the explicit gate-bypassing human action), not full per-session atomicity. Held rows drain **oldest-eligible-first**: a row is eligible when `not_before IS NULL OR not_before <= now`, so a pre-due delayed row (Spec 1313 round 3) is excluded from the scan and never blocks a later row that is already due. +5. **Write serialization — two locks, one order (Issue #1365).** `write-queue.ts` chains writes for one *agent* on completion (keyed by `agentKey` — per-agent, not per-PTY; a message's text and its Enter are one unit), so concurrent gated deliveries to one agent can't interleave/blob. The delivery's write edge then takes the per-*terminal* submission lock (`session-submit.ts`) as a **leaf** inside that per-agent serializer, via `submitMessagePaced` — the same lock `escape`/`interrupt`/the delayed `^C` take. **Lock order is always per-agent → per-terminal, never the reverse**, so there is no cycle; re-entrancy is impossible too, since `PtySession.write()` emits no `'submit'` signal (only `handleUserInput` does). The lock wraps the *write* only, never the gate classify — `--interrupt` is the human's escape hatch and must not queue behind a screen classification. Held rows drain **oldest-eligible-first**: a row is eligible when `not_before IS NULL OR not_before <= now`, so a pre-due delayed row (Spec 1313 round 3) is excluded from the scan and never blocks a later row that is already due. + + *Why converged.* Until #1365 the two locks were **disjoint** and the resulting cross-path race was an accepted boundary ("a gated delivery only ever writes onto a render-verified empty prompt; `interrupt` is an explicit human action"). That reasoning covered one ordering and missed two: a `^C` landing inside the *delivery's own* text→Enter window (50–130 ms+) cleared the composer, so the delivery's Enter submitted nothing — yet every byte reached the PTY, the write reported success, and the row was marked **`delivered` for a message the agent never saw**; `--escape` produced the truncated variant, and is the *more* likely trigger for a long body, whose exposed window is longest. The "a human is at this terminal" premise also fails for the **delayed** `^C`, which fires unattended. + + *Asymmetric acquisition.* A delivery **declines** a contended terminal (`trySubmitToSession` → hold `busy`, retry next pass) rather than queueing, because `MailboxDrainer.tick` walks agents sequentially and one blocked delivery would stall every other agent's mail plus that tick's escalation/prune. It costs nothing: a contended terminal means the in-lock precheck would have aborted anyway. Operators **block**, bounded by `OPERATOR_SUBMIT_WAIT_CEILING_MS` (2 s) — a paced write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB, so an unbounded wait could stall `afx interrupt` for minutes. **The ceiling may only bypass a DELIVERY write, never another operator** (`SubmissionKind` + a pending-operator count): operator-vs-operator was always fully serialized before #1365, so a ceiling that could skip a queued or in-flight operator would make that pair strictly *worse* than the old behaviour. With that restriction the "never worse than the old status quo" guarantee is true per pair — op↔op unchanged and unbounded; op↔delivery serialized under the ceiling and degraded to the old disjoint-lock behaviour above it; delivery↔delivery unchanged. Past the ceiling the operator write proceeds **unserialized** with a loud WARN **and** an explicit `degraded: true` in the `/api/send` response (surfaced by `afx send`), because the interrupt path has already claimed its row `delivered` and must not report an unqualified success for a body that may have interleaved. Degraded writes are counted per session, and a delivery that was raced reports `preempted` → **holds its row for redelivery** rather than reporting a delivery that may have been clobbered (trading a possible duplicate for never falsely reporting delivery, the same call the dropped-write branch already makes). + + *What is and isn't guaranteed.* **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a terminal while another's submission is in flight. The delivery additionally re-validates `writable`, the gate's `ringToken`, and the row's own status **inside** the lock — that narrows a window but does not close one, because `ringToken` counts *output*, so un-echoed input from an uncovered writer (the raw `/api/terminals/:id/write` passthrough, or a human's keystrokes — both deliberately uncovered) can still read as unchanged. That echo-lag residual is **#1473**. The in-lock precheck's structural value is that it keeps the acquisition policy a free choice: switching the delivery from declining to waiting (e.g. #1481 ordering an interrupt ahead of its body) stays safe. + + *Delayed `--interrupt` sequencing.* The timer fires **only** the `^C` (inside the lock, re-checking liveness there); the body is an ordinary `not_before` row that delivers through the gate afterwards. So the body can never land mid-turn — but `^C`→body is **not atomic** by design: the gate re-decides, which means the delayed interrupt guarantees "the turn was ended", never "this body is next". 6. **Rows address agents, not PTYs.** A respawned terminal for the same agent drains its predecessor's held mail on the first clean gate pass. Dead-session sends persist as `no-live-pty` and deliver on respawn (no drop-with-WARN). 7. **`--interrupt`** is the sole bypass — an explicit, deliberate sender action (interrupts the agent, writes without a gate check). It is a per-message command, not a timeout/valve, so it does not weaken the no-force-path invariant. `noEnter` sends are gate-checked staging (write text, no Enter) → report `delivered`. diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index 538b7c8c3..8ea684813 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -194,6 +194,9 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From 818] An acceptance criterion of "rule structurally identical to X" is a written-rule trap when the rule lives as duplicated prose in two views. Two copies drift even with diligence; the only durable enforcement is one shared function both views import. Extract when the second consumer lands — not before (no abstraction without users) and not later (drift starts on day one). - [From 1107] To place an *interactive* React widget (text input, buttons) inside an `innerHTML`-managed body, don't hand-build DOM there — inject an empty placeholder node in an effect and `createPortal` the React component into it. React owns the widget's state/focus/keyboard, while it still sits in normal document flow. Make the placeholder-injection effect idempotent (reuse a correctly-placed node; bail when `previousElementSibling` already matches the anchor) or the `setState`-on-inject loops; an `html` rebuild disconnects the node, which the same guard detects and re-creates. This is the read-while-write composer (#1107) but applies to any overlay/widget over imperatively-rendered content. - [From #1338] Retiring an entry from a shared resolver/registry must **fail closed at every resolution path**, not just delete the entry: a pure delete makes the explicit-name path throw a generic "unknown" error (no migration guidance) and the auto-detect path *silently* fall back to the default provider (here, the claude harness) — a dangerous mis-injection, not a visible failure. Keep the retired name in the detector and add a retirement sentinel checked BEFORE both exits so every path yields the same specific message; then grep every caller — spawn preflight, launch, and especially the reconnect/clean-exit relaunch paths that mint fresh sessions — because those are exactly the ones an "it's unreachable" analysis misses (three of them surfaced only under adversarial review here). +- [From #1365] "Every byte reached the PTY" is not "the message landed." A write-success boolean sees transport acceptance, not semantic loss: a `^C` or ESC that clears a TUI composer mid-write leaves every `write()` returning `true`, so the row was marked `delivered` for a message the agent never saw. Any success signal derived from "did the transport accept the bytes" needs a second question — "could anything have discarded them?" — answered from a source the transport can't lie about. Here that is a per-terminal counter of lock bypasses sampled around the write, deliberately NOT a re-read of the screen (detect-and-repair is the architecture Spec 1313 replaced). +- [From #1365] A structurally-typed port makes an omitted field compile AND pass, so a new lock/identity key needs a **runtime** guard, not just a type. A test double reaching the live wiring without an `id` keyed every per-terminal lock on `undefined` — one silently global lock, serialization that looks present and is not, with no failing assertion anywhere. Adding a throw on the missing key turned it into 13 loud failures immediately. The general shape: when a value becomes a *key* (lock, cache, registry), assert its presence at the boundary — type-checking the shape does not check the key. +- [From #1365] Converging two locks is not just "take the same lock": the acquisition POLICY has to match each caller's liveness needs. A blocking acquisition would have been a regression in both directions — a gated delivery blocking would stall the drainer's sequential walk over every OTHER agent, and an operator action blocking behind a paced write (bounded only by the 1 MiB body cap ≈ minutes) would stall the human's escape hatch. The shape that works is asymmetric: the background writer declines contention and retries on its existing schedule; the operator waits, but boundedly, degrading to the documented pre-existing behaviour with a loud log rather than to a hang. - [secfix-1] Adding the first **runtime (value)** import of a workspace package that was previously only **type-imported** (`import type`, erased at build) requires moving that dep from `devDependencies` to `dependencies` in the importing package. A devDependency is not installed for a published/deployed consumer, so the packaged build throws `Cannot find module` at **module load** — before any logger initializes, so it surfaces as a silent boot crash (e.g. a 30s startup timeout with zero log lines), not an obvious error. It is invisible to build+test because the monorepo symlinks everything at dev time. Verify the **packaged** artifact: `pnpm --filter deploy --prod --legacy ` (or a throwaway-prefix `npm install` of the tarballs) resolves only real `dependencies`, mimicking a published install — then load the entry module from it. Also update any hand-rolled local-install/packaging script that packs a *hand-picked subset* of workspace packages: it must now pack the newly-runtime dep too, or the install can't satisfy it. ## Process diff --git a/codev/reviews/1365-serializer-convergence-route-m.md b/codev/reviews/1365-serializer-convergence-route-m.md new file mode 100644 index 000000000..f0999e334 --- /dev/null +++ b/codev/reviews/1365-serializer-convergence-route-m.md @@ -0,0 +1,304 @@ +# PIR Review: Serializer convergence — one lock at the terminal write edge + +Fixes #1365 + +## Summary + +The gated mailbox delivery path and the `--interrupt` / `--escape` paths held **disjoint** +locks (per-agent vs per-terminal), so they could interleave on one terminal. The failure that +mattered was not a garbled composer but a **false `delivered`**: a `^C` landing inside a +delivery's own text→Enter window cleared the composer, the delivery's Enter submitted nothing, +every byte still reached the PTY so the write reported success, and the row was marked +delivered for a message the agent never saw. This PR routes the delivery's write edge through +the same per-terminal submission lock, taken as a leaf inside the per-agent serializer +(order: per-agent → per-terminal, no cycle), and lands the resulting model as one documented +boundary instead of three separately-reasoned decisions. + +The issue asked for an evaluation *before* a remedy. That evaluation is in +`codev/plans/1365-serializer-convergence-route-m.md` Part 1; it ratified convergence, and +this is its implementation. + +## Files Changed + +Implementation: + +- `packages/codev/src/agent-farm/servers/session-submit.ts` — `trySubmitToSession`, + `isSubmissionInFlight`, `OPERATOR_SUBMIT_WAIT_CEILING_MS` + `SubmitOptions`, `SubmissionKind` + + `pendingOperators`, `unserializedWriteCount` / `watchBypasses`, and the rewritten boundary + comment +- `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` (+144 / −20) — `DeliverySession.id`, + `WriteAbort` / `WriteResult`, the in-lock precheck, the outcome mapping +- `packages/codev/src/agent-farm/servers/message-write.ts` — `submitMessagePaced` + (replaces `writeMessagePaced`), `PacedSubmitResult` +- `packages/codev/src/agent-farm/servers/tower-routes.ts` — wait ceiling at the three + operator call sites, `logCeilingExpired`, `degraded` on the send response, updated scope comments +- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts` — binds the new write edge +- `packages/codev/src/agent-farm/commands/send.ts` — warns the sender on a degraded write +- `packages/sdk/src/tower-client.ts` — `degraded` / `degradedReason` on the sendMessage result + +**Two of those are outside the 21-file scope this PR originally stated** (`commands/send.ts` +and the SDK client), added by the review round below. They are the minimum needed to make a +degraded operator write visible to the *sender* rather than only to the Tower log, which +required crossing the server→client boundary. Flagged so the diff holds no surprises; the +boundary rule itself is respected (`codev-sdk` still imports only `codev-types`). + +Tests: + +- `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts` — new +- `packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts` — re-pointed +- `tower-routes.test.ts`, `send-delivery.test.ts`, `send-mailbox-repro.test.ts`, + `cron-delivery.test.ts`, `send-architect-identity.test.ts` — fakes updated + +Evidence + docs: + +- `packages/codev/scripts/spec-1365-e2e-evidence.mts` (+420 / −0) — dev-approval evidence script +- `codev/evidence/1365-dev-approval-transcript.txt` (+80 / −0) — its transcript +- `codev/resources/arch.md` (+10 / −2), `codev/resources/arch-critical.md`, + `codev/resources/lessons-learned.md` +- `codev/plans/1365-...md`, `codev/state/pir-1365_thread.md` + +## Commits + +- `2adbe0b9` [PIR #1365] Plan draft: evaluation of the three write paths + convergence design +- `a6cdbe27` [PIR #1365] Plan revised (rev 2): all 5 blocking review items + interrupt-latency ceiling +- `30af22b2` [PIR #1365] Lock primitives: try-acquire, wait ceiling, degraded-write counter +- `e9fd2d42` [PIR #1365] Route the mailbox write edge through the per-terminal submission lock +- `194685e1` [PIR #1365] Tests: interleaving, in-lock precheck, liveness, ceiling, key hygiene +- `dcf22a5f` [PIR #1365] Document the converged write-edge model in one place +- `54d57008` [PIR #1365] Thread log: implement phase +- `ee3a17df` [PIR #1365] dev-approval evidence: 4 scenarios against an isolated live Tower +- `1483d65c` [PIR #1365] Thread log: dev-approval evidence +- `ad824bf2` [PIR #1365] Review + retrospective +- `ece06a5e` [PIR #1365] Fix codex/claude finding: kind-aware ceiling + report degraded writes +- `0dc75d8e` [PIR #1365] Review round 2: byte-accurate bypass count, counter eviction, claim sweep + +## Test Results + +- `pnpm --filter @cluesmith/codev build`: ✓ pass +- `pnpm --filter @cluesmith/codev-sdk build`: ✓ pass +- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4934 passed / 0 failed / 48 skipped**, + 248 files, after merging `origin/main`. 28 new tests in + `spec-1365-serializer-convergence.test.ts`, plus the degraded-interrupt response test in + `tower-routes.test.ts`. (Before the merge, on the #1365 work alone: 4885 / 0 / 48, 246 files + — the delta is main's own tests arriving, not tests changing behaviour here.) +- **Merged `origin/main` after the `pr` gate was approved** (merge `ebbc495dc`, not a rebase — + the reviewed history is preserved). `main` had advanced 25 commits during the review round + (AIR #1489's `afx reset` → `afx refresh`, secfix-1's Tower auth hardening, PIR #1495). One + conflict, in `codev/resources/lessons-learned.md` § Architecture: an append-only collision + (three #1365 entries vs one secfix-1 entry at the same point), resolved **keep-both**, all + four entries present and unmodified. Every code file auto-merged; every #1365 hunk survives + byte-identical, and `message-write.ts`, `mailbox-delivery.ts`, `commands/send.ts` and + `spec-1365-serializer-convergence.test.ts` were untouched by the merge. +- **Manual verification** (dev-approval gate, human-approved): `afx dev` was not usable — + 4100 is shared by design and restarting the live Tower kills every builder session — so the + running-worktree evidence was scripted against an **isolated Tower on port 14650** with real + shellper-backed PTYs and real HTTP endpoints (routes → mailbox → render gate → locks → PTY, + nothing stubbed). **66/66 checks.** Full transcript: + `codev/evidence/1365-dev-approval-transcript.txt`; script: + `packages/codev/scripts/spec-1365-e2e-evidence.mts`. + - S1 — 10× long multi-line send raced by `--interrupt`: 10/10 bodies reached the wire + **whole**, zero fragmentation, zero duplication. + - S2 — `--delay 5 --interrupt` mid-turn: scheduled; `^C` at due time; body did **not** land + mid-turn; landed **exactly once** after the prompt cleared; `^C` before body. + - S3 — interrupt vs a busy line: returned in **2156 ms** rather than waiting out a ~4.1 s + paced write; Tower logged the degradation at WARN; the raced delivery reported + `preempted` and **held its row** (`delivered=false held=true reason=busy`). + - S4 — `--escape` unchanged (ESC + Enter on the wire); a dead terminal is refused, never + silently dropped. + - Live Tower on 4100 verified healthy and untouched afterwards; no orphan processes. + +## Architecture Updates + +**COLD — `codev/resources/arch.md` §7 item 5** (rewritten). The old text described the two +locks as disjoint with the cross-path race as an accepted boundary; that is now false. The +replacement carries the whole model in one place: which writers take the lock and which stay +deliberately uncovered, the per-agent → per-terminal order and why there is no cycle, the +deliveries-decline / operators-block asymmetry and the reason each side differs, the wait +ceiling and its degradation, the delayed-interrupt sequencing, and — stated honestly — that +serialization is the structural guarantee while the in-lock precheck only *narrows* the +echo-lag residual (#1473). + +**HOT — `codev/resources/arch-critical.md`**: the existing mailbox-first fact already governs +"any new message writer", so the lock-order invariant was **appended to that fact** rather +than added as an eleventh. This keeps the tier at its 10-fact cap with **no displacement** — +the hot tier gains the one clause a future author actually needs at decision time ("take +`submitToSession`; order is per-agent → per-terminal"), not a second entry on the same +subject. + +## Lessons Learned Updates + +**COLD — `codev/resources/lessons-learned.md` → Architecture**, three entries: + +1. *"Every byte reached the PTY" is not "the message landed."* A write-success boolean sees + transport acceptance, not semantic loss — a `^C` that clears the composer leaves every + write returning `true`. Any success signal derived from "did the transport accept the + bytes" needs a second question, answered from a source the transport can't lie about. +2. *A structurally-typed port makes an omitted field compile **and** pass.* When a value + becomes a **key** (lock, cache, registry), assert its presence at the boundary — type-checking + the shape does not check the key. Here a double without an `id` keyed every per-terminal + lock on `undefined`: a silently global lock, no failing assertion anywhere. +3. *Converging two locks is not just "take the same lock" — the acquisition **policy** has to + match each caller's liveness needs.* Blocking would have regressed both sides (the + sequential drainer, and the human's escape hatch). The shape that works is asymmetric: + the background writer declines contention and retries on its existing schedule; the + operator waits, but boundedly, degrading to documented prior behaviour rather than a hang. + +**Considered for HOT and deliberately not promoted**: both are real but narrower than the +current ten hot lessons, and promoting either would require *displacing* an existing one. +Displacement at the cap is the maintainer's call, not a builder's — flagged here rather than +taken unilaterally. + +## Review Round: two REQUEST_CHANGES, and what happened to each + +PIR's consultation is **single-pass** — there is no second automated round — so the human at +the `pr` gate is the only remaining reviewer of these dispositions. They are written out in +full rather than summarised. + +Two independent review sets ran, and they did not agree: + +| Reviewer | Protocol CMAP (`codev/projects/1365-.../`) | Architect's CMAP on PR #1492 | +|---|---|---| +| gemini | APPROVE | APPROVE | +| codex | APPROVE | **REQUEST_CHANGES** | +| claude | **REQUEST_CHANGES** | **REQUEST_CHANGES** | + +My own codex lane approved; the architect's codex lane found a real bug. I verified every +finding against the code before acting on it, and **none was dismissed on the strength of +another lane's APPROVE**. Both REQUEST_CHANGES lanes converged independently on the same two +blocking findings. + +**Blocking 1 — the ceiling could bypass another *operator's* submission.** ACCEPTED, real, +fixed in `ece06a5e`. `bounded` keyed only off "is anything in flight" without asking *what +kind* of writer was ahead, so a second `--interrupt` could skip a first one carrying a long +body after 2 s. Operator-vs-operator was **always** fully serialized before #1365 +(`submitToSession` had no ceiling at all — it is Spec 1273's `/clear` fusion bug), so my +ceiling made that one pair strictly *worse* than the status quo. That also falsified this +document's own "never worse" claim. Fix: chain entries carry a `SubmissionKind`, a +`pendingOperators` count tracks operators **queued as well as in flight**, and the ceiling arms +only when nothing ahead is an operator. Queued has to count — bypassing an operator that has +not started yet is the same violation as bypassing one mid-write. Pinned by *"operator vs +operator NEVER degrades"* and *"a THIRD operator does not bypass a QUEUED one"*. Note the +pre-existing ceiling test needed its holder changed from an operator to a delivery: **that +fixture change is the behaviour change**, not a workaround for it. + +**Blocking 2 — a ceiling-degraded `--interrupt` still reported unqualified success.** +ACCEPTED, real, fixed in `ece06a5e`. The row is claimed `delivered` before the write, so a +degraded interrupt returned `delivered: true` with only a Tower-side WARN — the same +lying-success-signal class this whole issue exists to remove, relocated from the delivery path +to the operator path. Claim-first is *kept* (un-claiming risks a double delivery, reasoned +through at CMAP round 3 of the implement phase); what changed is that the truth is now +surfaced: `/api/send` returns `degraded: true` + `degradedReason`, threaded through the SDK +client and warned about by `afx send`. An indicator nobody surfaces is half a fix. Pinned by +*"a body-bearing interrupt that crosses the wait ceiling reports degraded"* in +`tower-routes.test.ts`. + +**Non-blocking, taken anyway (this commit):** + +- *The bypass counter was bumped on ceiling expiry regardless of whether bytes went out.* The + delayed `^C` re-checks `isStillLive()` / `writable` **inside** the lock and can return having + written nothing; that no-op was still counted, forcing a concurrent delivery into a spurious + `preempted` re-delivery. The counter answers "did bytes bypass the lock while I held it?", so + only bytes may bump it: `SubmitOptions.wroteBytes` is consulted straight after the write + callback, with no `await` in between, so the ordering guarantee the old placement provided is + unchanged. Pinned by *"a degraded write that writes NOTHING is not counted as a bypass"*. +- *`unserializedWrites` was never pruned* — one entry per session that ever degraded, retained + for the life of the Tower. The leak class #1472 just fixed. It cannot self-delete on drain + the way `chains` and `pendingOperators` do, because it must **outlive** the submission whose + watcher is about to compare against it: a reset landing between a watcher's two reads would + read as "nobody raced me" — the exact false `delivered` this issue exists to eliminate. So + eviction is interlocked with an explicit `watchBypasses` window: refused while a watch is + open, attempted from *both* the chain's drain cleanup and the last watch's release, so + whichever runs second is the one that evicts and no ordering leaks. This needs **no + session-teardown hook** and therefore no `terminal/` → `agent-farm/` layer crossing. Pinned by + *"the degraded-write counter is evicted once the session goes idle"* and *"eviction cannot + land inside a watcher window and mask a race"*. +- *Stale `{@link writeMessagePaced}`* in `message-write.ts` — repointed at `submitMessagePaced`. +- *`DEGRADED_SUBMIT_REASON` was inserted between `logCeilingExpired`'s JSDoc and its function*, + orphaning the comment — moved above it. +- *The residual "never worse than the status quo" claim-sites* — swept and rewritten to state + the guarantee **per pair** (op↔op unchanged and unbounded; op↔delivery serialized under the + ceiling and degraded to the old disjoint-lock behaviour above it; delivery↔delivery + unchanged). `arch.md` §7 item 5 and the `session-submit.ts` boundary comment were corrected in + `ece06a5e`; `tower-routes.ts`'s `logCeilingExpired` doc comment and the degraded-path inline + comment in this one. + +**Non-blocking, NOT taken — flagged instead:** + +- *The ceiling timer is not cancelled when the predecessor wins the race.* `Promise.race` + leaves a ≤2 s `setTimeout` pending whose resolution is then discarded. Cancelling it means + adding abort semantics to the injected `SubmitClock` interface, which every test double + implements. The cost of leaving it is one short-lived timer per *contended* operator + submission; the cost of fixing it is a broader interface change late in a review round. A + reviewer who disagrees should say so — it is a small change, just not a free one. +- *`waited < 100` / `tickMs < 250` are timing-sensitive under CI load.* Real, and deliberate: + these are the assertions that make "the drainer does not stall" and "the escape hatch stays + responsive" *testable* claims rather than prose. Both have ≥2.5× headroom over the behaviour + they exclude. If they flake in CI, raising the bounds preserves the property. +- *Hot-tier `arch-critical.md` was appended to directly, where the plan said it would be + proposed.* Disclosed below; it is the human's call, and reverting it is a one-line edit. + +## Things to Look At During PR Review + +1. **The in-lock precheck's honest status.** With try-lock semantics the delivery never waits, + so in production today the precheck cannot observe a state change a pre-lock check missed — + no macrotask can interleave between them. I kept it (both plan reviewers asked for it, the + human ratified it) but documented its *actual* value rather than implying it closes a live + race: it backstops the injected port boundary, and it is what keeps the acquisition policy + a free choice if #1481 later wants the delivery to wait behind an interrupt. If a reviewer + would rather not carry code whose value is conditional on a future change, this is the + place to say so. +2. **The wait ceiling is a judgment call** (`OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000`, human- + ratified at the plan gate). It exists because `--interrupt` previously never waited, and a + paced write runs `(lines−1)×10+80` ms against a body capped only by `parseJsonBody`'s + 1 MiB — a 48 KB `--file` of short lines is ~8 minutes. It arms **only against a delivery + write**: behind another operator the wait stays unbounded, exactly as before #1365 (see the + review round above — a ceiling that could skip an operator made that pair strictly worse, + and that was a real blocking finding, not a hypothetical). So past the ceiling the operator + write falls back to precisely the pre-#1365 operator-vs-delivery behaviour — two disjoint + locks, no serialization — which is no worse for that pair, only no longer silent. The + guarantee is **per pair**, and the 2 s value itself remains a judgment call. +3. **`preempted` trades a possible duplicate for never falsely reporting delivery.** A + delivery raced by a ceiling-expired write holds its row instead of marking it delivered, so + if the message *did* land intact the gate may deliver it again later. That is the same call + the existing dropped-write branch already makes, and the opposite of the interrupt path's + claim-first tradeoff — the asymmetry is deliberate (an operator's own message vs an + autonomous background delivery), but it is worth a second opinion. +4. **Port signature churn.** `writeMessage` gained a 4th parameter and a typed result across + six test files. One override (`send-delivery.test.ts:604`) previously returned `undefined` + and relied on falsy ⇒ hold; it is now explicit. +5. **`writeMessagePaced` was removed**, not deprecated — its only live caller was the mailbox + wiring. Its drop-semantics test is re-pointed at `submitMessagePaced` so the #1198 + silent-loss guard stays on the live write edge rather than on a function nothing calls. +6. **The dev-approval transcript is committed under `codev/evidence/`, a new directory.** That + placement is a deliberate choice, not an accident: the evidence is part of the PIR record + for this project, the way `codev/specs/`, `codev/plans/` and `codev/reviews/` are, and a + gate approved on evidence that then vanishes leaves the approval unauditable. It is + nonetheless a new top-level convention in the repo, and **the maintainer may veto it** — + moving or dropping the file changes nothing else in the PR (the generating script, + `packages/codev/scripts/spec-1365-e2e-evidence.mts`, is re-runnable and is the durable + artifact). + +**Interlock for #1481 (`--interrupt-after`)**: "interrupt, then deliver this body" is now +expressible as ordered acquisitions of *one* lock rather than a race between two. Two +residuals it must design against, both documented: the `^C`→body gap is gate-mediated and +deliberately **not** atomic (the delayed interrupt guarantees "the turn was ended", never +"this body is next"), and a no-op `^C` is only logged. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder `pir-1365` → **Review Diff** +- **Run dev**: `afx dev pir-1365` — but note it will contend for the live Tower's port; the + isolated-Tower script below is why the dev-approval evidence took that route instead +- **Re-run the evidence**: `pnpm --filter @cluesmith/codev build && node + --experimental-strip-types packages/codev/scripts/spec-1365-e2e-evidence.mts` + (isolated Tower on 14650, ~90 s, exits non-zero on any failed check) +- **Unit**: `pnpm --filter @cluesmith/codev test spec-1365-serializer-convergence` +- **What to verify**: a long multi-line send raced by `afx interrupt` never fuses and never + leaves an `afx inbox show` row reading `delivered` whose text is absent or partial; another + agent's mail keeps flowing while a large body is mid-delivery; `afx interrupt` stays + responsive against a busy line; `--escape` behaviour is unchanged. + +## Flaky Tests + +None. No tests were skipped or quarantined, and no pre-existing unrelated failures were +touched. diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md new file mode 100644 index 000000000..8b1c86f61 --- /dev/null +++ b/codev/state/pir-1365_thread.md @@ -0,0 +1,254 @@ +# pir-1365 — Serializer convergence (issue #1365) + +## Plan phase (2026-08-17) + +Read the three write paths end to end before proposing anything, per the architect's +instruction that the issue's "Evaluate first" section governs. + +**What I found that changes the framing.** The issue (and the accepted-boundary comment in +`session-submit.ts`) reason about the *fusion* case: a gated delivery landing inside an +interrupt's `^C`→settle→text window. There is a second ordering nobody wrote down: the `^C` +landing inside the **delivery's own** text→Enter window (50–130 ms+). The `^C` clears the +composer, the delivery's Enter fires into nothing, `writeMessagePaced` returns `true` +(the PTY accepted every byte — it only detects #1198 socket drops, not semantic loss), and +`markDelivered` transitions the row. That is silent message loss with a false `delivered` +audit record — the one outcome Spec 1313's architecture exists to exclude. + +That, plus the fact that the delayed-interrupt path fires **unattended** (so the "an +operator is standing at this terminal" premise that makes the boundary acceptable does not +hold for it, and #1481 makes that co-occurrence routine), is why the evaluation lands on +**converge** rather than wontfix. + +**Design constraint I want the reviewer to weigh.** Taking the per-terminal lock *alone* +would make things worse, not better: a delivery that classified clean and then waits ~150 ms +behind an interrupt would write onto a screen the interrupt just changed. So the gate +verdict must be re-validated **inside** the lock (the same `writable` + `ringToken` pair the +code already checks pre-write, re-run at the write instant). The in-lock precheck is the +fix, not a refinement of it. Correspondingly the lock stays a leaf around the *write* only — +widening it to cover the async classify would queue `--interrupt`, the human's escape hatch, +behind a gate classification. + +Verified for deadlock-freedom: lock order is always per-agent → per-terminal (paths A/C +never enter the per-agent serializer), and `PtySession.write()` emits no `'submit'` signal +(only `handleUserInput` does; `'quiescence'` comes off an output timer), so nothing can +re-enter the other lock from inside a lock callback. + +Answers to the other two failure questions: the delayed-interrupt reshape leaves **no** +window for the body to land mid-turn (the body can only leave the mailbox through the gate); +escalation/held bookkeeping stays internally consistent but silently diverges from reality +in the Ordering-2 case, with no detector. + +Plan written to `codev/plans/1365-serializer-convergence-route-m.md`, committed, awaiting +`plan-approval`. Flagged for #1481: the `^C`→body gap is gate-mediated, not atomic, and a +no-op `^C` is only logged — both are things `--interrupt-after` must design against. + +**Standing constraint**: we are not cluesmith/codev maintainers. Never merge the PR; park it +after review and report protocol-complete. + +## Plan revision 2 (2026-08-17) — after the architect's 3-way review + +Verdicts gemini APPROVE / codex + claude REQUEST_CHANGES, with both REQUEST_CHANGES reviews +ratifying Part 1. Every blocking item was in Part 2's design, and every one of the five was +right. Verified each against the code before revising rather than taking the summaries as +ground truth: + +- **Item 5 confirmed on sight**: `tower-routes.test.ts:221` `gateSession()` is an + un-annotated literal with no `id`, and it reaches the *real* `mailbox-wiring` binding. That + would have keyed every lock on `undefined` — per-terminal serialization silently collapsing + into one global lock, with no failing assertion anywhere. Runtime guard in + `submitMessagePaced`, plus a different-terminals test. +- **Item 2 adopted as proposed** (asymmetric try-lock). The drainer awaits agents + sequentially, so a blocking acquisition would let one agent's terminal stall every other + agent's delivery *plus* that tick's escalation and prune. Deliveries now fail fast to + `busy` — which costs nothing, since a contended terminal means the precheck would have + aborted anyway — and the result is a *stronger* liveness property than today. +- **Item 1**: my revision-1 precheck re-validated writability + ringToken but not the row's + own status, which would have *widened* the dismiss→bytes-on-wire window from ~zero to the + whole lock wait. `getById` goes in the precheck; `WriteAbort` gains a `row-resolved` case + so "no hold, terminal state" stays expressible. +- **Item 3** was a fair hit. I wrote "the in-lock precheck is not a refinement — it is the + fix," which overclaims: `ringToken` tracks *output*, so un-echoed input from a writer that + doesn't take the lock still defeats it. Restated: serialization is the structural + guarantee, the precheck narrows the echo-lag residual, and that residual is #1473's. +- **Item 4**: escape is the *more* likely trigger for multi-line bodies, not a milder cousin — + the delivery's exposed window is longest exactly when the body is long, and the failure + mode is a truncated message marked `delivered` (worse than absent: it can be acted on). + +**New finding of my own (D3), flagged for the gate.** Nobody raised interrupt latency. Today +`--interrupt` never waits; after convergence it waits for any in-flight delivery write, and +body size is capped only by `parseJsonBody`'s 1 MiB default +(`agent-farm/utils/server-utils.ts:47`). A 48 KB `--file` of short lines is ~48k lines ≈ 8 +minutes of paced write. An unbounded block on the escape hatch would be a worse regression +than the bug. Proposed a bounded wait (2000 ms) that degrades to exactly today's unserialized +behaviour with a loud WARN — never worse than the status quo at any point, and no longer +silent. The ceiling value is a judgment call; flagged rather than assumed. + +Also noted: the suggested `arch-critical.md` hot-tier fact for the lock-order invariant needs +a *displacement* (the tier is at its 10-fact cap), so it is proposed in the review phase for +the maintainer rather than applied unilaterally. + +## Implement phase (2026-08-17) + +Human approved the gate including the D3 ceiling at 2000 ms. Four commits: lock primitives → +convergence → tests → docs. Full suite green (4879 passed, 0 failed), build clean. + +**Took the architect's optional degraded-path flag.** It earns its complexity for a specific +reason: my own D3 ceiling is what opens that hole. Without it I'd have traded one route to a +false `delivered` (the ^C mid-write) for a rarer one (an operator that gave up waiting and +wrote into a delivery already on the wire) — which is not a fix, it's a relocation. +Implemented as a per-session monotone counter of unserialized writes, sampled around the +delivery's own submission; a bump yields `preempted` → hold for redelivery. No screen +re-classification: the question is only "did anyone bypass the lock while I held it?", and a +counter answers exactly that. It does trade a possible **duplicate** for never falsely +reporting delivery — the same call the existing dropped-write branch already makes, and +documented as such. + +**The review's item-5 hazard was real and it bit immediately.** Adding the runtime id guard +turned 13 `tower-routes.test.ts` tests red — `gateSession` had no `id`, reached the live +wiring, and would have keyed every lock on `undefined`. Silent global lock, no failing +assertion, exactly as claude predicted. Worth recording as the general lesson: a +structurally-typed port makes an omitted field compile *and* pass, so a new lock key needs a +runtime guard, not just a type. + +**Honest note on the in-lock precheck.** With try-lock semantics the delivery never waits, so +the precheck cannot fire from a lock wait in production today — no macrotask can interleave +between the pre-lock checks and the in-lock ones. I kept it (the review asked for it, the +human ratified it) but documented its *actual* value rather than implying it closes a live +race: it backstops the injected port boundary, and it is what keeps the acquisition policy a +free choice if #1481 later wants the delivery to wait behind an interrupt. The tests exercise +it at its own level rather than pretending a production path reaches it. + +Also re-pointed `spec-1313-paced-write-drop` from the retired `writeMessagePaced` onto +`submitMessagePaced`, so the silent-loss guard stays on the live write edge instead of on a +function nothing calls. + +## dev-approval evidence (2026-08-18) + +`afx dev` was off the table (4100 is shared; restarting the live Tower kills every builder), +so I built the evidence the way `send-integration.e2e.test.ts` does: this worktree's Tower on +port 14650, real shellper-backed PTYs, real HTTP endpoints, nothing stubbed. Script + +transcript committed (`packages/codev/scripts/spec-1365-e2e-evidence.mts`, +`codev/evidence/1365-dev-approval-transcript.txt`). 66/66. Live Tower on 4100 verified +untouched afterwards; no orphan processes. + +**The oracle is the interesting part.** The echo terminal (`stty raw -echo; exec cat`) re-emits +every byte written to it in order, so `GET /api/terminals/:id/output` is a faithful ordered +record of what each writer actually put on the terminal. That turns "did these two writers +interleave?" into a string question instead of an inference. + +**Two wrong turns worth recording, both mine, both fixed rather than papered over:** + +1. First run: every send held `no-profile`, so nothing ever delivered and the whole scenario + asserted nothing. Cause: a shellper-backed session reports `command: ''`. Fix was to write + a real `.builder-start.sh` so the profile resolves through the *wrapped-launch fallback* — + which is how a genuine builder's profile resolves, so this is fidelity, not a workaround. +2. A run reported `held` at request time and then had bytes on the wire 400 ms later, which my + assertion called a lie. It wasn't: the fast trigger/backstop had legitimately delivered the + row after the response. My assertion was wrong. Rather than loosen it I made it *sharper* — + count whole bodies vs. count first-lines, so any fragment without a whole body behind it + fails. That is a better interleaving detector than "is the body present", because + fragmentation is precisely what interleaving looks like on the wire. + +**Scenario 3 is the strongest single piece of evidence** and it exercises the degraded path I +added: the interrupt returned in 2156 ms instead of waiting out a ~4.1 s paced write; Tower +logged the degradation at WARN; and the raced delivery reported `preempted` and *held its row* +— `delivered=false held=true reason=busy`. The ceiling and its compensating flag both firing +end to end, on the real wire. + +**Two limits stated rather than approximated:** the 503 `TERMINAL_NOT_WRITABLE` branch needs a +shellper socket that died while the session still reports `running`, which can't be produced +from the public API without staging it (covered by `tower-routes.test.ts:1560`, untouched by +this change); and this fixture's agent is only ever a live terminal, never a registry-known +builder, so a send after its death correctly 404s instead of exercising the +hold-instead-of-404 seam. + +--- + +## Review phase — two CMAP rounds, and the one that mattered was not mine + +**2026-08-18.** The protocol's own consultation lane came back gemini APPROVE / codex APPROVE / +claude REQUEST_CHANGES. The architect independently ran a *second* CMAP against PR #1492 and got +gemini APPROVE / **codex REQUEST_CHANGES** / claude REQUEST_CHANGES. Same model, opposite +verdicts, on the same branch. + +The lesson I want a future builder to take from this: **my codex lane approved and it was +wrong.** Two of the three lanes on the other CMAP converged, independently, on a real bug I had +introduced — and the temptation, when one lane says APPROVE, is to treat the outlier as noise. I +verified every finding against the code myself before acting on it. Both blocking ones were +real. An APPROVE is not evidence that a finding is false; it is evidence that one reviewer did +not find it. + +**The bug was my own D3 ceiling, and it was the classic shape: a fix that relocates its defect.** +`OPERATOR_SUBMIT_WAIT_CEILING_MS` existed so `--interrupt` could not be stalled for minutes +behind a long delivery. But `bounded` only asked "is anything in flight", never "*what kind* of +writer is ahead" — so a second `--interrupt` could bypass a first one carrying a 48 KB body. +Operator-vs-operator had been *fully* serialized since Spec 1273 (it is the `/clear` fusion bug), +so my ceiling made exactly one pair strictly worse than the status quo, in a PR whose review doc +claimed "never worse". Fixed in `ece06a5e` by tagging chain entries with a `SubmissionKind` and +counting operators that are **queued** as well as in-flight — bypassing an operator that has not +started yet is the same violation as bypassing one mid-write. The guarantee is now stated **per +pair**, which is the only way it is true. + +The second blocking finding was the same class one layer out: a degraded interrupt still +returned `delivered: true`. In a PR whose entire thesis is *a success signal must not lie*. That +one stung. Fixed by surfacing `degraded` through `/api/send` → SDK → `afx send` rather than by +un-claiming the row (un-claiming reopens double-delivery, which round 3 of the implement-phase +CMAP had already settled). + +**Round 2 (this commit)** took the two non-blocking correctness notes and swept the doc claims. +The counter-eviction one was the only part with real design content: `unserializedWrites` cannot +self-delete on drain the way `chains` does, because it has to *outlive* the submission whose +watcher is about to compare against it — a reset landing between a watcher's two reads reads as +"nobody raced me", which is the precise false `delivered` this whole issue exists to kill. So +eviction is interlocked with an explicit `watchBypasses` window and attempted from both ends +(drain cleanup and last release), whichever runs second winning. That also avoids needing a +session-teardown hook, which would have meant `terminal/` importing `agent-farm/` — a layer +crossing not worth a memory nit. + +I refused two nits and said why in the rebuttal rather than quietly skipping them: cancelling the +ceiling timer needs abort semantics on the injected `SubmitClock` that every test double +implements, and the timing assertions are load-bearing (they are what makes "the escape hatch +stays responsive" a testable claim instead of prose). + +Dispositions in `codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md`; +the human-facing version, including the two files that fell outside the stated PR scope and the +`codev/evidence/` placement the maintainer may veto, is in the review doc. + +Build clean (codev + sdk); full suite **4885 passed / 0 failed / 48 skipped**, 246 files. + +--- + +## Post-gate: merging main back in + +**2026-08-18.** The human approved the `pr` gate and porch reported the protocol complete +(`phase: verified`). Porch's final task says "merge the PR" — not taken; PR #1492 and issue +#1365 are parked for the maintainer, per standing order. + +Then GitHub flagged the PR `mergeable=CONFLICTING`: `main` had moved 25 commits during the +review round. I reported it rather than fixing it unasked, because merging would have changed +the tree the architect had just verified with a 129/129 re-run — and got the go-ahead. + +**The conflict was a nothing, and that is worth recording precisely because it looked +alarming.** Exactly one file: `codev/resources/lessons-learned.md`, § Architecture. Both sides +had *appended* — three #1365 lessons here, one secfix-1 lesson on main, at the same insertion +point. Not a competing edit; git simply cannot know that two appends at one anchor are +independent. Keep-both, all four entries intact. + +Every code file auto-merged. What the merge *did* change in this PR's files came entirely from +main: AIR #1489's `afx reset` → `afx refresh` rename landed in two of my comments +(`session-submit.ts`, `mailbox-wiring.ts`), and secfix-1's auth hardening rewrote parts of +`tower-routes.ts`, `tower-client.ts` (`codev-web-key` → `codev-tower-key`) and +`tower-routes.test.ts`. I verified this rather than assuming it: blob-hashed the nine files +before the merge and diffed each afterwards, then grepped every #1365 marker +(`wroteBytes`, `watchBypasses`, `pendingOperators`, `SubmissionKind`, `logCeilingExpired`, +`DEGRADED_SUBMIT_REASON`, the degraded-interrupt test) to confirm each survived. Four files +were byte-identical: `message-write.ts`, `mailbox-delivery.ts`, `commands/send.ts`, +`spec-1365-serializer-convergence.test.ts`. + +`pnpm install --frozen-lockfile` was necessary before rebuilding — main moved `pnpm-lock.yaml`, +and secfix-1's own lesson in that very file is about a dep that only fails in a *packaged* +install. Build clean (codev + sdk). Full suite **4934 passed / 0 failed / 48 skipped, 248 +files** — up from 4885/246, the delta being main's own new tests, not behaviour change here. + +Merge commit `ebbc495dc`. Merge, not rebase: rebasing would have rewritten 83 pushed commits +and destroyed the history the human just approved. diff --git a/packages/codev/scripts/spec-1365-e2e-evidence.mts b/packages/codev/scripts/spec-1365-e2e-evidence.mts new file mode 100644 index 000000000..9ea9f1d33 --- /dev/null +++ b/packages/codev/scripts/spec-1365-e2e-evidence.mts @@ -0,0 +1,420 @@ +/** + * Issue #1365 — dev-approval evidence against a real, ISOLATED Tower. + * + * Running `afx dev` from this worktree would bind the live Tower's port (4100 is shared by + * design) and restarting the live Tower kills every builder session, so the running-worktree + * evidence is produced the way `send-integration.e2e.test.ts` does instead: spawn this + * worktree's built Tower on a private port, register REAL shellper-backed PTY sessions, and + * drive the REAL HTTP endpoints. Nothing is stubbed — routes → mailbox → render gate → the + * locks → PTY is the full wire path under test. + * + * The oracle is an echo terminal (`stty raw -echo; exec cat`), the same fixture the existing + * e2e uses: `cat` re-emits every byte written to the PTY input, in order, into the output + * ring. So `GET /api/terminals/:id/output` is a faithful, ordered transcript of every byte + * every writer put on that terminal — which is exactly what "did these two writers + * interleave?" needs. Raw mode also means the `^C` is echoed as a byte rather than raising + * SIGINT, so the session survives and stays registered across scenarios. + * + * Usage: pnpm --filter @cluesmith/codev build && node --experimental-strip-types \ + * scripts/spec-1365-e2e-evidence.mts + */ + +import { spawn, type ChildProcess } from 'node:child_process'; +import { resolve } from 'node:path'; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import net from 'node:net'; + +const PORT = 14650; // private to this script (14500 = cli-tower-mode, 14600 = send-integration) +const BASE = `http://localhost:${PORT}`; +const TOWER = resolve(import.meta.dirname, '../dist/agent-farm/servers/tower-server.js'); + +const ESC = '\x1b'; +const CTRL_C = '\x03'; +const COMPOSER_RULE = '─'.repeat(22); +const CLEAR = `${ESC}[2J${ESC}[H`; +/** A CLEAN claude composer: marker + dim placeholder only → the render gate delivers. */ +const CLEAN_COMPOSER = `${CLEAR}❯ ${ESC}[2mTry "fix the flaky test"${ESC}[0m\r\n${COMPOSER_RULE}\r\n`; +/** An OCCUPIED composer: a draft at normal intensity → the render gate holds (mid-turn). */ +const DRAFT_COMPOSER = `${CLEAR}❯ ${ESC}[0mdeploy the hotfix to prod\r\n${COMPOSER_RULE}\r\n`; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +let failures = 0; +let checks = 0; + +function check(ok: boolean, label: string, detail = ''): void { + checks++; + if (!ok) failures++; + console.log(` ${ok ? 'PASS' : 'FAIL'} ${label}${detail ? ` — ${detail}` : ''}`); +} + +function section(title: string): void { + console.log(`\n${'='.repeat(78)}\n${title}\n${'='.repeat(78)}`); +} + +// ---------------------------------------------------------------- Tower lifecycle + +async function portListening(port: number): Promise { + return new Promise((r) => { + const s = new net.Socket(); + s.setTimeout(1000); + s.on('connect', () => { s.destroy(); r(true); }); + s.on('timeout', () => { s.destroy(); r(false); }); + s.on('error', () => r(false)); + s.connect(port, '127.0.0.1'); + }); +} + +/** Everything Tower itself logged — the server-side corroboration for the wire assertions. */ +const towerLog: string[] = []; + +/** Tower log lines matching a pattern, since a marker index (for per-scenario slicing). */ +function towerLogSince(since: number, pattern: RegExp): string[] { + return towerLog.slice(since).filter((l) => pattern.test(l)); +} + +async function startTower(): Promise { + const proc = spawn('node', [TOWER, String(PORT)], { + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, NODE_ENV: 'test', AF_TEST_DB: `test-1365-${PORT}.db` }, + }); + let stderr = ''; + const collect = (d: Buffer): void => { + for (const line of d.toString().split('\n')) if (line.trim()) towerLog.push(line); + }; + proc.stdout?.on('data', collect); + proc.stderr?.on('data', collect); + proc.stderr?.on('data', (d) => (stderr += d.toString())); + for (let i = 0; i < 75; i++) { + if (await portListening(PORT)) return proc; + await sleep(200); + } + proc.kill(); + throw new Error(`Tower did not start on ${PORT}. stderr:\n${stderr}`); +} + +// ---------------------------------------------------------------- workspace + terminals + +function makeWorkspace(): string { + const base = resolve(homedir(), '.agent-farm', 'test-workspaces'); + mkdirSync(base, { recursive: true }); + const ws = mkdtempSync(resolve(base, 'pir1365-')); + for (const d of ['codev', '.agent-farm', '.codev']) mkdirSync(resolve(ws, d), { recursive: true }); + writeFileSync( + resolve(ws, '.codev', 'config.json'), + JSON.stringify({ shell: { architect: 'sh -c "sleep 3600"', builder: 'bash', shell: 'bash' } }), + ); + // A REAL builder launches through this wrapper, so its PtySession.command is the shell, + // not the harness — and `resolveProfileForSession` recovers the harness by reading this + // file (the wrapped-launch fallback, same code path `afx reset` uses). Without it the gate + // holds every send `no-profile` and nothing would ever be delivered to assert about. This + // is fidelity, not a shortcut: it is exactly how a live builder's profile resolves. + writeFileSync( + resolve(ws, '.builder-start.sh'), + '#!/usr/bin/env bash\nexec claude --dangerously-skip-permissions\n', + ); + return ws; +} + +async function activate(ws: string): Promise { + const encoded = Buffer.from(ws).toString('base64url'); + for (let i = 0; i < 30; i++) { + const res = await fetch(`${BASE}/api/workspaces/${encoded}/activate`, { method: 'POST' }); + if (res.ok) break; + await sleep(500); + } + for (let i = 0; i < 60; i++) { + const list = await (await fetch(`${BASE}/api/workspaces`)).json(); + if (list.workspaces.some((w: { path: string }) => w.path === ws)) return; + await sleep(500); + } + throw new Error('workspace never activated'); +} + +/** A real shellper-backed PTY that echoes its input — see the header for why. */ +async function registerEchoTerminal(ws: string, roleId: string): Promise { + const res = await fetch(`${BASE}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: 'sh', + args: ['-c', 'stty raw -echo 2>/dev/null; exec cat'], + cwd: ws, + cols: 110, + rows: 32, + workspacePath: ws, + type: 'builder', + roleId, + persistent: true, + }), + }); + if (res.status !== 201) throw new Error(`terminal register failed: ${res.status}`); + return (await res.json()).id; +} + +async function paint(terminalId: string, screen: string): Promise { + await fetch(`${BASE}/api/terminals/${terminalId}/write`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ data: screen }), + }); + await sleep(250); // let the PTY echo land and the gate mirror catch up +} + +/** + * Every byte the echo PTY has emitted, in order — the interleaving oracle. + * + * `GET /api/terminals/:id/output` projects the ring as `{lines: string[]}`, so the lines are + * rejoined here. Reading the ring rather than a mock is the point: this is the same output + * stream the render gate classifies and the operator sees. + */ +async function transcript(terminalId: string): Promise { + const res = await fetch(`${BASE}/api/terminals/${terminalId}/output?lines=1000000`); + const data = await res.json(); + return Array.isArray(data.lines) ? data.lines.join('\n') : JSON.stringify(data); +} + +interface SendOptions { interrupt?: boolean; escape?: boolean; deliverAfter?: number } +interface SendResult { status: number; body: Record; elapsedMs: number } + +async function send(ws: string, to: string, message: string, options: SendOptions = {}): Promise { + const startedAt = Date.now(); + const res = await fetch(`${BASE}/api/send`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ to, workspace: ws, from: 'architect', message, options }), + }); + const body = await res.json().catch(() => ({})); + return { status: res.status, body, elapsedMs: Date.now() - startedAt }; +} + +/** The held rows `afx inbox` would show — metadata only, as the route projects them. */ +async function inbox(ws: string): Promise>> { + const res = await fetch(`${BASE}/api/inbox?workspace=${encodeURIComponent(ws)}`); + if (!res.ok) return []; + return res.json(); +} + +// ---------------------------------------------------------------- scenarios + +/** + * A body long enough to take the paced multi-line path and hold the line while the + * interrupt races it. 12 lines ≈ 11×10 + 80 = 190 ms of exposed text→Enter window. + */ +function longBody(i: number): string { + return Array.from({ length: 12 }, (_, n) => `S1-${i}-line${n}`).join('\n'); +} + +/** Occurrences of `needle` in `haystack`. */ +function countOf(haystack: string, needle: string): number { + return haystack.split(needle).length - 1; +} + +/** + * Scenario 1 — a long multi-line send raced by a concurrent interrupt, ×10. + * + * The property under test is NOT "the delivery always wins" — a gated delivery is entitled to + * hold when the line is busy, and a row reported `held` at request time may still be + * delivered moments later by the fast trigger or the backstop. Both are correct. + * + * The invariant is about FRAGMENTATION, which is what interleaving actually looks like on the + * wire: whenever any part of the body reaches the terminal, the WHOLE body reaches it as one + * contiguous run. So count the body's first line and count the whole body — if a racing `^C` + * ever split a delivery, there would be a first line with no whole body behind it, and the + * two counts would diverge. That is a sharper detector than "is the body present", and it is + * exactly the failure this issue exists to remove. + */ +async function scenario1(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 1 — 10x long multi-line send + concurrent --interrupt'); + let deliveredAtRequest = 0; + let heldAtRequest = 0; + let landedWhole = 0; + + for (let i = 0; i < 10; i++) { + await paint(terminalId, CLEAN_COMPOSER); + const before = (await transcript(terminalId)).length; + + const body = longBody(i); + const [normal] = await Promise.all([ + send(ws, agent, body), + send(ws, agent, `S1-${i}-INTERRUPT`, { interrupt: true }), + ]); + // Long enough for the paced writes AND for a fast-trigger/backstop redelivery of a row + // that was held at request time, so the observed end state is stable. + await sleep(2200); + + const after = (await transcript(terminalId)).slice(before); + const wholeBodies = countOf(after, body); + const firstLines = countOf(after, `S1-${i}-line0`); + + // THE assertion: no fragment of the body exists that is not part of a whole body. + check(firstLines === wholeBodies, + `run ${i}: body never fragmented on the wire (whole=${wholeBodies} starts=${firstLines})`); + check(wholeBodies <= 1, `run ${i}: the body was not duplicated`, `saw ${wholeBodies}`); + + if (normal.body.delivered === true) { + deliveredAtRequest++; + check(wholeBodies === 1, `run ${i}: reported delivered → the body IS on the wire, whole`); + } else { + heldAtRequest++; + // Held at request time is fine; it may still deliver via the fast trigger/backstop. + console.log(` note run ${i}: held (${String(normal.body.reason)}) at request time, ` + + `${wholeBodies === 1 ? 'delivered whole by a later gated pass' : 'still pending'}`); + } + if (wholeBodies === 1) landedWhole++; + + check(after.includes(`S1-${i}-INTERRUPT`), `run ${i}: the interrupt itself landed`); + check(after.includes(CTRL_C), `run ${i}: the interrupt's ^C landed`); + } + + console.log(`\n at request time: delivered=${deliveredAtRequest} held=${heldAtRequest}`); + console.log(` bodies that reached the wire: ${landedWhole}/10 — every one of them WHOLE`); + console.log(' (holding is a correct outcome; fragmenting or lying about delivery is not)'); + + const held = await inbox(ws); + console.log(` rows still held after scenario 1: ${held.length}` + + (held.length ? ` (${held.map((r) => String(r.reason)).join(', ')}) — still deliverable, nothing lost` : '')); +} + +/** Scenario 2 — `--delay 5 --interrupt` against a mid-turn agent. */ +async function scenario2(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 2 — --delay 5 --interrupt against a MID-TURN agent'); + await paint(terminalId, DRAFT_COMPOSER); // mid-turn: the gate must hold the body + const before = (await transcript(terminalId)).length; + + const BODY = 'S2-DELAYED-BODY'; + const scheduled = await send(ws, agent, BODY, { deliverAfter: 5, interrupt: true }); + check(scheduled.body.scheduled === true, 'the send is SCHEDULED, not written at request time'); + check(!(await transcript(terminalId)).slice(before).includes(BODY), 'no body on the wire at request time'); + + await sleep(6500); // past the due time + const afterDue = (await transcript(terminalId)).slice(before); + check(afterDue.includes(CTRL_C), 'the ^C fired at due time'); + check(!afterDue.includes(BODY), 'the body did NOT land mid-turn (the screen is still a draft)'); + + // The turn ends: a clean prompt appears, and the gate lets the body through. + await paint(terminalId, CLEAN_COMPOSER); + for (let i = 0; i < 20 && !(await transcript(terminalId)).slice(before).includes(BODY); i++) await sleep(300); + + const final = (await transcript(terminalId)).slice(before); + const occurrences = final.split(BODY).length - 1; + check(occurrences === 1, 'the body landed EXACTLY once on the clean prompt', `saw ${occurrences}`); + check(final.indexOf(CTRL_C) < final.indexOf(BODY), 'the ^C preceded the body'); +} + +/** Scenario 3 — an interrupt must not stall behind a long delivery (the D3 ceiling). */ +async function scenario3(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 3 — --interrupt against a busy line returns within the 2000ms ceiling'); + await paint(terminalId, CLEAN_COMPOSER); + + // ~400 lines ⇒ 399×10 + 80 ≈ 4.07 s of paced write: comfortably longer than the ceiling, + // so an UNBOUNDED wait would show up as a ~4 s interrupt. This is not a pathological body — + // it is the size of a modest --file attachment. + const hugeBody = Array.from({ length: 400 }, (_, n) => `S3-line${n}`).join('\n'); + const logMark = towerLog.length; + const delivery = send(ws, agent, hugeBody); + await sleep(300); // let the delivery take the terminal lock + + const interrupt = await send(ws, agent, 'S3-INTERRUPT', { interrupt: true }); + console.log(` interrupt round-trip: ${interrupt.elapsedMs}ms (ceiling 2000ms)`); + check(interrupt.elapsedMs < 3200, 'the interrupt returned near the ceiling, not after the whole write', + `${interrupt.elapsedMs}ms`); + check(interrupt.status === 200, 'the interrupt still succeeded (the escape hatch works)'); + + const deliveryResult = await delivery; + console.log(` delivery outcome: delivered=${String(deliveryResult.body.delivered)} ` + + `held=${String(deliveryResult.body.held)} reason=${String(deliveryResult.body.reason)}`); + await sleep(500); + + // Server-side corroboration. Two lines matter, and together they are the whole D3 story: + // the operator announced its degradation, and the raced delivery refused to claim success. + const degraded = towerLogSince(logMark, /UNSERIALIZED/); + const preempted = towerLogSince(logMark, /raced by an unserialized/); + for (const l of [...degraded, ...preempted]) console.log(` tower: ${l.trim()}`); + check(degraded.length === 1, 'Tower logged the ceiling degradation loudly (WARN)'); + check(preempted.length === 1, 'the raced delivery reported PREEMPTED and held its row for redelivery'); + check(deliveryResult.body.delivered !== true, + 'the raced delivery did NOT claim delivered — the false-delivered failure cannot recur here'); +} + +/** Scenario 4 — `--escape` is unchanged, and a dead terminal is refused loudly. */ +async function scenario4(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 4 — --escape unchanged; a non-writable terminal is refused'); + await paint(terminalId, CLEAN_COMPOSER); + const before = (await transcript(terminalId)).length; + + const esc = await send(ws, agent, '', { escape: true }); + await sleep(300); + const after = (await transcript(terminalId)).slice(before); + check(esc.status === 200 && esc.body.ok === true, 'escape returns 200 ok'); + check(after.includes(ESC), 'the ESC byte reached the PTY'); + check(after.includes('\r'), 'its trailing Enter reached the PTY'); + + // Refusal path: kill the terminal, then retry. Documented honestly below — this exercises + // the no-live-session refusal, NOT the shellper-socket-down 503. + await fetch(`${BASE}/api/terminals/${terminalId}`, { method: 'DELETE' }); + await sleep(1200); + const dead = await send(ws, agent, '', { escape: true }); + console.log(` escape to a killed terminal → ${dead.status} ${String(dead.body.error)}`); + check(dead.status >= 400, 'an operator action to a dead terminal is REFUSED, never silently dropped'); + + const normal = await send(ws, agent, 'S4-NORMAL-AFTER-DEATH'); + console.log(` normal send to the same dead agent → ${normal.status} ` + + `held=${String(normal.body.held)} reason=${String(normal.body.reason)}`); + // This fixture's agent exists ONLY as a live terminal (registered via POST /api/terminals), + // never as a spawned builder in global.db, so once its terminal is killed the agent is + // unknown to the registry and 404 is the correct answer. The Spec 1313 "hold instead of + // 404" seam needs a registry-known agent, which this harness deliberately does not fake — + // it is covered by send-delivery/tower-routes unit tests against the real registry. + check(normal.status === 404, + 'an unknown-after-death agent 404s (registry seam needs a real builder — see note)'); + + console.log('\n NOT SCRIPTED HERE, and why:'); + console.log(' 503 TERMINAL_NOT_WRITABLE needs a shellper socket that has DIED while the'); + console.log(' session still reports status=running (#1198). That state cannot be forced'); + console.log(' from the public API without killing the shellper out of band, which would'); + console.log(' be staging the result rather than observing it. It is covered by'); + console.log(' tower-routes.test.ts:1560 against the real route, and this change does not'); + console.log(' touch that branch — only the lock the branch returns before reaching.'); +} + +// ---------------------------------------------------------------- main + +async function main(): Promise { + console.log(`Issue #1365 — dev-approval evidence`); + console.log(`isolated Tower on port ${PORT} (NOT 4100 — the live Tower is untouched)`); + console.log(`tower build: ${TOWER}`); + console.log(`started: ${new Date().toISOString()}`); + + let tower: ChildProcess | null = null; + let ws = ''; + try { + tower = await startTower(); + ws = makeWorkspace(); + await activate(ws); + + const agent = 'pir-1365-probe'; + const terminalId = await registerEchoTerminal(ws, agent); + console.log(`\nworkspace: ${ws}\nterminal: ${terminalId} (real shellper-backed PTY, echo oracle)`); + + await scenario1(ws, agent, terminalId); + await scenario2(ws, agent, terminalId); + await scenario3(ws, agent, terminalId); + await scenario4(ws, agent, terminalId); + + section('RESULT'); + console.log(` ${checks - failures}/${checks} checks passed`); + console.log(failures === 0 ? ' ALL SCENARIOS PASSED' : ` ${failures} FAILED`); + } finally { + if (tower) { + tower.kill('SIGTERM'); + await sleep(1500); + tower.kill('SIGKILL'); + } + if (ws) rmSync(ws, { recursive: true, force: true }); + } + process.exit(failures === 0 ? 0 : 1); +} + +void main(); diff --git a/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts b/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts index 79fe5c467..3cd7298c3 100644 --- a/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts +++ b/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts @@ -32,6 +32,7 @@ const AGENT = 'main'; /** A minimal DeliverySession fake (records writes). */ function fakeSession(): DeliverySession { return { + id: 'term-1', bytesWritten: 0, info: { cols: 110, rows: 32 }, command: 'claude', @@ -81,9 +82,12 @@ function harness(): Harness { getSessionForAgent: () => session, resolveProfile: () => profile, classify: (_session: DeliverySession, _p: GateProfile): Promise => Promise.resolve(verdict), - writeMessage: (_s, formattedMessage, noEnter) => { + writeMessage: (_s, formattedMessage, noEnter, precheck) => { + // The precheck runs INSIDE the per-terminal lock in the live binding (Issue #1365). + const abort = precheck(); + if (abort) return { status: 'aborted', abort }; writes.push({ formattedMessage, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => { diff --git a/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts b/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts index b4af14bc8..063c1a068 100644 --- a/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts @@ -97,10 +97,12 @@ function realSeamPorts( // The REAL production classify seam (Spec 1313 round 2): read the session's persistent // mirror (seeded here via attachShellper's replay) and classify its viewport. classify: (s, prof) => classifyAgentScreen(s, prof), - writeMessage: (s, msg, noEnter) => { + writeMessage: (s, msg, noEnter, precheck) => { + const abort = precheck(); + if (abort) return { status: 'aborted' as const, abort }; writes.push({ msg, noEnter }); s.write(msg); // drive the real session's write path (fake shellper records it) - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => {}, diff --git a/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts b/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts index a9a497b00..2043a1b4f 100644 --- a/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts @@ -43,6 +43,7 @@ const BUSY: GateVerdict = { clean: false, reason: 'busy', detail: 'user-text' }; function fakeSession(overrides: Partial = {}): DeliverySession & { writes: string[] } { const writes: string[] = []; return { + id: 'term-fake', bytesWritten: 0, info: { cols: 110, rows: 32 }, command: 'claude', @@ -120,9 +121,13 @@ function harness(): Harness { resolveProfile: () => profile, classify: (session: DeliverySession, p: GateProfile): Promise => classifyOverride ? classifyOverride(session, p) : Promise.resolve(verdict), - writeMessage: (_s, formattedMessage, noEnter) => { + writeMessage: (_s, formattedMessage, noEnter, precheck) => { + // Mirror the live binding's ordering: the precheck runs INSIDE the lock, before the + // first byte (Issue #1365), so an abort must record no write at all. + const abort = precheck(); + if (abort) return { status: 'aborted', abort }; writes.push({ formattedMessage, noEnter }); - return h.writeResult; + return h.writeResult ? { status: 'written' } : { status: 'dropped' }; }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => { @@ -318,6 +323,7 @@ describe('deliverAgentMail (Spec 1313, Phase 4)', () => { // (the false-clean the gate exists to prevent). let bytes = 0; const session: DeliverySession = { + id: 'term-moving', get bytesWritten() { return bytes; }, @@ -422,7 +428,7 @@ describe('deliverAgentMailSerialized — concurrent-send serialization (Spec 131 h.ports.writeMessage = (_s, formattedMessage, noEnter) => Promise.resolve().then(() => { h.writes.push({ formattedMessage, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }); h.setSession('spir-1', fakeSession()); mailbox.enqueue(db, { workspacePath: '/ws/a', toAgent: 'spir-1', body: '1', formattedMessage: 'F' }, 1000); @@ -546,6 +552,7 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => let bytes = 7; // A moving token needs a live getter (a fakeSession spread would freeze bytesWritten to its value). h.setSession('spir-1', { + id: 'term-moving', get bytesWritten() { return bytes; }, info: { cols: 110, rows: 32 }, command: 'claude', @@ -604,6 +611,7 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => h.ports.writeMessage = (_s, formattedMessage, noEnter) => { h.writes.push({ formattedMessage, noEnter }); if (formattedMessage === 'm1') mailbox.dismiss(db, m1.id, 1002); // operator dismisses during the paced write + return { status: 'written' as const }; }; const drainer = new MailboxDrainer({ intervalMs: 999999 }); drainer.start(h.ports, db); @@ -618,8 +626,8 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => it('invalidates the memo even when writeMessage REJECTS after partial output (CMAP round 4 — Codex)', async () => { const h = harness(); // Round-4 completion of Fix 1: memo.delete must run on a write REJECTION too (via try/finally), - // not only a clean return. writeMessage's port contract is boolean|Promise, so a binding - // could reject after putting bytes on the wire; without the finally the stale CLEAN survives and a + // not only a clean return. writeMessage's port contract is WriteResult|Promise, so a + // binding could reject after putting bytes on the wire; without the finally the stale CLEAN survives and a // follow-up could memo-hit it. Here writeMessage records partial output then rejects → the row // stays held (deliverAgentMail throws, caught by the per-agent tick guard) → the NEXT tick must // re-classify fresh, not memo-hit. Static ring, so a re-classify can only come from invalidation. @@ -1050,6 +1058,7 @@ describe('MailboxDrainer owner starvation notice (Spec 1313 round 3, change 3)', // verdict memo re-classifies (a static token would serve the cached BUSY and never deliver). let bytes = 1; h.setSession('spir-1', { + id: 'term-moving', get bytesWritten() { return bytes; }, info: { cols: 110, rows: 32 }, command: 'claude', diff --git a/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts b/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts index 743ad8b3d..720b0841d 100644 --- a/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts @@ -58,6 +58,7 @@ function flipSession(command = 'claude'): FlipSession { let bytes = 0; let screen = new SessionScreen(COLS, ROWS); return { + id: 'term-flip', get bytesWritten() { return bytes; }, @@ -99,9 +100,11 @@ function realGatePorts( const { term, cols, rows } = await session.screen.read(); return classifyBuffer(term, cols, rows, prof); }, - writeMessage: (_s, msg, noEnter) => { + writeMessage: (_s, msg, noEnter, precheck) => { + const abort = precheck(); + if (abort) return { status: 'aborted' as const, abort }; writes.push({ msg, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => {}, diff --git a/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts index 45b7c1aa3..40cf6af75 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts @@ -7,17 +7,24 @@ * discarded the boolean and resolved on a pure timer — a message could be reported * `delivered` while zero bytes reached the terminal. * - * `writeMessagePaced` now threads the per-write result and resolves `false` when ANY - * scheduled write dropped. The load-bearing property the architect called out is that - * this must catch BOTH the first (synchronous) write AND the DELAYED writes — the - * trailing Enter and the per-line writes of a multi-line message — because a socket can - * die anywhere across the 10–130ms+ paced sequence, not only at t=0. These tests drive - * the real pacing under fake timers and assert the aggregate for each drop position. + * The write edge threads the per-write result and reports `dropped` when ANY scheduled + * write dropped. The load-bearing property the architect called out is that this must catch + * BOTH the first (synchronous) write AND the DELAYED writes — the trailing Enter and the + * per-line writes of a multi-line message — because a socket can die anywhere across the + * 10–130ms+ paced sequence, not only at t=0. These tests drive the real pacing under fake + * timers and assert the aggregate for each drop position. + * + * Issue #1365 re-pointed them from the retired `writeMessagePaced` onto `submitMessagePaced`, + * which is the same paced write performed under the per-terminal submission lock. The + * drop-threading contract is unchanged — only its vocabulary is (`written`/`dropped` instead + * of a bare boolean) — so these keep guarding the silent-loss fix at the live write edge + * rather than at a function nothing calls. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { writeMessagePaced } from '../servers/message-write.js'; +import { submitMessagePaced } from '../servers/message-write.js'; import type { WritableSession } from '../servers/message-write.js'; +import { resetSubmissionChains } from '../servers/session-submit.js'; /** * A WritableSession fake whose `write` returns false (a dropped write) whenever @@ -25,9 +32,10 @@ import type { WritableSession } from '../servers/message-write.js'; */ function makeSession( shouldDrop: (data: string, callIndex: number) => boolean = () => false, -): WritableSession & { writes: string[] } { +): WritableSession & { id: string; writes: string[] } { const writes: string[] = []; return { + id: 'term-drop', write: (data: string): boolean => { const idx = writes.length; writes.push(data); @@ -37,20 +45,33 @@ function makeSession( }; } -/** Run every scheduled paced write + the resolve timer, then await the promise. */ -async function settle(p: Promise): Promise { +/** + * Drive the write edge to completion and report whether the whole submit landed. + * + * `runAllTimersAsync` covers both the paced writes and the submission lock's own + * completion sleep, which is scheduled on the same (fake) clock. + */ +async function settle(p: Promise<{ status: string }>): Promise { await vi.runAllTimersAsync(); - return p; + return (await p).status === 'written'; } -describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss fix)', () => { - beforeEach(() => vi.useFakeTimers()); +/** The write edge as mailbox-wiring binds it: no in-lock refusal, so the write always runs. */ +function write(session: WritableSession & { id: string }, message: string, noEnter: boolean) { + return submitMessagePaced(session, message, noEnter, () => null); +} + +describe('submitMessagePaced — dropped-write threading (Spec 1313 silent-loss fix)', () => { + beforeEach(() => { + resetSubmissionChains(); + vi.useFakeTimers(); + }); afterEach(() => vi.useRealTimers()); describe('short message (single write + delayed Enter)', () => { it('all writes land → resolves true, text + Enter both on the wire', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(true); expect(session.writes).toEqual(['hello', '\r']); @@ -59,7 +80,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('the FIRST (synchronous) write drops → resolves false', async () => { // The socket is already dead when the text write fires at t=0. const session = makeSession((_d, i) => i === 0); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(false); expect(session.writes[0]).toBe('hello'); // it WAS attempted @@ -69,7 +90,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f // The critical case the t=0 `writable` precheck cannot see: text writes fine, then // the socket dies before the Enter fires 50ms later, so the submit never completes. const session = makeSession((d) => d === '\r'); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(false); expect(session.writes).toContain('\r'); // the Enter was attempted (and dropped) @@ -77,7 +98,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('noEnter, text lands → resolves true, no Enter written', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, 'hi', true)); + const result = await settle(write(session, 'hi', true)); expect(result).toBe(true); expect(session.writes).toEqual(['hi']); @@ -85,7 +106,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('noEnter, text drops → resolves false', async () => { const session = makeSession((_d, i) => i === 0); - const result = await settle(writeMessagePaced(session, 'hi', true)); + const result = await settle(write(session, 'hi', true)); expect(result).toBe(false); }); @@ -96,7 +117,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('all lines + Enter land → resolves true, Enter last', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(true); expect(session.writes.at(-1)).toBe('\r'); // Enter delivered after every line @@ -107,7 +128,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('a DELAYED middle line drops → resolves false', async () => { // Line 2 ("b\n") fires ~10ms in — a delayed write, not the synchronous first one. const session = makeSession((d) => d === 'b\n'); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(false); expect(session.writes).toContain('b\n'); // attempted mid-pace, dropped @@ -115,7 +136,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('the DELAYED trailing Enter drops (all lines landed) → resolves false', async () => { const session = makeSession((d) => d === '\r'); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(false); expect(session.writes).toContain('a\n'); // the lines themselves went out diff --git a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts new file mode 100644 index 000000000..30d1f7746 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts @@ -0,0 +1,632 @@ +/** + * Serializer convergence (Issue #1365) — the gated mailbox write edge takes the same + * per-terminal submission lock as `--interrupt` / `--escape`. + * + * Before this change the two paths held DISJOINT locks (per-agent for deliveries, + * per-terminal for operator actions), so they could interleave on one terminal. The + * failure that mattered was not a garbled composer but a **false `delivered`**: a `^C` + * landing inside a delivery's text→Enter window cleared the composer, the delivery's Enter + * submitted nothing, every byte still reported success, and the row was marked delivered + * for a message the agent never saw. `--escape` produces the truncated variant. + * + * The composer double below is what makes that visible: it models the one TUI behaviour the + * bug depends on — `^C`/ESC discard pending input, Enter submits whatever has accumulated. + * Each corruption test is paired with a bypass-the-lock control that reproduces the original + * bug, so these assert the fix rather than merely exercising it. + * + * Real timers throughout (the paced writer schedules on real `setTimeout`), with the + * production pacing constants: lines 10 ms apart, Enter 80 ms after the last line. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import Database from 'better-sqlite3'; +import { GLOBAL_SCHEMA } from '../db/schema.js'; +import * as mailbox from '../db/mailbox.js'; +import { + submitToSession, + trySubmitToSession, + isSubmissionInFlight, + pendingSubmissionSessions, + resetSubmissionChains, + unserializedWriteCount, + bypassCountedSessions, + OPERATOR_SUBMIT_WAIT_CEILING_MS, +} from '../servers/session-submit.js'; +import { + submitMessagePaced, + writeMessageToSession, + writeEscapeToSession, +} from '../servers/message-write.js'; +import { + deliverAgentMail, + MailboxDrainer, + type DeliveryPorts, + type DeliverySession, + type WriteAbort, +} from '../servers/mailbox-delivery.js'; +import type { GateProfile, GateVerdict } from '../servers/render-gate.js'; + +const PROFILE: GateProfile = { app: 'claude', markerPattern: /^❯/, regionEndPatterns: [] }; +const CLEAN: GateVerdict = { clean: true, detail: 'empty' }; +const WS = '/ws/a'; +const AGENT = 'spir-1'; + +/** A body long enough to take the paced multi-line path (≥4 lines) and so hold the line ~110 ms. */ +const MULTILINE = 'L1\nL2\nL3\nL4'; +/** Time for a MULTILINE paced write to finish: last line at 30 ms + the 80 ms Enter delay. */ +const MULTILINE_DONE_MS = 3 * 10 + 80; + +const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); + +/** + * A composer that models the failure. `^C` and ESC discard pending input the way a real + * TUI does; Enter submits whatever has accumulated. Everything else is text. + */ +function makeComposer(id = 'term-1') { + let pending = ''; + const submitted: string[] = []; + const bypasses: string[] = []; + const session = { + id, + write(data: string): boolean { + if (data === '\r') { + submitted.push(pending); + pending = ''; + } else if (data === '\x03') { + bypasses.push('interrupt'); + pending = ''; + } else if (data === '\x1b') { + bypasses.push('escape'); + pending = ''; + } else { + pending += data; + } + return true; + }, + }; + return { + session, + submitted, + bypasses, + get pending() { + return pending; + }, + }; +} + +/** The immediate `--interrupt` submission, exactly as tower-routes issues it. */ +function interruptSubmission(session: { id: string; write(d: string): boolean }, body: string) { + return () => { + session.write('\x03'); + return writeMessageToSession(session, body, false, 100); + }; +} + +/** A gated delivery's write edge, as mailbox-wiring binds it. */ +function deliveryWrite(session: { id: string; write(d: string): boolean }, body: string) { + return submitMessagePaced(session, body, false, () => null); +} + +describe('Issue #1365 — the gated write edge and the operator paths share one lock', () => { + beforeEach(() => resetSubmissionChains()); + + it('an interrupt cannot land inside a delivery text→Enter window (the false-delivered bug)', async () => { + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + const [result] = await Promise.all([delivery, interrupt]); + + // The delivery's body was submitted WHOLE, and the interrupt's separately. + expect(c.submitted).toEqual([MULTILINE, 'INT']); + // ...and only then may the row be marked delivered. + expect(result).toEqual({ status: 'written' }); + }); + + it('CONTROL: bypassing the lock reproduces the original bug — body lost, write still "succeeds"', async () => { + const c = makeComposer(); + + // Exactly the pre-#1365 delivery edge: a paced write under no per-terminal lock. + writeMessageToSession(c.session, MULTILINE, false); + await submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + await sleep(MULTILINE_DONE_MS + 20); + + // The ^C cleared the composer mid-write; the delivery's Enter submitted an empty line. + // Every byte reached the PTY, so the old boolean-returning port reported success and the + // row was marked delivered — a message the agent never saw. + expect(c.submitted).not.toContain(MULTILINE); + expect(c.submitted).toContain(''); + }); + + it('an escape cannot truncate an in-flight multi-line delivery', async () => { + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + const escape = submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)); + const [result] = await Promise.all([delivery, escape]); + + expect(c.submitted[0]).toBe(MULTILINE); // whole, not a tail of it + expect(result).toEqual({ status: 'written' }); + }); + + it('CONTROL: bypassing the lock lets an escape submit a TRUNCATED body', async () => { + const c = makeComposer(); + + writeMessageToSession(c.session, MULTILINE, false); + await submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)); + await sleep(MULTILINE_DONE_MS + 20); + + // The ESC discarded the lines written so far; the escape's own Enter then submitted the + // remainder as if it were the whole message. + expect(c.submitted[0]).not.toBe(MULTILINE); + expect(MULTILINE.endsWith(c.submitted[0])).toBe(true); // a strict tail — i.e. truncated + expect(c.submitted[0].length).toBeLessThan(MULTILINE.length); + }); + + it('two deliveries to one terminal still cannot interleave', async () => { + const c = makeComposer(); + + const first = deliveryWrite(c.session, MULTILINE); + // A second delivery arriving mid-write is DECLINED, not queued — the caller re-holds. + const second = await deliveryWrite(c.session, 'SECOND'); + expect(second).toEqual({ status: 'contended' }); + expect(await first).toEqual({ status: 'written' }); + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('deliveries to DIFFERENT terminals do not serialize (the lock key is the real session id)', async () => { + const a = makeComposer('term-a'); + const b = makeComposer('term-b'); + + const [ra, rb] = await Promise.all([deliveryWrite(a.session, MULTILINE), deliveryWrite(b.session, MULTILINE)]); + + // Both ran. Were the key `undefined` for every session — the hazard a structurally-typed + // fake without an `id` would create — the second would have been declined as contended. + expect(ra).toEqual({ status: 'written' }); + expect(rb).toEqual({ status: 'written' }); + expect(a.submitted).toEqual([MULTILINE]); + expect(b.submitted).toEqual([MULTILINE]); + }); + + it('a session with no id throws instead of keying every lock on undefined', async () => { + const c = makeComposer(); + const idless = { ...c.session, id: '' as string }; + + await expect(submitMessagePaced(idless, 'hi', false, () => null)).rejects.toThrow(/non-empty string/); + expect(c.submitted).toHaveLength(0); + }); + + it('the returned promise resolves only AFTER the trailing Enter', async () => { + const c = makeComposer(); + const result = await deliveryWrite(c.session, MULTILINE); + // Resolution implies submission — the property the per-agent serializer's completion + // chaining depends on. No extra wait: the Enter has already fired. + expect(c.submitted).toEqual([MULTILINE]); + expect(result).toEqual({ status: 'written' }); + }); + + it('reports a dropped write (#1198) rather than a delivery', async () => { + const session = { id: 'term-dead', write: () => false }; + expect(await submitMessagePaced(session, MULTILINE, false, () => null)).toEqual({ status: 'dropped' }); + }); + + it('an in-lock precheck refusal writes nothing', async () => { + const c = makeComposer(); + const abort: WriteAbort = { kind: 'hold', reason: 'busy' }; + + const result = await submitMessagePaced(c.session, MULTILINE, false, () => abort); + + expect(result).toEqual({ status: 'aborted', abort }); + expect(c.submitted).toHaveLength(0); + expect(c.pending).toBe(''); // not one byte, not even un-submitted text + }); + + it('leaves no chain behind once mixed traffic settles', async () => { + const c = makeComposer(); + await Promise.all([ + deliveryWrite(c.session, MULTILINE), + submitToSession(c.session.id, interruptSubmission(c.session, 'INT')), + submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)), + ]); + await sleep(10); + expect(pendingSubmissionSessions()).toBe(0); + expect(isSubmissionInFlight(c.session.id)).toBe(false); + }); +}); + +describe('Issue #1365 — deliveries decline contention, operators wait (bounded)', () => { + beforeEach(() => resetSubmissionChains()); + + it('a delivery declines a contended terminal IMMEDIATELY instead of queueing', async () => { + const c = makeComposer(); + // An operator holds the line for well over a backstop tick. + const holder = submitToSession(c.session.id, () => { + c.session.write('slow'); + setTimeout(() => c.session.write('\r'), 300); + return 300; + }); + + const startedAt = Date.now(); + const result = await trySubmitToSession(c.session.id, () => 0); + const waited = Date.now() - startedAt; + + expect(result).toBe(false); + expect(waited).toBeLessThan(100); // did not wait out the 300 ms holder + await holder; + }); + + it('the drainer keeps delivering to OTHER agents while one terminal is held', async () => { + // The liveness property: MailboxDrainer.tick walks agents sequentially, so a delivery + // that blocked on a terminal lock would stall every other agent behind it. + const busy = makeComposer('term-busy'); + const free = makeComposer('term-free'); + const db = new Database(':memory:'); + db.exec(GLOBAL_SCHEMA); + try { + const sessions = new Map([ + ['agent-busy', sessionFor(busy.session)], + ['agent-free', sessionFor(free.session)], + ]); + const ports = livePorts(sessions); + mailbox.enqueue(db, { workspacePath: WS, toAgent: 'agent-busy', body: 'a', formattedMessage: 'A' }, 1000); + mailbox.enqueue(db, { workspacePath: WS, toAgent: 'agent-free', body: 'b', formattedMessage: 'B' }, 1000); + + const holder = submitToSession('term-busy', () => { + setTimeout(() => busy.session.write('held'), 250); + return 250; + }); + + const startedAt = Date.now(); + const drainer = new MailboxDrainer({ intervalMs: 999_999 }); + drainer.start(ports, db); + await drainer.tick(); + drainer.stop(); + const tickMs = Date.now() - startedAt; + + expect(free.submitted).toEqual(['B']); // the unblocked agent was served + expect(tickMs).toBeLessThan(250); // and the tick did not wait out the held terminal + await holder; + } finally { + db.close(); + } + }); + + it('an operator submission stops waiting at the ceiling and says so', async () => { + const c = makeComposer(); + const ceilingMs = 40; + const expired: number[] = []; + + // The holder must be a DELIVERY write: since the codex review of PR #1492 the ceiling + // deliberately refuses to bypass another OPERATOR, so an operator holder would (correctly) + // never expire this ceiling at all. + const holder = trySubmitToSession(c.session.id, () => { + setTimeout(() => c.session.write('\r'), 400); + return 400; + }); + const startedAt = Date.now(); + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: ceilingMs, + onCeilingExpired: (ms) => expired.push(ms), + }); + const waited = Date.now() - startedAt; + + expect(expired).toEqual([ceilingMs]); // degraded, and announced + expect(waited).toBeLessThan(400); // did not wait out the holder + expect(c.bypasses).toEqual(['interrupt']); // the ^C did go out — the escape hatch still works + expect(unserializedWriteCount(c.session.id)).toBe(1); + await holder; + }); + + it('an operator submission below the ceiling still serializes normally', async () => { + const c = makeComposer(); + const expired: number[] = []; + + const delivery = deliveryWrite(c.session, MULTILINE); // ~110 ms, well under the ceiling + await submitToSession(c.session.id, interruptSubmission(c.session, 'INT'), undefined, { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (ms) => expired.push(ms), + }); + + expect(expired).toEqual([]); + expect(await delivery).toEqual({ status: 'written' }); + expect(c.submitted).toEqual([MULTILINE, 'INT']); + }); + + it('operator vs operator NEVER degrades — the wait stays unbounded, as before #1365', async () => { + // The regression codex caught in PR #1492 review. Before this change `submitToSession` had + // no ceiling at all, so two operator submissions to one terminal were ALWAYS fully + // serialized. A ceiling that could skip a body-bearing operator would make this one pair + // strictly WORSE than the old behaviour — the opposite of the point. + const c = makeComposer(); + const expired: number[] = []; + + // A body-bearing interrupt that holds the line far longer than the ceiling. + const first = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-ONE'), undefined, { + waitCeilingMs: 30, + onCeilingExpired: (ms) => expired.push(ms), + }); + await sleep(5); + const second = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-TWO'), undefined, { + waitCeilingMs: 30, // would fire long before the first operator finishes, if it were armed + onCeilingExpired: (ms) => expired.push(ms), + }); + await Promise.all([first, second]); + + expect(expired).toEqual([]); // neither operator gave up on the other + expect(c.submitted).toEqual(['OP-ONE', 'OP-TWO']); // strictly ordered, neither clobbered + }); + + it('a THIRD operator does not bypass a QUEUED one (a waiting operator counts, not just a writing one)', async () => { + const c = makeComposer(); + const expired: number[] = []; + const opts = { waitCeilingMs: 30, onCeilingExpired: (ms: number) => expired.push(ms) }; + + // Head is a DELIVERY (bypassable); B queues behind it; C must not skip B. + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); + const b = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-B'), undefined, opts); + await sleep(5); + const c3 = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-C'), undefined, opts); + await Promise.all([delivery, b, c3]); + + // B may degrade past the delivery (that is the ceiling's whole purpose); C may not + // degrade past B, so at most one degradation is recorded and the operators stay ordered. + expect(expired.length).toBeLessThanOrEqual(1); + const ops = c.submitted.filter((s) => s.startsWith('OP-')); + expect(ops).toEqual(['OP-B', 'OP-C']); + }); + + it('a delivery raced by a ceiling-expired write reports preempted, never written', async () => { + // The one hole the ceiling opens: an operator that gave up waiting writes into a + // delivery already on the wire. Detected by counting lock bypasses — no re-classify. + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); // let the delivery take the lock + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: 0, // give up at once — the degraded path + }); + + expect(await delivery).toEqual({ status: 'preempted' }); + }); + + it('a degraded write that writes NOTHING is not counted as a bypass', async () => { + // The delayed `^C` re-checks liveness INSIDE the lock and can legitimately return without + // writing a byte. Counting that as a bypass would hold and re-deliver a message that + // nothing actually raced — a duplicate charged for a race that never happened (claude + // review of PR #1492). + const c = makeComposer(); + const expired: number[] = []; + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); // let the delivery take the lock + await submitToSession( + c.session.id, + () => 0, // the liveness re-check failed: no ^C, no bytes + undefined, + { + waitCeilingMs: 0, // give up at once — the degraded path + onCeilingExpired: (ms) => expired.push(ms), + wroteBytes: () => false, + }, + ); + + expect(expired).toEqual([0]); // it DID give up waiting, and still says so + expect(unserializedWriteCount(c.session.id)).toBe(0); // but nothing bypassed the line + expect(await delivery).toEqual({ status: 'written' }); // so the delivery is not re-held + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('the degraded-write counter is evicted once the session goes idle', async () => { + // `chains` and `pendingOperators` self-delete when they drain; this counter cannot, because + // it must outlive the submission whose watcher is about to compare against it. Left + // unevicted it would retain one entry per session that ever degraded, for the life of the + // Tower — the leak class #1472 fixed (claude review of PR #1492). + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: 0, + }); + + expect(unserializedWriteCount(c.session.id)).toBe(1); // counted while it still matters + expect(await delivery).toEqual({ status: 'preempted' }); // and the watcher saw it + await sleep(0); // let the chain's drain cleanup run + + expect(bypassCountedSessions()).toBe(0); // then it is gone, with no teardown hook + expect(pendingSubmissionSessions()).toBe(0); + }); + + it('eviction cannot land inside a watcher window and mask a race', async () => { + // The interlock that makes eviction safe: a watch pins the count. If the drain cleanup + // could reset it to 0 between the watcher's two reads, a raced delivery would read + // "nobody bypassed me" and report `written` — the exact false `delivered` this issue + // exists to eliminate. Two deliveries, both raced, both must report preempted. + const a = makeComposer('term-a'); + const b = makeComposer('term-b'); + + const first = deliveryWrite(a.session, MULTILINE); + const second = deliveryWrite(b.session, MULTILINE); + await sleep(5); + await submitToSession(a.session.id, () => { a.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + await submitToSession(b.session.id, () => { b.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + + expect(await first).toEqual({ status: 'preempted' }); + expect(await second).toEqual({ status: 'preempted' }); + await sleep(0); + expect(bypassCountedSessions()).toBe(0); + }); +}); + +/** Wrap a composer as the DeliverySession the delivery path expects. */ +function sessionFor(session: { id: string; write(d: string): boolean }): DeliverySession { + return { + id: session.id, + bytesWritten: 0, + info: { cols: 110, rows: 32 }, + command: 'claude', + launchArgs: [], + cwd: WS, + writable: true, + write: (d: string) => session.write(d), + }; +} + +/** Delivery ports whose write edge is the REAL locked one, so the lock is under test. */ +function livePorts(sessions: Map, log: string[] = []): DeliveryPorts { + return { + getSessionForAgent: (_ws, agent) => sessions.get(agent) ?? null, + resolveProfile: () => PROFILE, + classify: () => Promise.resolve(CLEAN), + writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck), + broadcast: () => {}, + onHeldStateChange: () => {}, + onEscalation: () => {}, + onLiveness: () => {}, + log: (m) => log.push(m), + now: () => 1000, + }; +} + +describe('Issue #1365 — a raced delivery is held, never marked delivered', () => { + let db: Database.Database; + beforeEach(() => { + resetSubmissionChains(); + db = new Database(':memory:'); + db.exec(GLOBAL_SCHEMA); + }); + afterEach(() => db.close()); + + const enqueue = (formattedMessage = MULTILINE) => + mailbox.enqueue(db, { workspacePath: WS, toAgent: AGENT, body: 'hi', formattedMessage }, 1000); + + it('an uncontended delivery still delivers and marks the row', async () => { + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.delivered).toEqual([row.id]); + expect(mailbox.getById(db, row.id)?.status).toBe('delivered'); + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('a delivery racing an interrupt HOLDS the row instead of reporting it delivered', async () => { + // The regression this issue exists to close. Pre-#1365 this row read `delivered` while + // the agent's composer had been cleared — silent loss with a false audit record. + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + // The interrupt takes the terminal first; the delivery must decline, not write into it. + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + const out = await deliverAgentMail(ports, db, WS, AGENT); + await interrupt; + + expect(out).toEqual({ delivered: [], reason: 'busy' }); + const stored = mailbox.getById(db, row.id); + expect(stored?.status).toBe('held'); // still deliverable — nothing was lost + expect(stored?.reason).toBe('busy'); + expect(c.submitted).toEqual(['INT']); // only the operator's message went out + }); + + it('the held row delivers on the next pass, once the terminal is free', async () => { + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + await deliverAgentMail(ports, db, WS, AGENT); // declined + await interrupt; + const out = await deliverAgentMail(ports, db, WS, AGENT); // retried + + expect(out.delivered).toEqual([row.id]); + expect(c.submitted).toEqual(['INT', MULTILINE]); + }); + + it('a row dismissed while the write edge runs is NOT re-held (row-resolved)', async () => { + const c = makeComposer(); + const sessions = new Map([[AGENT, sessionFor(c.session)]]); + const ports = livePorts(sessions); + let heldChanges = 0; + ports.onHeldStateChange = () => { heldChanges++; }; + const row = enqueue(); + + // Dismiss inside the lock, at the write instant — the window the in-lock row-status + // re-check exists to cover. The port relays the delivery module's own precheck. + ports.writeMessage = (session, msg, noEnter, precheck) => + submitMessagePaced(session, msg, noEnter, () => { + mailbox.dismiss(db, row.id, 1001); + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out).toEqual({ delivered: [], reason: null }); // terminal state — not re-held + expect(mailbox.getById(db, row.id)?.status).toBe('dismissed'); + expect(heldChanges).toBeGreaterThan(0); // the indicator was refreshed + expect(c.submitted).toHaveLength(0); // and nothing went on the wire + }); + + it('a session that became unwritable inside the lock holds no-live-pty', async () => { + const c = makeComposer(); + let writable = true; + const session: DeliverySession = { ...sessionFor(c.session), get writable() { return writable; } }; + const ports = livePorts(new Map([[AGENT, session]])); + const row = enqueue(); + + ports.writeMessage = (s, msg, noEnter, precheck) => + submitMessagePaced(s, msg, noEnter, () => { + writable = false; // the shellper socket dies while we hold the lock + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('no-live-pty'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); + expect(c.submitted).toHaveLength(0); + }); + + it('a screen that moved inside the lock holds busy', async () => { + const c = makeComposer(); + let bytes = 10; + const session: DeliverySession = { ...sessionFor(c.session), get bytesWritten() { return bytes; } }; + const ports = livePorts(new Map([[AGENT, session]])); + const row = enqueue(); + + ports.writeMessage = (s, msg, noEnter, precheck) => + submitMessagePaced(s, msg, noEnter, () => { + bytes += 1; // new output landed between the gate and the first byte + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('busy'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); + expect(c.submitted).toHaveLength(0); + }); + + it('a preempted write holds the row and logs why', async () => { + const c = makeComposer(); + const logs: string[] = []; + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]]), logs); + const row = enqueue(); + + ports.writeMessage = async (s, msg, noEnter, precheck) => { + const write = submitMessagePaced(s, msg, noEnter, precheck); + await sleep(5); + await submitToSession(s.id, () => { c.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + return write; + }; + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('busy'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); // redelivered later, never falsely delivered + expect(logs.some((l) => l.includes('unserialized'))).toBe(true); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts index 014fb70b8..f279706cc 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -21,7 +21,7 @@ import { SessionScreen } from '../../terminal/session-screen.js'; // generation); submitToSession lets a test pre-occupy a session's lock to drive the // shutdown-during-lock-wait window deterministically. import { shutdownDelayedSends } from '../servers/delayed-send.js'; -import { submitToSession, resetSubmissionChains } from '../servers/session-submit.js'; +import { submitToSession, trySubmitToSession, resetSubmissionChains } from '../servers/session-submit.js'; // ============================================================================ // Mocks @@ -222,11 +222,17 @@ function makeRes(): { res: http.ServerResponse; body: () => string; statusCode: * requires that proven lower bound, else a bare marker is an indeterminate partial and is held). * `bytesWritten` is the monotone change token the delivery path samples. */ -function gateSession(mockWrite: (data: string) => void, ring: string, writable = true) { +function gateSession(mockWrite: (data: string) => void, ring: string, writable = true, id = 'term-001') { const raw = `${ring}\r\n${'─'.repeat(20)}\r\n`; const gateScreen = new SessionScreen(80, 24); gateScreen.feed(raw); return { + // The per-terminal submission lock's key (Issue #1365). It MUST be a real, distinct id: + // this double reaches the live mailbox-wiring binding, and because the port is + // structurally typed an omitted `id` would compile and silently key every lock on the + // same `undefined` — serialization that looks present and is not. `submitMessagePaced` + // throws on a missing id so that mistake fails loudly instead of quietly. + id, // Model a live PTY: every write lands. The delivery path now threads the write's // boolean (Spec 1313 silent-loss fix), so a double whose write returned undefined // would read as a DROPPED write and be held. Wrap mockWrite so call-assertions still @@ -1724,6 +1730,41 @@ describe('tower-routes', () => { // Message SHOULD be written — user is idle (Bugfix #492) expect(mockWrite).toHaveBeenCalled(); }); + + it('a body-bearing interrupt that crosses the wait ceiling reports degraded (Issue #1365)', async () => { + // codex review of PR #1492: the interrupt claims its mailbox row `delivered` BEFORE the + // write (un-claiming would risk a double delivery), so if the ceiling expires and the + // write goes out unserialized — possibly interleaving with the delivery it skipped — an + // unqualified `delivered: true` would be a lie of omission. The response must say so. + mockParseJsonBody.mockResolvedValue({ + to: 'architect', message: 'urgent', workspace: '/tmp/ws', options: { interrupt: true }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-ceiling', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => gateSession(mockWrite, '❯ ', true, 'term-ceiling'), + listSessions: () => [], + }); + + // Occupy the terminal with a DELIVERY write long enough to outlast the ceiling. A + // delivery is the only thing the ceiling is allowed to bypass. + const holder = trySubmitToSession('term-ceiling', () => 4000); + + const req = makeReq('POST', '/api/send'); + const ctx = makeCtx(); + const { res, statusCode, body } = makeRes(); + await handleRequest(req, res, ctx); + + expect(statusCode()).toBe(200); + const parsed = JSON.parse(body()); + expect(parsed.delivered).toBe(true); // claim-first is preserved... + expect(parsed.degraded).toBe(true); // ...but the sender is told it was not serialized + expect(parsed.degradedReason).toBe('submit-wait-ceiling-expired'); + expect(mockWrite).toHaveBeenCalled(); // the escape hatch still landed + await holder; + }, 10_000); }); // ========================================================================== diff --git a/packages/codev/src/agent-farm/commands/send.ts b/packages/codev/src/agent-farm/commands/send.ts index 6be2cbd6f..a58cc9625 100644 --- a/packages/codev/src/agent-farm/commands/send.ts +++ b/packages/codev/src/agent-farm/commands/send.ts @@ -385,6 +385,18 @@ export async function send(options: SendOptions): Promise { ); } else { logger.success(`Message delivered to ${result.resolvedTo ?? target}`); + // Issue #1365: an interrupt/escape that gave up waiting for the terminal's submission + // lock wrote unserialized, so its bytes may have interleaved with the delivery it + // skipped. The row is claimed `delivered` before the write (un-claiming would risk a + // double delivery), so without this the sender would read an unqualified success for a + // possibly-mangled body. Warn rather than fail: the write did happen. + if (result.degraded) { + logger.warn( + `...but it was NOT serialized against a write already in flight on that terminal ` + + `(${result.degradedReason ?? 'wait ceiling expired'}), so it may have interleaved. ` + + `Check the agent's prompt before assuming it read cleanly.`, + ); + } } } catch (error) { fatal(error instanceof Error ? error.message : String(error)); diff --git a/packages/codev/src/agent-farm/servers/mailbox-delivery.ts b/packages/codev/src/agent-farm/servers/mailbox-delivery.ts index 096bf4447..1b3cc54c7 100644 --- a/packages/codev/src/agent-farm/servers/mailbox-delivery.ts +++ b/packages/codev/src/agent-farm/servers/mailbox-delivery.ts @@ -12,9 +12,16 @@ * TWO deliberate exceptions write a body OUTSIDE this path, both explicit human * gate-bypasses documented at their `tower-routes.ts` call sites: immediate `--interrupt` * (Ctrl+C then the message) and `--escape` (a bare ESC). They are the operator's "I am at - * this terminal now" actions and take the separate per-terminal submission lock - * (`session-submit.ts`), not the per-agent serializer here — every autonomous/scheduled/ - * held send, by contrast, delivers through this gate. + * this terminal now" actions — every autonomous/scheduled/held send, by contrast, delivers + * through this gate. + * + * They are no longer on a DISJOINT lock, though (Issue #1365). This path's write edge takes + * the same per-terminal submission lock (`session-submit.ts`) those bypasses take, as a leaf + * inside the per-agent serializer below — so a `^C`/ESC can no longer land inside a + * delivery's own text→Enter window, clear or truncate the composer, and leave the row marked + * `delivered` for a message the agent never saw whole. Lock order is always per-agent → + * per-terminal; see `session-submit.ts` for the full boundary, including what the lock does + * NOT cover. * * This module replaces the in-memory `SendBuffer` (retired in this phase): held * messages now live in the durable `mailbox` table, so nothing is lost to a Tower @@ -41,6 +48,7 @@ import { } from '../db/mailbox.js'; import type { DbMailbox, MailboxReason } from '../db/types.js'; import type { GateProfile, GateVerdict } from './render-gate.js'; +import type { PacedSubmitResult } from './message-write.js'; import { KeyedSerializer } from './write-queue.js'; /** @@ -50,6 +58,13 @@ import { KeyedSerializer } from './write-queue.js'; * imports the terminal layer. */ export interface DeliverySession { + /** + * The live terminal/session id — the key of the per-terminal submission lock the write + * edge takes (Issue #1365). `PtySession` already carries it; a test double MUST supply a + * real, distinct one, and {@link submitMessagePaced} throws if it is missing rather than + * letting every lock collapse onto a single `undefined` key. + */ + readonly id: string; /** * The session's MONOTONE cumulative output-byte counter (Spec 1313 render-gate round 2) — * the gate's change token. It advances on ANY new output and NEVER decreases, so two samples @@ -79,6 +94,19 @@ export interface DeliverySession { write(data: string): boolean; } +/** + * Why a gated write abandoned inside the per-terminal lock (Issue #1365), decided by + * {@link deliverAgentMail}'s precheck at the write instant rather than before the lock. + */ +export type WriteAbort = + /** Re-hold the row for this reason and retry on a later clean pass. */ + | { kind: 'hold'; reason: MailboxReason } + /** The row was dismissed/superseded under us — a terminal state, so it must NOT be re-held. */ + | { kind: 'row-resolved' }; + +/** Outcome of the gated write edge. See {@link PacedSubmitResult}. */ +export type WriteResult = PacedSubmitResult; + /** Broadcast frame for a delivered message (the dashboard/inbox message event). */ export interface DeliveredBroadcast { type: 'message'; @@ -104,17 +132,29 @@ export interface DeliveryPorts { */ classify(session: DeliverySession, profile: GateProfile): Promise; /** - * Write a formatted message (text + Enter, unless `noEnter`) to the session and - * report whether every byte reached the terminal. Resolves `true` when the paced - * write — including the trailing Enter — has fully completed; `false` when any - * write was dropped (#1198: a shellper socket that died mid-pace). The delivery - * `await`s it for two reasons: (1) completion chaining — the per-agent serializer - * holds the line until the submit is entirely on the wire, so the next delivery - * never starts mid-write; (2) the boolean gates markDelivered — a dropped write - * holds the row (`no-live-pty`) instead of falsely reporting delivery (Spec 1313 - * integration review — the silent-loss finding). + * Write a formatted message (text + Enter, unless `noEnter`) to the session as ONE + * submission on the session's per-terminal lock, and report what actually happened. + * Resolves only once the paced write — trailing Enter included — has completed. + * + * The delivery `await`s it for two reasons: (1) completion chaining — the per-agent + * serializer holds the line until the submit is entirely on the wire, so the next + * delivery never starts mid-write; (2) the result gates markDelivered — anything but + * `written` must never be reported as delivered (Spec 1313 integration review — the + * silent-loss finding; Issue #1365 extended the same rule to in-lock refusals). + * + * `precheck` is invoked by the binding INSIDE the lock, immediately before the first + * byte, and returning non-null aborts with nothing written. It exists because taking + * the lock alone would only move the race: a delivery that classified a clean screen + * and then waited behind another submission would write onto the screen that + * submission just changed. All of the reason authority stays here in the delivery + * module — the binding only relays the verdict. */ - writeMessage(session: DeliverySession, formattedMessage: string, noEnter: boolean): boolean | Promise; + writeMessage( + session: DeliverySession, + formattedMessage: string, + noEnter: boolean, + precheck: () => WriteAbort | null, + ): WriteResult | Promise; /** Emit the delivered-message broadcast frame. */ broadcast(frame: DeliveredBroadcast): void; /** @@ -423,11 +463,32 @@ export async function deliverAgentMail( // delivered off the paced-write timer. if (!session.writable) return hold('no-live-pty'); - // Default false so an unobserved result is the SAFE failure mode (hold, never a false - // delivery); the try either assigns the real boolean or throws past this point. - let written = false; + // Re-validate EVERYTHING at the write instant, inside the per-terminal lock (Issue #1365). + // The three checks above (screen unchanged, session writable, row still held) were made + // before the lock; between them and the first byte another writer may hold the terminal, + // and this delivery may have waited. Without this, routing the write through the lock would + // merely relocate the race it exists to close: a delivery could write onto a screen that an + // `--interrupt`/`--escape` had just cleared. Cheap enough to repeat — a synchronous ring + // read and one indexed better-sqlite3 lookup. + // + // The residual it CANNOT close: `ringToken` counts OUTPUT bytes, so input written by a + // path that does not take this lock (the raw `/api/terminals/:id/write` passthrough, or a + // human's keystrokes over the WebSocket) can sit un-echoed on the line and read as + // unchanged. Serialization — not this precheck — is what makes the lock-taking writers + // safe; the echo-lag residual for the rest is #1473's territory. + const precheck = (): WriteAbort | null => { + if (!session.writable) return { kind: 'hold', reason: 'no-live-pty' }; + if (ringToken(session, profile) !== tokenBefore) return { kind: 'hold', reason: 'busy' }; + const stillHeld = getById(db, row.id); + if (!stillHeld || stillHeld.status !== 'held') return { kind: 'row-resolved' }; + return null; + }; + + // Default to a hold so an unobserved result is the SAFE failure mode (hold, never a false + // delivery); the try either assigns the real result or throws past this point. + let result: WriteResult = { status: 'aborted', abort: { kind: 'hold', reason: 'busy' } }; try { - written = await ports.writeMessage(session, current.formatted_message, current.no_enter === 1); + result = await ports.writeMessage(session, current.formatted_message, current.no_enter === 1, precheck); } finally { // Invalidate the memo on EVERY write outcome — a clean `true`, a dropped-write `false`, OR a // rejection — and BEFORE the markDelivered/held decisions below (CMAP round 3 moved it above the @@ -447,15 +508,46 @@ export async function deliverAgentMail( memo?.delete(cacheKey); } - // A dropped PTY write (#1198) means zero-or-partial bytes reached the terminal — the exact silent - // loss this spec exists to prevent (Spec 1313 integration review — Codex). The t=0 `writable` - // precheck above cannot catch a socket that dies mid-pace (the text/lines/Enter fire across - // setTimeout gaps), so writeMessage threads the per-write result: `false` → no complete submit - // landed. Hold the row (`no-live-pty`, retried on the next clean gate pass) instead of marking it - // delivered. Any bytes already on the wire only make the line dirty; the render gate then holds on - // that draft until the session recovers or is torn down — it can never be marked delivered on a - // dead PTY. - if (!written) return hold('no-live-pty'); + // Anything short of a complete submit holds the row — a delivery is marked delivered only when + // every byte, Enter included, reached the terminal. + // + // • `dropped` — a PTY write was dropped (#1198): zero-or-partial bytes reached the terminal, + // the exact silent loss this spec exists to prevent (Spec 1313 integration review — Codex). + // The t=0 `writable` precheck cannot catch a socket that dies mid-pace (the text/lines/Enter + // fire across setTimeout gaps). Any bytes already on the wire only make the line dirty; the + // render gate then holds on that draft until the session recovers or is torn down. + // • `contended` — another submission (an `--interrupt`/`--escape`, or a delivery to an agent + // sharing this terminal) held the lock, so nothing was written. `busy` is the honest reason: + // the line is occupied. Declining rather than queueing is deliberate — see + // {@link trySubmitToSession}: the drainer walks agents sequentially, so a blocking wait here + // would stall every OTHER agent's delivery behind this one terminal. + // • `aborted` — the in-lock precheck refused, with nothing written. A `hold` abort re-holds for + // the stated reason; `row-resolved` means the row was dismissed/superseded while we waited, + // which is a TERMINAL state and must not be re-held (same handling as the pre-lock check + // above, which this one backstops for the duration of the lock wait). + // • `preempted` — the bytes went out, but an operator submission whose wait ceiling expired + // wrote unserialized while they did, so the composer may have been cleared or truncated + // under them. Hold rather than mark delivered. This trades a possible DUPLICATE (if the + // message did land intact, the gate re-delivers it later) for never reporting a delivery + // that did not happen — the same call the `dropped` branch already makes, and the failure + // this whole issue exists to remove. It is the one hole the ceiling opens, and it is + // detected by counting lock bypasses, not by re-reading the screen. + if (result.status === 'dropped') return hold('no-live-pty'); + if (result.status === 'preempted') { + ports.log( + `[mailbox] write to ${toAgent} @ ${path.basename(workspacePath)} was raced by an unserialized ` + + `operator write — holding ${row.id.slice(0, 8)}… for redelivery rather than reporting it delivered`, + ); + return hold('busy'); + } + if (result.status === 'contended') return hold('busy'); + if (result.status === 'aborted') { + if (result.abort.kind === 'row-resolved') { + ports.onHeldStateChange(); // the held set changed under us → refresh the indicator + return { delivered: [], reason: null }; + } + return hold(result.abort.reason); + } // markDelivered is guarded (held→delivered only). If it did NOT transition, the row // was dismissed/superseded during the paced write — accept that terminal state and diff --git a/packages/codev/src/agent-farm/servers/mailbox-wiring.ts b/packages/codev/src/agent-farm/servers/mailbox-wiring.ts index 9a4a0c5b5..f6af52682 100644 --- a/packages/codev/src/agent-farm/servers/mailbox-wiring.ts +++ b/packages/codev/src/agent-farm/servers/mailbox-wiring.ts @@ -18,7 +18,7 @@ import { loadConfig } from '../../lib/config.js'; import { terminalDeliverySignals, type PtySession } from '../../terminal/pty-session.js'; import { getWorkspaceTerminals, getTerminalManager } from './tower-terminals.js'; import { broadcastMessage, resolveAgentInRegistry, isResolveError } from './tower-messages.js'; -import { writeMessagePaced } from './message-write.js'; +import { submitMessagePaced } from './message-write.js'; import { classifyBuffer, type GateProfile, type GateVerdict } from './render-gate.js'; import { resolveProfile } from './gate-profiles.js'; import { harnessFromLaunchScript, type ContextFsPort } from '../commands/reset/context.js'; @@ -212,7 +212,11 @@ export function makeDeliveryPorts(log: LogFn): DeliveryPorts { getSessionForAgent: (ws, agent) => resolveLiveSessionForAgent(ws, agent), resolveProfile: (session) => resolveProfileForSession(session), classify: (session, profile) => classifyAgentScreen(session, profile), - writeMessage: (session, msg, noEnter) => writeMessagePaced(session, msg, noEnter), + // Issue #1365: the write edge takes the session's per-terminal submission lock as a + // LEAF inside the per-agent serializer, so a gated delivery and a concurrent + // `--interrupt`/`--escape` can no longer interleave. The precheck is the delivery + // module's, re-run inside that lock. + writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck), broadcast: (frame) => broadcastDelivered(frame), onHeldStateChange: () => broadcastHeldStateChange(), onEscalation: (info) => broadcastEscalation(info), diff --git a/packages/codev/src/agent-farm/servers/message-write.ts b/packages/codev/src/agent-farm/servers/message-write.ts index e19f927fa..57ca118de 100644 --- a/packages/codev/src/agent-farm/servers/message-write.ts +++ b/packages/codev/src/agent-farm/servers/message-write.ts @@ -5,12 +5,14 @@ * tower-routes.ts and tower-cron.ts. */ +import { trySubmitToSession, watchBypasses, type SubmitClock } from './session-submit.js'; + /** Minimal writable session interface — avoids coupling to PtySession. */ export interface WritableSession { /** * Write input to the underlying PTY. Returns `false` when the write was dropped * (#1198: a shellper-backed session whose socket has died still reports status - * 'running', yet its writes silently no-op). {@link writeMessagePaced} threads this + * 'running', yet its writes silently no-op). {@link submitMessagePaced} threads this * boolean so a mailbox delivery whose bytes never reached the terminal is held, not * marked delivered (Spec 1313 integration review — the silent-loss finding). */ @@ -111,36 +113,106 @@ export function writeMessageToSession( } /** - * Paced write of a message (text + trailing Enter unless `noEnter`) that reports - * whether every byte reached the PTY. Resolves `true` when the whole submit landed, - * `false` when ANY scheduled write was dropped (#1198: a shellper socket that died - * mid-pace). This is the delivery layer's authoritative success signal — a mailbox - * delivery holds a row whose bytes never made it instead of marking it delivered - * (Spec 1313 integration review — the silent-loss finding). + * Outcome of a {@link submitMessagePaced} attempt. Generic in the caller's own abort + * vocabulary so this module stays free of mailbox concepts — the delivery layer + * instantiates `A` with its hold reasons. + */ +export type PacedSubmitResult = + /** The whole submit — text and, unless `noEnter`, the trailing Enter — reached the PTY. */ + | { status: 'written' } + /** #1198: a scheduled write was dropped mid-pace (the shellper socket died). */ + | { status: 'dropped' } + /** + * The bytes went out, but an operator submission bypassed the lock while they did + * (the ceiling-expired degraded path), so this submit cannot be trusted to have + * landed intact. + */ + | { status: 'preempted' } + /** Another submission held the terminal. NOTHING was written; the caller may retry later. */ + | { status: 'contended' } + /** The caller's in-lock precheck refused. NOTHING was written. */ + | { status: 'aborted'; abort: A }; + +/** + * Paced write of a message (text + trailing Enter unless `noEnter`) performed as ONE + * submission on the session's per-terminal lock (Issue #1365). + * + * This is the mailbox delivery path's write edge. Before #1365 it wrote directly, under + * the per-agent serializer only, so a gated delivery could interleave with a concurrent + * `--interrupt`/`--escape` on the same terminal: a `^C` or ESC landing between the text + * and its Enter cleared or truncated the composer while every byte still reported + * success, and the row was marked `delivered` for a message the agent never saw whole. + * Taking the same lock those paths take is what makes that impossible. * - * `writeMessageToSession` fires the text, any subsequent lines, and the trailing - * Enter across `setTimeout` gaps (10–130ms+), and a t=0 `writable` precheck cannot - * see a socket that dies *during* that sequence. So wrap the session and record - * whether any of those writes returned false. The returned promise resolves at the - * final scheduled offset (`doneMs`); `writeMessageToSession` registers the Enter's - * `setTimeout` at that same offset *before* this resolve is scheduled, so the Enter - * executes first and its result is observed by resolution time. + * Two properties are load-bearing and easy to lose in a refactor: * - * Awaiting the promise is also what makes the per-agent write serializer's - * completion-chaining real — the next delivery cannot begin until this submit - * (Enter included) is entirely on the wire. + * - **`precheck` runs INSIDE the lock**, immediately before the first byte. Acquiring + * the lock without it would merely move the race: a delivery that classified a clean + * screen, then waited behind another submission, would write onto the screen that + * submission just changed. Returning non-null aborts with nothing written. + * - **Contention is declined, not queued** (see {@link trySubmitToSession}). The gated + * delivery path must never block, because the drainer walks agents sequentially. + * + * The completion semantics callers depend on are unchanged from the pre-#1365 + * `writeMessagePaced`: the returned promise resolves only after the trailing Enter has + * been written. `writeMessageToSession` registers the Enter's `setTimeout` before + * `submitToSession` schedules its own equal-offset sleep, so the Enter still executes + * first — which is what makes the per-agent serializer's completion-chaining real. */ -export function writeMessagePaced( - session: WritableSession, message: string, noEnter: boolean, -): Promise { - let delivered = true; - const tracked: WritableSession = { - write: (data: string): boolean => { - const ok = session.write(data); - if (!ok) delivered = false; - return ok; - }, - }; - const doneMs = writeMessageToSession(tracked, message, noEnter); - return new Promise((resolve) => setTimeout(() => resolve(delivered), doneMs)); +export async function submitMessagePaced( + session: WritableSession & { id: string }, + message: string, + noEnter: boolean, + precheck: () => A | null, + clock?: SubmitClock, +): Promise> { + // Fail LOUD on a missing id rather than keying the lock on `undefined`. Sessions reach + // this through structurally-typed ports, so a double without an id compiles fine and + // would silently collapse every per-terminal lock into one global lock — serialization + // that looks present and is not. A throw here surfaces as a held row (the delivery path + // never marks a row delivered on a throw), which is the safe failure. + if (typeof session.id !== 'string' || session.id === '') { + throw new Error('submitMessagePaced: session.id must be a non-empty string (the per-terminal lock key)'); + } + + // The one thing the lock cannot stop is an operator submission whose wait ceiling expired + // and wrote anyway. Watch the session's degraded-write counter across our own submission: + // a bump means a `^C`/ESC bypassed us mid-write, so the composer may have been cleared or + // truncated under our bytes. Cheaper and more direct than re-classifying the screen — and it + // is the difference between re-holding the row and falsely reporting a delivery, which is + // the whole point of Issue #1365. The watch also pins the counter against eviction for + // exactly as long as we need to compare it; `finally` is what keeps that pin from leaking. + const bypasses = watchBypasses(session.id); + try { + let delivered = true; + let abort: A | null = null; + const tracked: WritableSession = { + write: (data: string): boolean => { + const ok = session.write(data); + if (!ok) delivered = false; + return ok; + }, + }; + + const ran = await trySubmitToSession( + session.id, + () => { + abort = precheck(); + if (abort !== null) return 0; // refused in-lock: not one byte goes out + return writeMessageToSession(tracked, message, noEnter); + }, + clock, + ); + + if (!ran) return { status: 'contended' }; + // Read through a cast: both flags are assigned inside the callback above, which + // TypeScript's flow analysis does not track back to this scope. + const refused = abort as A | null; + if (refused !== null) return { status: 'aborted', abort: refused }; + if (!(delivered as boolean)) return { status: 'dropped' }; + if (bypasses.raced()) return { status: 'preempted' }; + return { status: 'written' }; + } finally { + bypasses.release(); + } } diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index 6dbc50aad..00282ec9b 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -41,38 +41,112 @@ * * ## Exactly what it covers — this is NOT blanket per-session atomicity * - * A lock only serialises writers that take it. Currently that is the `escape` - * and `interrupt` paths of `/api/send`. Every other PTY writer still writes - * directly, and it is worth being precise about why: - * - * - The mailbox delivery path (`deliverAgentMailSerialized`, Spec 1313) — every - * normal `/api/send` AND every cron notification (Phase 6 rerouted cron here; the - * old blind `writeMessageToSession` is gone). NOT covered by this lock, and does not - * need it: it runs its OWN per-agent write serializer that completion-chains each - * delivery on the prior one's paced write (text + Enter), so two mailbox deliveries - * to one agent cannot interleave. That is a disjoint lock from this per-session one, - * so a mailbox delivery is not serialised against a concurrent `escape`/`interrupt` - * here — but a normal mailbox delivery only ever writes onto a render-gate-verified - * empty prompt, and `interrupt` is the explicit gate-bypassing human action (see - * tower-routes.ts), so that residual cross-path race is an accepted, documented - * boundary, not a regression this lock must close. - * - `POST /api/terminals/:id/write` — NOT covered. It is a raw passthrough - * with no Enter semantics of its own. - * - `tower-websocket.ts` keystrokes and the shellper frame relay — DELIBERATELY - * not covered. That is a human typing into their own terminal; serialising - * it behind an agent's message would make the UI feel stuck, and the human - * is the composer's owner. - * - * So the guarantee is: **two lock-taking `/api/send` submissions (escape/interrupt) - * to one session cannot interleave**, which is the failure that reached production. - * Anything stronger requires the remaining writers to take the lock too. + * A lock only serialises writers that take it. As of Issue #1365 that is every writer + * that puts a *message* on a terminal: + * + * - `escape` and `interrupt` on `/api/send`, and the delayed `--interrupt` `^C` + * (`tower-routes.ts`) — the explicit human gate-bypasses. + * - The mailbox delivery path (`deliverAgentMailSerialized`, Spec 1313) — every normal + * `/api/send`, every cron notification, every backstop drain. It takes this lock as a + * LEAF inside its own per-agent serializer, via `submitMessagePaced`. + * + * These stay deliberately uncovered: + * + * - `POST /api/terminals/:id/write` — a raw passthrough with no Enter semantics of its own. + * - `tower-websocket.ts` keystrokes and the shellper frame relay — a human typing into + * their own terminal. Serialising that behind an agent's message would make the UI feel + * stuck, and the human is the composer's owner. + * + * ### Why the mailbox path was converged (Issue #1365) + * + * Until #1365 the delivery path held only its per-AGENT serializer — a disjoint lock — and + * the resulting cross-path race was accepted on the grounds that a gated delivery only ever + * writes onto a render-verified empty prompt and `interrupt` is an explicit human action. + * That reasoning covered one ordering (a delivery landing inside the interrupt's `^C`→settle + * →text window) and missed two: + * + * - a `^C` landing inside the DELIVERY's own text→Enter window (50–130 ms+) cleared the + * composer, so the delivery's Enter submitted nothing — yet every byte had reached the + * PTY, so the write reported success and the row was marked `delivered`. Silent loss with + * a false audit record, which is the one outcome Spec 1313 exists to exclude; + * - `--escape` (ESC, then Enter 50 ms later) produced the truncated variant, and is the + * MORE likely trigger for a multi-line body, because the delivery's exposed window is + * longest exactly when the body is long. + * + * The "a human is standing at this terminal" premise also does not hold for the DELAYED + * `^C`, which fires unattended on a timer. + * + * ### Lock order, and why there is no cycle + * + * Always per-agent → per-terminal. The delivery path takes this lock as a leaf inside + * `KeyedSerializer`; the operator paths take only this one and never enter the per-agent + * serializer. Re-entrancy is impossible in the other direction too: `PtySession.write()` + * emits no `'submit'` signal (only `handleUserInput` does, for human keystrokes), so no + * write from inside a lock can schedule a delivery synchronously. + * + * The lock is a leaf around the WRITE only, never the gate classify. `--interrupt` is the + * human's escape hatch for a wedged agent; making it queue behind a screen classification + * would be a real regression for the one action that must always get through. + * + * ### Asymmetric acquisition: deliveries decline, operators wait + * + * A delivery uses {@link trySubmitToSession} and abandons its turn on contention, because + * `MailboxDrainer.tick` walks agents SEQUENTIALLY — one delivery parked on a terminal lock + * would stall every other agent's mail, plus that tick's escalation and prune passes. It + * costs nothing: a contended terminal means the delivery's in-lock precheck would have + * aborted it anyway, and the row simply re-delivers on the next clean pass. + * + * Operators block — bounded by {@link OPERATOR_SUBMIT_WAIT_CEILING_MS}, because a paced + * write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB. + * + * The ceiling may only ever bypass a DELIVERY write, never another operator (codex review of + * PR #1492). Operator-vs-operator was ALWAYS fully serialized before #1365 — `submitToSession` + * had no ceiling — so a ceiling that could skip a queued or in-flight operator would make that + * one pair strictly WORSE than the old behaviour, which is the opposite of the point. The + * {@link SubmissionKind} tag plus the pending-operator count is what keeps the guarantee true + * per pair: + * + * - operator vs operator — fully serialized, unbounded wait, exactly as before #1365; + * - operator vs delivery — serialized under the ceiling, and above it degraded to the + * pre-#1365 behaviour (two disjoint locks, i.e. no serialization at all), so never worse; + * - delivery vs delivery — the per-agent serializer, unchanged, plus a declined contention. + * + * ### What is guaranteed, and what is not + * + * **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a + * terminal while another lock-taking writer's submission is in flight. That is a property of + * the lock, not of any check. + * + * The delivery path ALSO re-validates its preconditions inside the lock (`writable`, the + * gate's `ringToken`, and the row's own status). That re-check narrows a window; it does not + * close one, and it should not be described as if it did: + * + * - `ringToken` counts OUTPUT bytes, so input written by an uncovered path (the raw + * passthrough, or a human's keystrokes) can sit un-echoed on the line and read as + * unchanged. That echo-lag residual survives this change by design — it is #1473's + * territory. + * - Its real structural value is that it makes the ACQUISITION POLICY a free choice: were + * the delivery ever switched from declining to waiting (e.g. to order an interrupt ahead + * of its own body, per #1481), the precheck is what would keep that safe. + * + * The one hole this lock opens is its own degraded path: an operator whose ceiling expires + * writes unserialized. Rather than leave that as a second silent-loss route, degraded writes + * are COUNTED per session ({@link unserializedWriteCount}); `submitMessagePaced` holds a + * {@link watchBypasses} watch across its write and reports `preempted`, so the delivery holds + * its row for redelivery instead of reporting a delivery that may have been clobbered. + * Deliberately no screen re-classification — the question is only "did anyone bypass the lock + * while I held it?", and a counter answers exactly that. The count is BYTES, not intent: a + * degraded write whose callback declines to write anything (the delayed `^C` re-checks + * liveness inside the lock) bumps nothing, because there is nothing for a delivery to have + * been raced by. */ /** * Tail of the in-flight submission chain per session. * * A session's entry is deleted once its chain drains, so this cannot grow - * without bound across a long-lived Tower. + * without bound across a long-lived Tower. Its presence is also the contention + * signal both {@link isSubmissionInFlight} and {@link trySubmitToSession} read. */ const chains = new Map>(); @@ -85,6 +159,181 @@ const realClock: SubmitClock = { sleep: (ms: number) => new Promise(resolve => setTimeout(resolve, ms)), }; +/** + * What kind of writer a submission is (Issue #1365, codex review). + * + * Load-bearing for the wait ceiling: the ceiling may only ever let a writer bypass a + * DELIVERY write, never another operator. See {@link SubmitOptions.waitCeilingMs}. + */ +export type SubmissionKind = 'operator' | 'delivery-write'; + +/** Options for an operator submission that must not wait unboundedly (Issue #1365). */ +export interface SubmitOptions { + /** + * What this submission is. Defaults to `operator` — every pre-#1365 caller of this + * function is one, and defaulting to the kind that is never bypassable is the safe way + * round. + */ + kind?: SubmissionKind; + /** + * Max ms to wait for an in-flight submission before proceeding UNSERIALIZED. + * Omitted (the default) means wait as long as it takes. + * + * **Only armed when nothing ahead of us is an operator submission.** Two operator + * submissions to one terminal were ALWAYS fully serialized before #1365, and a ceiling + * that could bypass an operator would make this pair strictly worse than that — the one + * corner where "never worse than the old status quo" would otherwise be false. The + * ceiling exists to keep the escape hatch responsive against a long DELIVERY, which is + * the only thing it may skip. + */ + waitCeilingMs?: number; + /** Called instead of the write's serialization when {@link waitCeilingMs} expires. */ + onCeilingExpired?: (waitedMs: number) => void; + /** + * Consulted immediately after the write callback returns, on the DEGRADED path only: + * did the write actually put bytes on the terminal? Defaults to yes. + * + * The delayed `^C` re-checks `isStillLive()` and `writable` INSIDE the lock and can + * legitimately write nothing. Counting that as a bypass would make a concurrent delivery + * report `preempted` and re-deliver — a duplicate charged for a race that never happened. + * {@link unserializedWriteCount} answers "did bytes bypass the lock while I held it?", so + * only bytes may bump it. + */ + wroteBytes?: () => boolean; +} + +/** Marker resolved by the ceiling timer so the race can tell who won. */ +const CEILING_EXPIRED = Symbol('ceiling-expired'); + +/** + * How long an OPERATOR submission (`--interrupt`, `--escape`, the delayed `^C`) waits for an + * in-flight submission before proceeding unserialized (Issue #1365). + * + * Needed because a paced write's duration is `(lines−1)×10+80` ms and a request body is capped + * only by `parseJsonBody`'s 1 MiB, so a 48 KB `--file` of short lines is ~8 minutes on the wire. + * Blocking `--interrupt` — the human's escape hatch for a wedged agent — behind that would be a + * worse regression than the interleaving this lock closes. Two seconds comfortably covers every + * realistic message while keeping the escape hatch responsive. + * + * It applies ONLY against a delivery write; behind another operator the wait stays unbounded + * (see {@link SubmitOptions.waitCeilingMs}). And when it does expire, the caller must SAY so — + * `/api/send` reports `degraded: true` — because a degraded operator write may interleave with + * the write it skipped, and the interrupt path has already claimed its row `delivered`. + */ +export const OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000; + +/** + * Per-session count of submissions that gave up waiting and wrote UNSERIALIZED + * (Issue #1365 — the {@link OPERATOR_SUBMIT_WAIT_CEILING_MS} degraded path). + * + * A monotone counter, not a flag, so a concurrent writer can sample it before and after + * its own write and detect a race that started *and* finished in between. Entries are + * per session id and only ever created on the degraded path, which is rare by + * construction — it needs a write long enough to hold the line past the ceiling AND a + * concurrent operator action. + * + * Evicted by {@link evictBypassCountIfIdle} once the session has no submission in flight and + * no {@link watchBypasses} watch outstanding, so a long-lived Tower does not retain one entry + * per session that ever degraded (claude review of PR #1492 — the leak class #1472 fixed). + * Unlike {@link chains} it cannot simply self-delete on drain: it must outlive the submission + * whose watcher is about to compare against it. + */ +const unserializedWrites = new Map(); + +/** + * Open {@link watchBypasses} watches per session — the interlock that makes eviction safe. + * + * A watcher compares the counter before and after its own write. Resetting the counter to 0 + * between those two reads would read as "nobody raced me", which is exactly the false + * `delivered` this whole issue exists to eliminate. So eviction is refused while a watch is + * open, and re-attempted when the last one closes. + */ +const bypassWatchers = new Map(); + +/** A live comparison window over a session's degraded-write count. See {@link watchBypasses}. */ +export interface BypassWatch { + /** Did a degraded write put bytes on this terminal since the watch opened? */ + raced(): boolean; + /** Close the watch. Idempotent; call it from a `finally` so no path leaks a watcher. */ + release(): void; +} + +/** + * Watch a session for degraded (ceiling-bypassing) writes across your own write. + * + * Open before the first byte, {@link BypassWatch.raced} after the last, and + * {@link BypassWatch.release} in a `finally`. Holding the watch is what pins the underlying + * count in place: without it the entry may be evicted the moment the session goes idle, and + * a reset between the two reads would look like "no race". + */ +export function watchBypasses(sessionId: string): BypassWatch { + const before = unserializedWrites.get(sessionId) ?? 0; + bypassWatchers.set(sessionId, (bypassWatchers.get(sessionId) ?? 0) + 1); + let released = false; + return { + raced: () => (unserializedWrites.get(sessionId) ?? 0) !== before, + release: () => { + if (released) return; // idempotent: a `finally` may run after an explicit release + released = true; + const remaining = (bypassWatchers.get(sessionId) ?? 1) - 1; + if (remaining > 0) { + bypassWatchers.set(sessionId, remaining); + } else { + bypassWatchers.delete(sessionId); + evictBypassCountIfIdle(sessionId); + } + }, + }; +} + +/** + * Drop a session's degraded-write count once nothing can still be comparing against it. + * + * Called from BOTH ends of the interlock — the chain's drain cleanup and the last watch's + * release — so whichever happens second is the one that evicts, and neither ordering leaks. + * Requires no session-teardown hook: a session with no chain and no watcher is quiescent by + * definition, and the next degraded write simply re-creates the entry at 0. + */ +function evictBypassCountIfIdle(sessionId: string): void { + if (bypassWatchers.has(sessionId)) return; // a comparison window is open — resetting would lie + if (chains.has(sessionId)) return; // a submission is live and could still degrade + unserializedWrites.delete(sessionId); +} + +/** + * Per-session count of OPERATOR submissions queued or in flight (Issue #1365, codex review). + * + * The ceiling consults this before arming: while any operator is ahead of us — running OR + * merely queued — we wait as long as it takes, exactly as every submission did before the + * ceiling existed. A queued operator counts because bypassing one that has not started yet + * is the same violation as bypassing one mid-write. + */ +const pendingOperators = new Map(); + +/** + * How many unserialized (ceiling-expired) writes this session has seen. + * + * The point of comparison for a writer that wants to know whether its own submission was + * raced: sample before the first byte, compare after the last. Cheaper and far more + * direct than re-classifying the screen — it asks "did anyone bypass the lock while I + * held it?", which is exactly the question, and needs no terminal rendering at all. + */ +export function unserializedWriteCount(sessionId: string): number { + return unserializedWrites.get(sessionId) ?? 0; +} + +/** + * Whether a submission is queued or in flight for this session RIGHT NOW. + * + * Read by {@link trySubmitToSession} and exposed for telemetry. Safe to act on + * without a lock of its own: the runtime is single-threaded and every mutation of + * `chains` happens with no await in between, so a caller that observes `false` + * cannot be beaten to the install by another caller in the same tick. + */ +export function isSubmissionInFlight(sessionId: string): boolean { + return chains.has(sessionId); +} + /** * Run a write against a session so that it completes before any other * submission to the same session begins. @@ -94,52 +343,162 @@ const realClock: SubmitClock = { * keystroke (the Enter) has been written. This is exactly what * `writeMessageToSession` / `writeEscapeToSession` already * return, so callers pass them through unchanged. + * @param clock injectable sleeper; real timers by default + * @param options {@link SubmitOptions} — the operator paths pass a wait ceiling * @returns resolves once the submission is complete */ export function submitToSession( sessionId: string, write: () => number, clock: SubmitClock = realClock, + options: SubmitOptions = {}, ): Promise { const previous = chains.get(sessionId) ?? Promise.resolve(); + // A failed predecessor must not poison the chain — the next submission is a + // separate message and is still entitled to run. + const previousSettled = previous.then( + () => undefined, + () => undefined, + ); - const current = previous - // A failed predecessor must not poison the chain — the next submission is a - // separate message and is still entitled to run. - .catch(() => undefined) - .then(async () => { - const completesInMs = write(); - // Wait out the scheduled Enter. Zero means the write was fully synchronous - // (`noEnter`), so there is nothing pending to wait for. - if (completesInMs > 0) await clock.sleep(completesInMs); - }); + // Only a CONTENDED submission can wait, so an uncontended one must not arm a + // ceiling timer it would then leave dangling for its whole duration. + const contended = chains.has(sessionId); + const ceilingMs = options.waitCeilingMs; + // An operator ahead of us — in flight or merely queued — makes this an operator-vs-operator + // wait, which was UNBOUNDED before #1365 and must stay unbounded (see waitCeilingMs). + const behindOperator = (pendingOperators.get(sessionId) ?? 0) > 0; + const bounded = contended && ceilingMs !== undefined && ceilingMs >= 0 && !behindOperator; - chains.set(sessionId, current); + // Count ourselves only AFTER the check above, so we do not read our own presence as a + // reason to block, and BEFORE any await, so a later operator sees us while we are queued. + const kind = options.kind ?? 'operator'; + if (kind === 'operator') pendingOperators.set(sessionId, (pendingOperators.get(sessionId) ?? 0) + 1); - // Drop the entry once this is the last submission in flight, so the map does - // not accumulate one promise per session for the life of the process. - // - // The rejection is swallowed HERE and re-surfaced only through the returned - // promise: without the catch, this bookkeeping branch would raise an - // unhandled rejection for any failed write, even when the caller handled it. - void current - .then( + const current = (async () => { + let bypassed = false; + if (bounded) { + const winner = await Promise.race([ + previousSettled, + clock.sleep(ceilingMs).then(() => CEILING_EXPIRED), + ]); + // Ceiling expired → proceed WITHOUT serialization. This is a deliberate, + // announced degradation to exactly the pre-Issue-#1365 behaviour for the ONE pair it + // can affect: an operator write against a delivery, which held a disjoint lock and so + // was never serialized against an operator at all. (Operator-vs-operator never arms + // the ceiling — see `bounded` above — so that pair keeps its unbounded wait.) Taken + // only when the alternative is stalling `--interrupt`, the human's escape hatch, + // behind a write that may run for minutes. See the boundary comment above. + if (winner === CEILING_EXPIRED) { + bypassed = true; + options.onCeilingExpired?.(ceilingMs); + } + } else { + await previousSettled; + } + const completesInMs = write(); + // Count the bypass only once bytes actually went out: a degraded write that declined to + // write anything raced nobody. Placed straight after `write()` with NO await in between, + // so a delivery holding the line still cannot observe our bytes without also observing + // the bump when it re-samples after its own write. + if (bypassed && (options.wroteBytes?.() ?? true)) { + unserializedWrites.set(sessionId, unserializedWriteCount(sessionId) + 1); + } + // Wait out the scheduled Enter. Zero means the write was fully synchronous + // (`noEnter`), so there is nothing pending to wait for. + if (completesInMs > 0) await clock.sleep(completesInMs); + })(); + + // The stored tail settles only once BOTH this submission and its predecessor are + // done. Identical to `current` on the normal path (current already awaited the + // predecessor); it matters on the degraded path, where the predecessor is still + // running — a third submission must not be released by our early finish. + const tail = bounded + ? Promise.all([previousSettled, current.then(() => undefined, () => undefined)]).then(() => undefined) + : current.then( + () => undefined, + () => undefined, + ); + + chains.set(sessionId, tail); + + // Release our operator claim as soon as OUR write is done — a later operator may then be + // bypass-eligible again if only deliveries remain ahead of it. + if (kind === 'operator') { + void current.then( () => undefined, () => undefined, - ) - .then(() => { - if (chains.get(sessionId) === current) chains.delete(sessionId); + ).then(() => { + const remaining = (pendingOperators.get(sessionId) ?? 1) - 1; + if (remaining > 0) pendingOperators.set(sessionId, remaining); + else pendingOperators.delete(sessionId); }); + } + + // Drop the entry once this is the last submission in flight, so the map does + // not accumulate one promise per session for the life of the process. + // + // The tail already swallows both outcomes, so this bookkeeping branch can never + // raise an unhandled rejection for a failed write — the rejection surfaces only + // through the returned `current`. + void tail.then(() => { + if (chains.get(sessionId) === tail) chains.delete(sessionId); + // The session may now be quiescent — try to drop its degraded-write count too. Refused + // while a watch is open; that watch's release re-tries, so the second one wins. + evictBypassCountIfIdle(sessionId); + }); return current; } +/** + * {@link submitToSession} for a writer that would rather skip its turn than wait + * for one — the gated mailbox delivery path (Issue #1365). + * + * Resolves `false` **without writing anything** when another submission already + * holds the session. That is the right trade for a gated delivery and the wrong one + * for an operator action, which is why the two differ: + * + * - A contended terminal means some other writer is mid-submission, so the + * delivery's in-lock precheck would have aborted it anyway (the screen has + * moved, or is about to). Nothing is lost by declining now: the row stays held + * and the backstop re-delivers within one tick. + * - Blocking instead would be a genuine liveness regression, because + * `MailboxDrainer.tick` walks agents SEQUENTIALLY — one agent parked on a + * terminal lock would stall every other agent's delivery, plus that tick's + * escalation, owner-notice and prune passes. + * + * @returns `true` when the write ran (and completed), `false` when the session was + * contended and nothing was written. + */ +export async function trySubmitToSession( + sessionId: string, + write: () => number, + clock: SubmitClock = realClock, +): Promise { + if (isSubmissionInFlight(sessionId)) return false; + await submitToSession(sessionId, write, clock, { kind: 'delivery-write' }); + return true; +} + /** Number of sessions with a submission in flight. Test/observability only. */ export function pendingSubmissionSessions(): number { return chains.size; } -/** Drop all chains. Test-only; a live Tower should let them drain. */ +/** Drop all chains and degraded-write counters. Test-only; a live Tower should let them drain. */ export function resetSubmissionChains(): void { chains.clear(); + unserializedWrites.clear(); + pendingOperators.clear(); + bypassWatchers.clear(); +} + +/** + * How many sessions still carry a degraded-write count. Test/observability only — the + * assertion that {@link unserializedWrites} self-evicts rather than growing for the life of + * a Tower. + */ +export function bypassCountedSessions(): number { + return unserializedWrites.size; } diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index 7cbc158b8..5ae1e6217 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -64,10 +64,11 @@ import { type EnqueueInput, } from '../db/mailbox.js'; import type { MailboxReason } from '../db/types.js'; -// Spec 1273 per-terminal submission lock — preserved across the Spec 1313 merge for -// the two explicit human-bypass paths (escape + interrupt), which do NOT route -// through the mailbox's per-agent serializer and so need their own anti-fusion lock. -import { submitToSession } from './session-submit.js'; +// Spec 1273 per-terminal submission lock. Originally the anti-fusion lock for the two +// explicit human-bypass paths (escape + interrupt); since Issue #1365 the gated mailbox +// delivery path takes it too, so it now serializes the whole write edge of a terminal. +// The operator paths pass a wait ceiling — see OPERATOR_SUBMIT_WAIT_CEILING_MS. +import { submitToSession, OPERATOR_SUBMIT_WAIT_CEILING_MS } from './session-submit.js'; // Spec 1307 `--delay` — Tower-side deferred delivery, re-homed onto the Spec 1313 // mailbox (the merge that carried this feature was flattened by a later rebase, so it // is grafted here explicitly): the due-time callback enqueues to the mailbox and @@ -1624,6 +1625,34 @@ function holdAndRespond( }); } +/** Machine-readable reason paired with `degraded: true` on an operator send response. */ +const DEGRADED_SUBMIT_REASON = 'submit-wait-ceiling-expired'; + +/** + * Announce that an operator submission gave up waiting for the per-terminal lock and wrote + * unserialized (Issue #1365). + * + * This is the degraded path, and it is deliberately loud. The ceiling arms only against a + * DELIVERY write (behind another operator the wait stays unbounded, as before #1365), so what + * expiry falls back to is exactly the pre-#1365 operator-vs-delivery behaviour: two disjoint + * locks, no serialization. That one pair is therefore no worse than the old status quo — but + * it used to be invisible, and now it is not, here and in the `degraded` flag on the response. + */ +function logCeilingExpired( + ctx: RouteContext, + action: string, + toAgent: string, + terminalId: string, + waitedMs: number, +): void { + ctx.log( + 'WARN', + `${action} → ${toAgent} (terminal ${terminalId.slice(0, 8)}...) waited ${waitedMs}ms for an in-flight ` + + `write and proceeded UNSERIALIZED — it may interleave with that write. A message long enough to ` + + `hold the line this long is the usual cause.`, + ); +} + /** Inputs for a delayed (`--delay`) send, captured from the parsed request. */ interface DelayedSendParams { to: string; @@ -1735,18 +1764,33 @@ function handleDelayedSend( if (!isStillLive()) return; // shutdown before/at due → drop the ^C nudge (body survives) if (terminalId) { let fired = false; - void submitToSession(terminalId, () => { - // Re-check EVERYTHING inside the lock: the queued submission can acquire the lock only - // AFTER a shutdown or a session teardown/respawn that landed while it waited behind an - // in-flight write. Re-fetch the session and re-check live + writable here; bail with no - // ^C otherwise (the body still delivers via the gate). - if (!isStillLive()) return 0; - const live = getTerminalManager().getSession(terminalId); - if (!live || !live.writable) return 0; - live.write('\x03'); // Ctrl+C only — end the turn; body follows via the gate - fired = true; - return 0; // no body, no Enter on this path - }) + void submitToSession( + terminalId, + () => { + // Re-check EVERYTHING inside the lock: the queued submission can acquire the lock only + // AFTER a shutdown or a session teardown/respawn that landed while it waited behind an + // in-flight write. Re-fetch the session and re-check live + writable here; bail with no + // ^C otherwise (the body still delivers via the gate). + if (!isStillLive()) return 0; + const live = getTerminalManager().getSession(terminalId); + if (!live || !live.writable) return 0; + live.write('\x03'); // Ctrl+C only — end the turn; body follows via the gate + fired = true; + return 0; // no body, no Enter on this path + }, + undefined, + // The same operator ceiling as the immediate paths (Issue #1365). This one fires + // UNATTENDED, so it is the writer most likely to meet a gated delivery mid-pace — + // which is precisely why the delayed `^C` must take this lock at all. + { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'delayed interrupt ^C', toAgent, terminalId, waitedMs), + // This is the one operator path whose write can be a NO-OP (the liveness re-check + // above). A degraded no-op raced nobody, so it must not bump the bypass counter — + // that would make a concurrent delivery hold and re-deliver for nothing. + wroteBytes: () => fired, + }, + ) .then(() => ctx.log('INFO', fired ? `Delayed interrupt ^C fired → ${toAgent} (terminal ${terminalId.slice(0, 8)}...); body delivers via the gate` @@ -1970,7 +2014,17 @@ async function handleSend( if (escape) { // Awaited: the response must not claim delivery before the ESC and its // Enter have actually been written (Spec 1273 verify). - await submitToSession(result.terminalId, () => writeEscapeToSession(session, noEnter)); + // Issue #1365: escape is an operator submission — it blocks behind an in-flight write + // (including a gated mailbox delivery, which now takes this same lock) so its ESC and + // Enter can no longer truncate a delivery mid-pace, but only up to the wait ceiling. + let escapeDegraded = false; + await submitToSession(result.terminalId, () => writeEscapeToSession(session, noEnter), undefined, { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => { + escapeDegraded = true; + logCeilingExpired(ctx, 'ESC', toAgent, result.terminalId, waitedMs); + }, + }); broadcastMessage({ type: 'message', from: { project: path.basename(senderWorkspace), agent: from ?? 'unknown' }, @@ -1980,7 +2034,13 @@ async function handleSend( timestamp: new Date().toISOString(), }); ctx.log('INFO', `Interrupt (ESC) sent: ${from ?? 'unknown'} → ${toAgent} (terminal ${result.terminalId.slice(0, 8)}...)`); - sendJson(res, 200, { ok: true, terminalId: result.terminalId, resolvedTo: toAgent, deferred: false }); + sendJson(res, 200, { + ok: true, + terminalId: result.terminalId, + resolvedTo: toAgent, + deferred: false, + ...(escapeDegraded ? { degraded: true, degradedReason: DEGRADED_SUBMIT_REASON } : {}), + }); return; } @@ -2024,16 +2084,35 @@ async function handleSend( // during the 100 ms gap. `writeMessageToSession(..., 100)` schedules the text 100 ms after // the ^C (the settle) and returns the completion offset, so the lock is held until the // whole interrupt is on the wire; uncontended, it runs at once. - // Scope of the guarantee: this serializes interrupt against interrupt/escape — the only - // /api/send writers that take this per-terminal lock. It does NOT serialize against a - // concurrent mailbox/backstop delivery (which writes through the per-AGENT serializer, a - // disjoint lock); interrupt is the explicit gate-bypassing human action, and closing that - // cross-path race would require the mailbox write edge to take this lock too (a separate, - // larger change — flagged, not done here). - await submitToSession(result.terminalId, () => { - session.write('\x03'); // Ctrl+C - return writeMessageToSession(session, formattedMessage, noEnter, 100); - }); + // Scope of the guarantee (Issue #1365 — this used to end at "interrupt vs interrupt/escape"): + // the gated mailbox delivery path now takes this SAME per-terminal lock, so an interrupt is + // serialized against a concurrent delivery too. That closes the case where a `^C` landed + // inside a delivery's own text→Enter window, clearing the composer while every byte still + // reported success — the row read `delivered` for a message the agent never saw. The lock + // order is per-agent → per-terminal (the delivery takes this one as a leaf), never the + // reverse, so there is no cycle. See session-submit.ts for the full boundary. + // Whether this interrupt gave up waiting and wrote unserialized. It must reach the SENDER, + // not just the Tower log (codex review of PR #1492): the row above was claimed `delivered` + // before the write, so a degraded interrupt would otherwise report an unqualified success + // for a body that may have interleaved with the delivery it skipped. We keep the + // claim-first tradeoff (un-claiming risks a double delivery — see above) and tell the + // truth about it instead. + let degraded = false; + await submitToSession( + result.terminalId, + () => { + session.write('\x03'); // Ctrl+C + return writeMessageToSession(session, formattedMessage, noEnter, 100); + }, + undefined, + { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => { + degraded = true; + logCeilingExpired(ctx, 'interrupt', toAgent, result.terminalId, waitedMs); + }, + }, + ); broadcastMessage({ type: 'message', from: { project: path.basename(senderWorkspace), agent: from ?? 'unknown' }, @@ -2052,6 +2131,8 @@ async function handleSend( held: false, mailboxId: row.id, reason: null, + // Additive and present only when it happened, so older clients are unaffected. + ...(degraded ? { degraded: true, degradedReason: DEGRADED_SUBMIT_REASON } : {}), }); return; } diff --git a/packages/sdk/src/tower-client.ts b/packages/sdk/src/tower-client.ts index 616b06d0d..a7e5a6956 100644 --- a/packages/sdk/src/tower-client.ts +++ b/packages/sdk/src/tower-client.ts @@ -821,6 +821,17 @@ export class TowerClient { held?: boolean; reason?: string; mailboxId?: string; + /** + * Issue #1365: an `interrupt`/`escape` submission that gave up waiting for the terminal's + * submission lock and wrote UNSERIALIZED, so its bytes may have interleaved with the + * delivery write it skipped. Present only when it happened; absent means normally + * serialized. It matters most for `interrupt`, whose mailbox row is claimed `delivered` + * BEFORE the write (un-claiming would risk a double delivery), so `delivered: true` alone + * would otherwise be an unqualified success for a possibly-mangled body. + */ + degraded?: boolean; + /** Machine-readable companion to {@link degraded} (`submit-wait-ceiling-expired`). */ + degradedReason?: string; /** * Spec 1313 round 3: due time (epoch ms) of a scheduled (`deliverAfter`) send. Present * only when `scheduled` — the row is persisted at request time and delivers not before @@ -837,6 +848,8 @@ export class TowerClient { held?: boolean; reason?: string | null; mailboxId?: string; + degraded?: boolean; + degradedReason?: string; notBefore?: number; }>( '/api/send', @@ -872,6 +885,8 @@ export class TowerClient { held: result.data!.held, reason: result.data!.reason ?? undefined, mailboxId: result.data!.mailboxId, + degraded: result.data!.degraded, + degradedReason: result.data!.degradedReason, notBefore: result.data!.notBefore, }; }