supervisor: route.closing and route.closed so a reload's outcome is knowable - #31
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #22, and closes the buildable half of #5. subc's first daemon→client channel-0 push family.
The bool
wait_for_forwarding_quiescencereturns now reaches the client instead of only awarn!.The two emissions, in the drain block
drainedis bound once from the wait and passed straight through — your criterion, and it's structural rather than promised: there is no second source it could come from.endpoint_routeslost its#[allow(dead_code)]; the consumer it was reserved for is here.Your abandoned-set requirement, and what the build found
The finding is sharper than my brief's premise, and it's worth reading before the rest.
endpoint_routescovers exactly the committed routes thatrelease_module_endpoint_routeslater releases — but it excludes pendingroute.bindrelays, which are the abandoned set. Those have a module-onlyGoodbyeTarget, and their clients already receivemodule_reloadingrather than a route GOODBYE. So they were never addressable by this push in the first place.That makes
abandoned: u32the honest shape rather than the fallback you allowed: the count says "N binds were forced down before the wait, anddrainedsays nothing about them." Adrained: truecannot paint over them because they are structurally outside the set the push addresses.Crash asymmetry
Emitted from
cleanup_connection, not the drain path:No
RouteClosingprecedes it — there are only twoRouteClosingemission sites and both are in the planned drain. Soclosedwith no priorclosingmeans nobody planned this, as normative text rather than convention.Contract text
Plus the daemon-originated-only property pinned, per #6: there is no module→client push relay to forge through, and the push constructor lives daemon-side, so a module cannot express one on its control channel. First instance of this direction is the right moment to write that down.
Zero structural changes
No
ctx.egressthreading —git diff origin/master -- crates/ | grep '^+.*ctx\.egress'returns nothing. Emission goes through per-connectionFrameSinks exactly assend_route_goodbyesalready did three lines above the wait.supervisor.restartkeeps its completion semantics and the supervisor op mutex is untouched, per your #5 disposition.The ordering test
route_lifecycle_enqueues_closing_drain_closed_then_released_goodbyesreads frames sequentially from one client connection:Single ordered stream, so any reorder fails an assertion rather than needing a separate order check. 10/10 in a loop.
Also covered: timed-out drain yields
drained: false, quiesced yieldsdrained: true, both from the real wait; crash yieldsclosedwith noclosing; and a disable path asserting theroute.closingbody field-by-field.Gates
Golden diff is three op-list insertions, nothing else.
subc-control→ 0.4.0,subc-core→ 0.6.0,subc-client-rscaret pin →"0.4".Windows unverified — twin whenever convenient.
One note
I previously told you
ctx.egressthreading was the structural change this needed, you accepted it, and I retracted it. This PR is the retraction being true: the brief for it said do not build that, and stop if you find yourself needing it. Nothing needed it.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds daemon→client channel‑0 pushes for route teardown so clients can observe reload outcomes. Previously clients only saw GOODBYE and the quiescence result was log‑only; now they receive
route.closingbefore a planned drain androute.closedafter, including whether live routes drained and how many pending binds were abandoned. Crashes emitroute.closed { reason: crash, drained: false, abandoned: 0 }with no priorroute.closing.route.closingthenroute.closed; for crash:route.closedonly. Both are enqueued on channel 0 before released‑route GOODBYEs.drainedis the exactwait_for_forwarding_quiescenceresult;abandonedcounts pendingroute.bindrelays forced down and is not covered bydrained.FrameSinks; on enqueue failure, the target connection may be closed per sink policy.route.closedduring connection cleanup; no structural forwarding changes.Review notes
supervise.rs: begins drain by sendingClientControlPush::RouteClosing, waits, then sendsRouteClosedand GOODBYEs; reason threaded viaRouteCloseReason.control.rs:send_route_control_pusheshelper; crash emission inhandle_connection_closed.subc-control: addsClientControlPushandRouteCloseReason; ops list exposesroute.closing/route.closed; docs updated.forwarding.rsanddaemon_config.rs; stub gainsFAKE_AFT_BIND_NEVER_REPLY_AFTERto exercise abandoned binds.Rollout
subc-control→0.4.0,subc-core→0.6.0;subc-client-rspinssubc-control"0.4".route.closing/route.closed.Written for commit 7dbda42. Summary will update on new commits.