GOODBYE cannot say whether the drain finished, so every client must treat a clean reload as outcome-unknown

A client that receives a route GOODBYE while a request is in flight cannot tell whether its request was delivered and answered, or cut mid-execution. The daemon knows which — it computes the fact and discards it.

This arrives from a second consumer: broca has a Phase-2 dispatch journal (fsync-before-dispatch intent, idempotency keys, Completed/NotSent/SentButOutcomeUnknown) that models everything broca initiated, but its route-level discriminator is a client-side guess (route_failure_is_known_dead). Its maintainer's question — whether reconciliation belongs in the client or the transport — is what sent me back through this code.

The fact the daemon already has

begin_forwarding_drain_with, unchanged at 06a2e76:

if !wait_for_forwarding_quiescence(forwarding, target.endpoint, runtime.drain_timeout).await? {
    warn!(
        module_id = %spec.module_id,
        waited = ?runtime.drain_timeout,
        "{operation} drain timed out before request quiescence; forcing teardown"
    );
}

let released_routes = forwarding
    .release_module_endpoint_routes(target.endpoint)
    .map_err(SuperviseError::Forwarding)?;
send_route_goodbyes(forwarding, released_routes);

wait_for_forwarding_quiescence returns a bool that answers the client's question exactly:

quiesced — every request in flight at drain start was allowed to finish. A client that sent a request and got no response before the GOODBYE did not have it cut; the daemon waited.
timed out — teardown was forced with work still outstanding. Outcome genuinely unknown.

That bool reaches a log line and nothing else. Both branches fall through to the same release and the same GOODBYE, so the two cases are indistinguishable on the wire. Every consumer then has to assume the worst case for the common case — and a clean, deliberate reload is the common case.

This is the same discard I reported in #11 from the other side. There it was a diagnostic gap; here it's a semantic one.

Why the obvious placements don't work

Not in the GOODBYE frame. FrameType::Goodbye.is_pure_header() is true — it carries no body by design.

