Hacktoberfest 2026:维护者为十月标记出来的 issue,仍然开放、适合新手。 浏览 Hacktoberfest issue

createStoreMutex is not mutually exclusive: tryLock's callback means "retry", not "granted"

未关闭
#218 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

维护者通常 1 天内回复

还没有人认领这个 Issue。

评估

难度
3/5
预计耗时
1-2 天
新手友好度
74/100
Issue 类型
缺陷
描述清晰度
描述清楚
活跃度
活跃
技术栈
javascript

调研方向

Start with packages/plugin/src/util/mutex.js and compare its tryLock callback handling with the documented storage-engine behavior. Update the mutex tests and their fakes in test/mutex.test.js and test/runState.test.js, then run the full plugin suite and confirm queued callers do not overlap in the provided concurrency scenarios.

由索引模型根据 Issue 内容生成。

描述

bug

TL;DR

  • createStoreMutex resolves lock() when tryLock's callback fires, as if the lock had been handed over. Neither storage engine hands locks over. unlock erases the lock and fires every queued callback, so every woken waiter, plus any caller that arrives in the gap, runs the critical section at once.
  • Reproduced on real lmdb 3.5.6 and @harperfast/rocksdb-js 2.8.0 (observed): up to 7 concurrent holders with 8 worker threads. A claim simulation built on the plugin's own runClaimPass and renderLease.js handed out 22–71 of 800 due URLs more than once across 8 runs.
  • Scope: every release since prerender-v0.2.0 (mutex.js is unchanged since that tag), on both engines. It guards claim, setPause, refreshQueueStatus (which also resets the claim floor and reconciles the lease gauge), resetClaimFloor, runState and visitFilter.
  • Fix: retry tryLock from the callback and resolve only when it returns true. The test fakes model hand-off, which is why the suite passes today. A patch with corrected fakes is ready (below); the full plugin suite passes with it.

Mechanism

