Fix mino_pcall re-throw via set_eval_diag (STM lock leak)

mino_pcall's catch arm called set_eval_diag to publish the caught
error's message via mino_last_error. But set_eval_diag itself
longjmps to the next-outer try frame when one exists -- so any
pcall caller that ran inside a Clojure try would see its catch
path hijacked by the longjmp and unwind to the outer frame,
leaking any bookkeeping the caller depended on.

In the STM commit path, that bookkeeping was the global commit
lock: a validator throw inside a (try (dosync ...) (catch ...))
would longjmp out of run_ref_validator past stm_unlock, leaving
S->stm_commit_lock permanently held. The next dosync deadlocked.

Extract a non-throwing variant record_eval_diag from set_eval_diag
and route mino_pcall's catch arm through it. The throw-conversion
path of set_eval_diag is preserved for its primary use case
(turning eval-phase diagnostics into catchable exceptions).

Adds tests/stm_test.clj validator-throw-does-not-deadlock-stm-lock:
the first tx errors via retry exhaustion; the second tx completes
instead of hanging.

The agent code's agent_try_call (added in E.4 as a workaround) is
left in place; a follow-up commit can fold it back into mino_pcall
if preferred.

Internal suite 1513 / 7169 / 0. External baseline unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: leifericf

CHANGELOG.md

@@ -361,6 +361,40 @@ fire watches anywhere.
 upstream, and the atom / ref / var arms pass cleanly. The agent
 arm of each still errors (out of scope until agents land).
 
+### Fix `mino_pcall` re-throw via `set_eval_diag`
+
+`mino_pcall`'s catch arm called `set_eval_diag` to publish the
+caught error's message via `mino_last_error` /
+`mino_last_error_map`. But `set_eval_diag` itself longjmps to the
+next-outer try frame when one exists -- so any pcall caller that
+ran inside a Clojure `try` would see its catch path hijacked by
+the longjmp and unwind to the outer frame, **leaking any
+bookkeeping the caller depended on**.
+
+In the STM commit path, that bookkeeping was the global commit
+lock: a validator throw inside a `try (dosync ...) (catch ...)`
+would longjmp out of `run_ref_validator` past `stm_unlock`,
+leaving `S->stm_commit_lock` permanently held. The next
+`dosync` deadlocked.
+
+Fix: extracted a non-throwing variant `record_eval_diag` from
+`set_eval_diag` and routed `mino_pcall`'s catch arm through it.
+The throw-conversion path of `set_eval_diag` is preserved for
+its primary use case (turning eval-phase diagnostics into
+catchable exceptions).
+
+`tests/stm_test.clj`'s new
+`validator-throw-does-not-deadlock-stm-lock` regression test
+proves the lock is released after a validator throw (the first
+tx errors via retry exhaustion; the second tx completes
+normally instead of hanging).
+
+The agent code's `agent_try_call` (added in E.4 to work around
+the same pcall bug) is left in place. It's not strictly
+necessary now but documents the historical pcall contract; a
+follow-up commit can fold it back into `mino_pcall` if the
+agent maintainers prefer one mechanism.
+
 ### Agents (MVP)
 
 mino now ships agents: `agent`, `agent?`, `send`, `send-off`,

src/runtime/error.c

@@ -87,6 +87,33 @@ void set_error_at(mino_state_t *S, const mino_val_t *form, const char *msg)
     }
 }
 
+/* Store an eval-phase diagnostic without converting to a thrown
+ * exception. Use this from places that need to record an error
+ * without disturbing control flow -- in particular from a catch
+ * arm that has already absorbed a thrown exception and now needs
+ * to publish a message via mino_last_error / mino_last_error_map.
+ *
+ * set_eval_diag's longjmp path is the right behavior for primitive
+ * error reporting (an error inside a try block becomes a catchable
+ * exception). It is the wrong behavior in a catch arm: a second
+ * longjmp would unwind further than the catcher intends, leaving
+ * locks held and bookkeeping unrun. */
+void record_eval_diag(mino_state_t *S, const mino_val_t *form,
+                      const char *kind, const char *code, const char *msg)
+{
+    mino_diag_t *d = diag_new(kind, code, "eval", msg);
+    if (d != NULL && form != NULL && form->type == MINO_CONS
+        && form->as.cons.file != NULL && form->as.cons.line > 0) {
+        mino_span_t span;
+        memset(&span, 0, sizeof(span));
+        span.file   = form->as.cons.file;
+        span.line   = form->as.cons.line;
+        span.column = form->as.cons.column;
+        diag_set_span(d, span);
+    }
+    set_diag(S, d);
+}
+
 /* Classified eval-phase diagnostic from a form with source info. */
 void set_eval_diag(mino_state_t *S, const mino_val_t *form,
                    const char *kind, const char *code, const char *msg)
