Skip to content

fix(server): keep newer forced workspace scan results - #19465

Open
Exotic209093 wants to merge 2 commits into
pingdotgg:mainfrom
Exotic209093:fix/workspace-forced-scan-ownership
Open

Exotic209093 wants to merge 2 commits into
pingdotgg:mainfrom
Exotic209093:fix/workspace-forced-scan-ownership

Conversation

@Exotic209093

@Exotic209093 Exotic209093 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

When an ordinary workspace scan finishes while a newer forced scan is running, its publication changes the snapshot baseline. The registry then discards the newer result, leaving newly installed skills absent. This fixes the overlapping-scan reproduction in #16866.

Give each in-flight instance/directory scan an ownership token. A forced scan takes ownership before refreshing the provider; the registry checks ownership within the atomic snapshot update. An older scan cannot publish or remove the newer claim. Ordinary request deduplication, instance identity checks, and the existing protection for intervening session snapshots remain in place.

Validation: a Deferred-gated regression fails against the original production source and passes with the fix. It covers both completion orders, ordinary request deduplication, separate directory scans, and instance rebuilding. The focused test passes independently. Scoped server typecheck, lint, formatting, and independent source review pass. The complete registry file passes 61 tests locally with one existing Windows EBUSY temporary-directory cleanup failure; CI runs the complete file. No client or provider was launched.

Refs #16866, reproduction 2 only. #18309 covers failed skill discovery; the client retry behavior in reproduction 3 is outside this fix.

Linux Test Server 4 on d6d5ecc passes the complete ProviderRegistry.test.ts file (62 tests). All CI checks and CodeRabbit pass. The initial run hit an unrelated SSH zombie-process fixture failure; the retry commit has the same code tree.

Implemented with GPT-6.1-Sol through the Codex harness in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at b6b9929

Macroscope's review found this PR approvable — This is a localized server bug fix that uses per-scan ownership tokens to prevent stale workspace scans from overwriting newer forced-scan results. The production change is contained to existing scan coordination, and targeted regression tests cover the overlapping-scan scenarios.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2fcc216d-429b-4798-80cb-c87a63367316
📥 Commits

Reviewing files that changed from the base of the PR and between 7ac952c and b6b9929.

📒 Files selected for processing: 2
  • apps/server/src/provider/ProviderRegistry.test.ts
  • apps/server/src/provider/ProviderRegistry.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Workspace scan tracking now uses per-directory ownership tokens. A forced scan can replace an active scan’s claim, and only the scan that still owns the claim can publish results or clear it. The concurrency tests verify this behavior.

Changes: Workspace scan ownership

Layer / File(s) Summary
Claim and validate workspace scans
apps/server/src/provider/ProviderRegistry.ts, apps/server/src/provider/ProviderRegistry.test.ts
ProviderRegistry assigns ownership tokens to scans by working directory. A scan publishes results and clears its claim only if its token remains current. Tests queue scan gates and verify that an older scan cannot publish before a forced scan completes.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to d6d5e

Merge Risk: ⚪ Minimal · up to d6d5e

No actionable merge-blocking issue is identified; the change is mergeable after normal checks.

Security architecture risk: 🔵 Low · up to b6b99

The change strengthens scan coordination without changing request inputs or expanding provider access. Existing instance and snapshot protections remain in place. Residual uncertainty is concentrated in failure and cancellation behavior, which was inspected in source but not exercised during this review.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — Scan ownership is isolated by provider-instance identity and directory. Separately, a forced refresh continues to invalidate that directory's snapshots on other configured instances before scanning; this broader cache effect predates the PR and is not introduced by token ownership.

Trust Boundaries and Controls

  • observed — The existing request path supplies the selected instance, directory, and forced-refresh flag. Registry preconditions reject missing or disabled providers and instances without workspace-snapshot support. The PR changes which overlapping scan may publish, without adding a new caller or altering these preconditions.

Resilience and Maintainability Implications

  • observed — The scan pipeline installs an ensuring finalizer that deletes only its own claim, leaving a newer token untouched. Existing failure recovery preserves cached providers for non-interruption failures and propagates interruption. Failure/interruption cleanup is supported by source inspection, but was not directly runtime-validated in this review.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main fix: preserving results from newer forced workspace scans.
Description check Passed The description covers the problem, implementation, scope, references, verification steps, test results, known limitation, and out-of-scope issues. It provides sufficient detail for review.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant