diff --git a/backend/cpp/audio-cpp/inference_lane.h b/backend/cpp/audio-cpp/inference_lane.h index 636feb475..c9df3aa43 100644 --- a/backend/cpp/audio-cpp/inference_lane.h +++ b/backend/cpp/audio-cpp/inference_lane.h @@ -4,12 +4,13 @@ // owns. Standard library only. // // Why a plain mutex is not enough: an audio.cpp session is not reentrant, so -// concurrent gRPC handlers have to take turns. But a GPU call that wedges cannot -// be cancelled from userspace, so an unbounded queue behind one wedged run would -// swallow every gRPC worker thread until the process is useless. A caller -// therefore needs to be able to walk away, and needs to be able to tell "the -// lane is busy with normal work and I ran out of patience" apart from "the run -// in the lane has already outlived the patience I brought". +// concurrent gRPC handlers have to take turns. But once a GPU call stops making +// progress there is nothing a host thread can do to take it back, and an +// unbounded queue behind such a run would absorb the handler threads one by one +// until nothing is left to answer with. A caller therefore needs to be able to +// walk away, and needs to be able to tell "the lane is busy with normal work and +// I ran out of patience" apart from "the run in the lane has already outlived +// the patience I brought". #include #include @@ -93,6 +94,13 @@ class InferenceLane { // including a caller that returns from the middle of a long stream. Throws // LaneUnavailable if the lane could not be taken, in which case there is no // object and nothing to release. +// +// Not reentrant, and it does not detect reentrancy: a second entry constructed +// while the calling thread already holds the same lane waits for a lane only +// that thread can release. With a positive budget that surfaces as +// LaneUnavailable, but in unbounded mode the thread parks with no diagnostic at +// all. Keep entries one per call: a handler that holds one across a stream must +// not let a helper it calls construct another. class LaneEntry { public: // budget_ms <= 0 waits indefinitely. Pass the output of @@ -106,8 +114,20 @@ class LaneEntry { // Deliberately immovable rather than carefully movable. A moved-from entry // would have to stop releasing the lane while the lane still records it as // occupied, and that hazard is not worth the convenience: the lane can only - // be recovered by whoever took it. Callers needing to extend the lifetime - // past a scope should hold the entry indirectly instead. + // be recovered by whoever took it. + // + // To hold a lane for longer than one scope, construct the entry in place + // instead of moving one in. Two shapes work: + // std::optional held; // member or local + // held.emplace(lane, budget_ms); // takes the lane, held.reset() gives it back + // auto held = std::make_unique(lane, budget_ms); // also returnable + // Both outlive the acquiring scope and still release exactly once, when they + // are reset or destroyed. Prefer the optional for a member whose lifetime is + // the handler's; use the unique_ptr when the entry has to be returned, since + // an optional of an immovable type is itself immovable and cannot be. A + // factory may instead write `return LaneEntry(lane, budget_ms);`, which C++17 + // guarantees to elide, whereas `LaneEntry entry(...); return entry;` does not + // compile, because that form is a move. LaneEntry(LaneEntry &&) = delete; LaneEntry &operator=(LaneEntry &&) = delete; diff --git a/backend/cpp/audio-cpp/inference_lane_test.cpp b/backend/cpp/audio-cpp/inference_lane_test.cpp index 8b4205d8a..bee36b04b 100644 --- a/backend/cpp/audio-cpp/inference_lane_test.cpp +++ b/backend/cpp/audio-cpp/inference_lane_test.cpp @@ -292,6 +292,15 @@ static void test_bounded_wait_times_out() { // The waiter arrives immediately, so the holder's elapsed time is far below // the budget and the fail-fast path must not trigger here. + // + // Load-sensitive margin, and the tightest one in this file: what makes this + // the timeout path rather than the fail-fast path is the holder's age + // staying under 120 ms at the arrival check. All that sits between the + // holder's stamp and this call is one signal handover, microseconds against + // a 120 ms allowance, but unlike the other margins here load pushes this one + // toward failing rather than away from it. If it ever does flip, the symptom + // is the wording assertions below going red, not a hang, and the fix is a + // larger budget rather than a weaker assertion. const EntryOutcome outcome = try_entry(lane, kBudgetMs); release.raise(); holder.join(); @@ -543,12 +552,23 @@ static void test_failure_messages_are_diagnosable() { }); check(held.await(5000), "B10 holder took the lane"); + // The two budgets have to straddle the run's age, or both callers take the + // same path and the comparisons below are between two fail-fast messages + // that differ only in the budget they print. + // + // Margin: 30 ms is far under the ~120 ms age, and load only ages the run + // further, so `fast` fails fast. 400 ms is far over it, with the same + // slack and the same safe direction as the B3 test, so `slow` queues and + // then times out. nap(120); const EntryOutcome fast = try_entry(lane, 30); - const EntryOutcome slow = try_entry(lane, 60); + const EntryOutcome slow = try_entry(lane, 400); release.raise(); holder.join(); + check(mentions(fast.message, kStillRunningPhrase) && + mentions(slow.message, kTimedOutPhrase), + "B10 one caller took the fail-fast path and the other timed out"); check(fast.message != slow.message, "B10 the two failure modes do not share one message"); check(!fast.message.empty() && !slow.message.empty(),