You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Treat frame-boundary stdin EOF as normal server shutdown instead of repeatedly logging empty headers. Cancel admitted subprocess probes through ownership cleanup, without waiting indefinitely for blocked transport I/O.
Distinguish clean EOF from truncated frames and terminal transport failures; preserve errors that race shutdown.
Use bounded FIFO output with a dedicated owned stdout handle, explicit saturation errors, and documented cancellation semantics.
Supervise probe admission and cleanup; report cleanup failures or the three-second cleanup deadline explicitly.
Close test-client stdin and bound normal exit, forced termination, and reader teardown.
Add deterministic transport/output/supervisor coverage and native EOF, unread-output, broken-pipe, and active-descendant regressions.
Validation: Windows and Linux affected suites and CI-feature native tests pass; 80 concurrent lifecycle regression cases pass. Workspace formatting/Clippy and targeted all-target Clippy pass. Independent Reviewer returned no findings. macOS and hosted quality results remain pending.
Multi-header framing and envelope validation remain separate work under #532.
A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.
A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.
A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.
Current-head Copilot review completed with no findings. CodeQL's three analyses report zero results/errors/warnings, and Windows/Linux performance improved against the exact base.
Keeping this draft while addressing two quality findings rather than relying on green performance budgets:
Linux line coverage improved (+0.345pp), but function coverage decreased by 0.153pp. Native isolated test launchers clear LLVM_PROFILE_FILE, leaving actual server entry points unrecorded despite successful native tests. A narrowly scoped profiling-environment fix is being measured with instrumented subprocess tests.
macOS refresh RTT P50 increased from 133ms to 155ms, with larger startup-to-first-environment drift. Preparing a manual-only, read-only same-host base/head control with counterbalanced order and exact inventory identity checks before accepting that drift.
No signing/notarization/release services are involved in these checks.
Keep LLVM_PROFILE_FILE in isolated PET subprocess environments so normal EOF shutdown writes profiles into the coverage collector instead of untracked default files. Preserve fixture overrides and minimal environment isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Establish a bounded info exchange before closing the same stdout pipe, so startup is not charged to the unchanged one-second shutdown deadline. Explicitly terminate and bounded-join the readiness helper on setup failure.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Concurrent Unix fork/exec can retain another fixture pipe reader until exec, allowing the sole response write to succeed before the final reader disappears. Run the same real-pipe scenario in an isolated test subprocess; keep the one-second measured deadline and bounded cleanup unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The same-host macOS control completed: https://github.com/microsoft/python-environment-tools/actions/runs/36168462886. Eight counterbalanced passes (B-H-H-B-H-B-B-H), exact base 094217e and production head d9ee6aa, verified binary hashes, and identical full inventories (10 environments, 1 manager). Later commits change tests only.
These are medians of per-pass percentiles, not pooled percentiles:
Metric
Base P50/P95
Head P50/P95
Refresh round trip
127 / 163.5 ms
118 / 162 ms
Startup to first environment
103 / 132.5 ms
104 / 129.5 ms
Cold refresh round trip
292 / 428 ms
281.5 / 331 ms
Request to first environment
29.5 / 41.5 ms
34 / 49 ms
Cold request to first environment
29 / 39.5 ms
38.5 / 50.5 ms
The large aggregate slowdown in unmatched CI snapshots was not reproduced. The smaller first-notification increase is real, especially the consistent cold P50 difference, not dismissed as noise. Accepted as an explicit small-absolute-latency tradeoff for bounded transport/shutdown, with stable end-to-end first-environment latency and improved complete refresh time. Independent review found no added normal-path timer/batching delay; thread handoffs are plausible, not a proven attribution. Scope: x86_64 under Rosetta on one arm64 macOS host; four passes per revision, so this is not a universal performance claim. Budgets remain unchanged.
The coverage-profile inheritance fix has also been verified by hosted Linux and Windows coverage improvements, without exclusions or budget changes.
Further fixture stress exposed a separate, reproducible Unix fork/exec race: another test's fork can temporarily retain a CLOEXEC pipe reader, so the only response write succeeds before that last reader closes. The server then correctly has no write failure to observe. 60d8d5c isolates the real-pipe scenario in its own test process, retaining the one-second measured shutdown assertion, stdin-open proof, and bounded failure cleanup. Ten full parallel Linux suites (140 cases) passed, as did Windows validation and independent review. The latest test-only head is going through hosted checks; keeping this PR draft until those finish.
Allow the outer test-process guard to cover readiness, both forced-shutdown waits, reader completion, and fallback cleanup without changing the one-second shutdown assertion. Record the verified subprocess testing pitfalls in the existing Rust skill.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Fixture cleanup has an exit-versus-kill race that can skip reaping and falsely report failure.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Recheck child exit after kill error to avoid false cleanup failure
crates/pet/tests/process_utils.rs:35
A child can exit after wait_for_exit times out but before kill() runs. On platforms where killing an already-exited process returns an error, ? skips the final reap and falsely reports fixture cleanup failure. Recheck try_wait on a kill error before propagating it so this deadline race still reaps a normally exited child.
Poll the actual descendant lease using only the remainder of the existing four-second shutdown budget instead of requiring an instantaneous lock transition at server exit. Test held-lock timeout and success after release; preserve cancellation and elapsed-time assertions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The review threads are clear, but the final Windows job exposed an instantaneous lease-observation failure in the active-descendant fixture. 22a0e93 now waits for the actual OS-held lease using only the remainder of the original four-second shutdown budget, following the existing subprocess test pattern. No production code or shutdown deadline changed. An independent holder test proves that an unreleased lock times out and succeeds only after release.
Validation: all 15 native cases passed on Windows and Linux; 40 concurrent Windows active-descendant cases passed; formatting/Clippy and independent review are clean. An additional 80-case process-handle diagnostic did not reproduce the immediate lock race locally, so the precise hosted scheduling cause is not claimed as proven. Current-head Copilot review and hosted CI have been requested. Keeping draft until those gates are verified.
Release the independent holder only after the same polling invocation observes WouldBlock, and assert exactly two real lock attempts. This removes scheduling assumptions and rejects an immediate-timeout mutation without changing shutdown semantics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The corrected Windows job now passes, including the active-descendant test. A new macOS performance run exceeded discovery budgets (253 ms warm / 515 ms cold P50) despite no production changes since the previously passing 124 / 260 ms run. The earlier exact-base/head same-host control did not show this aggregate regression. I am retaining this failed evidence and rerunning only the macOS performance job once to check runner variability; budgets and code remain unchanged. The separate exact combined-protocol same-host control is also running. Auto-merge remains off until the results are inspected.
Final validation for 0fee376: all hosted build/test/coverage/performance/CodeQL checks have passed, including Windows active-descendant shutdown and both macOS architectures. Current-head Copilot review has no findings and there are no review threads. Linux/Windows line coverage improves by +1.995/+2.122 percentage points; all three CodeQL analyses report zero results, errors, and warnings. Linux performance is effectively neutral and Windows improves. The single macOS rerun passed without code or budget changes (warm/cold discovery P50 163/339 ms versus the anomalous 253/515 ms); the earlier failed run remains recorded above. Accepting runner-sensitive macOS aggregate variance based on the prior exact-production same-host control, while retaining its explicitly measured small first-notification tradeoff. Ready for protected auto-merge; required human approval must still be satisfied.
Merge main after #559 landed. The resolved tree is identical to the reviewed protocol head; the final PR diff now contains only protocol framing, envelope validation, tests, and documentation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Parse bounded multi-header JSONRPC frames and validate request envelopes
before dispatch, while preserving request IDs, notifications, and legacy
method-level error behavior.
- Read headers through the separator; accept case-insensitive names,
optional Content-Type, LF/CRLF, fragmented bytes, and consecutive
frames.
- Enforce exact 8 KiB header and 16 MiB payload limits before
allocation; reject duplicate, missing, invalid, and overflowing lengths.
- Return explicit errors for invalid envelopes/parameter containers and
recover after complete malformed JSON/UTF-8 frames.
- Add deterministic fragmentation, native wire, and
rejection-to-valid-dispatch regression coverage; document the supported
contract.
Validation: 51 library and 19 native cases; workspace formatting/Clippy
and independent code review pass. #559 is merged and main integrated;
only the five protocol files remain in this PR. Exact-main Linux/Windows
line and function coverage now improve, with no budget or exclusion
changes. Current-head Copilot has no code findings and all CodeQL
analyses are clean. The independently reviewed Windows matched-host
control found no material regression; detailed platform snapshots,
accepted tail outliers, and sampling limitations are recorded in the PR
comments. All 35 automated checks now pass, including both native macOS
jobs. Ready for required human approval and protected auto-merge.
Fixes#532
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Treat frame-boundary stdin EOF as normal server shutdown instead of repeatedly logging empty headers. Cancel admitted subprocess probes through ownership cleanup, without waiting indefinitely for blocked transport I/O.
Validation: Windows and Linux affected suites and CI-feature native tests pass; 80 concurrent lifecycle regression cases pass. Workspace formatting/Clippy and targeted all-target Clippy pass. Independent Reviewer returned no findings. macOS and hosted quality results remain pending.
Multi-header framing and envelope validation remain separate work under #532.
Fixes #529