From 5cf52e7e125fcba06a6665e20aad598aba712cdb Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 22 Sep 2026 19:21:28 +0200 Subject: [PATCH] Fix uk locale key parity, capture claim race and approval window reuse --- client/server/capture.go | 39 +++++++++++-------- client/server/capture_evict_test.go | 12 +----- .../src/modules/approval/ApprovalDialog.tsx | 27 ++++++++++--- client/ui/i18n/locales/uk/common.json | 3 -- 4 files changed, 47 insertions(+), 34 deletions(-) diff --git a/client/server/capture.go b/client/server/capture.go index 327ee9776..e57cd0921 100644 --- a/client/server/capture.go +++ b/client/server/capture.go @@ -118,13 +118,6 @@ func (s *Server) StartCapture(req *proto.StartCaptureRequest, stream proto.Daemo return err } - if err := engine.SetCapture(sess); err != nil { - s.releaseCapture(sess) - sess.Stop() - pw.Close() - return status.Errorf(codes.Internal, "set capture: %v", err) - } - // Send an empty initial message to signal that the capture was accepted. // The client waits for this before printing the banner, so it must arrive // before any packet data. @@ -305,20 +298,18 @@ func (s *Server) cleanupBundleCapture() { s.bundleCapture = nil } -// claimCapture reserves the engine's capture slot for sess. If another -// capture is already running it is evicted: a previous streaming session -// whose gRPC client died and never freed the slot stays stuck otherwise, -// and a bundle capture is just informational state. +// claimCapture reserves the engine's capture slot for sess and installs sess on +// the engine. If another capture is already running it is evicted: a previous +// streaming session whose gRPC client died and never freed the slot stays stuck +// otherwise, and a bundle capture is just informational state. The returned +// engine already has sess installed; the caller must not install it again. func (s *Server) claimCapture(sess *capture.Session, cancel func()) (*internal.Engine, error) { s.mutex.Lock() stopEvicted := s.evictActiveCaptureLocked() - engine, err := s.getCaptureEngineLocked() - if err == nil { - s.activeCapture = sess - s.activeCaptureCancel = cancel - } + engine, err := s.installCaptureLocked(sess, cancel) s.mutex.Unlock() + // Waits for the evicted session's writer, so it must run outside s.mutex. stopEvicted() if err != nil { @@ -327,6 +318,22 @@ func (s *Server) claimCapture(sess *capture.Session, cancel func()) (*internal.E return engine, nil } +// installCaptureLocked installs sess on the engine and records it as the slot's +// owner, so no other claim can interleave between the two. On failure the slot +// is left unowned. Caller must hold mutex. +func (s *Server) installCaptureLocked(sess *capture.Session, cancel func()) (*internal.Engine, error) { + engine, err := s.getCaptureEngineLocked() + if err != nil { + return nil, err + } + if err := engine.SetCapture(sess); err != nil { + return nil, status.Errorf(codes.Internal, "set capture: %v", err) + } + s.activeCapture = sess + s.activeCaptureCancel = cancel + return engine, nil +} + // evictActiveCaptureLocked releases the engine's capture slot from whatever // capture currently owns it so a fresh claim can succeed, and returns the rest // of that teardown as a function. The returned function is never nil, is safe diff --git a/client/server/capture_evict_test.go b/client/server/capture_evict_test.go index d27306d2c..18306617a 100644 --- a/client/server/capture_evict_test.go +++ b/client/server/capture_evict_test.go @@ -77,18 +77,10 @@ func TestClaimCapture_EvictionDoesNotHoldMutex(t *testing.T) { t.Fatal("claimCapture blocked evicting a stalled capture") } - locked := make(chan struct{}) - go func() { - s.mutex.Lock() - s.mutex.Unlock() - close(locked) - }() - - select { - case <-locked: - case <-time.After(5 * time.Second): + if !s.mutex.TryLock() { t.Fatal("s.mutex still held after evicting a stalled capture") } + s.mutex.Unlock() select { case <-sess.Done(): diff --git a/client/ui/frontend/src/modules/approval/ApprovalDialog.tsx b/client/ui/frontend/src/modules/approval/ApprovalDialog.tsx index f2cb3532e..eca4409f6 100644 --- a/client/ui/frontend/src/modules/approval/ApprovalDialog.tsx +++ b/client/ui/frontend/src/modules/approval/ApprovalDialog.tsx @@ -73,6 +73,13 @@ export default function ApprovalDialog() { return out; }, [initiator, peerPubKey, peerName, sourceIP, username, t]); + // A second prompt reuses this window by changing its URL, so the component + // can stay mounted across requests. Track which request is on screen now, + // and close only while it is still the one being answered — otherwise a + // slow response to the previous request dismisses the new prompt unseen. + const currentRequestID = useRef(requestID); + currentRequestID.current = requestID; + const respond = useCallback( async (accept: boolean, viewOnly: boolean) => { if (busy) return; @@ -84,17 +91,15 @@ export default function ApprovalDialog() { } catch (e) { console.error("respond approval failed", e); } finally { - WindowManager.CloseApproval().catch(console.error); + if (currentRequestID.current === requestID) { + WindowManager.CloseApproval().catch(console.error); + } } }, [busy, requestID], ); const [armed, setArmed] = useState(false); - useEffect(() => { - const id = globalThis.setTimeout(() => setArmed(true), ARMING_MS); - return () => globalThis.clearTimeout(id); - }, []); // The dialog is non-modal, so the browser's own Escape-to-cancel does not // apply and denying has to be wired up by hand. @@ -111,6 +116,18 @@ export default function ApprovalDialog() { const secondsLeft = () => Math.max(0, Math.ceil((deadline - Date.now()) / 1000)); const [remaining, setRemaining] = useState(secondsLeft); const closedRef = useRef(false); + + // Every per-request flag has to be cleared when the window is reused, or the + // new prompt inherits the previous one's: already armed, so Allow is live on + // the first frame; still busy, so both buttons are dead; already closed, so + // the deadline never dismisses it. + useEffect(() => { + setBusy(false); + setArmed(false); + closedRef.current = false; + const id = globalThis.setTimeout(() => setArmed(true), ARMING_MS); + return () => globalThis.clearTimeout(id); + }, [requestID]); useEffect(() => { const id = globalThis.setInterval(() => { const left = secondsLeft(); diff --git a/client/ui/i18n/locales/uk/common.json b/client/ui/i18n/locales/uk/common.json index dbb81d4d9..6693757cf 100644 --- a/client/ui/i18n/locales/uk/common.json +++ b/client/ui/i18n/locales/uk/common.json @@ -1456,8 +1456,5 @@ }, "settings.privilege.oneWayInverted": { "message": "Ви можете увімкнути це, але щоб вимкнути знову, знадобиться {actor}:" - }, - "settings.ssh.privilege.authorizePending": { - "message": "Очікування авторизації…" } }