An execute refused by the shutdown gate takes an ordering ticket it never releases, contradicting remote.hpp's own stated invariant

[Yaraslaut]

Summary

An execute envelope refused by RemoteServer's shutdown gate takes a per-model ordering ticket that it never releases. This violates an invariant the file states explicitly about itself.

Found while documenting the execute-ordering gate for #342.

The mechanism

handleImpl takes the ticket before posting to the pool, for any well-formed execute with a non-zero modelId (include/morph/core/remote.hpp:337-340):

if (auto peek = ::morph::wire::decode(msg); peek.kind == "execute" && peek.modelId != 0) {
    ::morph::exec::detail::ModelId const mid{peek.modelId};
    ticket.emplace(mid, takeExecuteTicket(mid));
}

It then posts, and the pool thread calls dispatchMessage(msg, reply, cid, ticket) (:349).

dispatchMessage's shutdown gate returns before reaching dispatchExecute, and therefore before any rejectAndRelease (:937-941):

if ((env.kind == "register" || env.kind == "execute" || env.kind == "attach") &&
    _shuttingDown.load(std::memory_order_acquire)) {
    reply(::morph::wire::encode(::morph::wire::makeErr("server shutting down", env.callId)));
    return;
}

execute is in that list, so the ticket taken at :337 is dropped without release.

The invariant it breaks

Stated in the file itself, at :1211-1218:

Every early-return branch below that follows the ticket-taking site must release it (via releaseExecuteTicket) before returning — an unreleased ticket permanently stalls every later ticket for the same model.

The shutdown gate is exactly such an early-return branch, and it does not release.

Why it is benign today, and why it should still be fixed

beginShutdown() is irreversible — nothing un-sets _shuttingDown (see the comment at :1698). So every later execute for that model is refused at the same gate and never reaches awaitExecuteTurn. Nothing is left waiting on the stalled ticket, and no caller hangs.

What remains is a leaked gate map entry on a shutting-down server, and — more importantly — a stated invariant that is false. The next person to add an early return after the ticket site will read :1211-1218, see it presented as universally held, and reasonably assume the shutdown path already honours it. An invariant with a live exception that the comment does not mention is a trap, independent of today's blast radius.

Verification status

Read from code on master c429195c, not reproduced. handleImpl's ticket site, dispatchMessage's gate, and the invariant comment were each read directly; the early return is plainly upstream of every rejectAndRelease in that function. No test was written to observe a leaked entry.

Suggested direction

Either release the ticket in the shutdown branch before returning, or move the gate above the ticket-taking site in handleImpl so a refused envelope never takes one. The second is arguably cleaner — a message that will be refused outright has no ordering to preserve — but it moves a shutdown check onto the transport thread, which is a deliberate placement decision (:333-341 takes the ticket there precisely so send order is captured before the pool can reorder), so it deserves a moment's thought rather than a mechanical move.

Whichever is chosen, :1211-1218's wording should end up true rather than aspirational.

[Yaraslaut]

Triage: triage: valid — mechanism confirmed, but "benign today" is wrong: it is a permanent hang

The mechanism is exactly as reported. Verified at master 82c0d7bc: handleImpl takes the ticket at :354, before _pool.post at :363; dispatchMessage's shutdown gate returns at :953-957; every releaseExecuteTicket call lives in dispatchExecute (:1245 inside rejectAndRelease, and :1504), which starts at :1240. The gate is ~290 lines upstream of the nearest release.

The part that needs correcting is the severity section.

"Nothing is left waiting on the stalled ticket, and no caller hangs" — reproduced false

The issue's reasoning is that beginShutdown() is irreversible, so every later execute for that model is refused at the same gate and nothing ever waits on the stalled ticket. That misses the window the ordering gate exists because of: the two tickets are taken in send order on the transport thread, but the two pool threads read _shuttingDown in whatever order the pool happens to run them. When the later ticket reads false and the earlier one reads true, the later ticket is already inside awaitExecuteTurn waiting for a ticket that will never be released.

awaitExecuteTurn (:1612) is a bare cv.wait with no deadline and no shutdown predicate, so that wait is permanent.

Reproduced deterministically. A and B are two same-model executes posted back-to-back to a 2-thread pool; B (small envelope, decodes fast) passes the gate and calls beginShutdown() from inside its own authorize(); A carries a 6 MB body purely as a timing lever so its dispatchMessage decode finishes after that. Output:

  [authorize] first execute admitted; calling beginShutdown() now
A (ticket 0) replied: yes -> {"kind":"err",...,"message":"server shutting down"}
B (ticket 1) replied: NO

>>> A was refused by the shutdown gate and dropped ticket 0 without releasing it;
    B is stuck in awaitExecuteTurn(mid, 1) forever.
drainedWithin(3s) = FALSE (took 3000 ms)

The 6 MB body only makes the window wide enough to hit on demand — it is not part of the bug. The natural window is the two pool threads' gate reads, which is nanoseconds wide; a jittered 400-iteration race loop did not hit it, which is why the deterministic lever was needed. Narrow is not the same as unreachable, and beginShutdown() is called precisely when the server is busiest with in-flight work.

Three consequences, not one

1. The caller never gets a reply. No error, no timeout, nothing — with default limits.
2. A pool worker thread is lost permanently. Enough occurrences starve the pool.
3. drainedWithin() can never succeed. _inFlightExecutes is incremented at :1372, before awaitExecuteTurn at :1427, so the stuck call is counted as in-flight forever. That breaks the orchestrated drain beginShutdown()'s own doc comment (:548-553) advertises — the exact scenario in which this bug fires.

~ThreadPoolExecutor would also hang joining that worker; the repro has to _Exit.

A configured executeTimeout masks it, and lies while doing so

Same repro with LimitPolicy::executeTimeout = 1000ms:

A (ticket 0) replied: yes -> {"message":"server shutting down"}
B (ticket 1) replied: yes -> {"message":"timeout"}
drainedWithin(3s) = true (took 0 ms)

The caller gets a spurious "timeout" for a call that never ran, and the drain reports success while a worker thread is still permanently blocked. Worth knowing when triaging priority: the default configuration (executeTimeout{0}) is the one that hangs outright.

Suggested direction

Between the issue's two options, release the ticket in the shutdown branch is the right one, and the hang makes the case stronger rather than weaker. Option 2 (moving the gate above the ticket site in handleImpl) puts a shutdown check on the transport thread, and :333-341 places the ticket-take there deliberately so send order is captured before the pool can reorder — that placement is load-bearing and should not move to fix a missing release.

Worth considering while there: rejectAndRelease already is the "release on every early return" extraction, but it only covers dispatchExecute. dispatchMessage holds the same ticket across ~290 lines with no such guard, which is why this branch was missed. An RAII ticket holder that releases unless explicitly committed would put the guarantee in one place instead of asking every future early return to remember it — this is the second early return in that function's history to get it wrong.

Whichever is chosen, :1226-1233's "Every early-return branch below that follows the ticket-taking site must release it" must end up true. It is currently false, and it is stated as a universal.

Verdict

triage: valid, area: core, bug. Implement as written; treat the severity as "silent permanent hang plus broken graceful shutdown", not "leaked map entry on a dying server".