Not in the flags. Bits 6-7 are reserved-must-be-zero and decode_header rejects non-zero with ReservedFlagBits, so every existing Rust receiver refuses the frame. Reserved flag bits are a decode tripwire, not additive space (#6).

Not a new FrameType — the envelope is locked.

Which leaves a channel-0 push, and ClientControlPush does not exist. Only ModuleControlPush (module→daemon, route.status) does. The protocol doc reserves the subc→client push direction and requires clients to ignore unrecognized channel-0 pushes, so the vocabulary is additive-ready — the plumbing isn't built.

That's the same missing primitive #5 needs, which is worth noting because it's now two independent consumers waiting on one piece of work rather than one speculative ask.

What I'm proposing

A daemon→client push on channel 0 before teardown, carrying the outcome the daemon already computed:

route.closing { reason: "reload" | "restart" | "disable" | "crash", drained: bool }

drained: true means the daemon waited for quiescence and got it. drained: false means forced teardown. On crash there is no drain, so drained: false is honest rather than pessimistic.

This is not a durable event store, and deliberately so — you ruled that out on #12 and I agree with the boundary. Nothing is retained: the signal is emitted at teardown from state the daemon holds at that instant, and is gone immediately after. A client that misses it is exactly where it is today.

Three constraints I'd want in the issue rather than discovered later

1. The first GOODBYE batch structurally cannot carry this. send_route_goodbyes(target.abandoned_bindings) fires before the quiescence wait; only send_route_goodbyes(released_routes) fires after. Routes abandoned up front are torn down before the answer exists. Either the push covers only the second batch — and says so — or the abandoned set is reported separately with its own honest drained: false.

2. Handlers can't emit mid-execution. handle_control_frame and friends return Result<Vec<Frame>, RouterError> and the router writes only after they return. A push issued during a drain needs ctx.egress passed into the handler. Same structural blocker as #5.

3. FrameSink::send enqueues, it does not complete the write. So "pushed before the GOODBYE" can only ever mean "enqueued before" — worth stating in the contract, because a client author will otherwise read an ordering guarantee the architecture doesn't provide.

What I explicitly considered and rejected

The larger version of this — subc remembering which correlation ids it fully forwarded versus never forwarded, queryable after reconnect — does not work, and the reason is structural rather than expensive:

The daemon is corr-blind on the data path. lookup_data_route(connection_id, channel, epoch) takes no corr. Routing is channel+epoch; nothing per-request is retained. Per-corr state exists only on the control plane (pending_relays, pending_control_rpcs, health_probe_tombstones, all keyed (ModuleEndpointId, u64) and bounded in volume).
There is no identity to serve the answer to. After a GOODBYE the client reconnects as a new ConnectionId. consumer_identity attests a module, but set_spawn_nonce rotates the nonce on respawn precisely so stale identities are invalidated — and direct clients have no identity at all. Post-reconnect, the daemon cannot tell which prior connection is asking.
Corr is unique per route channel, not globally, so the keys collide across clients even if identity were solved.

Making that work would mean giving clients durable identity across reconnect, which is a much larger change than the problem justifies. The one-bit-at-teardown version needs none of it.

Happy to build this if the shape is right — though it lands on control.rs/supervise.rs, so it queues behind #10 with the rest.

Accepted — and it converges with the disposition I just posted on #5, which asked for route.closing from the other direction. Read that comment first; the two issues now specify one push family, two emissions, and the split resolves your constraint 1 cleanly rather than working around it.

The tension between the two consumers is real and points at the shape: #5 needs a pre-drain signal (classify the GOODBYEs that are about to arrive — a verdict-less warning), and this issue needs the post-drain verdict (drained: bool, which does not exist until the quiescence wait returns). One push cannot carry both honestly. So, intent/outcome split — the same shape as every commit-then-enrich contract in this codebase:

route.closing { reason }                    — enqueued before the drain begins, no verdict claimed
route.closed  { reason, drained: bool }     — enqueued after the quiescence wait, before the released-batch GOODBYEs

Your constraint 1 then dissolves instead of being documented around: the abandoned batch is covered by route.closing (warned) and reported in route.closed as drained: false for that set — honest, because abandonment is forced teardown. If carrying per-batch honesty in one field is awkward, route.closed { drained } covering the released batch plus an abandoned: u32 count is acceptable; what may not happen is drained: true painting over an abandoned set.

The rest as you stated it, now normative for the pair:

Crash: drained: false, honest rather than pessimistic; no route.closing precedes a crash, and that asymmetry is itself signal (a closed with no prior closing means nobody planned this).
Enqueue-order is the only claim (your constraint 3) — goes in the contract text verbatim, and the ordering test asserts enqueue sequence per the dispatch_spike pattern.
ctx.egress into the handler (constraint 2) is the one structural change, now demanded by two consumers rather than one speculative ask — which is exactly the bar it needed. ck module restart reports "route closed by subc (GOODBYE)" when drain forces teardown — restart actually succeeded #5's early-ack no longer competes for it.
Nothing retained. Your rejection of the corr-ledger version is correct and worth keeping in this issue's text as the boundary: the daemon is corr-blind on the data path by design, there is no post-reconnect identity to serve an answer to, and the one-bit-at-teardown version needs neither. broca's journal keeps owning reconciliation; this gives its discriminator an authoritative input instead of a guess.
Clients must ignore unrecognized channel-0 pushes — already required by the doc, which is what makes both pushes additive.

Sequencing behind #10 on the shared surface is right. One review criterion set now, instrument named: the ordering test asserts the enqueue sequence closing → drain → closed → released-batch GOODBYEs and reddens on any reorder — and the drained bool must be the same value the quiescence wait returned, not recomputed (one source, per #11's lesson).

Shape accepted — the intent/outcome split is better than what I filed, and drained: true never painting over an abandoned set is the right invariant. Before I start: my constraint 2 is wrong, and it's the one you budgeted the structural change for.

`ctx.egress` isn't needed for this

I carried that constraint over from the #5 early-ack mapping without re-deriving it for the push family. They're different targets:

Early ack → the requester's control response, which does flow through handle_control_frame's Result<Vec<Frame>> return. Genuinely blocked. (Moot now — you dropped it.)
route.closing/route.closed → route-holders, whose sinks the drain already has in hand.

send_route_goodbyes proves it, in the same function:

if let Some(target) = drain_target.as_ref() {
    send_route_goodbyes(forwarding, target.abandoned_bindings.clone());   // <- emits HERE
    if !wait_for_forwarding_quiescence(...).await? { warn!(...); }
    let released_routes = forwarding.release_module_endpoint_routes(target.endpoint)?;
    send_route_goodbyes(forwarding, released_routes);
    send_module_goodbye(&spec.module_id, forwarding, target);
}

That first call emits frames mid-execution, before the quiescence wait, while the handler that ultimately called this has not returned. It works because send_route_goodbyes writes through released.sink.try_send(frame) — a FrameSink from the forwarding table — rather than through the handler's return value.

And FrameSink is per-connection, not per-route. Your own doc comment on it:

Cheaply cloneable handle to one connection's bounded outbound frame queue. Backends emit responses, streaming frames, and future PUSH frames through this single path.

So a channel-0 push to a route-holder goes through the identical sink that carries its route GOODBYE. The plumbing you budgeted a structural change for is already there and already exercised.

So the "one structural change demanded by two consumers" is now zero structural changes demanded by one. Worth knowing before you schedule around it.

I'd rather correct this now than have you accept a scope estimate I inflated. It's also the third time this arc I've asserted something about this codebase from a prior mapping rather than re-reading it — the MASKED verdict on #11 and the both-directions compatibility claim on #21 were the same move.

The one thing that genuinely needs a new affordance

route.closing fires before the drain, and at that point the only route set in hand is ModuleDrainTarget.abandoned_bindings. The routes that will be released after quiescence aren't enumerated until release_module_endpoint_routes runs — and that call releases as it enumerates, so it can't be used for a pre-drain read.

There's no read-only enumeration for a module endpoint's routes today (release_module_endpoint_routes is the only thing in that shape). So route.closing needs one — a routes_for_endpoint(endpoint) -> Vec<GoodbyeTarget>-ish read under the existing lock, taking no mutation.

That's small, and it's the same read #17's census needs. If you'd rather I build it once and have both use it, say so and I'll shape it for the census's needs too rather than fitting it narrowly here.

Confirming the criterion

drained comes from the wait_for_forwarding_quiescence return value, threaded to the emission — not recomputed, not re-derived from route state. One source, and the ordering test asserts enqueue sequence closing → drain → closed → released GOODBYEs per dispatch_spike.

The crash asymmetry — closed with no prior closing meaning nobody planned this — I'll pin in the contract text as normative rather than leaving it implied.

Starting on this now that #10 has landed, unless the enumeration question changes what you want built first.

Correction accepted, and it improves the schedule: zero structural changes, one new read, built once in #17 for both consumers.

Your send_route_goodbyes citation settles it — mid-execution emission through per-connection FrameSinks is already production behavior in the very function this push family extends, and the doc comment naming "future PUSH frames" means the sink was designed for exactly this. The ctx.egress budget dies; I'd rather lose a planned structural change to a re-read than keep it on momentum. (Your naming of the third prior-mapping assertion is noted and the discipline is the right one — the codebase at 06a2e76 outranks any mapping of it, including mine.)

Build order settled: the read-only endpoint-route enumeration lands in #17 first, shaped for the census, and #22 consumes it. Two shape requirements from this side so it serves both without a second pass: it returns enough per route to build a GoodbyeTarget (so route.closing can address the same set the drain will), and it reports draining state per route (the census's own requirement — a route already receiving its GOODBYE must not read as live collateral). Under the existing read lock, no mutation, no frames.

Criterion confirmed as you stated it: drained is the wait_for_forwarding_quiescence return threaded to the emission — one source; enqueue-order test closing → drain → closed → released GOODBYEs; crash asymmetry (closed with no prior closing = nobody planned this) pinned as normative text. Start when #17's read lands.

forwarding: a read-only route census so blast radius is knowable befo…

Navigation Menu

Repository navigation

GOODBYE cannot say whether the drain finished, so every client must treat a clean reload as outcome-unknown #22

The fact the daemon already has

Why the obvious placements don't work

What I'm proposing

Three constraints I'd want in the issue rather than discovered later

What I explicitly considered and rejected

`ctx.egress` isn't needed for this

The one thing that genuinely needs a new affordance

Confirming the criterion

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

GOODBYE cannot say whether the drain finished, so every client must treat a clean reload as outcome-unknown #22

Description

The fact the daemon already has

Why the obvious placements don't work

What I'm proposing

Three constraints I'd want in the issue rather than discovered later

What I explicitly considered and rejected

Activity

ualtinok commented on Aug 15, 2026

iceteaSA commented on Aug 15, 2026

ctx.egress isn't needed for this

The one thing that genuinely needs a new affordance

Confirming the criterion

ualtinok commented on Aug 15, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`ctx.egress` isn't needed for this