Skip to content

Commit 0cf357c

Browse files
leifericfclaude
andcommitted
intern: assert state_lock-held on the shared-table mutators
intern_lookup_or_create_ns and mino_defrecord both mutate state-wide tables (the open-addressing intern hash + entries array, the record-type linked list) and their documented contract was "caller holds state_lock", but nothing enforced it. A missing lock anywhere would tear the table silently and only surface as a collapsed lookup or a crashed entries[] read much later. Add MINO_ASSERT_STATE_SAFE -- a debug-build invariant that fires if the caller is on a multi-threaded state without holding state_lock recursively -- to the head of both functions. Two real missing-lock call sites surfaced under the assert and are fixed in the same commit: - worker_run's convey-binding loop in runtime/host_threads.c interned symbols before mino_call acquired the lock. Take mino_lock around the loop. - agent_worker_run held the raw state_lock mutex via mino_state_lock_acquire, which does not bump lock_depth. Switch to mino_lock so the recursion counter and the assert see the held lock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 469aa2e commit 0cf357c

6 files changed

Lines changed: 67 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,29 @@
11
# Changelog
22

3+
## v0.389.9 — Lock-Invariant Asserts on Shared Tables
4+
5+
The intern table and the record-type registry are shared across
6+
host worker threads but mutated with plain stores; the documented
7+
contract was "caller holds `state_lock`" and nothing enforced it.
8+
Add a debug-build assert (`MINO_ASSERT_STATE_SAFE`) at the head of
9+
`intern_lookup_or_create_ns` and `mino_defrecord` so a missing
10+
lock surfaces at the offending call site instead of letting a
11+
torn table escape into production.
12+
13+
Two genuine missing-lock call sites were uncovered and fixed by
14+
the assert:
15+
16+
- The future worker (`worker_run` in `runtime/host_threads.c`)
17+
built its convey-binding chain before `mino_call` acquired
18+
state_lock; the `mino_symbol` interns ran unprotected. The
19+
convey loop now wraps the intern calls in `mino_lock` /
20+
`mino_unlock`.
21+
- The agent worker (`agent_worker_run` in `prim/agent.c`) took
22+
state_lock via the raw `mino_state_lock_acquire` mutex, which
23+
does not bump `lock_depth`. Switch to `mino_lock` so the
24+
recursion counter stays accurate; the assert and any future
25+
lock-aware introspection now see the held lock.
26+
327
## v0.389.8 — `eval_try` Catch-Rethrow Protection Hygiene
428

529
The catch-with-finally branch was gated on

src/mino.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
*/
2929
#define MINO_VERSION_MAJOR 0
3030
#define MINO_VERSION_MINOR 389
31-
#define MINO_VERSION_PATCH 8
31+
#define MINO_VERSION_PATCH 9
3232

3333
/*
3434
* Human-readable version string of the *linked* runtime, e.g. "0.48.0".

src/prim/agent.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -468,10 +468,15 @@ static void agent_worker_run(mino_state *S, agent_pool_kind_t kind,
468468
n = agent_runq_pop(S, kind);
469469
agent_mu_unlock(&S->agent.mu);
470470

471-
/* Run the action under state_lock so eval invariants hold. */
472-
mino_state_lock_acquire(S);
471+
/* Run the action under state_lock so eval invariants hold.
472+
* Use mino_lock here rather than the raw state_lock_acquire so
473+
* lock_depth tracks the recursion: the convey-binding build
474+
* inside agent_worker_run_one interns symbols, and the intern
475+
* table's caller-must-hold-lock assert checks lock_depth, not
476+
* the underlying mutex. */
477+
mino_lock(S);
473478
agent_worker_run_one(S, n);
474-
mino_state_lock_release(S);
479+
mino_unlock(S);
475480

476481
/* Decrement the agent's in_flight and signal await waiters. */
477482
agent_mu_lock(&S->agent.mu);

src/runtime/coordination.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,4 +73,16 @@ void mino_worker_list_lock_release(mino_state *S);
7373
int mino_yield_lock(mino_state *S);
7474
void mino_resume_lock(mino_state *S, int saved_depth);
7575

76+
/* Debug-only invariant: assert that the caller is in a state-safe
77+
* window -- either single-threaded (no other writer can interpose,
78+
* including the init path before any future has been spawned) or
79+
* holding state_lock recursively on this thread. Used to guard
80+
* shared-table mutators (intern table, record-type registry) whose
81+
* documented contract is "caller must hold state_lock"; the assert
82+
* surfaces a missing lock at the offending call site under a debug
83+
* build instead of letting a torn table escape into production. */
84+
#define MINO_ASSERT_STATE_SAFE(S) \
85+
assert(!(S)->threading.multi_threaded \
86+
|| mino_current_ctx(S)->lock_depth > 0)
87+
7688
#endif /* RUNTIME_COORDINATION_H */

src/runtime/host_threads.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,12 @@ static void worker_run(mino_future *impl, char *stack_anchor)
316316
dyn_binding_t *bhead = NULL;
317317
size_t i;
318318
int oom = 0;
319+
/* Symbol interning mutates the shared intern table; on a
320+
* multi-threaded state every writer must hold state_lock.
321+
* The thunk's mino_call acquires it just below, but the
322+
* convey-binding build runs before that, so take the lock
323+
* explicitly for the symbol/string installs. */
324+
mino_lock(S);
319325
for (i = 0; i < snap->as.map.len; i++) {
320326
mino_val *key = vec_nth(snap->as.map.key_order, i);
321327
mino_val *val = map_get_val(snap, key);
@@ -331,6 +337,7 @@ static void worker_run(mino_future *impl, char *stack_anchor)
331337
b->next = bhead;
332338
bhead = b;
333339
}
340+
mino_unlock(S);
334341
if (!oom) {
335342
conveyed = (dyn_frame_t *)malloc(sizeof(*conveyed));
336343
if (conveyed == NULL) { dyn_binding_list_free(bhead); }

src/values/val.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,14 @@ mino_val *intern_lookup_or_create_ns(mino_state *S, intern_table_t *tbl,
182182
mino_val *v;
183183
size_t ns_len;
184184

185+
/* Caller-must-hold-state_lock contract: the intern table is shared
186+
* across host worker threads, but its open-addressing hash, the
187+
* append-only entries[] array, and the GC-coupled allocations
188+
* inside the insert path all assume serialized writers. Surface
189+
* a missing lock at the call site (debug builds) instead of
190+
* letting a torn entries[] cell escape into production. */
191+
MINO_ASSERT_STATE_SAFE(S);
192+
185193
/* ns_len resolution: callers that constructed via 2-arg (keyword
186194
* ns name) pass an explicit `ns_len_hint`. Single-string callers
187195
* pass (size_t)-1; we derive ns_len from the LAST '/' in `s` so a
@@ -563,6 +571,13 @@ mino_val *mino_defrecord(mino_state *S,
563571
return NULL;
564572
}
565573

574+
/* Caller-must-hold-state_lock contract: the linked record-type
575+
* registry is shared across host worker threads but mutated with
576+
* a plain prepend below; concurrent defrecords would race on the
577+
* head pointer. Surface a missing lock at the call site (debug
578+
* builds) before the torn list escapes. */
579+
MINO_ASSERT_STATE_SAFE(S);
580+
566581
/* Intern ns and name strings via the symbol table so we can compare
567582
* with pointer equality and the storage outlives any caller-owned
568583
* buffer. */

0 commit comments

Comments
 (0)