Fix mino_pcall catch arm: drop publish to last_error

The previous attempt at the lock-leak fix routed pcall's catch arm
through a non-throwing record_eval_diag variant. That fixed the
immediate longjmp problem, but it left a stale diag in last_error
that downstream code misread as a fresh error -- specifically
eval_impl's "evaled == NULL && mino_last_error != NULL" gate would
fire on a subsequent successful call's NULL arg-list and propagate
the stale diag, breaking the test runner mid-suite.

Drop the publish entirely. pcall now restores the eval bookkeeping
that was active at entry and returns -1 without touching
last_error / last_diag. Callers that want a diag set after pcall
returns -1 call set_eval_diag explicitly. record_eval_diag is
removed (no callers).

The lock-leak regression test from the previous attempt still
passes -- the throw is caught, the lock is released, the second
dosync completes -- it just doesn't observe a stale diag in
last_error anymore.

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

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

Author: leifericf

CHANGELOG.md

@@ -377,23 +377,22 @@ 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.
+Fix: `mino_pcall`'s catch arm no longer publishes anything to
+`last_error` / `last_diag`. Callers that want a diag set after
+`pcall` returns -1 do it explicitly. (An interim attempt routed
+the publish through a non-throwing `record_eval_diag` variant,
+but that left a stale diag in `last_error` that `eval_impl`'s
+`evaled == NULL && mino_last_error != NULL` check would then
+misread as a fresh error during a later call — flushed via the
+follow-up commit that drops the publish entirely.)
+
+`tests/stm_test.clj`'s `validator-throw-does-not-deadlock-stm-lock`
+regression test proves the lock is released after a validator
+throw.
+
+The agent code's `agent_try_call` workaround (added in E.4) is
+left in place by this commit; the follow-up replaces it with a
+direct `mino_pcall` call now that the catch arm is well-behaved.
 
 ### Agents (MVP)
 

src/runtime/error.c

@@ -87,33 +87,6 @@ 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)
@@ -125,7 +98,19 @@ 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);
     }
-    record_eval_diag(S, form, kind, code, 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);
+    }
 }
 
 /* Return a short human-readable label for a value's type. */

src/runtime/internal.h

@@ -891,13 +891,6 @@ 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

@@ -761,30 +761,20 @@ int mino_pcall(mino_state_t *S, mino_val_t *fn, mino_val_t *args, mino_env_t *en
     mino_current_ctx(S)->try_stack[mino_current_ctx(S)->try_depth].saved_load_len = S->load_stack_len;
     if (setjmp(mino_current_ctx(S)->try_stack[mino_current_ctx(S)->try_depth].buf) != 0) {
         /* Landed here from longjmp -- error was thrown. */
-        mino_val_t *ex = mino_current_ctx(S)->try_stack[saved_try].exception;
         S->current_ns    = mino_current_ctx(S)->try_stack[saved_try].saved_ns;
         S->fn_ambient_ns = mino_current_ctx(S)->try_stack[saved_try].saved_ambient;
         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(). 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)) {
-                (void)slen;
-                record_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", s);
-            } else {
-                record_eval_diag(S, mino_current_ctx(S)->eval_current_form, "eval/contract", "MCT001", "unhandled exception");
-            }
-        }
-        if (out != NULL) {
-            *out = NULL;
-        }
+        /* pcall does NOT publish to last_error / last_diag on a caught
+         * throw. The eval loop has read sites that treat
+         * mino_last_error as "an error happened during my call" (see
+         * eval_impl's "evaled == NULL && mino_last_error != NULL"
+         * gate), so leaving a stale diag from a successfully-caught
+         * pcall would mislead the next failing call into thinking
+         * its own error had already been reported. Callers that
+         * want a diag published after pcall returns -1 call
+         * set_eval_diag explicitly. */
+        if (out != NULL) *out = NULL;
         return -1;
     }
     mino_current_ctx(S)->try_depth++;

tests/stm_test.clj

@@ -157,11 +157,12 @@
 
 (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
+  ;; 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 removing the publish-to-
+  ;; last_error step from pcall's catch arm; pcall now just unwinds
+  ;; to the caller without re-throwing. The first tx errors; the
   ;; second tx must complete instead of hanging.
   (let [r (ref 1)]
     (set-validator! r (fn [v] (if (zero? v)