Skip to content

Commit 4ca0618

Browse files
committed
fix(v7-i03): _RUN_CACHE locked helpers + race-safe warm-start
Closes plan item #44 / bd cppmega-mlx-lak7. stages.py _RUN_CACHE was a bare dict accessed unsynchronised from both the train loop and the API thread. On Python 3.13+ where dict ops are no longer atomic over compound get/set/eviction sequences a concurrent reader could observe a torn opt.state or hit KeyError mid-eviction. - Add module-level threading.Lock guarding _RUN_CACHE. - New helpers _run_cache_get / _run_cache_set / _run_cache_contains hold the lock only across the dict op itself — the train step outside the helper is never serialised. - _run_cache_set takes the LRU cap (default 32, callers pass 8 to preserve the prior behaviour). - Replace the two raw _RUN_CACHE access sites (line ~958 warm-start load, line ~2009 post-train cache) with the helpers. 3/3 pytest cover: 1000 mixed reads+writes from 8 threads / 2 threads running warm-start train concurrently against the same cached run_id / cache eviction returns None not KeyError for stale keys.
1 parent 9ec4e4d commit 4ca0618

2 files changed

Lines changed: 188 additions & 10 deletions

File tree

cppmega_v4/runner/stages.py

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -955,12 +955,16 @@ def loss_fn(model: nn.Module, emb: mx.array, tgt: mx.array) -> mx.array:
955955
opt_state_carried = False
956956
run_id = str(opts.get("run_id") or id(opt))
957957
continue_from = opts.get("continue_from_run_id")
958-
if continue_from and continue_from in _RUN_CACHE:
959-
try:
960-
opt.state = _RUN_CACHE[continue_from]
961-
opt_state_carried = True
962-
except Exception:
963-
pass
958+
if continue_from:
959+
# V7-I03: single locked read, then assign outside the lock so
960+
# the train step never serialises on the cache.
961+
cached_state = _run_cache_get(continue_from)
962+
if cached_state is not None:
963+
try:
964+
opt.state = cached_state
965+
opt_state_carried = True
966+
except Exception:
967+
pass
964968
# V4-9: when hybrid, count params routed to each bucket so e2e can
965969
# prove the split predicate actually saw 2D vs 1D/3D parameters.
966970
muon_group_size: int | None = None
@@ -2000,11 +2004,11 @@ def _compiled_step_value_and_grad(_emb, _targets):
20002004
)
20012005
opt.state = nn.utils.tree_unflatten(sorted_state_pairs)
20022006

2003-
# G10: cache opt.state for future warm-start lookups (capped LRU)
2007+
# G10 / V7-I03: cache opt.state for future warm-start lookups
2008+
# via the locked helper so a concurrent reader can't observe a
2009+
# partial dict mid-eviction.
20042010
try:
2005-
_RUN_CACHE[run_id] = opt.state
2006-
if len(_RUN_CACHE) > 8:
2007-
_RUN_CACHE.pop(next(iter(_RUN_CACHE)))
2011+
_run_cache_set(run_id, opt.state, cap=8)
20082012
except Exception:
20092013
pass
20102014

