Well, I was thinking you only need to remove it in two cases: when returning a packet, and when timing out. But I was also forgetting that satisfied() isn't called in the timeout case, and anyway it's a moot point.
There is also at least one more case, suspended thread handling. If a thread gets suspended while in NtRemoveIoCompletion it is essentially not a in a wait queue, other threads can get the completion. This works correctly with generic server wait queues logic.
Oh that's weird. Definitely would not have expected that one.
But as of 2/6 the completion_wait is still destroyed when a completion is destroyed, and the associated packet with it. This is fixed by the "if (!wait->msg)" before calling cleanup_thread_completion(), but that's not done until 4/6. Or maybe I'm missing something here?
Anyway, in v2 I reshuffled things a bit and introducing the possibility of 'detached' completion wait (with wait->completion == NULL) only in 4/6. This way it surely doesn't fix any of those two issues (both ends up fixed in 4/6) and is mostly an intermediate step. The the patch message says "ntdll: Assign completion to thread when wait for completion is satisfied.", it doesn't promise any fixes related to handle close? The functional effect is that it introduces LIFO wake up and denies stealing completions by "newcomer" threads when the wait for this completion is already satisfied.
I guess, but given the names of the commits I would have expected that bug to be fixed in 2/6 rather than 4/6. Maybe it's not worth bikeshedding.