@@ -98,20 +125,7 @@ void set_eval_diag(mino_state_t *S, const mino_val_t *form,
         mino_current_ctx(S)->try_stack[mino_current_ctx(S)->try_depth - 1].exception = ex;
         longjmp(mino_current_ctx(S)->try_stack[mino_current_ctx(S)->try_depth - 1].buf, 1);
     }
-
-    {
-        mino_diag_t *d = diag_new(kind, code, "eval", msg);
-        if (d != NULL && form != NULL && form->type == MINO_CONS
-            && form->as.cons.file != NULL && form->as.cons.line > 0) {
-            mino_span_t span;
-            memset(&span, 0, sizeof(span));
-            span.file   = form->as.cons.file;
-            span.line   = form->as.cons.line;
-            span.column = form->as.cons.column;
-            diag_set_span(d, span);
-        }
-        set_diag(S, d);
-    }
+    record_eval_diag(S, form, kind, code, msg);
 }
 
 /* Return a short human-readable label for a value's type. */

src/runtime/internal.h

@@ -891,6 +891,13 @@ const char *source_cache_get_line(mino_state_t *S, const char *file,
 void        set_eval_diag(mino_state_t *S, const mino_val_t *form,
                           const char *kind, const char *code,
                           const char *msg);
+/* Non-throwing variant: stores the diagnostic without converting it
+ * to a thrown exception. Use from catch arms that have already
+ * absorbed a throw and just need to publish the message via
+ * mino_last_error / mino_last_error_map. */
+void        record_eval_diag(mino_state_t *S, const mino_val_t *form,
+                             const char *kind, const char *code,
+                             const char *msg);
 const char *type_tag_str(const mino_val_t *v);                    /* static string */
 void        push_frame(mino_state_t *S, const char *name,     /* name: borrowed */
                        const char *file, int line,            /* file: borrowed */

src/runtime/state.c

@@ -767,14 +767,19 @@ int mino_pcall(mino_state_t *S, mino_val_t *fn, mino_val_t *args, mino_env_t *en
         load_stack_truncate(S, mino_current_ctx(S)->try_stack[saved_try].saved_load_len);
         mino_current_ctx(S)->try_depth = saved_try;
         /* Populate last_error from the exception value so the host
-         * can inspect it via mino_last_error(). */
+         * can inspect it via mino_last_error(). Use record_eval_diag
+         * (non-throwing) -- set_eval_diag would re-throw to any
+         * outer try frame, leaking out of pcall's catch arm and
+         * skipping bookkeeping the caller depends on (e.g. the STM
+         * commit lock). */
         if (mino_last_error(S) == NULL || mino_last_error(S)[0] == '\0') {
             const char *s = NULL;
             size_t slen = 0;
             if (ex != NULL && mino_to_string(ex, &s, &slen)) {
-                set_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", s);
+                (void)slen;
+                record_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", s);
             } else {
-                set_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", "unhandled exception");
+                record_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", "unhandled exception");
             }
         }
         if (out != NULL) {

tests/stm_test.clj

@@ -154,3 +154,20 @@
 
 (deftest type-keyword
   (is (= :ref (type (ref 1)))))
+
+(deftest validator-throw-does-not-deadlock-stm-lock
+  ;; Regression: a validator that throws would previously leak the
+  ;; STM commit lock because mino_pcall's catch arm called set_eval_diag,
+  ;; which longjmps to any enclosing try frame -- bypassing tx_commit's
+  ;; stm_unlock. The next dosync would then hang on the leaked lock.
+  ;; Fixed by routing pcall's catch arm through record_eval_diag (the
+  ;; non-throwing variant). The first tx errors via the retry path; the
+  ;; second tx must complete instead of hanging.
+  (let [r (ref 1)]
+    (set-validator! r (fn [v] (if (zero? v)
+                                 (throw (ex-info "validator-boom" {}))
+                                 (pos? v))))
+    (is (thrown? (dosync (alter r dec))))
+    (is (= 1 @r))
+    (dosync (alter r inc))
+    (is (= 2 @r))))