Runtime static pr c cache discovery - #86
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 2500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Move discovery cache, refresh tasks, and classification cache off process-wide static state onto DI-owned instances. Bind refresh to IQuickShellLifetime, inject IExtensionThreadScheduler, and convert ExtensionCallbackQueue to a provider-scoped service. Two ServiceProviders no longer share cache state.
… empty catch block' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <tonythethompson@hotmail.com>
…ath.Combine' may silently drop its earlier arguments' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <tonythethompson@hotmail.com>
2739577 to
03e508e
Compare
There was a problem hiding this comment.
All reported issues were addressed across 93 files
Architecture diagram
sequenceDiagram
participant Client as UI / Command
participant QSS as QuickShellServices
participant Exec as ShortcutLaunchExecutor
participant HC as WorkspaceHealthCheck
participant Env as IWorkspaceEnvironmentProbe
participant GitOps as IWorkspaceGitOperations
participant Gate as WorkspaceGitLaunchGate
participant Term as TerminalLauncher
participant Comp as CompanionAppLauncher
participant Proc as IProcessStarter
participant GitIdx as IGitRepoIndex
participant ClassCache as IProjectClassificationCache
Note over Client,ClassCache: DI-scoped services – no process-wide static state
Client->>QSS: Request shortcut launch
QSS->>Exec: Launch() (injected via DI)
Exec->>HC: Check(shortcut, ...)
HC->>Env: ExecutableExists(), PortInUse(), ProcessNames()
HC->>GitOps: TryGetStatus(directory)
Env-->>HC: boolean / list
GitOps-->>HC: status or null
HC-->>Exec: WorkspaceHealthResult
alt HasBlockingErrors
Exec-->>Client: StayOpen with error
else
Exec->>Gate: EvaluateBeforeLaunch(directory, blockDirty)
Gate->>GitOps: TryGetStatus(), TryResolveWorktreeKey()
GitOps-->>Gate: status, worktreeKey
Gate->>GitOps: TrySwitchBranch() if needed
GitOps-->>Gate: result
Gate-->>Exec: WorkspaceGitLaunchGateResult
alt Git blocked
Exec-->>Client: StayOpen with git message
else
Exec->>Comp: TryLaunch(shortcut, onDemand, error)
Comp->>Proc: TryStart(processInfo)
Proc-->>Comp: success/fail
Comp-->>Exec: bool + error
Exec->>Term: Open(shortcut, terminalId, defaultProfile)
Term->>Proc: TryStart(startInfo)
Proc-->>Term: success/fail
Term-->>Exec: TerminalLaunchAttempt
Exec-->>Client: ShortcutLaunchResult
end
end
Note over Client,ClassCache: Status snapshot for list items uses injected services
Client->>QSS: Request list items for workspace
QSS->>GitIdx: GetAll() or Search() (instance, not static)
QSS->>ClassCache: Classify(directory) (instance, not static)
Note over QSS: WorkspaceStatusService.CaptureForList() now requires<br/>healthChecker + gitOperations (obtained from QSS)
QSS->>HC: Check(shortcut, ..., includeVolatile, includeGit)
QSS->>GitOps: TryGetStatus(directory)
HC-->>QSS: health findings
GitOps-->>QSS: git status
ClassCache-->>QSS: classification
GitIdx-->>QSS: repo candidates
QSS-->>Client: list items with tags and subtitles
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <tonythethompson@hotmail.com>
Address cubic review on PR 86: dispose Process instances from ProcessNames(), and align the xUnit collection name with the WtProfilesService.TestLocationsOverride seam it actually isolates. Co-authored-by: Cursor <cursoragent@cursor.com>
…tunity' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <tonythethompson@hotmail.com>
tonythethompson
left a comment
There was a problem hiding this comment.
Post-merge follow-up review. These findings still appear applicable after the static-state refactor and are narrow enough to address independently.
| _hasCompletedRefreshForRoot = false; | ||
| _refreshInFlight = null; | ||
| }); | ||
| } |
There was a problem hiding this comment.
P1: cancel the refresh that Invalidate() removes from the index.
This clears _refreshInFlight without cancelling the linked token. The next GetAll/Search can immediately start another full discovery while the old scan continues in the background. Since page reloads call Invalidate() for ordinary edits/reorders, repeated invalidations can create overlapping filesystem scans and defeat the isolation/performance goal of this refactor.
Capture the old handle under the lock, clear the state, then cancel it outside the lock. The existing stale-continuation branch in CompleteRefresh can remain responsible for disposing its CTS:
public void Invalidate()
{
ThrowIfDisposed();
RefreshInFlight? inFlight;
lock (_sync)
{
inFlight = _refreshInFlight;
_cache = [];
_refreshedUtc = DateTime.MinValue;
_cacheRootKey = string.Empty;
_hasCompletedRefreshForRoot = false;
_refreshInFlight = null;
}
if (inFlight is not null)
{
try
{
inFlight.LinkedCts.Cancel();
}
catch (ObjectDisposedException)
{
}
}
}Please add a test that blocks discovery, calls Invalidate(), starts a replacement refresh, and verifies the first token is cancelled and only the replacement result is published.
| _extensionSynchronizationContext = SynchronizationContext.Current ?? GitRepoIndex.ExtensionSynchronizationContext; | ||
| _extensionSynchronizationContext = SynchronizationContext.Current; | ||
| _searchDebouncer = new SearchDebouncer(ApplyQueryDebounced); | ||
| #if CMDPAL_HOVER_ACTIONS |
There was a problem hiding this comment.
P1: the debounced query callback can now mutate the CmdPal page from a ThreadPool thread.
SearchDebouncer is backed by System.Threading.Timer. Removing the fallback to the provider-captured extension context means that when this page is constructed on an MTA thread with no SynchronizationContext, ApplyQueryDebounced runs directly on the timer thread and eventually calls RaiseItemsChanged there.
That is the exact thread-affinity problem this PR's scheduler abstraction is intended to solve. Please route the debounce callback through IExtensionThreadScheduler and keep the actual query application in a separate extension-thread method, for example:
_searchDebouncer = new SearchDebouncer(query =>
_services.ExtensionThreadScheduler.Post(() => ApplyQueryOnExtensionThread(query)));This likely requires exposing the scheduler through IQuickShellServices. A no-context/MTA test should assert that the timer callback is posted rather than executing RefreshItems inline.
| { | ||
| _queue.Enqueue(callback); | ||
| return; | ||
| } |
There was a problem hiding this comment.
P1: the no-context fallback is passive, so completion callbacks can remain queued forever.
When _context is null, Post only enqueues work. The queue is drained from GetItems, but a Git refresh commonly completes after the page's last fetch. In that case nothing requests another fetch, OnGitRefreshCompleted never runs, and the discover page can remain on its scanning/empty state until the user types or revisits the page.
Please give the fallback an active wake-up mechanism or use a host dispatcher that can execute/post independently of a future GetItems. At minimum, add an integration-style test for:
- provider created with
SynchronizationContext.Current == null; - page performs its initial
GetItems; - background Git refresh completes afterward;
- refreshed rows are published without another user interaction.
Simply adding more queue drains will not close this race because the missing event is what triggers the next drain.
| IProjectAnalysisService projectAnalysis, | ||
| IProjectClassificationCache classificationCache, | ||
| int maxCount = MaxPills) | ||
| { |
There was a problem hiding this comment.
P2: the new instance-scoped classification cache is still bypassed by a process-wide result cache.
classificationCache is now provider-owned, but CommandSuggestionService._resultCache remains static and its key contains only directory + used commands. For the TTL window, provider B can therefore receive pills computed with provider A's IProjectAnalysisService/IProjectClassificationCache, which preserves a cross-host static-state leak this PR is otherwise removing.
The clean fix is to make command suggestion caching an injected provider-scoped service. A minimal compatibility fix would include the producing service instances in the cache entry and require reference equality on lookup:
private sealed record SuggestionResultCache(
string Directory,
string UsedKey,
IProjectAnalysisService ProjectAnalysis,
IProjectClassificationCache ClassificationCache,
DateTime CreatedUtc,
IReadOnlyList<CommandSuggestionPill> Pills);Then reject the cached entry unless both service references match. Please add a two-service-provider test using different analysis/cache fakes so provider B cannot observe provider A's suggestions.
Summary by cubic
Replaced process-wide static seams and caches with DI-scoped services. Git repo discovery, project classification, and UI callback dispatch are now instance-based, improving test parallelism and host isolation.
Refactors
IExtensionThreadSchedulerwithSyncExtensionThreadScheduler(core default) andCmdPalExtensionThreadScheduler(CmdPal host), plusIExtensionCallbackQueue; list pages drain_services.CallbackQueue.GitRepoIndexto a provider-owned cache withPrewarm/Search/GetAll,RunAfterNextRefresh, andTryRunAfterNextRefreshIfInFlight; refresh work bound toIQuickShellLifetime.Searchnow acceptssavedDirectoriesandmaxResults.ProjectClassificationCachetoIProjectClassificationCacheinstance service and wired it through suggestions and templates (CommandSuggestionService,SuggestionPillPresentation,ShortcutFormTemplateJson).IExtensionThreadScheduler,IProjectClassificationCache,IGitRepoIndex. Host routing buildsQuickShellServiceswithGitRepos,ClassificationCache, andCallbackQueue._services.CallbackQueuefor deferred UI work; fallback/home pages call_services.GitReposinstead of static APIs.WorkspaceEnvironmentProbe: runwhere.exewithout redirected stdio and disposeProcessinstances fromProcessNames().Migration
AddQuickShellCore()/AddQuickShellCommandRouting()and replace static calls with injected services:IShortcutLaunchExecutor.ITerminalLauncher.OpenGroup(...).IGitRepoIndexandIWorkspaceGitOperations/WorkspaceGitLaunchGate.WorktreeBranchTargetStore.GetTargetForDirectory(dir, git)/TrySetTargetForDirectory(...).IProjectClassificationCachetoCommandSuggestionService.GetPills,SuggestionPillPresentation.BuildDataFields, andShortcutFormTemplateJson.BuildDataJson._services.CallbackQueue; rely onIExtensionThreadScheduler.Written for commit a5b89b7. Summary will update on new commits.