fix(security): eliminate live-pointer data races in scanner registry and engine - #1038
Open
Dumbris wants to merge 2 commits into
Open
fix(security): eliminate live-pointer data races in scanner registry and engine#1038Dumbris wants to merge 2 commits into
Dumbris wants to merge 2 commits into
Conversation
…and engine Two pre-existing races carried forward from the #1032 review. Registry: Get/List handed out the live *ScannerPlugin records the registry keeps, while InstallScanner/ConfigureScanner/syncRegistryFromStorage wrote Status, ConfiguredEnv and ImageOverride straight onto them outside the lock — racing every concurrent reader, including GET /api/v1/security/scanners. Get/List now return defensive copies and every mutation goes through a locked method (UpdateStatus, new SetConfiguredEnv/SetRuntimeConfig), following the InProcessRunnableIDs pattern. loadBundledRegistry also clones the package-level bundled records instead of stamping Status onto memory shared by every Registry in the process. Engine: executeScan wrote job.Status/Error/CompletedAt without holding e.mu while the job was still in activeScans, so GetActiveJob (REST scan-status, which JSON-encodes it) read a live record. The terminal-status decision and write now happen in one locked section, and every job handed outside the engine — GetActiveJob, StartScan's return value, and the scan callbacks (which persist and encode the job while sibling scanner goroutines still update ScannerStatuses) — is a snapshot. No API shapes or external behavior change.
Deploying mcpproxy-docs with
|
| Latest commit: |
0de2269
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ba644ebf.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-scanner-registry-engine.mcpproxy-docs.pages.dev |
… completion
Cross-model review of the registry/engine race fix found two more defects in
the same activeScans invariant.
executeScan's cleanup deleted the slot by server name alone. CancelScan drops a
job from activeScans immediately while its scanner goroutines keep running, so a
replacement scan for the same server can take the slot before the cancelled scan
unwinds — and that cleanup then evicted the REPLACEMENT. With the slot empty,
StartScan would accept a second concurrent scan of the same server and
GetScanStatus would report "no active scan" while one was running. The cleanup
now runs through Engine.clearActiveJob, which releases the slot only if the
job that owns it is still the one being torn down.
CancelScan accepted a job that had already reached a terminal status. A job
stays in activeScans for a short window after executeScan writes Status/Error/
CompletedAt — the completion callback persists the report and emits the
completion event inside it. A cancel arriving there returned 200 and flipped
Status to cancelled while the already-cloned COMPLETED job was still being
persisted, so the API reported a cancellation the stored job contradicted.
Cancelling a settled job now fails instead, leaving the terminal status intact.
Both are reachable from POST /api/v1/servers/{name}/scan/cancel.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 32765287606 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes two pre-existing data races in
internal/security/scanner, flagged during the #1032 review as carried-forward defects, plus twoactiveScanslifecycle defects that cross-model review surfaced in the same seam.1. Registry handed out live pointers
Registry.Get/Listreturned the live*ScannerPluginrecords the registry keeps, whileInstallScanner,ConfigureScannerandsyncRegistryFromStoragewroteStatus/ConfiguredEnv/ImageOverridestraight onto them outsideRegistry.mu— a real race against every concurrent reader, includingGET /api/v1/security/scanners.Get/Listnow return defensive copies (ScannerPlugin.clone).UpdateStatusplus newSetConfiguredEnvandSetRuntimeConfig(env + image override in one locked update, so a reader never sees new env against the old image).loadBundledRegistryclones the package-levelbundledScannersrecords instead of stampingStatusonto memory shared by everyRegistryin the process.Registerstores its own copy so a caller keeping its pointer can't reach into registry state.2. Engine wrote job state on a live job
executeScanwrotejob.Status/Error/CompletedAtwithout holdinge.muwhile the job was still inactiveScans, soGetActiveJob(→GetScanStatus→ REST scan-status handler, which JSON-encodes it) read the same record.GetActiveJob,StartScan's return value, and the scan callbacks — which persist and JSON-encode the job while sibling scanner goroutines are still updatingScannerStatusesunder the lock.ScanContextis shared by pointer on purpose (populated before job creation, never written again); documented onScanJob.clone.3.
activeScansslot could be handed to the wrong scanFound by cross-model review of the above, in the same invariant.
executeScan's cleanup deleted the slot by server name alone.CancelScandrops a job fromactiveScansimmediately while its scanner goroutines keep running, so a replacement scan for the same server can take the slot before the cancelled scan unwinds — and that cleanup then deleted the replacement. With the slot empty,StartScanwould accept a second concurrent scan of the same server andGetScanStatuswould report "no active scan" while one was running. Cleanup now goes throughEngine.clearActiveJob, which releases the slot only when the stored pointer is still the job being torn down.CancelScanaccepted an already-settled job. A job stays inactiveScansfor a short window afterexecuteScanwrites its terminal status — the completion callback persists the report and emits the completion event inside it. A cancel arriving there returned success and flippedStatusto cancelled while the already-cloned completed job was still on its way to storage, so the API reported a cancellation the stored job contradicted. Cancelling a settled job now fails and leaves the terminal status intact.Both are reachable from
POST /api/v1/servers/{name}/scan/cancel.Key decisions
Getreturning a copy means the implicit "mutate the returned record and the engine picks it up" side effect is gone; the two call sites that relied on it (InstallScannerreusing stored env/override,ConfigureScanner) now write back explicitly through the locked setter. No behavior change.omitempty) are byte-identical.activeScansuntilclearActiveJobruns, soGetScanSummarykeeps answering "scanning" until the completion callback has persisted the report — the windowawaitScanVerdict(MCP scan_server) documents and depends on.Deferred (pre-existing, out of scope)
Cross-model review also raised three items that are byte-identical to
mainand belong to different seams; filing separately rather than widening a race-fix PR:SetDeepScan/SetIsolationMode/SetScannerDisableNoNewPrivileges) writesdeepScanEnabled,deepScanScanners,isolationModeanddisableNoNewPrivilegeswith no synchronization against a running scan. A correct fix has to cover therunSingleScannerreader too.syncRegistryFromStoragerestoresConfiguredEnvbut notImageOverride, so a custom scanner image is lost from the registry across a restart until the scanner is reconfigured.RemoveScanneronly resetsStatus; the registry record keepsConfiguredEnv/ImageOverridefor the process lifetime.Tests
New
-racestress tests (extending the #1032TestGetOverview_ConcurrentScannerStatusUpdateIsRaceFreepattern), all of which reproduce the races onmain:registry_race_test.go— Get/List copy contracts; List/Get/InProcessRunnableIDsvsUpdateStatus/SetRuntimeConfig/SetConfiguredEnv;InstallScannervsListScanners+GetOverview;ConfigureScannervs registry readers.engine_race_test.go—GetActiveJobsnapshot contract;GetActiveJob+JSON-encode hammered against 25 scan completions; callbacks receive snapshots.engine_activescans_test.go—clearActiveJobevicts only its own job across a cancel + replacement;CancelScanrejects a settled job and preserves its terminal status; the running-job cancel path still works.Verified:
go test -race ./internal/security/scanner(also-count=2),go test -race -skip 'E2E|Binary|MCPProtocol|...' ./internal/server(226s, ok),./internal/httpapi,./cmd/scan-eval, both edition builds, golangci-lint v2 clean.