diff --git a/src/docs/persistence-and-ha-design.md b/src/docs/persistence-and-ha-design.md index 26bc77e..2402d99 100644 --- a/src/docs/persistence-and-ha-design.md +++ b/src/docs/persistence-and-ha-design.md @@ -467,6 +467,24 @@ Since indices are stable (hash table maps key→same index always), a `vput(42, This is required on ALL architectures, not just ARM — compiler reordering is sufficient to cause the race even on x86-64. The kvmgr READ lock in `vget()` callers provides happens-before between `vput()` and `vget()` on the **same vcpu thread** (serialized by lock/unlock), but does not prevent the compiler from reordering stores within a single `vput()` call. The atomic store on `.inuse` is the only reliable barrier against both compiler and CPU reordering. +**Concurrent `vstore` race under READ lock (CRITICAL)**: During COW mode, multiple vcpu threads may hold the kvmgr READ lock simultaneously and call `vstore()`. The kvmgr READ lock does NOT serialize these calls — it allows concurrent readers. Two concurrent `vstore()` calls hitting the "no free slot in `[shadow_caps, a->caps)`" path will both call `xzarray_ensure_items(a, a->caps + a->delta)`, and both will write to `a->data[old_caps]` — the second overwrites the first, silently losing data. **Fix**: During COW mode, `vstore()` must acquire the inner xzarray mutex (`mylock_lock(&priv->lock)`) around the growth-and-assign section. This is the ONLY place in the COW protocol where the inner mutex is required — normal-mode vput and vstore rely on the kvmgr WRITE lock for serialization. The growth section in `xzarray_store()` must be: +```c + /* COW mode growth: serialize around the grow-and-assign */ + mylock_lock(&priv->lock); + if (a->data[idx].inuse) { /* Re-check after acquiring lock */ + int old_caps = a->caps; + if (a->caps > SIZE_MAX - a->delta) { mylock_unlock(&priv->lock); return -ENOMEM; } + xzarray_ensure_items(a, a->caps + a->delta); + a->data[old_caps].data = data; + atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse, 1, memory_order_release); + a->count++; + a->pos = old_caps + 1; + mylock_unlock(&priv->lock); + return old_caps; + } + mylock_unlock(&priv->lock); +``` + #### vstore — Must Skip Shadow Indices **Design invariant**: After `xzarray_freeze()`, the new active array is empty (`count=0`, `pos=shadow_caps`). The `vstore` scan during COW mode must use a **linear scan bound to `[shadow_caps, a->caps)`** — it must NOT wrap around to index 0 when reaching `a->caps`. If all slots in `[shadow_caps, a->caps)` are occupied, `vstore` grows the array via `xzarray_ensure_items()` and assigns from the new capacity. This prevents index reuse below `shadow_caps`, which would cause silent data corruption when `vget` falls through to the stale shadow entry. @@ -504,15 +522,20 @@ static int xzarray_store(meta_obj_t* obj, void* data) { } } /* No free slot in [shadow_caps, a->caps) — grow array. - * Overflow check: must not attempt growth if caps + delta overflows. */ + * Must serialize around growth: concurrent vstore calls under READ lock + * would both hit this path. See the concurrent vstore race fix above. */ int old_caps = a->caps; - if (a->caps > SIZE_MAX - a->delta) return -ENOMEM; - xzarray_ensure_items(a, a->caps + a->delta); + mylock_lock(&priv->lock); + if (a->data[old_caps].inuse) { /* Re-check: another thread may have grown */ + if (a->caps > SIZE_MAX - a->delta) { mylock_unlock(&priv->lock); return -ENOMEM; } + xzarray_ensure_items(a, a->caps + a->delta); + } a->data[old_caps].data = data; atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse, 1, memory_order_release); a->count++; a->pos = old_caps + 1; + mylock_unlock(&priv->lock); return old_caps; } @@ -1942,6 +1965,8 @@ xce_reply(ep, c->myhost, &reply); **Replication scope**: Sync replication (`repl_wait_ack`) only triggers for `WAL_JOB_SUBMIT` entries, which originate from API server connections. The controller only handles connections from workers, scheduler, and API servers — never end users. At 1–3 API server connections, the vcpu pool is not at risk of exhaustion from blocking `repl_wait_ack()` calls. +**Array job sync replication latency bound (CRITICAL)**: Array jobs submit multiple sub-jobs in a single `handle_jobsubmit()` call, each firing an `AFTER_EVENT` hook that triggers sync replication. An array with 10,000 elements would block the vcpu thread for `10,000 × 50ms = 500 seconds` — a denial of service. **Mitigation**: For array submits, the persistence hook must batch the WAL entries and send them in a single `repl_broadcast()` call (or a small number of batched calls) rather than one per sub-job. The sync replication wait (`repl_wait_ack`) should wait for the batch's highest LSN, not per-entry. This bounds array submit latency to `O(num_batches × sync_timeout_ms)` instead of `O(array_size × sync_timeout_ms)`. Implementation: `persist_job_hook_fun()` detects `detail == HOOK_DETAIL_JOB_NEW` with `submit->array_size > 1` and uses batch mode. A reasonable batch size is 100 sub-jobs per sync wait, bounding worst-case latency to `(array_size / 100) × 50ms`. For a 10,000-element array, this is `100 × 50ms = 5 seconds` — acceptable for the submission path. + ### 8.4 Registration in `cluster_init()` Hook registration is **conditional on role** and happens **after** `persist_recover()` completes. This resolves two constraints: