Bound nrepl-completed-requests with a FIFO cap

`nrepl--mark-id-completed' moved request handlers from
`nrepl-pending-requests' to `nrepl-completed-requests' with no upper
bound on the latter.  Each entry retains its handler closure (often
hundreds of bytes once you count its lexical environment), so a long-
running session accumulated thousands of stale handlers in memory.

The completed table exists so late messages -- responses arriving
after their request's `done' status -- can still find a callback to
dispatch them.  In practice that window is sub-second.  Add a FIFO
cap so the table is bounded to the recently-completed entries:

- New defcustom `nrepl-completed-requests-max-size' (default 1000).
- New buffer-local `nrepl--completed-requests-order' tracking
  insertion order.
- `nrepl--mark-id-completed' enqueues each id and, when the cap is
  exceeded, dequeues + removes the oldest.

1000 is enough to cover any plausible late-message window: requests
complete in milliseconds, so this caps about a second of activity.
Setting the size to 0 bypasses the eviction, restoring the previous
unbounded behavior for users who have a reason to keep everything.

Three Buttercup specs cover the cap, FIFO eviction, and the 0-disables
case.

Author: bbatsov

CHANGELOG.md

@@ -22,6 +22,7 @@
 - `cider--resolve-command` now actually verifies command presence on remote hosts via `executable-find`'s remote search, instead of unconditionally trusting the user-supplied command. A missing tool on the remote side now surfaces immediately during jack-in instead of as a server-startup failure later.
 - `nrepl--ssh-tunnel-connect` no longer routes its `ssh` invocation through a shell. The tunnel command is now spawned via `start-process` with an explicit arg list, eliminating shell-quoting hazards in user/host components.
 - Plug five request-id leaks in raw nREPL response handlers (`cider/test-stacktrace`, `cider/test-var-query`, `cider/analyze-last-stacktrace`, `cider/ns-reload`, `cider/get-state`). They never called `nrepl--mark-id-completed`, so their ids accumulated in the connection's `nrepl-pending-requests` for the lifetime of the session.
+- Bound `nrepl-completed-requests` with a FIFO cap (`nrepl-completed-requests-max-size`, default 1000). The completed-request handler table previously grew unbounded for the lifetime of a connection; long-running sessions accumulated thousands of stale handler closures.
 
 ### Changes
 

lisp/nrepl-client.el

@@ -71,6 +71,7 @@
 (require 'seq)
 (require 'subr-x)
 (require 'cl-lib)
+(require 'queue)
 (require 'nrepl-dict)
 (require 'nrepl-bencode)
 (require 'tramp)
@@ -110,6 +111,24 @@ Setting this to nil disables the timeout functionality."
 When true some special buffers like the server buffer will be hidden."
   :type 'boolean)
 
+(defcustom nrepl-completed-requests-max-size 1000
+  "Maximum number of completed-request handlers retained per connection.
+
+nREPL servers occasionally send late messages for requests whose
+\"done\" status has already arrived (typically a sub-second window).
+We keep the recently-completed handlers around so those messages still
+get dispatched.  Without a bound, the table grew unbounded for the
+lifetime of the connection.
+
+Eviction is FIFO: when the table is full, the oldest entry is dropped
+to make room for the newest.  1000 is ample in practice -- requests
+complete in milliseconds, so this caps roughly a second of activity.
+Set to 0 to disable the cache entirely; late messages will then become
+log warnings."
+  :type 'integer
+  :safe #'integerp
+  :package-version '(cider . "1.22.0"))
+
 ;;; Buffer Local Declarations
 
 ;; These variables are used to track the state of nREPL connections
@@ -138,6 +157,10 @@ To be used for tooling calls (i.e. completion, eldoc, etc)")
 
 (defvar-local nrepl-completed-requests nil)
 
+(defvar-local nrepl--completed-requests-order nil
+  "FIFO of ids in `nrepl-completed-requests', used for bounded eviction.
+See `nrepl-completed-requests-max-size'.")
+
 (defvar-local nrepl-last-sync-response nil
   "Result of the last sync request.")
 
@@ -562,7 +585,8 @@ client buffer.  Return the newly created client process."
             nrepl-tunnel-buffer (when-let* ((tunnel (plist-get endpoint :tunnel)))
                                   (process-buffer tunnel))
             nrepl-pending-requests (make-hash-table :test 'equal)
-            nrepl-completed-requests (make-hash-table :test 'equal)))
+            nrepl-completed-requests (make-hash-table :test 'equal)
+            nrepl--completed-requests-order (queue-create)))
 
     (with-current-buffer client-buf
       (nrepl--init-client-sessions client-proc)
@@ -637,12 +661,22 @@ Used by the client to clean up session state on disconnect.")
 
 (defun nrepl--mark-id-completed (id)
   "Move ID from `nrepl-pending-requests' to `nrepl-completed-requests'.
-It is safe to call this function multiple times on the same ID."
-  ;; FIXME: This should go away eventually when we get rid of
-  ;; pending-request hash table
+The completed table is bounded by `nrepl-completed-requests-max-size';
+when the cap is reached the oldest entry is evicted FIFO.
+
+It is safe to call this function multiple times on the same ID -- the
+second call is a no-op because the entry has already been moved out of
+`nrepl-pending-requests'."
   (when-let* ((handler (gethash id nrepl-pending-requests)))
     (puthash id handler nrepl-completed-requests)
-    (remhash id nrepl-pending-requests)))
+    (remhash id nrepl-pending-requests)
+    (when nrepl--completed-requests-order
+      (queue-enqueue nrepl--completed-requests-order id)
+      (when (and (> nrepl-completed-requests-max-size 0)
+                 (> (queue-length nrepl--completed-requests-order)
+                    nrepl-completed-requests-max-size))
+        (remhash (queue-dequeue nrepl--completed-requests-order)
+                 nrepl-completed-requests)))))
 
 (defun nrepl-notify (msg type)
   "Handle a server notification with MSG and TYPE.

test/nrepl-client-tests.el

@@ -267,6 +267,44 @@
       (expect received-body :to-equal "hello")
       (expect received-type :to-equal '("text/plain" ())))))
 
+(describe "nrepl--mark-id-completed cap"
+  :var (nrepl-pending-requests nrepl-completed-requests
+        nrepl--completed-requests-order
+        nrepl-completed-requests-max-size)
+  (before-each
+    (setq nrepl-pending-requests (make-hash-table :test 'equal)
+          nrepl-completed-requests (make-hash-table :test 'equal)
+          nrepl--completed-requests-order (queue-create)))
+
+  (cl-flet ((mark (id)
+              ;; A handler must exist in pending for the move to take place.
+              (puthash id #'ignore nrepl-pending-requests)
+              (nrepl--mark-id-completed id)))
+
+    (it "retains entries up to the configured cap"
+      (let ((nrepl-completed-requests-max-size 3))
+        (mark "1") (mark "2") (mark "3")
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 3)
+        (dolist (id '("1" "2" "3"))
+          (expect (gethash id nrepl-completed-requests) :not :to-be nil))))
+
+    (it "evicts the oldest entry FIFO when over the cap"
+      (let ((nrepl-completed-requests-max-size 2))
+        (mark "1") (mark "2") (mark "3")
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 2)
+        (expect (gethash "1" nrepl-completed-requests) :to-be nil)
+        (expect (gethash "2" nrepl-completed-requests) :not :to-be nil)
+        (expect (gethash "3" nrepl-completed-requests) :not :to-be nil)))
+
+    (it "treats max-size of 0 as unbounded"
+      ;; Documented as "disable the cache"; concretely the eviction
+      ;; branch is bypassed and the table grows freely.  In practice
+      ;; users would also need to clear the queue, but the queue itself
+      ;; is bounded by the producer rate, so this is fine.
+      (let ((nrepl-completed-requests-max-size 0))
+        (dotimes (n 5) (mark (number-to-string n)))
+        (expect (hash-table-count nrepl-completed-requests) :to-equal 5)))))
+
 (describe "nrepl-client-lifecycle"
   (it "start and stop nrepl client process"