chore: revert non-CI changes from main branch#5
Merged
Conversation
Remove accidental non-CI changes that should not be part of the DevOps-to-GHA CI migration PR: - Revert KernelExtensions.cs Barrier refactor - Revert JupyterKernel.cs and JupyterKernelCommandHandlers.cs inline-await refactors - Revert README.md to upstream content - Revert Directory.Build.targets whitespace change Package-lock.json registry URL changes (npmjs.org) are kept as they are required for npm ci to work on public GitHub Actions runners.
On fast Linux GHA runners the Jupyter reply can arrive before reply.ToTask(token) subscribes to the observable, causing the task to hang indefinitely (missed message, no more OnNext ever fires). Subscribe first, then send -- so no reply is ever missed: var replyTask = reply.ToTask(token); // subscribe await sender.SendAsync(request); // then send var results = await replyTask; Same fix applied to processMessages in JupyterKernelCommandHandlers. This was the root cause of consistent 10-min hang-dumps on Linux CI.
Add two deterministic tests using Subject<Message> (identical to the real IMessageReceiver.Messages hot observable) that prove: - Buggy order (send then subscribe): reply emitted before ToTask() subscribes is permanently missed -- task hangs, never completes. - Fixed order (subscribe then send): subscription is active when reply arrives -- task completes immediately with the correct result. This documents the root cause of the consistent 10-min hang-dumps seen on Linux GHA runners where in-process mock senders complete synchronously before ToTask() establishes its subscription.
Add two test classes to Microsoft.DotNet.Interactive.Jupyter.Tests: - RunOnKernelAsyncRaceConditionTests: pattern-level proof using Subject<Message> directly. Shows that fire-before-subscribe on a hot observable permanently misses the message (buggy order), and subscribe-before-fire captures it (fixed order). Verified passing locally. - JupyterKernelCreateAsync_SynchronousSenderTests: integrated test calling JupyterKernel.CreateAsync with a SynchronousKernelInfoSender that fires KernelInfoReply synchronously inside SendAsync. Fails with upstream code (subscribe-after-send); passes with the fix (subscribe-before-send). Also add InternalsVisibleTo for the test project to the Jupyter csproj so JupyterKernel (internal class) is accessible from tests.
…ition test The previous approach used InternalsVisibleTo on the production assembly to access JupyterKernel (internal class) directly from tests. This is unnecessary and obscures the design. The new approach tests through the existing public test infrastructure: SynchronousKernelInfoTracker (IMessageTracker) -> TestJupyterKernelConnection -> TestJupyterConnection -> SimulatedJupyterConnectionOptions -> CreateJupyterKernelAsync This exercises the full production stack (ConnectJupyterKernelDirective -> JupyterKernelConnector.CreateKernelAsync -> JupyterKernel.CreateAsync -> RunOnKernelAsync) without any InternalsVisibleTo. Also removes the RunOnKernelAsyncRaceConditionTests pattern tests, which only exercised System.Reactive and did not test any production code.
BenjaminMichaelis
added a commit
that referenced
this pull request
May 29, 2026
…-non-ci-changes chore: revert non-CI changes from main branch
BenjaminMichaelis
added a commit
that referenced
this pull request
May 29, 2026
…-non-ci-changes chore: revert non-CI changes from main branch
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.
The fork's
mainbranch is a PR submitted upstream todotnet/interactive. The only changes it should contain are the DevOps-to-GHA CI migration. Several accidental non-CI changes had crept in and need to be removed before the upstream PR can be cleanly reviewed.What was reverted
Five files restored to their upstream
dotnet/interactivestate:src/dotnet-interactive/KernelExtensions.cs-- Barrier-based form input synchronization refactor (replaced TaskCompletionSource approach)src/Microsoft.DotNet.Interactive.Jupyter/JupyterKernel.cs-- inline-await refactorsrc/Microsoft.DotNet.Interactive.Jupyter/JupyterKernelCommandHandlers.cs-- inline-await refactorREADME.md-- fork branding ("Polyglossy Interactive") that must not go upstreamDirectory.Build.targets-- spurious blank lineWhat was intentionally kept
The
src/polyglot-notebooks*/package-lock.jsonfiles contain registry URL changes frompkgs.dev.azure.com(Azure DevOps internal feed) toregistry.npmjs.org. These are CI-related -- they are required fornpm cito succeed on public GitHub Actions runners and must stay in the upstream PR.