The mutex (mutex.js#L23-L31):

new Promise((resolve) => {
	if (store.tryLock(lockKey, resolve)) resolve();
});

Its header (L10) says unlock "grants it to the next queued waiter". Both engines do something else (verified in their source):

Engine unlock Behavior
lmdb-js 3.5.6 ExtendedEnv::unlock notifyCallbacks(all), then lockCallbacks.erase
rocksdb-js 2.8.0 DBDescriptor::lockReleaseByKey moves the queue out, erases the lock, calls every callback

The rocksdb-js README documents the callback as onUnlocked, a signal "to retry acquiring the lock".

The failure sequence:

  1. A holds the lock. B and C call lock(); tryLock returns false and queues their callbacks.
  2. A calls unlock(). The lock is erased and both callbacks fire, so B and C both resolve and run.
  3. D calls lock() while they run. tryLock finds the key free and succeeds, so D runs too.
  4. B finishes and calls unlock(), which releases the lock D is holding (unlock is keyed by key, not by owner).

A single woken waiter never overlaps the caller that just released. Overlap needs two or more waiters, or an arrival while the woken waiter is running.

Why the tests pass: the fakes in test/mutex.test.js and test/runState.test.js hand the lock to the next waiter on unlock (shift the queue, call that one callback), which neither engine does.

Reproduction

Engine semantics, observed identically on both engines:

const fired = [];
store.tryLock('k', () => fired.push('A')); // true (callback never fires)
store.tryLock('k', () => fired.push('B')); // false, queued
store.tryLock('k', () => fired.push('C')); // false, queued
store.unlock('k');
// 50 ms later: fired = ['B', 'C'], store.hasLock('k') === false

Mutex harness. This runs the plugin's createStoreMutex (byte-identical copy) against a real store shared by worker_threads. Each caller runs 150 iterations. Every scenario runs in three critical-section modes: a 1 ms timer await, one setImmediate turn, and a synchronous spin. The table shows the timer mode, which is closest to claim because claim awaits inside the lock. Environment: Node 24.15, macOS arm64.

Scenario (timer mode) Current: max holders Current: lost counter updates Current: unlocks by a caller that never acquired the lock Fixed
2 threads 2 29–32 / 300 125–138 / 300 1 holder, 0 lost, 0 such unlocks
4 threads 4 296–299 / 600 515–528 / 600 same
8 threads 7 883–889 / 1200 1164–1175 / 1200 same
1 thread, 4 async callers 4 360 / 600 480 / 600 same
4 threads × 3 async callers 10–12 1491–1494 / 1800 1792–1795 / 1800 same
  • Ranges span both engines.
  • "Unlocks by a caller that never acquired the lock" includes both releasing a key another caller held and releasing an already-free key.
  • The fixed mutex held one holder, lost no updates and had no such unlocks in all 15 scenario × mode configurations on each engine.
  • On the current mutex, the only runs without overlap were ones where the critical section did little or no awaiting.

Claim level. Worker threads run the plugin's runClaimPass (diff-checked verbatim against this commit), renderLease.js and hash.js, with the lease table in the real store's shared buffer, under the mutex. Two parts are modeled, not the plugin's:

  • The index read is an in-memory sorted corpus that yields to the event loop every 50 rows.
  • The wrapper is a harness loop, not RenderQueue.claim or claimSchedules.

8 threads × 10 claims over 800 due rows:

Engine Mutex Runs Max concurrent passes Jobs granted URLs granted more than once Passes refused a lease
rocksdb-js 2.8.0 current 4 5–7 829–846 28–45 3–6
lmdb 3.5.6 current 4 6–7 823–878 22–71 0–6
both fixed 2 each 1 800 0 0

A duplicate grant happens when a concurrent pass's grant() finds the other pass's slot for the same key, treats it as a renewal and returns true. In most runs, total grants minus 800 exceeds the duplicate-URL count, so some URLs went out three or more times.

Impact per critical section

Critical section Effect of concurrent entry Label
claim, index-scan path Same URL granted to two or more renderers, causing duplicate renders and multiple failure strikes when the render fails observed in the claim simulation (index read modeled)
claim, ready-set path The atomic cursor hands concurrent ready passes different entries. But a short ready set falls back to claimFromIndex, so a ready pass and an index pass can still race on one URL hypothesis, not simulated
grant() write-before-CAS hi/expires/due are stored before the hashLo compare-and-swap, so two keys racing for one free slot can corrupt it. A targeted race of 20,000 rounds: 17,820–18,218 slots torn, and 19,042–19,063 rounds where grant() returned true but no lease was left, i.e. a job that will be granted again. The end-to-end claim simulations produced 0 torn slots race observed; frequency in production is a hypothesis
refreshQueueStatus / setPause refreshQueueStatus also runs maybeResetFloor and reconcileLeaseGauge under this lock. An overlapping status sync can re-read the old pause intent and un-pause the node for up to one sync interval. scanLive walks the slots and then does a plain Atomics.store of the count, which can overwrite a concurrent grant's increment hypothesis
resetClaimFloor Safe against a concurrent pass: a floor advance is a compare-and-swap against the pass's starting value, so a mid-pass reset survives observed (deterministic interleaving harness)
Lease occupancy gauge 0 drift from grant races in the claim-only simulations: a renewal doesn't increment it, and a lost compare-and-swap returns before incrementing. Gauge reconcile running alongside claims was not simulated (see the previous row) observed, claims only
runState claimRun Woken waiters re-read the holder's published run-state row and are refused. A duplicate sweep needs that row to be missing when they read it: a failed publish, a run ending mid-overlap, or the uncommitted-write case in Open items hypothesis
visitFilter persist Bits are OR-merged; worst case some bits persist one interval late hypothesis

When it triggers (hypothesis)

  • Steady state: rare. Overlap needs two or more queued waiters, or a waiter plus a new arrival while the woken one runs, all within a short claim pass. Pass durations were not measured.

  • Bursts:

    • An MQTT empty → queued wake sends the whole fleet at one node at nearly the same moment.
    • Fleet restarts.
    • Harper restarts, where the ready set is empty and claims fall back to the longer index scan.

    Expect a few duplicate grants per burst. That is likely hard to tell apart from the duplicates a rolling restart already produces.

  • Nothing in production has been traced to this yet. Grant races can hit the "publish CAS lost a race" branch added in 07d0e12. It surfaces as [prerender] claim could not record a lease for a due row (RenderQueue.js#L1414-L1422).

    • In the claim simulations, 0–6 passes per run were refused a lease with the current mutex, and 0 with the fix.
    • The same line also fires for a full probe window, so a hit in the logs is suggestive, not proof.

Proposed fix

const lock = () =>
	new Promise((resolve, reject) => {
		const attempt = () => {
			// The callback runs from a native threadsafe function, so a throw here
			// (store closed during shutdown) would otherwise be an uncaught exception.
			try {
				if (store.tryLock(lockKey, attempt)) resolve();
			} catch (error) {
				reject(error);
			}
		};
		attempt();
	});
  • Not FIFO, and every release wakes all waiters. That cost 1.0–11.3 tryLock calls per acquisition across the harness configurations, and no starvation was observed. Fairness was not measured.
  • Native withLock rejected: only rocksdb-js has it (lmdb 3.5.6 does not), and it shares the per-key queue with tryLock, so the two can't be mixed on one key.
  • Tests:
    • Replace the hand-off fakes in test/mutex.test.js and test/runState.test.js with wake-all fakes. Otherwise runState's "two simultaneous starts" test times out with the fix; confirmed at an 8 s timeout.
    • mutex.test.js + runState.test.js with the new fakes: 5 of 24 fail against the current mutex, 24 of 24 pass with the fix.
    • Full plugin suite: 1412/1412 passing with the fix.
  • A patch covering those three files applies cleanly to this commit, and prettier and eslint pass on it. I can open it as a PR.

Open items

  • Untested:
    • The ready-set claim path.
    • Gauge reconcile racing claims.
    • Any run on Linux or inside a live Harper process; the harness drives the stores directly.
  • Hypothesis, not tested, separate from this fix: the claimRun and setPause writes may join the request's ambient transaction and still be uncommitted when the lock is released. If so, no lock can serialize them.
  • Related: rocksdb-js#848, a queued tryLock callback from a terminated worker aborting the process on unlock under Node 22; now closed. The retry loop re-queues callbacks more often, so confirm the pinned rocksdb-js includes that fix.
  • No production log search has been done yet.
主要语言
JavaScript
星标
0
派生
0
平均合并
8 小时 48 分钟
30 天内合并 PR
58

环境准备

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

HarperFast/prerender-plugin 的其他 Issue

查看 HarperFast/prerender-plugin 的全部 Issue

相似的 Issue

更多 JavaScript Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。