fix(tasks): a task_yield on one thread no longer suspends every other (#739 item 3)

CHANGELOG.md

@@ -171,6 +171,25 @@ All notable changes to EigenScript are documented here.
 
 ### Fixed
 
+- **A `task_yield` on one thread silently made every other thread return `null`
+  (#739 item 3).** The cooperative scheduler lives on `EigsThread` — tasks are
+  single-threaded by construction — but the suspend request that drives it was
+  a plain global, set at four sites and polled by every `vm_run` on every
+  thread at `CASE(CALL)`. So a `task_yield` on the main thread drove an
+  unrelated OS worker's next call into `vm_suspend_halt`. Having no scheduler
+  of its own, the victim's `task_save_slice(task_current_running())` was handed
+  NULL and saved nothing, and `vm_run` returned NULL mid-evaluation with its
+  frames deliberately undrained (the suspend path preserves them on purpose) —
+  so a spawned worker silently produced `null` instead of its result. Measured:
+  a worker computing 400000 returned `null` on 3 of 3 runs while main was
+  yielding, and 400000 on 3 of 3 with the identical program minus the yields.
+  The flag is now per-thread. It sits on `EigsThread` beside the `task_sched`
+  it drives rather than inside `TaskScheduler` as first sketched, so the hot
+  `CASE(CALL)` poll stays a single load off the already-hot `eigs_current`
+  with no NULL check for the (overwhelmingly common) no-scheduler thread.
+  `tests/test_tasks.eigs` covers it; the suite had no case where cooperative
+  tasks and OS threads ran at the same time.
+
 - **One `exit of N` permanently disabled `try`/`catch` for the rest of the
   process (#739 item 2).** `exit` is deliberately uncatchable: `builtin_exit`
   sets `g_exit_requested` and `CHECK_ERROR` consults it to refuse routing the

docs/CONCURRENCY.md

@@ -49,6 +49,23 @@ unsafe pattern — unsynchronized read-modify-write on a shared list from two
 workers — is caught by the ThreadSanitizer gate below
 (`tests/tsan_seeded_race.eigs`).
 
+## Cooperative tasks are per-thread
+
+`task_spawn`/`task_yield` (#408) are a *different* model from `spawn`:
+one OS thread, round-robin, deterministic by construction. The two
+compose, and the scoping is what makes that safe — the scheduler, its
+ready queue, and the suspend request that drives it all live on
+`EigsThread`. A `task_yield` hands control to the next task **on the
+calling thread only**; a `spawn`ed worker with no tasks of its own is
+never suspended by someone else's yield.
+
+That last part was a bug until #739: the suspend request was a plain
+global polled by every `vm_run` at `CASE(CALL)`, so one thread's
+`task_yield` sent an unrelated worker into the suspend path. Having no
+scheduler, the victim saved no slice and its `vm_run` returned `null`
+mid-evaluation — a worker silently produced `null` instead of its
+result. `tests/test_tasks.eigs` now runs both models at once.
+
 ## The multithreaded performance cliff
 
 The first `spawn` in a program permanently flips the runtime into

docs/SPEC.md

@@ -1255,6 +1255,14 @@ result (or re-raises its uncaught error as the same `{kind, message,
 line}` dict — see [Error handling](#error-handling)). `task_alive of
 id` is 1 until the task finishes.
 
+Tasks are scoped to the OS thread that spawned them: each thread has its
+own scheduler and its own ready queue, so `task_yield` hands control to
+the next task **on this thread** and never disturbs another. Mixing the
+two models is therefore well-defined — a `spawn`ed worker runs its own
+program while the parent interleaves tasks. (Before #739 the suspend
+request was process-wide, so one thread's `task_yield` made every other
+thread's next call return `null` mid-evaluation.)
+
 ```eigenscript
 order is []
 define step(tag) as:

src/eigenscript.h

@@ -725,6 +725,14 @@ struct EigsThread {
      * first task_spawn, freed at thread detach. Per-OS-thread because tasks
      * are single-threaded by construction. */
     void                *task_sched;
+    /* #739: the suspend request that drives task_sched. Per-OS-thread for the
+     * same reason the scheduler is — it was a plain global, so a task_yield on
+     * one thread drove EVERY other thread's next CALL into vm_suspend_halt:
+     * with no scheduler of its own the victim saved nothing and its vm_run
+     * silently returned NULL mid-evaluation, frames deliberately undrained.
+     * Lives here rather than inside TaskScheduler so the CASE(CALL) poll stays
+     * one load off the already-hot eigs_current, with no NULL check. */
+    int                  task_suspend_request;
     /* Registry list — set by eigs_thread_attach. */
     EigsThread *next;
 };
@@ -808,6 +816,7 @@ extern __thread EigsThread *eigs_current;
 #define g_json_depth          (eigs_current->json_depth)
 #define g_native_call_depth   (eigs_current->native_call_depth)
 #define g_task_sched          (eigs_current->task_sched)
+#define g_task_suspend_request (eigs_current->task_suspend_request)
 #define g_entry_threshold     (eigs_current->state->jit_entry_threshold)
 #define g_iter_threshold      (eigs_current->state->jit_iter_threshold)
 #define g_osr_threshold       (eigs_current->state->jit_osr_threshold)

src/vm.c

@@ -5570,8 +5570,6 @@ static Value *vm_run(EigsChunk *chunk, Env *env) {
  * interleaving is a pure function of program order.
  * ======================================================================== */
 
-int g_task_suspend_request = 0;
-
 #define TASK_READY_MAX HANDLE_TABLE_SIZE
 
 typedef struct {

src/vm.h

@@ -557,7 +557,9 @@ void task_free(Task *t);
  * above the outermost vm_execute, so C-stack depth stays flat across any
  * number of task switches. Suspension is deterministic-by-construction — no
  * tape records. */
-extern int g_task_suspend_request;   /* set by a suspending builtin; actioned at CASE(CALL) */
+/* g_task_suspend_request (set by a suspending builtin, actioned at CASE(CALL))
+ * is a per-thread bridge macro in eigenscript.h — #739: as a global it drove
+ * other threads' CALL sites into a suspend they never asked for. */
 void task_sched_on_spawn(int id);    /* enqueue a freshly spawned task, arm the scheduler */
 void task_request_yield(void);       /* current task → tail of the ready queue */
 int  task_request_join(int target);  /* current task blocks on `target`; 0 = bad target */

tests/test_tasks.eigs

@@ -499,4 +499,31 @@ assert_eq of [task_join of _det_t3, null, "self-detached task is reaped at finis
 assert_eq of [task_detach of 0, 0, "task_detach of main is refused"]
 assert_eq of [task_detach of 99999, 0, "task_detach of an unknown id is refused"]
 
+# #739 item 3: a task_yield on THIS thread must not suspend another OS thread.
+# The suspend request used to be a plain global while the scheduler it drives
+# is per-thread, so every other thread's next CALL saw the flag and jumped to
+# vm_suspend_halt. Having no scheduler of its own, the victim saved no slice
+# and its vm_run returned NULL mid-evaluation — a spawned worker silently
+# produced `null` instead of its result, with its frames deliberately
+# undrained. Measured before the fix: null on 3 of 3 runs; 400000 after.
+# The counts are sized so main is still yielding while the workers run.
+define _susp_busy(n) as:
+    local acc is 0
+    local i is 0
+    loop while i < 400000:
+        acc is acc + (abs of (0 - 1))
+        i is i + 1
+    return acc
+
+_susp_w1 is spawn of [_susp_busy, 1]
+_susp_w2 is spawn of [_susp_busy, 2]
+_susp_m is 0
+loop while _susp_m < 100000:
+    task_yield of null
+    _susp_m is _susp_m + 1
+assert_eq of [thread_join of _susp_w1, 400000,
+              "task_yield does not suspend another OS thread (#739)"]
+assert_eq of [thread_join of _susp_w2, 400000,
+              "second worker also unaffected by a foreign suspend request"]
+
 test_summary of null