Skip to content

Two PTY write paths bypass the delivery seam; the fleet flush re-sends after a possible write #1832

Description

@khaliqgant

Summary

Relay has three PTY write paths. Phase 0 (relay#1825) put one behind the
delivery seam. The other two still write directly, so the seam's four rules do
not govern them, and one of the two re-sends after a write that may already
have landed.

Both pre-date the seam and neither is a regression. They bound what phase 0 can
be said to have achieved, and phase 1 must not assume the rules hold repo-wide.

The two paths

1. Fleet flush — crates/broker/src/runtime/delivery.rs

timeout(retry_interval, workers.deliver(worker_name, delivery))

The timeout can fire after the frame has been admitted to the sole writer
queue, and an admitted command is still emitted — send_to_worker_with_commit_boundary
in worker.rs says so explicitly, and deliberately refuses to return a timeout
once a command is queued, precisely so callers do not retry and duplicate
terminal input.

This caller does not observe that boundary. Its caller in runtime/fleet.rs
sets result.failure and breaks, which leaves the message at the head of the
FIFO, so the next flush writes it again.

That is both a fall-back after a possible write (rule 1) and a re-send on
doubt (rule 2), on the fleet path, unclassified and ungoverned.

2. Obligation injection — crates/broker/src/runtime/maintenance.rs

if let Err(error) = self.workers.deliver(&recipient, relay_delivery).await {

A direct write whose error is only logged. No route is recorded (rule 3),
no pre-write/committed classification happens (rule 1), and a failure here
is invisible to the pending/dead-letter lifecycle.

Why it was not fixed in phase 0

Routing these through the seam required a seam that outlives a single call.
That was itself a phase-0 finding (the seam was rebuilt per call, making its
duplicate guard, route recording and bounded eviction inert) and is now fixed —
BrokerRuntime owns a long-lived DeliverySeam. So the blocker is gone and
this is now mechanically possible.

It was left out because changing the fleet flush path's retry semantics is a
behavioural change on a hot path, with its own blast radius, and phase 0 was
already carrying two double-delivery fixes.

What a fix needs

  1. Route the fleet flush through DeliverySeam::send with the runtime's
    long-lived seam, so a committed failure stops instead of leaving the message
    at the head of the FIFO.
  2. Route the obligation injection through the same seam so its failures are
    classified and its route recorded.
  3. A test that a fleet-flush timeout firing after admission does NOT produce a
    second write — the shape that currently duplicates. The pattern is in
    crates/broker/src/delivery/pty.rs: register a worker whose child has
    exited, so the write fails at the pipe after crossing the commit boundary.

Evidence

Both sites are annotated in place. Found by adversarial review of relay#1825
(finding F19); see also relay#1831 for the headless route's rule-4 violation.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions