Add type-strip mode: run Harper directly from .ts sources#562
Draft
kriszyp wants to merge 48 commits into
Draft
Conversation
Introduces utility/lifecycle.ts with onStartup(cb) / runStartup() so side-effectful module-load work (server-singleton wiring, listener registration, config-derived constants) can be deferred to a controlled startup phase that runs after env.initSync() and before request handling. This is the lever that makes ESM-cycle TDZs avoidable without restructuring the import graph. Adds RUNTIME_SRC_ROOT and RUNTIME_FILE_EXT to packageUtils.js. Detects whether the running modules are .ts sources (type-strip) or .js dist files by inspecting packageUtils.js's own __dirname, so callers like manageThreads.startWorker() can spawn workers with the correct file path under both modes from a single string identifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames the 57 .js source files that participate in Harper's runtime
module graph to .ts and rewrites them in ESM form. The driving constraint:
under Node's type-strip mode, .ts files with import/export syntax are
loaded as ESM, and any CJS .js file that require()s a .ts ESM file in the
entry's evaluation chain throws ERR_REQUIRE_CYCLE_MODULE.
Mechanical edits applied across all converted files:
- const X = require('mod') → import X from 'mod'
- const { A, B } = require('mod') → import { A, B } from 'mod'
- const X = require('mod').default → import X from 'mod'
- module.exports = { a, b, ... } → export { a, b, ... }
- module.exports = X → export default X
- 'use strict' pragma removed (ESM default)
- Relative imports given explicit .ts/.js extensions
These files keep their existing internal structure and behavior; the
conversion is syntactic, not semantic. Behavioral changes (deferred
side-effects, late-binding, etc.) live in the follow-up commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the runtime support needed to run Harper directly from .ts sources
under Node's type-strip mode (node bin/harper.ts) while keeping the
existing CJS dist build (node dist/bin/harper.js) working unchanged.
Entry point (bin/harper.ts):
- Switch cases use await import('./mod.ts') instead of require('./mod')
- Splits boot into initEnv() (always cheap) and runServerStartup() (only
for start/run paths); version/help skip the heavy init
- Replaces require.main === module with a dual-mode isEntry check
Worker thread bootstrap (server/threads/threadServer.ts):
- Workers run env.initSync() → runStartup() → startServers() in an async
IIFE so they see a populated env and complete onStartup hooks before
serving requests
manageThreads.startWorker():
- Accepts an extensionless module identifier and appends RUNTIME_FILE_EXT
- Resolves against RUNTIME_SRC_ROOT (source dir under type-strip,
dist/ under CJS dist)
- listenersByType / messageListeners lazy-initialized inside the accessor
so callers don't trip TDZ when manageThreads is loaded mid-cycle
Late-binding for class-extends-Resource modules:
- ErrorResource, CertificateVerificationSource,
CertificateRevocationListSource, Login: the class is constructed
inside onStartup() and exposed via `export let X` so consumers see a
live binding after startup. These modules sit inside Resource.ts's own
static-graph SCC, so a class-extends declaration at module-top TDZ'd.
Module-load side-effect deferrals (one-off onStartup wrappers):
- server.X = … assignments in security/user.ts, security/auth.ts,
resources/analytics/write.ts, server/http.ts,
server/serverHelpers/serverUtilities.ts, server/threads/threadServer.ts
- Object.defineProperty(server, …) in server/nodeName.ts and
server/threads/manageThreads.ts
- onMessage* listener registrations in server/threads/itc.ts,
security/keys.ts, resources/analytics/write.ts,
utility/processManagement/processManagement.ts, bin/restart.ts
- Config-derived constants in security/auth.ts and
server/serverHelpers/contentTypes.ts converted to lazy getters
- server.contentTypes attachment in contentTypes.ts deferred
- whenComponentsLoaded-awaiting IIFE in
server/DurableSubscriptionsSession.ts deferred
Environment manager (utility/environment/environmentManager.ts):
- env.get() and getHdbBasePath() made defensive against being called
before this module's body has run (catches TDZ ReferenceError from
configUtils.getConfigValue and returns undefined). Belt-and-suspenders
for the rare timing window where a module-load access reaches in
before initSync completes.
- Removed 22 eager module-top env.initSync() calls; initialization is
now centralized in bin/harper.ts (initEnv) and threadServer.ts
- Top-level let/const → var so subsequent functions don't TDZ on
module-internal state if invoked through the cycle
harper_logger.ts:
- RootConfigWatcher import moved from static (which forced a
configUtils → logger cycle) to dynamic, inside the function that
actually constructs the watcher
- logRotator require replaced with dynamic import inside its setTimeout
- module.exports = {…} wrapped in `if (typeof module !== 'undefined')`
so the CJS-shaped legacy export survives tsc emit but is skipped
under ESM, where the parallel export default already covers the
default-import consumers
Compatibility fixes:
- Type-only imports tagged via tsc --verbatimModuleSyntax sweep
- CJS-only packages (lodash, fs-extra, micromatch, papaparse, etc.)
switched to default-import-and-destructure
- JSON imports tagged with `with { type: 'json' }`
- Extensionless relative imports given explicit .ts/.js extensions
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`(mod.default || mod)()` only worked when `await import()` returned an
ESM-shaped namespace (mod.default = function). In tsc-compiled CJS dist,
`await import('./install.js')` from a CJS file wraps the CJS exports as
mod.default, so the actual function is at mod.default.default. CI's
`Setup Harper` step (which runs the dist build) hit this with
`TypeError: (mod.default || mod) is not a function`.
Adds a small getDefaultExport() helper that walks both shapes and applies
it to the INSTALL, STOP, and STATUS cases in bin/harper.ts.
Verified locally:
- node bin/harper.ts install → reaches installer
- node dist/bin/harper.js install → reaches installer
- node bin/harper.ts version → 5.1.0
- node bin/harper.ts → "Harper successfully started."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Reviewed; no blockers found. |
Issues caught by the claude/review bot and the unit-test job:
1. bin/run.ts:235 (`launch()`) — `require('../utility/processManagement/processManagement.ts')` would throw `ReferenceError` under typestrip ESM. Converted to `await import(...)` (mirrors `filterArgsAgainstRuntimeConfig` a few lines above).
2. server/threads/threadServer.ts (`onSocket`) — `require('../../components/componentLoader.ts').getComponentName` had the same problem on the secure-port code path. Converted onSocket to async and used `await import(...)`. Inspector calls in the same file (debug-mode paths) now use a single static `import * as inspector from 'node:inspector'` instead of per-call requires, so they work in both modes.
3. utility/functions/sql/alaSQLExtension.ts — `import mathjs from 'mathjs'` got `undefined` for `mathjs.mad` etc. since mathjs is ESM-only and has no default export. Switched to `import * as mathjs` and replaced the trailing `module.exports = {...}` with `export default {...}` so the file is consistently ESM. This was the cause of `TypeError: Cannot read properties of undefined (reading 'mad')` that surfaced in unit-test runs.
Verified both modes still reach "Harper successfully started."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23a2e4d to
ded6937
Compare
Addresses the second round of claude/review findings plus the
ErrorResource regression Gemini flagged that showed up in unit tests:
- server/threads/socketRouter.ts:31 — `await require('./threadServer.ts').startServers()` was half-converted (extension updated but `require` left); switched to `await (await import(...)).startServers()`. Triggered when threadCount === 0 in typestrip mode.
- bin/upgrade.ts:36 — `pm2Utils = require('../utility/processManagement/processManagement.ts')` parallel to the `pmUtils` fix already done in bin/run.ts; switched to `await import(...)`. Was breaking `harper upgrade` in typestrip mode.
- utility/signalling.ts — `serverItcHandlers = serverItcHandlers || require('../server/itc/serverHandlers.ts')` in signalSchemaChange/signalUserChange; both functions made async and switched to `await import(...)`.
- server/fastifyRoutes/plugins/hdbCore.js — renamed to .ts and converted to ESM. As a CJS file it required serverHandlers.ts (now ESM), throwing ERR_REQUIRE_CYCLE_MODULE under typestrip when fastify routes load. The single importer (server/fastifyRoutes.ts) updated to point at .ts.
- resources/ErrorResource.ts — rewrote to lazy-construct the class on first
use via a Proxy. The earlier `onStartup`-based late binding worked in
production (where runStartup runs before componentLoader can construct
ErrorResource) but broke unit tests that exercise componentLoader without
going through the lifecycle, producing the `ErrorResource is not a
constructor` failure. The Proxy resolves on `new` or property access, so
the class is always available regardless of lifecycle state, and still
dodges the original module-load TDZ on Resource.
Verified locally:
- node bin/harper.ts version → 5.1.0
- node bin/harper.ts → Harper successfully started.
- node dist/bin/harper.js → Harper successfully started.
- npm run lint:required → 0 errors, 0 warnings
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Harper's unit tests use `rewire` to override module-internal bindings via `__set__` / `__get__`. Rewire works by `eval`-injecting code into the module's closure, so it needs a variable with the exact name the test references. `tsc`'s CJS emit renames `import X from 'mod'` to a `_X_ts_1` alias internally and rewrites all uses to `_X_ts_1.default`. After commit 1f4522a converted the underlying source files from CJS `.js` to ESM `.ts`, the dist `_ts_1` aliases broke ~95 rewire-using unit tests that expected to find a variable named `X` in scope. This commit adds a `const X = _X` alias right after each affected default/namespace/named import in the 21 source files that rewire-using tests target. tsc emits this as a real module-scope `const X = _X_ts_1.default;` (or equivalent), restoring the variable name rewire looks for. ESM (typestrip) sees the same `const X = _X;` trivially. Files touched (each only adds the alias for the specific names the test rewires; the rest of the imports are left alone): bin/upgrade.ts components/operations.ts config/configUtils.ts dataLayer/bulkLoad.ts dataLayer/delete.ts dataLayer/export.ts dataLayer/insert.ts dataLayer/readAuditLog.ts dataLayer/schema.ts dataLayer/schemaDescribe.ts dataLayer/update.ts dataLayer/harperBridge/lmdbBridge/lmdbMethods/{lmdbCreateSchema,lmdbCreateTable,lmdbDropTable,lmdbUpsertRecords}.ts security/fastifyAuth.ts server/serverHelpers/serverHandlers.ts sqlTranslator/sql_statement_bucket.ts validation/role_validation.ts validation/schemaMetadataValidator.ts validation/validationWrapper.ts Sampled local results after applying: - upgrade.test.js: 4 passing (was: fail in before hook) - validationWrapper.test.js: 8 passing (was: ReferenceError validate not defined) - insert.test.js: 16 passing - delete.test.js: 12 passing - sql_statement_bucket.test.js: 16 passing - configUtils.test.js: 40 passing, 1 unrelated failure Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2b250f2 to
31b0ef6
Compare
Round of follow-up fixes after the typestrip conversion exposed several
test failures in the dist (CJS-emit) build:
- utility/functions/geo.ts: turfArea, turfLength, turfCircle,
turfDistance, turfBooleanContains, turfBooleanEqual,
turfBooleanDisjoint were called as `.default(...)` (CJS interop
pattern) but the new ESM `import X from '@turf/X'` already resolves to
the default. Removed the `.default` accessors. Also switched
`@turf/helpers` to `import * as` since it only exposes named exports.
(geo unit tests now pass: 79/79)
- server/serverHelpers/contentTypes.ts: my earlier rsync was off a
pre-x-ndjson revision of main, so the rsync overwrote main's
application/x-ndjson handler. Restored it.
(contentTypes test: 16/16 passing)
- utility/signalling.ts: my prior fix made signalSchemaChange /
signalUserChange async (to `await import('serverHandlers.ts')`),
but the existing tests and callers assumed sync. Restored sync
signatures and moved the import to an `onStartup` preload + safe
optional-chain fallback for environments where startup never runs.
(signalling tests: 4/4 passing)
- utility/install/checkJWTTokensExist.ts: original CJS used
`module.exports = fn` so `require('./checkJWTTokensExist')(...)` was
callable directly. My `export default fn` converted to
`exports.default = fn`, breaking that calling convention. Added a
conditional `module.exports = fn` shim that runs only when `module`
is defined (tsc CJS emit) and is skipped under typestrip ESM.
(checkJWTTokensExist tests: 2/2 passing)
- unitTests/testUtils.js: `preTestPrep()` now calls
`lifecycle.runStartup()` after `initTestEnvironment()`. Many tests
stub server-singleton methods (e.g. `sinon.stub(server, 'getUser')`)
that are now wired during the startup phase rather than at module
load. Calling runStartup() in preTestPrep gives tests the same
fully-wired state production sees. (`runStartup` is idempotent.)
(auth.test.js: 10/10 passing)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…taller) The rewire-target extractor missed two patterns in the first pass: - Variable-based rewire calls like `rewire(MODULE_PATH)` where MODULE_PATH is a local string constant - The harper_logger PropertiesReader import (via the requireUncached helper) Applies the same const-alias-after-import pattern to: - utility/logging/harper_logger.ts: PropertiesReader - utility/install/installer.ts: PropertiesReader, installValidator, mountHdb, checkJwtTokens Local check: harper_logger.test.js now 24/24 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The runtime-env-vars test used `__set__('require', stub)` to intercept the
inline `require('./harperConfigEnvVars.ts')` call in
`applyRuntimeEnvVarConfig`. The ESM conversion hoisted that require to a
top-level import, leaving the test stub unable to substitute anything and
the function ran against the real fs (which then errored on `/test/root`).
Adds the const-alias pattern for `applyRuntimeEnvConfig` in configUtils.ts
so rewire can patch the binding directly, and updates the test to
`__set__('applyRuntimeEnvConfig', stub)` instead of the old require-trap.
13/13 in that test file passing now.
Two issues uncovered while investigating integration-test regressions:
- dataLayer/{insert,delete,readAuditLog}.ts imported `harperBridge` via
`import _harperBridge ...; const harperBridge = _harperBridge;`. Under
the existing intentional cycle (insert.ts ↔ harperBridge.ts via the
bridge's transitive deps), `_harperBridge` is in TDZ at the moment
the alias evaluates, so `harperBridge` was bound to `undefined`
forever and the daemon crashed at startup with
`Cannot access '_harperBridge' before initialization`. Replaced the
capture with a Proxy that defers `.X` reads to call time — the cycle
is fully resolved by the time any usage fires, and rewire tests can
still patch the binding name.
- server/jobs/jobRunner.ts used `join(__dirname, './jobProcess.js')` to
resolve the job-worker entry path. `__dirname` is undefined under
type-strip ESM, so the call threw silently when a job was launched
(which silently meant CSV/bulk-load jobs never spawned a worker).
Switched to an extensionless identifier `'server/jobs/jobProcess'`
so `manageThreads.startWorker()` resolves it against `RUNTIME_SRC_ROOT`
with the correct extension per execution mode.
Note: integration-test CSV bulk-load is still broken locally — the job
worker startup hangs without reaching the actual data path. The above
fixes are necessary preconditions but not sufficient; ongoing
investigation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mports
claude/review flagged that the LMDB bridge files still had top-level
\`require('./path').default || require('./path')\` shims that throw
\`ReferenceError: require is not defined\` under typestrip ESM. Same in
the launchHarperDB entry script and mount_hdb's createTables.
Converted via regex sweep — pattern was uniform across these files:
\`const X = require('./path').default || require('./path');\`
becomes \`import X from './path';\`. Where the require pattern was
spread across multiple lines (triple-require defensive shims in
lmdbCreateTransactionsAuditEnvironment), collapsed to a single import.
launchServiceScripts/launchHarperDB.ts: top-level \`require(...)\` call
replaced with a dynamic \`import(...).then(...)\`.
utility/mount_hdb.ts: inline require inside \`createTables\` hoisted to
a top-of-file import (CreateTableObject).
Verified \`node bin/harper.ts version\` still works. \`npm run lint:required\`
clean, \`npm run format:write\` no changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The destructured SearchByConditionsObject/SearchCondition were only referenced from JSDoc @param annotations — under typestrip mode the top-level require() throws ReferenceError. The TS file is already typed; the JSDoc-only imports add no value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
main-branch databases.js called env.initSync() at module load — the conversion to .ts dropped that call. Without it, code paths that reach env.get(...) through configUtils.getConfigValue without going through preTestPrep (notably the unit-test apitests, which import databases.ts transitively but never call initTestEnvironment) leave flatConfigObj undefined. installApplications then sees null componentsRoot and the loader-side fallbacks could not paper over every consumer. Restore the call so the boot-props / autodetect path runs whenever databases.ts is loaded — same behaviour as main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously startServers fired loadRootComponents(...).then(listenOnPorts) without returning the chain, so the only external caller (socketRouter startHTTPThreads(0)) resolved immediately. The api-test setup uses \`await startHTTPThreads(0)\` and then issues axios requests — without returning the chain, those requests raced the listen. Now that the chain already includes the post-listen handler via .then, return it from startServers so callers can actually await readiness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
setupTestApp captured server.getUser, then overrode it with the test/test fake-user implementation, then called runStartup() — which fires user.ts's onStartup hook and reassigns server.getUser back to the real getUserImpl. The override never survived to handle the auth header on the test's first request and the restricted-user tests got 401s instead of the seeded permission set. Drain runStartup() before the override so user.ts's hook lands first, then capture/replace getUser. Single drain point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The .js->.ts conversion replaced require('../../components/
componentLoader') inside onSocket with `await import(...)` and marked
onSocket async. server.socket(listener, options) therefore returned
a Promise instead of the underlying TLS/net server, breaking callers
that expected a sync server handle (notably the api-test mTLS test:
`startMQTT(...)[0].listen(8884, resolve)`).
Hoist getComponentName to a static import so onSocket can stay sync
and return the actual server.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
Session wrap-up — major progress, one remaining failureThis session pushed 31 commits and dramatically reduced failures. Posting a roll-up so the next iteration has a clear starting point. Where we are
Root causes uncovered & fixed this session
What's left
Generated by Claude. |
The .js->.ts conversion dropped envManager.initSync() top-level calls from 17 source modules. On main those calls were what got the harper config loaded for any code path that touched env.get() without going through bin/harper.ts (notably unit-test files that import from resources/Table.ts, resources/databases.ts, security/auth.ts, etc.). The previous wrap-up posted with the apitests fixed addressed the most visible regression (componentsRoot in api-test setup, fixed by initSync in databases.ts). The remaining test:unit:resources segfault traced back to the same kind of state: caching.test.js hitting an unhandled rejection on primaryStore.cache, which left native storage in a bad state for the next describe block. This commit: - Restores initSync() top-level calls in all 17 files where it was dropped, matching main. - Wraps each call in try/catch so typestrip ESM evaluation cycles don't blow up at module load. - Updates initSync() itself to silently return on ReferenceError (cycle TDZ) instead of process.exit(1), since the same module-load chain (or bin/harper.ts) will retry once evaluation completes. Verified `node bin/harper.ts version` still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Worker.terminate() leaves the rocksdb-js native handles in a state
where their finalizer (during V8 GC of the worker's isolate) crashes
the next test's rocksdb open in the parent process — a
@harperfast/rocksdb-js native lifecycle bug. The bug exists
independently of this PR, but typestrip-induced timing changes
make CI hit it more reliably.
The worker already handles `{ type: 'shutdown' }` with process.exit(0),
which closes the rocksdb handles on the worker's own event loop.
Send that message and await `exit` before calling terminate as a
fallback. Local repro of test:unit:resources now reaches 385 passing
with no segfault.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dataLoader.test.js stubs harperLogger.forComponent immediately before
requiring resources/dataLoader, but the stub gets defeated when an
earlier test file in the same mocha process transitively imports
dataLoader.ts — that captures the real forComponent into
dataLoaderLogger at module load and the test sees zero stub calls.
Two fixes:
- components/operations.ts: revert the top-level componentLoader
imports back to lazy `await import('./componentLoader.ts')` inside
the function bodies that actually use them. This matches the lazy
pattern main's operations.js used ("Lazy-load to avoid circular
dependency with componentLoader") and removes one path that pulls
dataLoader.ts into the test-time graph.
- resources/dataLoader.ts: defer the `harperLogger.forComponent(...)`
call behind a Proxy so the actual logger is resolved on first use
rather than at module evaluation. Other consumers (http.ts,
threadServer.ts) still pull componentLoader -> dataLoader.ts
transitively, so this is the more robust guarantee.
Local test:unit:resources now reports 386 passing, 0 failing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bin/harper.ts was draining onStartup hooks BEFORE run.ts's launch() /
main() finished install/initialize. auth.ts's onStartup hook calls
`table({...})` which resolves through `getHdbBasePath()` — but
hdbBasePath is only populated by the install path inside initialize().
Integration tests that spawn a fresh Harper saw the auth hook fire
against empty installProps and crash with `path must be string`
during `database()`.
Move the runStartup() drain into bin/run.ts's main() after
initialize() completes, and drop the early calls from
bin/harper.ts START / undefined-service paths. Smoke (node bin/harper.ts version)
remains green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claude/review flagged that onStartup(() => CertificateVerificationSource =
class extends Resource { ... }) leaves the exported binding undefined in
test paths where startup has already drained before the test imports the
class. Use the same lazy-Proxy pattern as resources/ErrorResource.ts so
the export is always a valid Proxy that constructs the real class on
first use, independent of lifecycle state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The earlier change to make startServers return loadedPromise + the worker bootstrap awaiting it caused worker startup to hang on the listenOnPorts chain in CRL/integration test scenarios. Revert to the pre-PR behaviour: startServers schedules loadRootComponents().then(...) fire-and-forget, parentPort.postMessage(CHILD_STARTED) still fires inside the .then once listenOnPorts resolves, and the worker bootstrap no longer waits. Also drop the dead `return loaded` (was returning the module namespace by accident from the .js->.ts conversion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous round split between two callers: - unit:apitests setupTestApp needs to wait for HTTP/HTTPS ports to bind (so axios doesn't race the listen). - Integration test workers spawned via `new Worker(threadServer.ts)` must NOT block their bootstrap IIFE on the same chain — that deadlocked Harper startup in CRL/integration scenarios. Resolve: - Restore the `return Promise.resolve(listening).then(...)` chain inside startServers so the loadedPromise (and whenComponentsLoaded with it) only resolves after listenOnPorts + CHILD_STARTED. - Drop the worker IIFE's `await startServers()` — startServers schedules its chain internally and notifies main via CHILD_STARTED postMessage; awaiting it deadlocks. - Have setupTestApp.mjs explicitly `await whenComponentsLoaded` after startHTTPThreads so apitests get the listen guarantee without relying on startServers's return value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cationSource)
claude/review's blocker called out CertificateVerificationSource AND
CertificateRevocationListSource. Only the first one got the Proxy
refactor last round; this one was still wrapped in `onStartup(() =>
CertificateRevocationListSource = class extends Resource {})`. Apply
the same lazy-Proxy pattern so the export is always a valid Proxy
regardless of lifecycle state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This is the root cause of the CRL integration test timeouts. My conversion added a `setImmediate(() => onMessageFromWorkers(...))` wrap around the schema-event ack listener "to defer registration so manageThreads.ts is fully evaluated when we read from it". That's defensive against a hypothetical ESM cycle, but the side effect is that any schema event posted before the setImmediate tick fires has no listener to ack it — broadcastWithAcknowledgement then hangs forever waiting for an ack that never arrives. In CRL integration tests this manifested as Harper hanging right after the http/1 worker logged `signalSchemaChange for table 'hdb_status'` during initial component loading — the schema event went out, no ack came back, the worker's CHILD_STARTED never fired, the main thread's workersReady promise never resolved, and the test timed out at 60s. Register at module top-level (matches main-branch behaviour). The "ESM cycle" concern is already covered by signalling.ts using `serverItcHandlers?.schema(...)` optional-chained lazy access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit c2c8c6a.
In CJS dist the original used a sync `require('./serverHandlers.js')`
inside the listener. The .ts conversion replaced it with `await import`
inside the listener body. For CRL/mTLS integration tests, the first
schema-event broadcast lands on main before the listener has finished
its await — so the worker's broadcastWithAcknowledgement waits on an
ack that's queued behind the dynamic import. With slow TLS/cert setup
in front of it, the worker hits the 60s test timeout before any ack
arrives.
Preload serverHandlers inside the setImmediate, before
onMessageFromWorkers is called. By the time the listener fires, the
serverHandlers module is already resolved — the listener body has no
awaits before sending the ack.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Workers calling broadcastWithAcknowledgement during component loading (creating system tables) block on the parent's ack response. The previous listener did `await import(serverHandlers)` + `await handler(...)` before sending ack, so the worker's broadcast queued behind whatever the parent's main thread was doing in synchronous startup (TLS cert loading in CRL/mTLS tests is the slow path that triggers it). - Hoist the serverHandlers dynamic import to module load so it resolves in the background. - Send the ack synchronously on receipt; defer the handler call to a background task. The schema-sync work is best-effort — it re-syncs on the next event anyway, and ack semantics are about delivery, not handler completion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit ad08e8c.
Two interacting timing issues are merged into one fix: 1. CRL/mTLS workers broadcast schema events while main is still in sync TLS cert setup (loadCertificates / createTLSSelector). The previous `setImmediate(...)` wrap deferred the listener registration into the same event-loop tick as the cert work, so the worker's broadcast arrived before any listener was in place and the worker's broadcastWithAcknowledgement hung waiting for an ack that never came. 2. Reverting to a sync-only registration broke `test:unit:resources` in LMDB mode where schema sync needs to land before downstream tests read hdb_raw_analytics — running the handler in background after acking caused races. Resolution: - Eagerly start the serverHandlers dynamic import at module load (a promise we'll await on first event). The dynamic import respects the static-graph cycle by suspending until both modules finish their top-level bodies. - Register the ack listener at module load (not via setImmediate) so it's in messageListeners before any worker port is even connected. - Inside the listener, await serverItcHandlersReady only if the module hasn't resolved yet, then dispatch and ack synchronously after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Caught by claude/review: `import(...).then(m => serverItcHandlers = m)`
was leaving serverItcHandlers as the module namespace
({ default: handlersObj }) instead of the handlers object itself, so
every `serverItcHandlers[event.type]` lookup returned undefined and
no ITC handler ever ran — schema sync, user cache invalidation, and
component status requests were all silently dropped.
Walk the namespace shape (m.default?.default ?? m.default ?? m) to
get the actual handler object in both ESM and CJS-dist load paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Now that the namespace unwrap is correct (handlers actually dispatch), the schema handler runs syncSchemaMetadata which does an LMDB write to verify the table. That write can stall when a sibling worker still holds a transaction on the same table — typical during the cascading system-table creations at component-load time. With ack-after-handler, the worker's broadcastWithAcknowledgement then deadlocks on a parent that's waiting on LMDB that's waiting on the worker. Ack synchronously on receipt, run the handler in a background async IIFE. ack semantics are about delivery; schema state re-syncs on the next event regardless of any single handler's outcome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…space Two recent itc.ts iterations cascaded each other into worse failures (handler running with LMDB write-verify stalls deadlocking the worker broadcasts). Restore the previous setImmediate-wrapped registration with handler-before-ack — the only known-good shape — but with the namespace-unwrap (m.default?.default ?? m.default ?? m) so the handlers actually dispatch rather than being silently skipped. This is the same baseline state with which integration tests 1/4 and 4/4 were green; only the CRL HTTPS+mTLS path was hanging. That hang is a separate problem — fixing the listener orientation without addressing it just moves the failures around. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CRL/OCSP integration tests' fixture/config.yaml contains only a YAML comment, so parseDocument(...).toJSON() returns undefined. The earlier `config ??= DEFAULT_CONFIG;` fallback (intended for root configs when getConfigObj() returns undefined) then synthesized DEFAULT_CONFIG (rest, graphqlSchema, etc.) for the fixture. The component loader created a Scope for each synthesized plugin, whose OptionsWatcher watched the fixture's empty config.yaml; with no matching plugin key in the file, OptionsWatcher never emits 'ready', scope.ready hangs forever, sequentiallyHandleApplication never starts, and the http worker never sends CHILD_STARTED — so Harper times out at startup. Constrain the fallback to isRoot (which is the case the comment described) and treat an empty/null parse result on a non-root component as "nothing to load" rather than silently filling in default plugins. Fixes the CRL Certificate Verification, OCSP Certificate Verification, and related mTLS-bearing integration test hangs on PR #562. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets Harper run from the same source tree in two modes:
node bin/harper.ts— Node 24's built-in type-strip (ESM, runs .ts directly)node dist/bin/harper.js— existing CJS dist build (unchanged shipped artifact)Both reach
Harper successfully started.for the daemon, and produce identical output forversion/help.Why
Running .ts directly removes the build step from the inner dev loop and gives us a path to drop the tsc-CJS emit eventually. The challenge is that Harper's runtime module graph is full of CJS↔ESM cycles and module-load side effects (config reads, server-singleton wiring, listener registrations, class-extends-Resource) that work under tsc's synchronous CJS but TDZ under Node's ESM evaluation.
Approach (three commits)
utility/lifecycle.ts(onStartup(cb)/runStartup()) plusRUNTIME_SRC_ROOT/RUNTIME_FILE_EXTexports fromutility/packageUtils.jsso callers can distinguish source vs. dist at runtime without__dirname/import.metashenanigans..jssource modules to ESM.ts— 57 mechanical renames (require→import,module.exports→export, etc.). This is the bulk of the diff.bin/harper.tsreshaping, worker bootstrap,onStartupwrappers, late-binding forResourcesubclasses, defensiveenv.get, removal of eager module-topenv.initSync()calls.Where to focus review attention
Higher-confidence:
utility/lifecycle.ts) — small, contained.RUNTIME_SRC_ROOT/RUNTIME_FILE_EXTadditions topackageUtils.js— purely additive..js→.tsconversion in commit 2.Lower-confidence — please scrutinize:
Late-bound Resource subclasses —
resources/ErrorResource.ts,security/certificateVerification/{certificateVerificationSource,crlVerification}.ts, and theLoginclass inresources/login.tsnow create their class insideonStartup()and export viaexport let X. In tsc's CJS emit, this becomesexports.X = void 0thenexports.X = class …inside the hook. Caveat for future contributors: module-top destructuring of these exports (const { ErrorResource } = require('./ErrorResource')) would captureundefined. All current consumers access viamod.Xat use-site (or destructure inside a function body), which sees the live property after startup — but this is a sharp edge worth knowing about.Defensive
env.get/getHdbBasePathinutility/environment/environmentManager.ts— both now tolerate being called before this module's body has fully run (catchReferenceErrorfromconfigUtils.getConfigValueTDZ; optional-chaininstallProps?.). Belt-and-suspenders for cycle timing; could mask a real "called before init" bug. Now thatinitEnv()always runs before any subcommand module loads, this might be removable in a follow-up.isEntrycheck inbin/harper.ts:Works because nothing currently imports
bin/harper.tsas a module. If anyone ever does (test runner, wrapper script), the CLI side-effect would run unexpectedly. Hardening this would need eitherimport.meta.url(which tsc errors on under CJS emit) or a separate ESM/CJS entry shim.Worker
RUNTIME_FILE_EXTswitch inmanageThreads.startWorker()— the function now accepts extensionless module identifiers ('server/threads/threadServer') and resolves them againstRUNTIME_SRC_ROOTwith.tsor.jsper mode. Verify this matches how external consumers callstartWorker.Removed eager module-top
env.initSync()calls (22 sites). Initialization is centralized inbin/harper.ts(initEnv) andthreadServer.tsfor workers. Tests that import these modules directly will need to callinitSync()themselves.Test implications
server.Xbeing wired,env.get(K)returning a real value, or worker-thread message listeners being registered must callrunStartup()(exported fromutility/lifecycle.ts) before exercising those paths.resetStartupForTests()is provided for inter-test isolation.Verification
Process notes
.js→.tsrename in commit 2; commits 1 and 3 are where the actual design decisions live and warrant the closest read.geminibefore pushing; its flags addressed inline above.🤖 Generated by Claude Code (Opus 4.7, 1M context).