Prevent job wedges on runtime database opens - #2169
Conversation
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a database open lock mechanism to serialize native RocksDB opens across worker threads, preventing race conditions and contention for RocksDB's process-local LOCK file. It also tracks these locks on the main thread to ensure they are released if a worker thread exits prematurely. Additionally, it cleans up schema change signaling in ResourceBridge.ts, ensures fallback operations in schema.ts, and adds corresponding unit tests. Feedback is provided to update the worker_threads import in resources/databases.ts to use the node: prefix, as required by the repository style guide.
| import * as signalling from '../utility/signalling.ts'; | ||
| import { SchemaEventMsg } from '../server/threads/itc.js'; | ||
| import { workerData } from 'worker_threads'; | ||
| import { isMainThread, workerData } from 'worker_threads'; |
There was a problem hiding this comment.
According to the repository style guide, Node builtins must use the node: prefix. Please update the import to use node:worker_threads.
| import { isMainThread, workerData } from 'worker_threads'; | |
| import { isMainThread, workerData } from 'node:worker_threads'; |
References
- Node builtins use the
node:prefix. These are constraints, not style choices. (link)
| throw new Error(`Timed out acquiring RocksDB open lock for ${dbPath}`); | ||
| } | ||
| sleeper ??= new Int32Array(new SharedArrayBuffer(Int32Array.BYTES_PER_ELEMENT)); | ||
| Atomics.wait(sleeper, 0, 0, 10); |
There was a problem hiding this comment.
Main thread can block its own event loop for up to 30s here
What: acquireDatabaseOpenLock retries with a blocking Atomics.wait(sleeper, 0, 0, 10) in a tight loop (up to maxWaitMilliseconds, default 30s) with no yield back to the event loop between iterations. resources/databases.ts:363 calls this via acquireTrackedDatabaseOpenLock for every root open (!options.name) with no isMainThread exemption, unlike the message-tracking code right above it which is guarded by if (isMainThread)/if (!isMainThread).
The main thread does open root databases directly, not just workers: bin/run.ts → startHTTPThreads() → loadRootComponents() → getTables()/getDatabases() on initial boot, and — more concerningly — manageThreads.js's restartWorkers() (if (isMainThread) { ... await loadRootComponents(); }, lines ~456-471) re-runs this on every component/app deploy or reload, concurrently with worker threads restarting and opening the same runtime-created database for the first time. That's exactly the contention window this PR is designed to serialize.
Why it matters: If the main thread loses the race for this lock during a restart, it synchronously blocks in Atomics.wait for up to 30 seconds. The main thread is the process's IPC hub — manageThreads.js's message dispatch, the new onThreadExit handler that releases a worker's stranded open-lock token (the very mechanism this PR adds), and all other worker coordination stop dead for that window. A worker that dies while holding the lock can't be cleaned up until the main thread's own poll loop finishes, and no other IPC traffic (including HTTP routing setup, shutdown signals, etc.) can be serviced meanwhile. This risks reintroducing a wedge on the main thread instead of a worker.
Suggested fix: Skip the lock (or use a non-blocking/async wait, e.g. polling via setTimeout/microtask so the event loop keeps running) when isMainThread is true, mirroring the existing isMainThread branching already used for the message-tracking calls in this same function.
|
1 blocker found (inline, still open): the finding on dataLayer/restoreMarker.ts:105 remains unaddressed at current HEAD (f7306e9). acquireDatabaseOpenLock's blocking Atomics.wait retry loop still runs unconditionally with no isMainThread exemption, and resources/databases.ts:363 (acquireTrackedDatabaseOpenLock) calls it for every root open regardless of thread. The new test in unitTests/dataLayer/restoreMarker.test.js (excludes a concurrent worker-thread open until the first open completes) only exercises worker-thread contention -- it does not cover the main-thread blocking path. manageThreads.js's restartWorkers() still runs loadRootComponents() on the main thread during every deploy/reload (calling through getTables -> getDatabases -> openRocksDatabase on a first-time open), which is the exact contention window this PR serializes. See the existing inline thread for the full reachability trace. |
aa2205f to
d13208f
Compare
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
d13208f to
f7306e9
Compare
|
The root cause is in rocksdb-js's process-global database lifecycle, not Harper's resource-open path. rocksdb-js PR #787, "Serialize database destruction with concurrent opens" moves the invariant to the native registry: destruction owns the physical path, closes all in-process descriptors, waits for in-flight backup/checkpoint work, and blocks concurrent opens until teardown completes. Recommended dependency sequence:
This PR should remain draft until that artifact exists. The schema-change signal deduplication and LMDB alias cleanup are separable changes and do not need to depend on the rocksdb-js lifecycle fix. — GPT-5 Codex |
Summary
DestroyDBwhile its directory is being dropped.drop_databasepolling workaround and assert the direct operation succeeds.drop_database.Root cause
In the failing uWS integration run, the first stuck job starts at the same second RocksDB records a same-process
LOCKacquisition fortuckerdoodle. rocksdb-js 2.7.1 already serializes concurrent opens. Its registry, however, releases a database entry before it callsDestroyDB. The precedingdrop_databaseraces that destroy with schema propagation reopening the root in another thread; the job worker then hits the resulting nativeLOCKfailure during module initialization, beforejobProcessenters its error-handling IIFE.The fix uses the existing per-database mutex for both root opens and destruction. A reopen now waits for
DestroyDBto finish rather than reaching RocksDB while the directory is being removed.Verification
npm run buildnpm run lint:requirednpx mocha unitTests/dataLayer/restoreMarker.test.jsnpx mocha unitTests/server/threads/preloadSafeMode.test.jsHARPER_UWS_HTTP=1 node --experimental-strip-types --test integrationTests/apiTests/terminology.test.mjs(47 passing)npm run test:unit:mainexercised the new coverage but has two unrelated existing failures in this worktree: deployment-recorder timeout budget and the long-worktree domain-socket path expectation. The full integration fan-out exited nonzero without retaining the failing suite's log; the suspected final rate-limiter suite passes cleanly in isolation.Independent storage review was run before the final PR. It confirmed the
DestroyDBgap and identified broader follow-up work for pre-IIFE job failures; this stays a draft for human storage review.— GPT-5 Codex