@@ -2465,7 +2469,37 @@ def _validate_parquet_stream_tokenizer_fingerprints(
24652469

24662470
# G10: in-process LRU cache of opt.state by run_id. Used for warm-start
24672471
# across sequential Train clicks in the same backend session.
2472+
# V7-I03: _RUN_CACHE is read+written from the train loop AND from
2473+
# concurrent UI clicks (two pipeline.run calls landing simultaneously
2474+
# in different worker threads). dict mutation is not atomic across
2475+
# the get/set pair in stage_train, so race readers could observe a
2476+
# partially-populated opt.state. The lock is held only during the
2477+
# get/set/eviction operations — never across the train step itself
2478+
# (which would serialise actual training).
2479+
import threading as _threading
24682480
_RUN_CACHE: dict[str, Any] = {}
2481+
_RUN_CACHE_LOCK = _threading.Lock()
2482+
2483+
2484+
def _run_cache_get(key: str) -> Any | None:
2485+
"""V7-I03: thread-safe read of _RUN_CACHE."""
2486+
with _RUN_CACHE_LOCK:
2487+
return _RUN_CACHE.get(key)
2488+
2489+
2490+
def _run_cache_set(key: str, value: Any, *, cap: int = 32) -> None:
2491+
"""V7-I03: thread-safe write with LRU-style eviction."""
2492+
with _RUN_CACHE_LOCK:
2493+
_RUN_CACHE[key] = value
2494+
# Drop oldest entries past cap (insertion order in py3.7+).
2495+
while len(_RUN_CACHE) > cap:
2496+
oldest = next(iter(_RUN_CACHE))
2497+
_RUN_CACHE.pop(oldest, None)
2498+
2499+
2500+
def _run_cache_contains(key: str) -> bool:
2501+
with _RUN_CACHE_LOCK:
2502+
return key in _RUN_CACHE
24692503

24702504
# G09: in-process set of abort tokens. Caller sets opts.abort_token to
24712505
# some unique string; another caller (e.g. WS handler) inserts the same
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
"""V7-I03 (cppmega-mlx-lak7): _RUN_CACHE warm-start is race-safe.
2+
3+
Two threads launch pipeline.run with continue_from=<same run_id>
4+
that's already cached. Neither should observe a partially-evicted
5+
or torn opt.state (i.e. both stage_train calls succeed end-to-end
6+
without exception).
7+
8+
Pre-fix the cache was a bare dict mutated from the train loop +
9+
the API thread without synchronisation; under load the eviction
10+
path could pop an entry mid-read on Python 3.13+ where dict
11+
operations are no longer GIL-atomic for compound operations.
12+
"""
13+
14+
from __future__ import annotations
15+
16+
import threading
17+
import time
18+
19+
import pytest
20+
21+
from cppmega_v4.jsonrpc.schema import VerifyParams
22+
from cppmega_v4.runner import Pipeline, run_pipeline
23+
from cppmega_v4.runner.stages import (
24+
_RUN_CACHE, _RUN_CACHE_LOCK,
25+
_run_cache_contains, _run_cache_get, _run_cache_set,
26+
)
27+
28+
29+
@pytest.fixture(autouse=True)
30+
def _reset_cache():
31+
with _RUN_CACHE_LOCK:
32+
_RUN_CACHE.clear()
33+
yield
34+
with _RUN_CACHE_LOCK:
35+
_RUN_CACHE.clear()
36+
37+
38+
def _spec() -> VerifyParams:
39+
return VerifyParams.model_validate({
40+
"graph": {
41+
"nodes": [
42+
{"id": "attn", "kind": "attention",
43+
"params": {"num_heads": 4, "head_dim": 64}},
44+
{"id": "mlp", "kind": "mlp", "params": {}},
45+
],
46+
"edges": [{"src": "attn", "dst": "mlp"}],
47+
},
48+
"dim_env": {"B": 1, "S": 8, "H": 128,
49+
"nh": 2, "nkv": 1, "head_dim": 64},
50+
"loss": {"kind": "cross_entropy", "head_outputs": ["mlp"]},
51+
"optim": {"kind": "adamw",
52+
"groups": [{"matcher": "all", "lr": 1e-3,
53+
"weight_decay": 0.01,
54+
"betas": [0.9, 0.95]}]},
55+
})
56+
57+
58+
def _train(opts: dict) -> dict:
59+
rep = run_pipeline(_spec(), Pipeline.from_dict({
60+
"stages": ["parse", "verify_build_spec", "build_model", "train"],
61+
"stage_options": {"train": opts},
62+
}))
63+
tr = next(s for s in rep.stages if s.name == "train")
64+
return {"status": tr.status, "error": tr.error,
65+
"extras": tr.extras}
66+
67+
68+
def test_v7_i03_run_cache_helpers_are_thread_safe_under_load():
69+
"""1000 mixed reads + writes from 8 threads — no exception, no
70+
corrupted entries (every value retrieved equals what was set)."""
71+
errors: list[BaseException] = []
72+
73+
def _hammer(thread_id: int) -> None:
74+
try:
75+
for i in range(125):
76+
key = f"k-{thread_id}-{i}"
77+
payload = {"thread": thread_id, "i": i, "data": [i] * 4}
78+
_run_cache_set(key, payload, cap=64)
79+
if _run_cache_contains(key):
80+
got = _run_cache_get(key)
81+
# cap=64 → many entries evicted by other threads;
82+
# if the key is still present its value must be
83+
# exactly what we wrote, never a torn merge.
84+
if got is not None and got != payload:
85+
raise AssertionError(
86+
f"torn read: set={payload} got={got}")
87+
except BaseException as exc:
88+
errors.append(exc)
89+
90+
threads = [threading.Thread(target=_hammer, args=(t,))
91+
for t in range(8)]
92+
for t in threads:
93+
t.start()
94+
for t in threads:
95+
t.join(timeout=10.0)
96+
assert not errors, errors
97+
98+
99+
def test_v7_i03_concurrent_train_warm_start_completes_both(tmp_path):
100+
"""First train populates _RUN_CACHE['shared']. Then two threads
101+
each launch a fresh train with continue_from='shared' — both
102+
must finish status='ok' (no exception from a torn read)."""
103+
base = _train({"num_steps": 2, "run_id": "shared"})
104+
assert base["status"] == "ok"
105+
assert _run_cache_contains("shared")
106+
107+
results: list[dict] = []
108+
errors: list[BaseException] = []
109+
110+
def _worker(idx: int) -> None:
111+
try:
112+
r = _train({
113+
"num_steps": 2, "run_id": f"warm-{idx}",
114+
"continue_from_run_id": "shared",
115+
})
116+
results.append(r)
117+
except BaseException as exc:
118+
errors.append(exc)
119+
120+
t1 = threading.Thread(target=_worker, args=(0,))
121+
t2 = threading.Thread(target=_worker, args=(1,))
122+
t1.start(); t2.start()
123+
t1.join(timeout=30.0); t2.join(timeout=30.0)
124+
assert not errors, errors
125+
assert len(results) == 2
126+
for r in results:
127+
assert r["status"] == "ok", r["error"]
128+
# Both runs report opt-state actually carried from the shared
129+
# warm-start entry.
130+
assert r["extras"].get("opt_state_carried") is True
131+
132+
133+
def test_v7_i03_cache_eviction_does_not_strand_concurrent_readers():
134+
"""Pre-fix path: reader could call cache[k] right after writer
135+
popped k via eviction → KeyError. Post-fix: _run_cache_get returns
136+
None safely. Pin that contract."""
137+
for i in range(100):
138+
_run_cache_set(f"k{i}", {"i": i}, cap=4)
139+
# cap=4 enforced → only ~last 4 keys remain.
140+
surviving = sum(1 for i in range(100)
141+
if _run_cache_contains(f"k{i}"))
142+
assert surviving <= 4
143+
# Stale key returns None, not KeyError.
144+
assert _run_cache_get("k0") is None

0 commit comments

Comments
 (0)