Review changes by their owning module first, then apply the cross-cutting checks. Add new recurring failure patterns under the corresponding module instead of building one flat checklist.
Tasks move through INITED → RUNNABLE → BLOCKED → FINISHED → FINALIZED.
SharedState source/sink dependencies connected through inject_shared_state()?_try_to_reserve_memory() before materializing large structures?_memory_sufficient_dependency wired in where pressure should block, not overrun?revoke_memory() preserves the existing spill path?set_ready() fast-path precheck vs is_blocked_by() lock-first asymmetry respected?Dependency subclasses pair block() / set_ready() on every path?CountedFinishDependency::add() and sub() under _mtx?Async RPC completion callbacks may synchronously start another RPC. Review them as reentrant code, even when the surrounding queue permits only one network request to be in flight.
brpc::Controller remain alive until brpc invokes the completion closure?AutoReleaseClosure has an external callback owner while the callback result is still needed? Do not replace its weak_ptr with strong ownership that can retain QueryContext and other large query-scoped objects after cancellation.std::function target while callback-A is still on the stack? Prefer per-RPC callback state when reentrant sends are possible.Controller::Reset() is called only after the previous RPC and all code reading its result have finished?request_attachment() promptly? Runtime-filter attachments can contain large serialized Bloom filters. If a callback can start another RPC with the same Controller, clear the old attachment before invoking the callback, never after it.END_OF_FILE preserve the intended cancellation and cleanup semantics?Apply these checks to remote exchange, local exchange, broadcast paths, ExchangeSinkBuffer, Channel, VDataStreamRecvr, and every SenderQueue implementation.
END_OF_FILE, does the sender stop or turn off the correct channel and release queued regular and broadcast blocks without treating a normal receiver close as an unrelated transport failure?rpc_channel_is_idle and channel turn-off state consistent, so no queue waits for a callback that will never run?acquire() paired with a dequeue, drop, EOF, failure, cancellation, or teardown release on all exits?_total_queue_size, memory counters, queue capacities, and BroadcastPBlockHolderMemLimiter updated under the correct lock without underflow or double release when one broadcast block is shared by multiple channels?SenderQueue enforce _sender_queue_mem_limit and the receiver-wide exchange buffer limit for both serialized remote PBlocks and local Blocks?done callback as backpressure, is that callback invoked exactly once after consumption, cancellation, close, destruction, or a deserialize/error path? A missed callback leaves the remote sender permanently blocked.add_blocks_memory_usage() and sub_blocks_memory_usage() balanced on normal reads and every failure path, and does dropping below the limit promptly set the local-channel dependency ready?_source_dependency become ready when data arrives, the last sender reaches EOS, or the queue is cancelled/closed, and become blocked only when the queue is empty with live senders?Runtime-filter producers, local mergers, and the global merge node coordinate across parallel instances and recursive CTE stages. A callback often represents only the producer that sent the RPC, not every producer waiting for the result.
Manager, producer, merger, dependency, query-cancellation, and RPC callback code execute on different threads and can call back into one another. A recursive_mutex prevents only same-thread self-deadlock; it does not prevent cross-thread lock inversion.
GlobalMergeContext::mtx, producer/consumer _rmtx, merger locks, and dependency locks? New code must not introduce a reverse acquisition path._rmtx, if producer/consumer code can call back into the manager? Review the full call graph, not only the locks visible in the changed function._rmtx while entering manager/merger code when another path can hold a manager/merger lock and call set_synced_size(), signal(), publish(), or another producer method?brpc::Join(), dependency wait, DNS/stub lookup, or other blocking operation performed while holding a lock needed by its completion callback? The callback cannot signal completion if it must acquire the lock held by the waiting thread.sub()/set_ready()/set_always_ready(), RPC callbacks, and destruction of callback/controller owners invoked outside manager and producer locks when they may wake tasks, run cleanup, or re-enter runtime-filter code?Defer actions reviewed for what they execute while local lock guards exist?A classic inversion to reject during review is:
Thread A: Producer::_rmtx -> RuntimeFilterMgr lock Thread B: RuntimeFilterMgr lock -> producer->set_synced_size() -> Producer::_rmtx
Another common callback deadlock is:
Thread A: holds GlobalMergeContext::mtx -> waits for RPC/dependency completion Thread B: RPC completion callback -> needs GlobalMergeContext::mtx to signal completion
CountedFinishDependency::add() has exactly one matching sub() or a query-cancel path that unblocks all affected dependencies? Check all parallel producers, not only the final producer that sends the size RPC.END_OF_FILE handling cannot release only the sender's dependency while sibling producer dependencies remain blocked?stage under GlobalMergeContext::mtx, unlock, and then read or mutate the same stage's merger, done, targets, or callback storage.publish_callbacks and sync_size_callbacks resize/write/clear operations are synchronized with recursive reset, and an old stage cannot repopulate a new stage's callback owners?