From 4b86ad94ff88aab9d66b6df6b112cf40ccbed2e5 Mon Sep 17 00:00:00 2001 From: Ollie Agent Date: Tue, 18 Aug 2026 07:04:11 +0200 Subject: [PATCH] Make bypass resolution atomic --- cmd/toolsrv/internal/bypass/bypass.go | 12 +++++++----- doc/concurrency-resource-audit.md | 8 ++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/cmd/toolsrv/internal/bypass/bypass.go b/cmd/toolsrv/internal/bypass/bypass.go index da70897..8519949 100644 --- a/cmd/toolsrv/internal/bypass/bypass.go +++ b/cmd/toolsrv/internal/bypass/bypass.go @@ -36,8 +36,10 @@ var ( // If context is cancelled, returns (false, ctx.Err()). func markCancelled(req *Request) { mu.Lock() - delete(requests, req.ID) - req.cancelled = true + if current, ok := requests[req.ID]; ok && current == req { + delete(requests, req.ID) + req.cancelled = true + } mu.Unlock() } @@ -98,13 +100,13 @@ func NextPending(ctx context.Context) *Request { // Returns false if the request ID is not found (already resolved or invalid). func Resolve(id string, approved bool, errMsg string) bool { mu.Lock() - req, ok := requests[id] - mu.Unlock() + defer mu.Unlock() + req, ok := requests[id] if !ok { return false } - + delete(requests, id) req.approved = approved if errMsg != "" { req.err = errors.New(errMsg) diff --git a/doc/concurrency-resource-audit.md b/doc/concurrency-resource-audit.md index 916310d..1f4b3b1 100644 --- a/doc/concurrency-resource-audit.md +++ b/doc/concurrency-resource-audit.md @@ -256,13 +256,9 @@ Cancellation removes the request from the active set. `NextPending` discards sta ### M9. Concurrent bypass resolution can panic -**Evidence:** `cmd/toolsrv/internal/bypass/bypass.go:88-104`. +**Status:** Fixed in the working tree. -Resolution checks under a mutex, unlocks, then closes the request channel. Two concurrent resolvers can both close the same channel. - -**Impact:** duplicate approval messages can panic toolsrv. - -**Remediation:** remove and mark the request resolved atomically under the mutex. Only the winning resolver closes the completion channel. +`Resolve` now removes the request, updates its result, and closes the completion channel while holding the mutex. Only the first resolver can find the request and close the channel. Cancellation also verifies request ownership before removing it. ### M10. LSP shutdown is not idempotent