backend: skip stale completion-queue entries with a null result (IOCP + kqueue)#1
backend: skip stale completion-queue entries with a null result (IOCP + kqueue)#1nanasess wants to merge 1 commit into
Conversation
… + kqueue)
A completion popped from the loop's completion queue is unwrapped with
`c.result.?` ("Completion queue items MUST have a result set"). That
invariant can be violated by a completion-reuse race: if a completion is
re-initialized and re-queued elsewhere while it is still linked in the
completion queue, `loop.timer` (`c.* = .{...}`) resets `result` to null,
so the later pop hits `c.result.?` and panics ("attempt to use null
value").
This was observed on Windows/IOCP via the Ghostty cursor-blink timer
(renderer thread): the cursor timer completion is re-armed/reset/canceled
from several renderer-thread events, and an interleaving leaves a stale
entry whose result was cleared. Windows IOCP support is still WIP per the
README, and this path is uncovered by mitchellh#170 (which only clears `next` and
does not touch iocp.zig).
Guard both the IOCP and kqueue completion loops: when a popped entry has a
null result, treat it as stale and skip it WITHOUT clobbering its now-reused
state (the re-armed completion fires on its own).
Disclosure: drafted with AI assistance and verified empirically; the author
is not deeply familiar with Zig.
|
Warning Review limit reached
More reviews will be available in 25 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request adds a guard to skip stale completions with a null result in both the IOCP and kqueue backends. However, the review feedback highlights that this change masks severe underlying state corruption issues. Re-initializing a completion while it is still in the intrusive queue resets its next pointer, severing the queue and permanently orphaning subsequent completions. Additionally, resetting a pending timer can trigger double-cancellation and corrupt the intrusive heap. Instead of skipping null results, the event loop should prevent completions from being re-initialized while queued, or safely dequeue them first.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // state below; the re-armed completion fires on its own. Windows | ||
| // IOCP support is WIP (see README); this guards a completion-reuse | ||
| // race observed with the cursor blink timer (renderer thread). | ||
| if (c.result == null) continue; |
There was a problem hiding this comment.
⚠️ Critical Correctness & State Corruption Issues
While this guard prevents the immediate "attempt to use null value" panic, it papers over two extremely severe internal state corruptions within the event loop:
1. Intrusive Queue Corruption via c.* = .{...}
self.completions is an intrusive queue (queue.Intrusive(Completion)). It relies on the next pointer inside the Completion struct to maintain the linked list.
- When a completion
cis inself.completions(waiting for its callback to be processed), andloop.timer(c, ...)is called to re-initialize it, the struct assignmentc.* = .{...}is executed. - This assignment resets the entire struct, setting
c.nexttonull(its default value). - If there were other completions queued after
c(e.g.,c -> D -> E), the link toDis severed. - When
cis popped,self.completions.headbecomesc.next(which is nownull). Any subsequent completions (D,E, etc.) are permanently orphaned and lost, causing their callbacks to never fire (leading to silent hangs/stalls).
2. Double-Cancellation & Heap/Queue Corruption via timer_reset
If a timer T is canceled, it is pushed to self.completions with its state purposely remaining .active so the active count can be decremented later.
- If
timer_resetis called onTwhile it is still pending inself.completions,timer_resetsees the.activestate and schedules a new cancellationc_cancel2. - When
c_cancel2is processed,stop_completionis called onTagain. - This attempts to remove
Tfromself.timers(where it no longer exists, corrupting the intrusive heap) and pushesTtoself.completionsa second time (corrupting the queue or creating an infinite loop cycle).
Recommendation
Re-initializing or re-arming a completion that is currently pending in the completion queue is a violation of the event loop's ownership model. Instead of skipping null results, the event loop needs to prevent completions from being re-initialized while they are still members of an intrusive queue, or safely disassociate/dequeue them first.
| // state below; the re-armed completion fires on its own. This | ||
| // mirrors the same guard in the IOCP backend and protects against | ||
| // a completion-reuse race (e.g. the cursor blink timer). | ||
| if (c.result == null) continue; |
There was a problem hiding this comment.
⚠️ Critical Correctness & State Corruption Issues
While this guard prevents the immediate "attempt to use null value" panic, it papers over two extremely severe internal state corruptions within the event loop:
1. Intrusive Queue Corruption via c.* = .{...}
self.completions is an intrusive queue (queue.Intrusive(Completion)). It relies on the next pointer inside the Completion struct to maintain the linked list.
- When a completion
cis inself.completions(waiting for its callback to be processed), andloop.timer(c, ...)is called to re-initialize it, the struct assignmentc.* = .{...}is executed. - This assignment resets the entire struct, setting
c.nexttonull(its default value). - If there were other completions queued after
c(e.g.,c -> D -> E), the link toDis severed. - When
cis popped,self.completions.headbecomesc.next(which is nownull). Any subsequent completions (D,E, etc.) are permanently orphaned and lost, causing their callbacks to never fire (leading to silent hangs/stalls).
2. Double-Cancellation & Heap/Queue Corruption via timer_reset
If a timer T is canceled, it is pushed to self.completions with its state purposely remaining .active so the active count can be decremented later.
- If
timer_resetis called onTwhile it is still pending inself.completions,timer_resetsees the.activestate and schedules a new cancellationc_cancel2. - When
c_cancel2is processed,stop_completionis called onTagain. - This attempts to remove
Tfromself.timers(where it no longer exists, corrupting the intrusive heap) and pushesTtoself.completionsa second time (corrupting the queue or creating an infinite loop cycle).
Recommendation
Re-initializing or re-arming a completion that is currently pending in the completion queue is a violation of the event loop's ownership model. Instead of skipping null results, the event loop needs to prevent completions from being re-initialized while they are still members of an intrusive queue, or safely disassociate/dequeue them first.
Record / fork-local patch. This PR documents the fix carried by this fork; it is not intended for upstream merge as-is (upstream IOCP is WIP and the related upstream issues are stalled — see below). It is the single patch that
nanasess/ghostinthewslpins viabuild.zig.zon.Problem
A completion popped from the loop's completion queue is unwrapped with
c.result.?("Completion queue items MUST have a result set"). That invariant can be violated by a completion-reuse race: if a completion is re-initialized and re-queued elsewhere while it is still linked in the completion queue,loop.timer(c.* = .{...}) resetsresultto null, so the later pop hitsc.result.?and panics ("attempt to use null value").Observed on Windows/IOCP via the Ghostty cursor-blink timer (renderer thread): the cursor timer completion is re-armed/reset/canceled from several renderer-thread events, and an interleaving leaves a stale entry whose result was cleared.
Fix
Guard both the IOCP and kqueue completion loops: when a popped entry has a null result, treat it as a stale entry and skip it without clobbering its now-reused state (the re-armed completion fires on its own).
Scope / caveats
iocp.zig): the actually-affected backend; verified empirically (Debug build, multi-day soak, no recurrence; prior scroll-stall precursor gone).kqueue.zig): included for parity since it has the identicalc.result.?pattern, but untested on macOS — the consumer (ghostinthewsl) is Windows-only and never exercises this path.34fa508); this fork is intentionally "behind" upstream main (pinned commit + minimal patch).Relation to upstream
nextand does not touchiocp.zig, so this case is uncovered upstream.Consumer: nanasess/ghostinthewsl#4 (issue) / nanasess/ghostinthewsl#5 (override PR).
Disclosure: drafted with AI assistance and verified empirically; the author is not deeply familiar with Zig.
Closes #2.