fix(notebook): stop abandoned environment mutations - #2222
Open
wen2zhou wants to merge 1 commit into
Open
Conversation
wen2zhou
force-pushed
the
codex/notebook-task-lifecycle
branch
from
September 5, 2026 17:00
c6ae173 to
e7b3b79
Compare
wen2zhou
marked this pull request as ready for review
September 5, 2026 17:07
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
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.
Problem
manage_environmentsandmanage_packagescould return an MCP timeout while the Micromamba process continued running in the Electron main process. Cancellation stopped at the MCP boundary: named-environment creation passed no signal, the network-sandboxdefaultSpawnpath had neither an abort signal nor a bounded deadline, and repeated mutations for the same environment waited behind the abandoned operation. In production logs this left Micromamba consuming a CPU core for more than 13 minutes; another install reported failure at 60 seconds and completed in the background at 78 seconds.Real application reproduction also exposed three contributors to failed or excessively slow environment creation on Windows: a stale PATH-selected Micromamba could override the verified pinned runner, working-cache publication spawned a PowerShell ACL probe for every extracted directory, and USTC package downloads redirected to
mirrors.nju.edu.cn, which the Notebook network policy rejected.Proposed change
AbortSignalfrom the MCP server through local RPC, environment creation, package installation, sandbox preparation, and the spawned installer process tree.lstat, realpath containment, reparse-point, and digest checks.mirrors.nju.edu.cnredirect target without allowing arbitrary redirected domains.Scope and non-goals
This changes the product's Electron main-process Notebook lifecycle, shared mirror policy, and the Windows certification fixture. It is not a machine-specific configuration workaround and adds no renderer copy or UI behavior.
The Windows lane is included because process-tree termination, ACL validation, Micromamba selection, and the reported failure were Windows-specific. Linux and macOS keep the same lifecycle contract and unit coverage, but this machine could not run real Micromamba cancellation E2E on those platforms. Progress notifications help clients that reset deadlines on progress; cancellation and process termination remain the correctness mechanism for clients with a fixed hard deadline.
Acceptance criteria and validation
All commands below were run after the last material code edit at commit
c6ae173573168ce36354ac1b1cd3330055625a82.npx vitest run src/main/notebook/mcp-management-lifecycle.integration.test.ts src/main/notebook/local-rpc-server.test.ts src/main/notebook/local-rpc-notebook-adapter.test.ts src/main/notebook/environment-management.test.ts src/main/notebook/environment-operations.test.tsnpx vitest run src/main/notebook/package-process-sandbox.test.tsnpx vitest run src/main/notebook/windows-micromamba-runner.test.ts src/main/notebook/micromamba-archive-store.test.ts src/main/notebook/provisioner.test.ts src/main/notebook/package-manager.test.ts src/main/notebook/package-mutation.test.tsnpx vitest run src/shared/notebook-network.test.ts src/main/notebook/mirror-probe.test.ts src/main/notebook/network-sandbox-owner.test.tsOPEN_SCIENCE_E2E_REAL_MICROMAMBA=1 node node_modules/@playwright/test/cli.js test e2e/certification/notebook-lifecycle.spec.ts --grep "creates and executes in a real" --workers=1python-docx, and imported it from the newe2e-realprefix.npm run lint;npx prettier --check <changed .ts/.tsx/.js/.mjs/.json/.md files>;git diff --check 726e1fb1 HEADnpm run test:affected -- -- --base 726e1fb1 --head HEADEPERMwere reproduced with the identical tests at baseline726e1fb1; the affected tests and implementations are unchanged in this branch.npm run typecheck:nodeTS1117duplicate-property errors in unchangedsrc/main/delegation/production-composition.test.tslines 962, 964, 965, and 972. No changed file reports a type error.Review focus
Please focus on the ownership boundary between MCP requests and background mutations, especially abort propagation through local RPC and lease acquisition; the guarantees around killing and confirming the installer process tree; and the decision to preserve recovery state when termination cannot be confirmed. For the Windows fixes, please also review that reducing ACL subprocess calls does not weaken archive traversal checks, and that the added redirect allowance is limited to the USTC mirror's known NJU target.