thor/GRAMMAR_CACHE_THREADING.md

# Grammar cache thread-safety & allocator lifetime

Status: **resolved.** All three issues below are fixed. The cache was redesigned
so the library caches only the immutable, shareable objects and callers own the
per-thread ones. The rest of this document is kept as the rationale.

## Resolution (what shipped)

The `Grammar_Cache` (which mixed shareable `language`/`query` with per-thread
`parser`/`cursor`) was split:

- **`Grammar`** holds only the immutable `language` + `query`. It lives in a
  process-lifetime **`registry`** (`map[string]^Grammar`) guarded by one mutex.
- **`grammar(lang)`** is the thread-safe, self-initializing, lazy loader — fixes
  **#1a** (all map access under the lock) and lets tests run with no setup.
- The registry allocator is pinned with `os.heap_allocator()` — fixes **#2**; the
  cache no longer captures a transient `context.allocator`.
- Parsers/cursors are **caller-owned**: `open_parser(g)` mints one, the caller
  `defer`s `parser_delete` / `query_cursor_delete`. No parser is shared, so
  **#1b** is gone. `open_parser` is the seam where a future pool / thread-local
  strategy plugs in without touching call sites.
- Deleted: `ensure_parser`, `load_grammar`, `preload_grammar`, `grammar_worker`,
  `cache_mutex`; `preload_grammars` is now a serial registry warm-up. The minify
  tests dropped their `parse_mu` serialization and preload scaffolding entirely.

The **later** decision — pool vs per-thread parser reuse for genuine parallel
parsing — is still open, but intentionally so: it only matters once we parse in
parallel, and it now changes only `open_parser`, not the rest of the library.
The original investigation follows.

## How we got here

While adding a feature to minify inline `<style>` blocks (route the CSS body
through `minify_css`), the new unit tests exercised `minify_html` / `minify_css`
for the first time. Those are the first tests to touch the tree-sitter parser
cache, and under the multi-threaded test runner (default 16 threads) they failed
**intermittently** with a rotating cast of symptoms:

- `panic: unable to insert into a map`
- tree-sitter internal aborts: `ts_subtree_retain: Assertion 'self.ptr->ref_count > 0' failed`, `_array__erase: Assertion 'index < *size' failed`
- `malloc(): unaligned tcache chunk detected`
- assertions passing on the wrong data (e.g. minified output missing `<style>`)

The flakiness comes from **three distinct problems**, two thread-safety and one
allocator-lifetime. They are orthogonal — fixing one does not fix the others.

## The three issues

### #1a — concurrent cache **writes** (data race)

`ensure_parser` (`treesitter.odin`) reads and writes `grammar_store.cache`
with **no lock**. `cache_mutex` exists but is only taken on the parallel preload
path (`grammar_worker`), never in the lazy `ensure_parser` / `load_grammar`
paths. Two threads first-touching `"html"`/`"css"` at once corrupt the map.

### #1b — concurrent parser **use** (data race)

There is exactly **one cached `Grammar_Cache` (and one `parser`) per language**.
When two threads both call `minify_html`, they fetch the *same* `gc.parser` and
call `parser_parse_string` on it simultaneously. A tree-sitter `TSParser`
mutates its internal subtree pool during a parse, so concurrent parses corrupt
it — that's the `ts_subtree_retain` / `_array__erase` aborts.

This is **not fixed by preloading/"loading html+css statically."** Loading gives
you *one shared* parser; the race is about *using* that one parser from two
threads. It is fixed only by serializing the parse or giving each thread its own
parser.

### #2 — allocator lifetime (not a race)

`init_persistent` captures `context.allocator` into the process-lifetime
`grammar_store`. In `main` that happens to be the heap (it runs before the site
arena is installed at `main.odin`), so production is fine. But whatever
allocator is live at that instant becomes the cache's owner forever:

- Under the **test runner**, `context.allocator` is a **per-test tracking
  allocator** that is torn down when that test ends → the global cache is left
  pointing at dead memory (→ `unable to insert into a map`, garbage reads).
