Skip to content

[Spec 1273] Per-session submission lock: an awaited send now means submitted - #1320

Merged
waleedkadous merged 8 commits into
mainfrom
builder/1273-submission-lock
Aug 2, 2026
Merged

[Spec 1273] Per-session submission lock: an awaited send now means submitted#1320
waleedkadous merged 8 commits into
mainfrom
builder/1273-submission-lock

Conversation

@waleedkadous

Copy link
Copy Markdown
Contributor

The bug

The verify-phase e2e forensics showed afx reset's /clear was never executed. It arrived as literal text welded onto the front of the re-orientation, no separator, one merged user turn:

/clear### [ARCHITECT INSTRUCTION | ... ] ###
## CONTEXT RESET — re-orientation

No slash command ran. The probe's context was fully intact — it could still recite a secret word planted before the "reset". Every layer reported success.

Refs #1273

Root cause — confirmed in source

writeMessageToSession writes the text and schedules the Enter via setTimeout (SIMPLE_ENTER_DELAY_MS = 50, PACED_ENTER_DELAY_MS = 80), then returns that offset as a number without awaiting it. /api/send responded off that.

So an awaited send resolved ~50ms before its own message was submitted. Reset awaited sendRaw('/clear'), did a fast readOutput, then wrote the re-orientation — inside the window, into the same composer, ahead of the Enter. One Enter submitted both.

The channel choice was right. The mistake was treating an awaited send as proof of submission when it only proved scheduling — the third member of a family the ledger now tracks: an operation that reports success at a point earlier than the thing it claims.

It also retro-explains phase 6's clear-confirmation finding: at the moment I sampled output, the clear hadn't even been submitted. Two bugs, one seam.

Ordering is not atomicity

SendBuffer already serializes within a flush, and #1307's per-session FIFO fixes delivery order. Neither would have prevented this — the two writes were correctly ordered and still coalesced. Being second is not the same as being separate.

The primitive

submitToSession(sessionId, write) — a promise chain per session. Each submission waits for the previous one to finish including its Enter, so concurrent sends to one terminal cannot interleave in the composer.

Architect-assigned to me; #1307 adopts it unchanged and drops their narrower versions (deliverOrBuffer's writeCompletesInMs wait, SendBuffer.busyUntil, delayed-send.ts's chain). One mechanism, two adopters.

Wired to the escape path and the immediate message path only. The buffered path is deliberately not awaited — a deferred message can sit up to 60s, and awaiting it would hang the caller instead of returning deferred: true. That constraint came from 1307 before I wired anything.

I did not touch send-buffer.ts: it's 1307's active file, and their busyUntil work should adopt this primitive rather than collide with an edit from me.

A correction to my own earlier claim

I had told both the architect and 1307 that reset's writes bump _lastInputAt and so trip their own buffering. False, verified at 1307's prompting: recordUserInput() is called only from pty-manager.ts:310,317 — the websocket handler, i.e. a human typing. Tower's own writes never touch it.

Deliberate test change

delivers message + Enter as a single atomic write asserted writeCalls.length === 1 — i.e. that the route returned before the Enter was written. That was the bug, not the contract. It now asserts the property that actually mattered (Enter is a separate write, never appended), as properties rather than an exact count since formatted messages are paced.

Testing

7 new tests, mutation-verified: removing the chaining fails the coalescing test and the ordering test, and passes the rest. The headline one models a real composer — write appends to a pending buffer, enter submits it — so it fails in exactly the way production did.

Suite 4057 → 4058, build clean.

Still open

This makes the clear submittable. The live re-run against the planted-context probe is what will confirm it actually clears, and will finally calibrate the confirmation pattern against real post-clear output.

…d now means submitted

Verify-phase e2e forensics: afx reset's /clear was NEVER EXECUTED. It arrived
as literal text welded onto the front of the re-orientation, no separator, one
merged user turn:

  /clear### [ARCHITECT INSTRUCTION | ...] ###

No slash command ran; the probe's context was fully intact and it could still
recite a secret word planted before the "reset".

ROOT CAUSE (confirmed in source, not inferred). writeMessageToSession writes
the text and schedules the Enter via setTimeout — SIMPLE_ENTER_DELAY_MS = 50,
PACED_ENTER_DELAY_MS = 80 — then returns that offset as a NUMBER without
awaiting it. /api/send responded off that. So an awaited send resolved ~50ms
BEFORE its own message was submitted. Reset awaited sendRaw('/clear'), did a
fast readOutput, then wrote the re-orientation — inside the window, into the
same composer, ahead of the Enter. One Enter submitted both.

The channel was right; the mistake was treating an awaited send as proof of
submission when it only proved SCHEDULING. Third member of a family the ledger
now tracks: an operation that reports success at a point earlier than the thing
it claims.

