Repository navigation
fix: probe kernel readiness without racing ipykernel initialization (ROADMAP.md #82) - #97
Merged
Merged
Conversation
…ROADMAP.md #82) start_session waited for readiness with a single kernel_info_request on the control channel, sent the instant kernel.json appeared. ipykernel registers the control dispatcher (kernelbase.py:551) before it assigns _control_lock (:559), so a probe landing in that window raises TraitError kernel-side and is dropped — and the one request we sent was then never answered, burning the whole remaining budget (the 59.5s CI hang). Readiness now rides the shell channel, whose handler is registered only after _control_lock is assigned, so a shell reply proves initialization finished. No execution can be in flight during a start, so the shell channel's busy-serialization caveat does not apply there. The probe loop the two helpers duplicated is extracted into _kernel_info_reply(kc, channel, timeout); _kernel_alive stays the control wrapper and _kernel_idle becomes _kernel_answers_shell. _kernel_dead, is_session_running, session_status's liveness check and exec_code's silence probe still use control: they must answer for a busy kernel, which only control does. Adds a stand-in kernel that answers kernel_info on shell only and leaves control bound but unserviced, which fails in 30s against the old control probe and passes in ~1s against the new one, plus a real-kernel guard that fires a control probe the instant kernel.json appears.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesSession readiness and channel probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes kernel startup readiness use a reply that reliably reflects initialization completion, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant start_session
participant shell_channel
participant control_channel
participant kernel
start_session->>shell_channel: Send kernel_info_request
shell_channel->>kernel: Deliver request
kernel-->>shell_channel: Return kernel_info_reply
start_session->>control_channel: Run liveness probe
control_channel->>kernel: Deliver control request
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
=====================================
Coverage 97.9% 98.0%
=====================================
Files 23 23
Lines 5138 5131 -7
Branches 992 991 -1
=====================================
- Hits 5035 5029 -6
Misses 49 49
+ Partials 54 53 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stop_channels() closes the client socket with linger=0, which discards anything ZMQ has not yet handed over — and right after send() on loopback the connect handshake is usually still in flight. Measured against a bound ROUTER with the test's exact code shape: the probe arrived 2/20 times, so test_start_session_survives_control_probe_during_init was mostly firing nothing. Holding the socket open until the start finishes takes it to 20/20. Also from review: - the intruder now retries instead of giving up when it reads a half-written connection file, catches only the (OSError, ValueError) _client documents rather than everything, drops the exists()/stat() TOCTOU, and asserts it actually finished; - start_session's comment names the ipykernel version and line numbers the shell-channel ordering was verified against, since pyproject sets no ipykernel ceiling; - the stand-in's cleanup comment gives the real reason conftest's reaper never sees it (argv is `python -c`, not ipykernel_launcher); - test_start_session_kernel_never_ready_stops_kernel's docstring names _kernel_answers_shell, the helper start_session now probes with.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
start_sessiondecided a kernel was ready from a singlekernel_info_requeston the control channel, sent the instant
kernel.jsonappeared. When thatrequest landed in a window inside ipykernel's startup it was dropped kernel-side
and never answered, so the start burned its whole remaining budget and failed
with "Kernel did not become ready within 30s". Readiness is now probed on the
shell channel, which cannot land in that window.
Why
The failure was a hard 59.5 s hang followed by a start error, on a kernel that
was in fact perfectly healthy — the mode's least recoverable failure, since the
kernel is killed on the way out and the diagnostic blames a slow machine.
How
ipykernel/kernelbase.pyKernel.start()registers the control dispatcher(l.551) before it assigns
_control_lock(l.559), and the control socket isbound and
kernel.jsonwritten long beforestart()runs. A control messagealready queued when
on_recvregisters is delivered on the control thread whilethe main thread is still on its way to l.559;
dispatch_controlopens withasync with self._control_lock(l.347) and raisesTraitErroron the unsettrait. The stream keeps polling — but the request is gone, and we only sent one.
The shell handler is registered at l.561, after the lock is assigned, so a
shell
kernel_info_replyproves initialization finished. Nothing on the shelldispatch path is assigned late (
_main_asyncio_lockinKernel.__init__,ShellChannelThread.asyncio_lockin that thread's__init__).Start here:
_kernel_info_reply(kc, channel, timeout)indocx_editor/session.pyis the probe loop both helpers duplicated;
_kernel_aliveis now its controlwrapper and
_kernel_idleis renamed_kernel_answers_shell. The one behaviouralline is the readiness check in
start_session._kernel_dead,is_session_running,session_status's liveness check andexec_code's silenceprobe deliberately keep control — they must answer for a busy kernel, which
only control does, and they run against an already-initialized kernel.
Testing
test_start_session_ready_when_control_never_answersuses a stand-in kernel thatanswers
kernel_infoon shell only and leaves control bound but unserviced. Itdiscriminates: with the readiness check reverted to
_kernel_aliveit fails in30.2 s with
SessionError: Kernel did not become ready within 5.0s; with theshell probe it passes in ~1 s.
test_start_session_survives_control_probe_during_initadds a real-kernel guard that fires a control probe the instant
kernel.jsonappears, then requires start,
is_session_runningandeval_code("1 + 1")tosucceed. That probe has to survive
stop_channels(), which closes the socketwith
linger=0: measured against a bound ROUTER, the probe arrived 2/20 timeswhen the socket was closed straight after
send()and 20/20 when held openuntil the start finished, so the guard holds it open.
tests/test_session.pyserial: 90 passed on every run (173.6 s, 174.7 s, 174.4 s,174.9 s, and 174.1 s after the last review fix), no flakes. Full suite
-n 4:1867 passed, 8 skipped, 2 xfailed (307.3 s, and 335.1 s on the final tree).
ruff format --checkandruff checkclean;ty checkshows only the7 pre-existing warnings, none in the touched files.
Summary by CodeRabbit