Skip to content

fix: publish coherent request snapshots (Fixes #536) - #566

Draft
Karthik Nadig (karthiknadig) wants to merge 2 commits into
mainfrom
fix/issue-536-request-snapshots
Draft

Karthik Nadig (karthiknadig) wants to merge 2 commits into
mainfrom
fix/issue-536-request-snapshots

Conversation

@karthiknadig

Copy link
Copy Markdown
Member

Give find, resolve, and refresh coherent configuration/generation/locator snapshots while preparing replacement graphs outside the published-state lock.

  • Atomically publish configured graphs and retain generation-gated reporting/state synchronization.
  • Preserve compatible Poetry/global-cache behavior and rebind PyEnv/WindowsRegistry when Conda changes.
  • Keep callback panics from poisoning publication/configuration gates; add deterministic race, cache, rebinding, and panic regressions.
  • Document actual locator/cache lifetimes.

Validation: full PET package (123 tests), affected locator/cache suites, exact all-target/all-feature Clippy, mandatory precommit, independent source review, and signed commit 74c93c9.

Performance hold: all eight counterbalanced Windows fast/stress passes preserved inventory, response ownership, cache probes 1/0/0, and resource bounds. Stress 1000-environment median was 8.29s base / 7.56s head, but fast 100-environment timing was 431ms / 1061ms with substantial within-head variation on a busy shared host. All measurements are retained; this is not a no-regression claim. Keep draft pending hosted quality and further isolated performance evidence.

Fixes #536

Build configured locator graphs off-lock and atomically publish request state. Preserve compatible caches and rebind nested Conda dependencies. Keep callback panics from poisoning publication gates, and prove snapshot, cache, and generation boundaries with deterministic tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Performance Report (Linux)

Result: ✅ Within regression budgets

Metric PR Baseline Delta Change Blocking budget Status
Server startup P50 1ms 1ms +0ms +0.0% >5ms and >100% ➖
Server startup P95 1ms 1ms +0ms +0.0% >50ms and >200% ➖
Discovery duration P50 48ms 48ms +0ms +0.0% >25ms and >30% ➖
Discovery duration P95 51ms 52ms -1ms -1.9% >50ms and >50% ✅
Startup-to-first environment P50 10ms 10ms +0ms +0.0% >20ms and >100% ➖
Startup-to-first environment P95 14ms 12ms +2ms +16.7% >25ms and >100% 🔺
Cold discovery duration P50 123ms 108ms +15ms +13.9% >100ms and >50% 🔺
Refresh round-trip P50 48ms 48ms +0ms +0.0% >25ms and >30% ➖
Refresh round-trip P95 51ms 52ms -1ms -1.9% >50ms and >50% ✅
Request-to-first environment P50 9ms 9ms +0ms +0.0% >20ms and >100% ➖
Request-to-first environment P95 13ms 11ms +2ms +18.2% >25ms and >100% 🔺
Cold refresh round-trip P50 123ms 108ms +15ms +13.9% >100ms and >50% 🔺
Workload PR Baseline
Environments 5 5
Managers 1 1

A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Test Coverage Report (Linux)

Result: ✅ Within regression budget

Metric PR Baseline Delta
Lines 86.251% 85.548% +0.703pp
Functions 88.606% 88.368% +0.239pp

Allowed numerical tolerance: 0.01 percentage points.

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Performance Report (macOS)

Result: ✅ Within regression budgets

Metric PR Baseline Delta Change Blocking budget Status
Server startup P50 114ms 78ms +36ms +46.2% >100ms and >50% 🔺
Server startup P95 638ms 868ms -230ms -26.5% >750ms and >100% ✅
Discovery duration P50 166ms 146ms +20ms +13.7% >100ms and >50% 🔺
Discovery duration P95 244ms 223ms +21ms +9.4% >300ms and >100% 🔺
Startup-to-first environment P50 139ms 109ms +30ms +27.5% >150ms and >50% 🔺
Startup-to-first environment P95 190ms 175ms +15ms +8.6% >250ms and >100% 🔺
Cold discovery duration P50 355ms 269ms +86ms +32.0% >250ms and >50% 🔺
Refresh round-trip P50 167ms 147ms +20ms +13.6% >250ms and >50% 🔺
Refresh round-trip P95 245ms 231ms +14ms +6.1% >300ms and >100% 🔺
Request-to-first environment P50 39ms 41ms -2ms -4.9% >50ms and >50% ✅
Request-to-first environment P95 74ms 74ms +0ms +0.0% >100ms and >100% ➖
Cold refresh round-trip P50 356ms 270ms +86ms +31.9% >600ms and >50% 🔺
Workload PR Baseline
Environments 10 10
Managers 1 1

A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Performance Report (Windows)

Result: ✅ Within regression budgets

Metric PR Baseline Delta Change Blocking budget Status
Server startup P50 7ms 11ms -4ms -36.4% >10ms and >50% ✅
Server startup P95 10ms 14ms -4ms -28.6% >50ms and >100% ✅
Discovery duration P50 104ms 162ms -58ms -35.8% >150ms and >50% ✅
Discovery duration P95 120ms 173ms -53ms -30.6% >250ms and >100% ✅
Startup-to-first environment P50 16ms 29ms -13ms -44.8% >25ms and >50% ✅
Startup-to-first environment P95 24ms 45ms -21ms -46.7% >100ms and >100% ✅
Cold discovery duration P50 103ms 164ms -61ms -37.2% >150ms and >50% ✅
Refresh round-trip P50 105ms 163ms -58ms -35.6% >150ms and >50% ✅
Refresh round-trip P95 120ms 174ms -54ms -31.0% >250ms and >100% ✅
Request-to-first environment P50 8ms 18ms -10ms -55.6% >25ms and >50% ✅
Request-to-first environment P95 17ms 34ms -17ms -50.0% >100ms and >100% ✅
Cold refresh round-trip P50 103ms 165ms -62ms -37.6% >150ms and >50% ✅
Workload PR Baseline
Environments 8 8
Managers 1 1

A regression must exceed both the documented absolute and relative budget. Environment and manager inventories must match exactly within the same inventory schema.

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Test Coverage Report (Windows)

Result: ✅ Within regression budget

Metric PR Baseline Delta
Lines 84.136% 83.075% +1.061pp
Functions 86.344% 85.753% +0.591pp

Allowed numerical tolerance: 0.01 percentage points.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad concurrency and cache-ownership changes require final human review and the pending hosted performance evidence noted in the description.

Review effort: Balanced
Findings: None

What changed in this PR

Introduces coherent configuration snapshots for find, resolve, refresh, and telemetry while preparing locator graphs outside publication locks.

Changes:

  • Atomically publishes configuration, generation, and locator graphs.
  • Preserves/rebinds locator caches and dependencies across configuration changes.
  • Adds concurrency, panic, cache, and rebinding regression coverage and documentation.
File Description
docs/​LOCATOR_STATE.md Documents snapshot, locator, and cache lifetimes.
crates/​pet/​src/​jsonrpc.rs Implements coherent request snapshots and atomic publication.
crates/​pet-windows-registry/​src/​lib.rs Rebinds cached registry state to replacement Conda locators.
crates/​pet-python-utils/​src/​cache.rs Atomically retains and returns the effective cache directory.
crates/​pet-pyenv/​src/​lib.rs Rebinds PyEnv while preserving shared discovery caches.
crates/​pet-poetry/​src/​lib.rs Tests cache fidelity under identical configuration.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Seed the cache with production-normalized identities and assert an explicit Windows executable case alias. Preserve the missing-manager/project setup so fidelity still depends on cached metadata.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@karthiknadig

Copy link
Copy Markdown
Member Author

Published signed follow-up a7d7af8 after an independent clean review. The Windows CI failures shared one new test-fixture bug: the fixture seeded raw temporary-path aliases into Poetry's cache while real request identities are normalized. I reproduced the old failure locally with one exactly selected test under a case-aliased temporary directory; the repair uses the production identity constructor for the seeded cache and explicitly checks a Windows executable case alias.

Only test code changed. The exact default-environment test, all 42 Poetry tests, exact workspace Clippy, and mandatory precommit pass. The worker could not execute the temporary-directory override, so native Windows CI remains the decisive confirmation; I am not claiming that specific local variant passed.

All three hosted performance jobs on the unchanged production revision 74c93c9 passed (discovery P50: Linux 57ms vs48, Windows155 vs162, macOS137 vs146). Those ordinary-inventory checks do not resolve the separate local 100-environment timing anomaly. This PR remains draft with the explicit performance hold; no threshold changes or rerun-to-green attempts.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The concurrency-sensitive ownership redesign remains draft pending hosted quality checks and isolated performance validation.

Review effort: Balanced
Findings: None

@karthiknadig

Copy link
Copy Markdown
Member Author

Final-head quality inspection for a7d7af8: all automated checks now pass, including Windows x64/ARM64 and both native macOS test jobs. The current-head Copilot review has no findings, and there are no unresolved review threads. All three CodeQL analyses report zero results, warnings, or errors.

Coverage increased by +0.703pp on Linux, +1.061pp on Windows, and +1.037pp on native macOS. Ordinary matched-inventory discovery P50 is 48/48ms on Linux, 104/162ms on Windows, and 166/146ms on macOS (head/base); the macOS +20ms/+13.7% is within unchanged budgets.

The separate performance hold remains: the retained eight-pass local session experiment showed +146.46% at fast 100-environment warm refresh, with very large variation between the two head runs. Passing ordinary small-inventory CI does not resolve that result. Keep this draft until isolated, workload-matched evidence resolves or bounds it; no measurements were discarded and no budgets were changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give find and resolve coherent configuration snapshots without lock-held discovery

2 participants