perf: eliminate macOS discovery timeout tail (Fixes #504) - #506
Merged
Conversation
Pin comparisons to the exact PR base, fail invalid or regressed performance and coverage snapshots, and harden the E2E JSONRPC client against buffered-output loss and stderr backpressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Capture existing RefreshProgress notifications in the E2E benchmark and emit deterministic phase and locator percentile distributions so cold-tail latency can be attributed before product changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Classify existing timeout warnings into privacy-safe categories in the E2E benchmark so the macOS cold tail can be tied to exact fallback probe counts without exposing paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Map Apple /usr/bin/python3 aliases through the active developer directory, remove duplicate Xcode and CommandLineTools probes, and skip unresolved shims before the spawn fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Karthik Nadig (karthiknadig)
August 10, 2026 20:09
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets the macOS cold-refresh ~30s tail latency by avoiding duplicate serialized spawn-based resolution of Apple’s /usr/bin/python3 shim. It adds filesystem-based mapping from the shim to the active developer toolchain Python (Xcode / CommandLineTools) and extends the e2e performance harness to collect/aggregate RefreshProgress telemetry and privacy-safe interpreter timeout categories.
Changes:
- Add macOS-specific utilities to detect and resolve
/usr/bin/python3(andpython3.<minor>) shims to the active developer directory without spawning Python. - Update macOS Xcode and Command Line Tools locators to reuse the file-based mapping and add
/usr/bin/python3as an alias only when it truly matches the discovered interpreter. - Enhance the e2e performance test harness to collect
RefreshProgresstelemetry, assert phase coverage per iteration, and classify interpreter probe timeouts into privacy-safe buckets.
Show a summary per file
| File | Description |
|---|---|
| crates/pet/tests/e2e_performance.rs | Collects RefreshProgress notifications, aggregates phase/locator timing stats, and classifies interpreter spawn timeouts into privacy-safe labels for CI metrics. |
| crates/pet/src/locators.rs | Skips spawn-based resolution for unresolved macOS /usr/bin/python3* shims during fallback resolution to avoid deterministic timeout tails. |
| crates/pet-python-utils/src/macos.rs | New filesystem-based resolver for macOS system Python shims (developer dir selection + alias handling) without spawning. |
| crates/pet-python-utils/src/lib.rs | Exposes the new macos module. |
| crates/pet-mac-xcode/src/lib.rs | Resolves /usr/bin/python3* inputs via file mapping and adds /usr/bin/python3 alias via the shared helper instead of spawning. |
| crates/pet-mac-commandlinetools/src/lib.rs | Same as Xcode locator: file-based shim mapping + shared alias injection, removing spawn-based probing of /usr/bin/python3. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Performance Report (Linux)Result: ✅ Within regression budgets
|
Performance Report (macOS)Result: ✅ Within regression budgets
|
Performance Report (Windows)Result: ✅ Within regression budgets
|
Test Coverage Report (Linux)Result: ✅ Within regression budget
|
Test Coverage Report (Windows)Result: ✅ Within regression budget
|
Copilot started reviewing on behalf of
Karthik Nadig (karthiknadig)
August 10, 2026 21:24
View session
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Karthik Nadig (karthiknadig)
August 10, 2026 21:38
View session
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Karthik Nadig (karthiknadig)
August 10, 2026 21:58
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
crates/pet/tests/e2e_performance.rs:418
interpreter_probe_timeout_labels()derives timeout counts by scanningstderr_tail, but that tail is explicitly bounded (STDERR_TAIL_LINES = 100). If PET emits >100 stderr lines after a spawn timeout, the timeout warning can be evicted from the tail and the test will undercount (potentially masking a regression).
Consider tracking timeout labels/counts in spawn_stderr_reader as lines are read (store only matching labels/counters, which should stay small) instead of scanning the bounded tail for matches.
fn interpreter_probe_timeout_labels(&self) -> Vec<&'static str> {
self.stderr_tail
.lock()
.expect("PET stderr tail mutex poisoned")
.iter()
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Karthik Nadig (karthiknadig)
August 10, 2026 22:09
View session
7 tasks
Karthik Nadig (karthiknadig)
marked this pull request as ready for review
August 10, 2026 22:28
Paul (pwang347)
approved these changes
Aug 10, 2026
Karthik Nadig (karthiknadig)
added a commit
that referenced
this pull request
Aug 11, 2026
## Summary Recalibrate macOS P95 regression budgets now that #506 removed the deterministic 30-second interpreter-probe tail. The new limits preserve 4-6x observed hosted-runner variance while blocking multi-second drift that the old baseline-era limits allowed. ## Changes - tighten startup P95 from 10,000ms to 750ms absolute headroom - tighten full-refresh P95 from 5,000ms/25% to 1,000ms/50% - tighten time-to-first P95 from 10,000ms to 750ms absolute headroom - prove the worst observed post-fix run remains within budget - prove 1.5s/2.5s/1.5s synthetic regressions now fail - document calibration from three unchanged-content runs and the exact merged baseline ## Validation - `python -m unittest discover -s scripts/tests -p 'test_*.py' -v` (22 passed) - worst observed macOS artifact passes against merged baseline `f0c62d9` Fixes #507 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5 tasks
pull Bot
pushed a commit
to tangzixuan/zed
that referenced
this pull request
Aug 12, 2026
…#62534) # Objective Fixes zed-industries#62529. Prevent Python toolchain discovery from executing Apple's `/usr/bin/python3` Command Line Tools shim when no active developer Python exists. Executing that unresolved shim causes macOS to repeatedly prompt users to install the Command Line Tools, even when Python is managed through Nix, uv, or pyenv. ## Solution Update Python Environment Tools (PET) to `bb8e046`, which includes microsoft/python-environment-tools#506. That upstream change resolves active Xcode and Command Line Tools Python executables from filesystem state and skips unresolved macOS system Python shims before generic process probing. Adapt Zed to the updated PET API by: - displaying the newly supported Hatch environment kind - passing no refresh identifier when running in-process environment discovery ## Testing - `cargo check -p languages --locked` - PET unit tests covering unresolved macOS system Python shims - The Zed compilation check was run on Linux; the original dialog reproduction was not manually tested on macOS. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - Fixed Python toolchain discovery prompting installation of Apple Command Line Tools when using a separately managed Python installation.
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.
Summary
Eliminate the deterministic macOS cold-refresh tail caused by duplicate serialized probes of Apple's
/usr/bin/python3shim. Resolve active developer Python executables from filesystem state instead, while preserving legacy/usr/bin/pythonbehavior.Changes
DEVELOPER_DIR,xcode_select_link, and standard developer-directory fallbacks without spawning PythonValidation
Fixes #504