- A **lazy** first call during rendering would capture the **site arena**
  (`context.allocator` after `main.odin` swaps it in), which is destroyed every
  watch-mode rebuild → dangling parsers across builds.

This would break even single-threaded; threading just decides *which* test wins
the init race.

## tree-sitter's threading model (the root fact)

tree-sitter objects have specific, documented threading rules:

| Object | Shareable across threads? | Rule |
|---|---|---|
| `TSLanguage` (`tree_sitter_html()` etc.) | Yes | Immutable; share freely. |
| `TSParser` (`gc.parser`) | **No** | One per thread; never parse on one parser from two threads. |
| `TSTree` | Only via `ts_tree_copy` | Copy before using on another thread. |
| `TSQuery` (`gc.query`) | Yes | Immutable after compile. |
| `TSQueryCursor` (`gc.cursor`) | **No** | One per thread, like the parser. |

This explains the existing design:

- The preload fan-out (`preload_grammar` / `grammar_worker`) creates a parser
  **per worker** and only publishes the finished `Grammar_Cache` under
  `cache_mutex` — because parsers can't be shared during construction/use.
- `ensure_parser` + the shared cached parser is fine in production **only
  because rendering is single-threaded**.

A cached `Grammar_Cache` mixes shareable (`language`, `query`) and per-thread
(`parser`, `cursor`) objects in one shared struct, which makes it
single-threaded-use by construction — a fact that was previously undocumented.

## Where this actually bites

**Only in tests.** Confirmed by grep:

- Production parses on one thread: every real caller — `minify.odin` (html/css)
  and `markdown/highlight.odin` — runs inside `main`'s single-threaded render
  loop. No concurrent `ensure_parser` / `parser_parse_string`.
- The one threaded path in production, `preload_grammars`, is already safe (own
  parser per worker + `cache_mutex` on the write) and only handles non-builtin
  grammars.
- Only `minify_test.odin` touches tree-sitter from tests, and the test runner
  runs those on many threads.

So production is correct **by convention** (single-threaded-from-heap usage),
not **by construction** — nothing stops a future parallel render from breaking
it, and until now nothing documented the constraint.

## Chosen direction

**For now:** keep the single global grammar cache and make it safe by
**serializing the non-thread-safe behavior with a mutex** — only one thread uses
a parser at a time. This is simple, matches how production already behaves, and
unblocks the tests.

**Later (TODO):** decide whether true multi-threaded parsing is worth it for
performance. If yes, give each thread its own parser + cursor (as the preload
workers already do) rather than sharing one behind a lock.

## Implementation options (deferred — pick when we resume)

All assume the allocator fix (#2) below, which is independent and unambiguous.

- **#2 allocator (do regardless):** pin the heap allocator explicitly —
  `grammar_store.allocator = runtime.heap_allocator()` (or `os.heap_allocator()`)
  instead of `context.allocator`. Makes the cache persistent from any context;
  behavior-identical in production. Optionally make `init_persistent` idempotent
  and/or an `@(init)` so callers need no ordering.

Then, for the races:

- **Option A — library mutex (sustainable):** take `cache_mutex` in
  `ensure_parser` (fixes #1a) and add a locking `parse` helper that wraps
  `parser_parse_string` (fixes #1b), wired into the three call sites
  (`minify.odin` ×2, `markdown/highlight.odin` ×1). Tests then need no
  scaffolding. Serializes all parsing.

- **Option B — test-side only (minimal):** leave the library single-threaded by
  contract; make the tests not violate it. Either fold the minify tests into one
  `@(test)` (no concurrency at all → no locks), or wrap the `minify_*` calls in a
  test-local mutex. Production stays lock-free.

Recommendation captured during the discussion: the allocator one-liner is
worth doing immediately; for the races, Option A is the "correct" long-term
answer but only necessary if/when we want concurrent parsing — otherwise
Option B is enough.

## Pointers

- `treesitter/treesitter.odin`: doc comments on `Grammar_Cache`, `cache_mutex`,
  `init_persistent`, `ensure_parser` reference this file.
- Original feature that surfaced this: minify inline `<style>` bodies via
  `minify_css` (reverted; to be re-landed alongside whichever fix we choose).