Add clear script environment cache command (PEP 723 PR 13/16) - #8
Add clear script environment cache command (PEP 723 PR 13/16)#8StellaHuang95 wants to merge 7 commits into
Conversation
StellaHuang95
changed the title
|
🔒 Automated review in progress — @StellaHuang95 is auto-reviewing this PR. |
StellaHuang95
added
the
review-auto:changes-requested
## Summary Adds a package-manager-centric integration baseline that intentionally precedes and de-risks microsoft#1686, so the package-manager command refactor is exercised against behavior established on `main`. - drives one stateful install/list/direct-package/uninstall lifecycle per active profile - uses unique disposable projects and manager-owned disposable environments - exercises the live registered manager instances through a runtime-gated integration-test bridge - guards registry completeness so every registered package-manager ID has an active fixture or explicit deferral - covers normal Pip execution and Conda when their runtime prerequisites are available - records an uncached baseline instead of assuming a newly created environment is empty - restores workspace-scoped configuration from `inspect()` snapshots and performs guarded failure-safe cleanup - defers Poetry pending a Poetry-owned project/lockfile lifecycle - defers uv-backed Pip because changing the machine-scoped selection reliably within one extension host was not stable on `main`, while available-version lookup would also introduce `uv tool run pip` network seeding - pins the disposable integration-test user profile to normal Pip execution ## Validation - `npm run compile` - `npm run compile-tests` - `npm run lint` - `npm run unittest` - targeted `packageManagement.integration.test.js`: 3 passing, 2 prerequisite skips locally - Pip skipped because quick create selected Python 3.15.0 alpha, whose bundled Pip metadata is incomplete - Conda skipped because Conda is not installed - reviewer specialist: clean, no Critical or Important findings The active Pip and Conda fixtures require package-index/network access when their runtime prerequisites are present. Fixes microsoft#1701 --------- Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480 Copilot-Session: 3fd1a810-6840-4ac9-ac33-c8a9fda4bfc4
|
Additional review findings addressed in 6c5f4dd:
|
| } | ||
| await fsapi.rename(ownerMarker, retainedMarker); | ||
| } catch (_error) { | ||
| throw createLockError('Failed to mark the lock as retained', 'ERETAINFAILED', lockPath); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Set state to retained only after the rename succeeds. If retain() fails today, release() will not remove the surviving owner marker because the lock is already marked retained; add a retain-failure-then-release regression test.
| ); | ||
| try { | ||
| await fsapi.rename(path.join(lockPath, snapshot.marker), claimedMarker); | ||
| } catch (error) { |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
If the process is interrupted after renaming to .reclaim-* but before unlinking it, future inspection permanently classifies the lock as malformed. Add identity-safe recovery for abandoned reclaim markers and cover interruption between claim and unlink.
|
|
||
| if (workspaceEntries.length === 0) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
With no open workspace folders, this returns before inspecting global pythonProjects, so inline entries can remain after cache deletion. Handle global configuration independently and add an empty-window test.
| if (manager) { | ||
| if (manager && manager.id !== INLINE_SCRIPT_MANAGER_ID) { | ||
| await manager.clearCache(); | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The generic registry now hardcodes the inline manager's cache-clearing policy, including silently ignoring an explicitly scoped clear. Express bulk-clear eligibility as a manager capability or keep this routing policy in the command layer.
| const activeCreatesAtStart = this.activeCreateOperations; | ||
| return this.enqueueCacheMaintenance(() => | ||
| this.enqueueSelection(() => this.clearCacheInternal(activeCreatesAtStart)), | ||
| ); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
A create queued behind clear A increments activeCreateOperations before passing the barrier, causing queued clear B to reject even though creation cannot start until B finishes. Count active creation only after the barrier and add a clear-create-clear ordering test.
|
|
||
| private async clearCacheInternal(activeCreatesAtStart: number): Promise<void> { | ||
| if (activeCreatesAtStart > 0) { | ||
| const message = l10n.t( |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Locking, physical ownership checks, deletion, persistence reconciliation, and event invalidation are now embedded in an already broad manager. Extract an inline-cache maintenance component before TTL eviction extends this path further.
StellaHuang95
added
review-auto:approved
) Replace the existing activity bar icon with a new design more aligned with the wider codicon design language.  Co-authored-by: mrleemurray <mrleemurray@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3cb82ae9-7424-40a4-9156-8c54ac6e0895
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3cb82ae9-7424-40a4-9156-8c54ac6e0895
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3cb82ae9-7424-40a4-9156-8c54ac6e0895
Coordinate per-entry deletion locks, keep partial failures consistent, and clean inline project settings safely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
Claim exact stale or retained lock markers before inline cache cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
fa55826 to
8fb0a8b
Compare
|
Upstream review continues in microsoft#1724. Closing this fork-only review PR as superseded. |
| export type ProcessLiveness = 'live' | 'dead' | 'unavailable'; | ||
| export type FileLockState = 'missing' | 'held' | 'retained' | 'stale' | 'orphaned' | 'malformed' | 'unavailable'; | ||
|
|
||
| export interface InspectFileLockOptions { |
There was a problem hiding this comment.
Issue · Please address or respond
ProcessLiveness, FileLockState, InspectFileLockOptions, and getProcessLiveness have no external consumers. Keep them module-private; callers can use the injected callback structurally without exporting these implementation details.
| } | ||
|
|
||
| function resolvePythonProjectSettingSource( | ||
| setting: PythonProjectSettings, |
There was a problem hiding this comment.
Issue · Please address or respond
Move these named interfaces to the bottom of the file to comply with the repository's typescript-named-types-at-bottom convention.
|
|
||
| export async function addPythonProjectSetting(edits: EditProjectSettings[]): Promise<void> { | ||
| const noWorkspace: EditProjectSettings[] = []; | ||
| const workspaces = new Map<WorkspaceFolder, EditProjectSettings[]>(); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This generic settings module now selects inline-owned entries and decides which loaded projects to unload. Keep source-resolution primitives here, but move inline lifecycle orchestration behind an inline-script-owned maintenance boundary.
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/managers/builtin/inlineScript/envManager.ts:L1724.
This manager now combines maintenance coordination, lock reclamation, containment, deletion, persistence, and invalidation while depending on destructive-path predicates from |
| cacheRoot, | ||
| physicalCacheRootPath, | ||
| entryName, | ||
| ); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Another process can create a previously absent cache entry after this enumeration, letting cleanup report success while cache contents remain. Use a cross-process root maintenance lock shared with creation, or prove completeness through repeated locked enumeration.
| const updatedSettings = cloneSettings(value as PythonProjectSettings[] | undefined); | ||
| if (configurationTarget === ConfigurationTarget.Workspace) { | ||
| sharedWorkspaceValue = updatedSettings; | ||
| } else if (configurationTarget === ConfigurationTarget.WorkspaceFolder) { |
There was a problem hiding this comment.
Issue · Please address or respond
Move createProjectConfig, cloneSettings, and createSharedWorkspaceConfigs to the end of the enclosing suite to comply with tests-helper-placement.
| sinon.stub(windowApis, 'showWarningMessage').resolves('Clear Cache' as never); | ||
| const removeInlineSettings = sinon.stub(settingHelpers, 'removeInlineScriptPythonProjectSettings').resolves([]); | ||
|
|
||
| await assert.rejects(clearScriptEnvironmentCacheCommand(envManagers, projectManager), /could not be deleted/); |
There was a problem hiding this comment.
Issue · Please address or respond
This rejection assertion uses a partial regex for a deterministic stubbed error. Capture the rejection and compare its complete message or a stable structured identity.
|
|
||
| await assert.rejects( | ||
| unsafeManager.clearCache(), | ||
| /unsafe cache root/, |
There was a problem hiding this comment.
Issue · Please address or respond
The clear-cache suite uses partial regex assertions for deterministic PR-authored errors. Capture and assert complete messages, normalizing only genuinely dynamic path portions.
| await assert.rejects( | ||
| () => Promise.resolve(vscode.commands.executeCommand('python-envs.clearScriptEnvCache')), | ||
| /not found/i, | ||
| ); |
There was a problem hiding this comment.
Issue · Please address or respond
/not found/i is an undocumented partial assertion. The exact command-list assertion already proves absence; remove this host-text check or use the documented dynamic-output exception with stable validation.
|
|
||
| integration-tests-multiroot: | ||
| name: Integration Tests (Multi-Root) | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This cache-cleanup PR also includes API 1.2, package-management behavior, network integration, pip parsing, and logo changes. Split these feature streams so cache lifecycle changes can be reviewed, reverted, and released independently.
StellaHuang95
added
review-auto:changes-requested
3 participants
Part of microsoft#1602. Complete roadmap PR13 lifecycle cleanup implementation.
Summary
pythonProjects[]entries with workspace/workspace-folder/multiroot correctnessDestructive safety and locking
Default-off guarantee
When
python-envs.inlineScripts.enabledis absent/false:package.jsonor command-palette menuspython-envs.clearCachecommand is unchangedremovePythonProjectSetting()and existing project remove/delete behavior matchbde7cf8The command remains internal until the PEP 723 user experience is intentionally exposed.
Scope
TTL eviction remains PR14; routing, setup UX, telemetry, and status treatment remain separate roadmap work.
Validation