This also retro-explains phase 6's clear-confirmation finding — at the moment I
sampled output the clear had not even been submitted. Two bugs, one seam.

ORDERING IS NOT ATOMICITY. SendBuffer already serialises within a flush, and
1307's per-session FIFO fixes delivery ORDER. Neither would have saved this:
the two writes were correctly ordered and still coalesced. Being second is not
the same as being separate.

THE PRIMITIVE (architect-assigned to me; 1307 adopts it unchanged and drops
their narrower versions — deliverOrBuffer's writeCompletesInMs wait,
SendBuffer.busyUntil, delayed-send.ts's chain):

  submitToSession(sessionId, write) — a promise chain per session. Each
  submission waits for the previous one to finish INCLUDING its Enter, so
  concurrent sends to one terminal cannot interleave in the composer.

Wired to the escape path and the immediate message path only. The BUFFERED path
is deliberately NOT awaited — a deferred message can sit up to 60s and awaiting
it would hang the caller instead of returning deferred: true. That constraint
came from 1307, who flagged it before I wired anything.

CORRECTION TO MY OWN EARLIER CLAIM, verified at their prompting: I had said
reset's writes bump _lastInputAt and trip their own buffering. FALSE.
recordUserInput() is called only from pty-manager.ts:310,317 — the websocket
handler, i.e. a human typing. Tower's own writes never touch it.

DELIBERATE TEST CHANGE: "delivers message + Enter as a single atomic write"
asserted writeCalls.length === 1 — i.e. that the route returned BEFORE the
Enter was written. That was the bug, not the contract. It now asserts the
property that actually mattered (Enter is a separate write, never appended) as
properties rather than an exact count, since formatted messages are paced.

Tests mutation-verified: removing the chaining fails the coalescing test and
the ordering test, passes the rest.

Suite 4057 -> 4058 (7 new), build clean.
waleedkadous added a commit that referenced this pull request Aug 1, 2026
Architect ruled that 1273 owns the submission-lock primitive and this project
adopts it unchanged. PR #1320 is up; recorded what adopting it actually
involves rather than leaving it as an intention.

Measured with git merge-tree rather than assumed: merge-base 57c51a6, their
branch has none of this project's 36 phase-1 commits, and exactly ONE file
conflicts — servers/tower-routes.ts, where both sides edited handleSend and
deliverBufferedMessage. tower-routes.test.ts auto-merges. This project merges
second and resolves; both sides must survive (their submitToSession wiring;
this project's --delay parsing, escape+delay rejection, interruptFirst).
Verification is running both mutation-verified suites, not inspecting the
resolution.

Then delete the three narrower mechanisms built before the primitive existed.
With one caveat recorded as a test rather than an assumption: busyUntil guards
writes initiated by a buffer FLUSH, and flush() is on the path #1320
deliberately left alone. If submitToSession does not cover flush-initiated
writes, deleting busyUntil reopens the mid-flush interleave closed in
17db2e9 — the same failure shape #1320 exists to fix, one layer down. Ask for
a surface extension rather than keeping a local workaround.
…drain

Raised by aspir-1307 before deleting SendBuffer.busyUntil: submitToSession is
wired to the immediate path, not flush(), so deleting busyUntil could reopen
the mid-flush interleave they closed in 17db2e9 — a message writing into a
partially-delivered /clear, which is the shape of this spec's production
failure one layer down.

Checked rather than assumed: NO API extension is needed. `write` may perform
many writes and return the FINAL completion offset, so one submission can
reserve the session for an entire flush with its offset threading intact:

  submitToSession(sessionId, () => {
    let offset = 0;
    for (const msg of messages) offset = deliver(session, msg, offset);
    return offset;
  });

flush() is synchronous and does not need to await — `void submitToSession(...)`
still serialises the batch against direct sends, because the chain orders by
call, not by await.

Test pins the property so it lives in a suite rather than in a message: a batch
of paced writes and a concurrent direct send, asserting the direct send is
never welded into the batch's pending text. Mutation-verified — removing the
chaining fails it along with the other two ordering tests.

Suite 4058 -> 4059.
…very seam in the review

CMAP on #1320: Gemini APPROVE, Claude COMMENT, Codex REQUEST_CHANGES. Both
non-approving findings accepted; neither is a code defect.

1. "Per-session atomicity does not cover every PTY writer" — correct, and my
   framing was overbroad. A lock only serialises writers that TAKE it, and only
   the escape + immediate /api/send paths do. Now enumerated in the docstring
   with the reason for each:
     - deliverBufferedMessage (flush) — not covered; 1307's to wire, batch form
       already supported and tested so no API change is needed
     - tower-cron delivery — not covered, same shape
     - POST /api/terminals/:id/write — not covered, raw passthrough
     - websocket keystrokes + shellper relay — DELIBERATELY never covered; a
       human is the composer's owner and queueing their typing behind an
       agent's message would make the UI feel stuck
   The honest guarantee is "two /api/send deliveries to one session cannot
   interleave", which is the failure that reached production.

2. "The review artifact is stale" — correct. The review documented the verify
   e2e's resolution findings but not the delivery seam that the forensics
   actually exposed. Added: root cause, why ordering is not atomicity, the
   retro-explanation of the phase-6 confirmation defect (same seam), and the
   scope note above.

No behaviour change.
waleedkadous added a commit that referenced this pull request Aug 1, 2026
… a deletion

1273 corrected my framing, and the correction changes the work. #1320 wires
submitToSession into the escape and immediate paths ONLY — buffered and
delayed are deliberately left to whoever owns them. So this project must ADD
two call sites (delayed delivery, and flush's drain) BEFORE removing anything.
I would have discovered that mid-deletion.

Also recorded: write: () => number may perform many writes and return the
final offset, so a whole flush batch is one reservation with the existing
offset threading intact — 1273 pinned that with a mutation-verified batch
test rather than asserting it.

And the line they drew, which I am keeping: their test proves the PRIMITIVE
supports the pattern; it cannot prove this project's WIRING of it is correct.
Re-run the mid-flush ordering test after wiring and before deleting busyUntil.
If the property does not survive, take the failing case back rather than
reinventing a local guard.

Six-step sequence recorded so it can be held to, including sending the
resolved handleSend to 1273 for review — a bad merge resolution compiles and
passes most tests, so it fails silently.
waleedkadous added a commit that referenced this pull request Aug 1, 2026
Records the two repeated failure patterns from phase 1's eight review rounds
(artifacts asserting something adjacent to the real thing; fixes correct about
the mechanism but incomplete about its lifetime), and the one cheap check that
catches both.

Also the 1273 coordination: their _lastInputAt datum was false and retracted;
the merge surface measured rather than guessed; and their correction that
adopting #1320 is a replacement, not a deletion.

Merge of origin/main (architect-directed, PR #1324 stops agy e2e opening OAuth
windows on the human's machine) previewed clean and applied. #1320 had not
landed, so no adoption triggered. Re-ran my own suites explicitly rather than
trusting the aggregate: 186 Spec 1307 + 48 core green.
waleedkadous added a commit that referenced this pull request Aug 1, 2026
…he batch window

Architect confirmed the live-run hold: my e2e batches with 1273's probe
retest after #1320 merges, since my first question (does the /clear actually
EXECUTE) is exactly what that PR fixes.

Wrote the runbook now so the window is mechanical rather than improvised. The
load-bearing step is the canary: plant a distinctive fact before the cycle and
check it is GONE afterwards. That is the only observation distinguishing
'context cleared' from 'looks cleared' — 1273's probe used it and it is what
caught their silent failure. 'The send returned 200' is not evidence; it
returned 200 in the failing run too.

Also records: measure send -> session-ready-after-clear rather than
send -> clear-sent (the delay budget starts at the send while the clear cannot
execute until the turn ends), and exercise the manual re-send recovery
deliberately rather than assuming it, since the whole risk posture rests on it.

Thread updated with the Spec 1280 collision and its resolution.
…writer claim

Merge auto-resolved — no textual conflict with #1143's tower-cron rewrite, since
my change is in tower-routes/session-submit and theirs is in tower-cron.

RE-VERIFIED rather than ported. #1143 rewrote the cron delivery DECISION
(exitCode resolves as data; a conditioned task now delivers whenever its
condition is truthy, failures included) but NOT the delivery MECHANISM:
deliverMessage still calls writeMessageToSession directly (tower-cron.ts:338),
taking no lock. So the enumeration stands — cron remains an uncovered writer.

What changed is the claim's WEIGHT, and the docstring now says so: delivery used
to require a clean exit, so the uncovered-writer risk is now exercised on more
occasions than when the list was written. The claim is unchanged; its exposure
is not.

ALSO FIXED — a real regression the merge surfaced, caught by this spec's own
phase-7 parity tests. #1143 documented the new condition/exitCode cron semantics
in .claude/skills/afx/SKILL.md but NOT in the .codex twin, in BOTH trees
(workspace and skeleton). That is precisely the "updated one, forgot the twin"
defect the phase-7 tests were written to catch, and they caught it on the first
run after the merge:

  x keeps the two skill trees byte-identical          (spec-1273 phase 7)
  x self-hosted root: provider skill sets and bytes    (#1196 parity)
  x shipped skeleton: provider skill sets and bytes    (#1196 parity)

Ported the block verbatim to both .codex copies. Docs-only, no behaviour change.
Codex-driven agents would otherwise have had no documentation of `condition` or
`exitCode` at all.

Suite 4106 passing, 0 failed, build clean.
@waleedkadous
waleedkadous merged commit 3f622fe into main Aug 2, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant