Skip to content

Commit bfe13c8

Browse files
Steve Jobsclaude
andauthored
fix(types,cli): resolve host-declared packages through the import condition, and read the cluster registry instead of assuming it (#14042)
* wip: #13330 esm-condition declared leg + registry reading * test(types): pin the ESM-condition declared leg (#13330) * chore: changeset for #13330 * docs(pr): qualify the narrowness claim at the evaluation level; state the silent older-service-cluster case plainly (#14042 review) Contract-review corrections for #14042 — no code, test, or behaviour change: - the narrowness guarantee holds at the RESOLUTION level, not at EVALUATION: a dual-published package whose import build exists but throws while its require build works used to silently load the CJS build and now surfaces the break (measured: base LOADED build=cjs, head THREW). Qualified in the node.ts docblock and the changeset; the PR body carries the same clause. - the older-@objectstack/service-cluster case prints nothing; the changeset said it 'reports as unmeasured', which overstated. It now says the case is silent. The disclosed serve-diagnosis test gap is filed as #14054. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UngCYXF98BVpYA9hfz6NYk --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3795c5f commit bfe13c8

7 files changed

Lines changed: 727 additions & 12 deletions

File tree

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
"@objectstack/types": patch
3+
"@objectstack/service-cluster": minor
4+
"@objectstack/cli": patch
5+
---
6+
7+
fix(types,cli): resolve host-declared packages through the `import` condition, and read the cluster registry instead of assuming it (#13330)
8+
9+
`createHostImporter`'s declared leg resolved with `hostRequire.resolve(pkg)` — a
10+
**CommonJS** resolution, which answers the `require` condition. Every `tsup`
11+
dual build publishes `{ "import": "./dist/index.js", "require": "./dist/index.cjs" }`,
12+
so a package loaded through that leg evaluated as its **CommonJS** build while
13+
the callers (`packages/cli` is `"type": "module"`) held the **ESM** build of the
14+
same package. The process ended up with two instances of everything the loaded
15+
package shares with its caller, each with its own module-scope state.
16+
17+
Measured consequence, on the shipped EE multi-node path (ADR-0018): `os serve`
18+
loaded `@objectstack/service-cluster-redis` through this leg, the driver's
19+
load-time `registerClusterDriver('redis', …)` ran against the CommonJS copy of
20+
`@objectstack/service-cluster`, and the ESM `Runtime` read the ESM copy and
21+
found nothing — `OS_CLUSTER_DRIVER=redis` died at `defineCluster()` with
22+
`Cluster driver "redis" is not registered`, about a package that was installed,
23+
declared and resolvable. Any module-scope registry crossing this seam had the
24+
same defect; the cluster driver is the instance that shipped.
25+
26+
**The seam.** The declared leg now imports the entry the `import` condition
27+
names. The host anchor is untouched — the CJS resolver still answers *where*
28+
the package is, because no flagless Node API resolves a bare specifier against
29+
an arbitrary parent; only the *condition* is re-decided, by reading that
30+
package's own `exports` map. Deliberately narrow at the **resolution** level —
31+
no load that works today resolves differently unless the package itself
32+
publishes a valid, existing import-condition target: a package with no
33+
`exports` map is untouched (CJS resolution already returned `main`), a package
34+
publishing no import-condition target is untouched, and anything unreadable or
35+
absent on disk falls back to the CJS-resolved path. That narrowness does not
36+
extend to **evaluation**: a dual-published package whose `import` build exists
37+
but throws while its `require` build works used to mask that break by silently
38+
loading the CJS build, and now surfaces it — arguably the correct reading of a
39+
broken published build, but a behaviour change, not a no-op.
40+
41+
**The reading.** A residual split is still possible above the seam — two
42+
*physical* copies of one package are two instances in any module system, and no
43+
resolver condition merges them — so `os serve` no longer assumes the driver
44+
registered. `@objectstack/service-cluster` exports `listClusterDrivers()`, the
45+
registry `defineCluster()` itself consults, and `serve` queries it after the
46+
load. The silent `catch` is gone: a driver that loaded but stayed invisible, one
47+
that could not be resolved, and one that resolved and then crashed now read as
48+
three different diagnoses instead of arriving as `not registered` one line
49+
later. An app on an older `@objectstack/service-cluster` has no accessor to
50+
call; that case is silent — `serve` declines to claim either answer rather
51+
than printing one.
52+
53+
No behaviour downstream of the diagnosis changed: an absent driver still reaches
54+
`defineCluster()`'s documented error (`cluster.mdx` §8.1) rather than silently
55+
downgrading to the in-memory cluster, and the only documented downgrade here —
56+
a multi-node gate denial — is untouched.

packages/cli/src/commands/serve.ts

Lines changed: 103 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2406,7 +2406,11 @@ export default class Serve extends Command {
24062406
// The remote driver self-registers on import; import it dynamically so it
24072407
// works in BOTH config-boot and compiled-artifact mode. Open-core ships
24082408
// only the in-memory driver — remote drivers (e.g. redis) come from the EE
2409-
// distribution; if absent we fall back to the in-memory cluster.
2409+
// distribution. An absent driver does NOT fall back to the in-memory
2410+
// cluster: `clusterConfig` still names it and `defineCluster()` raises
2411+
// its documented error (cluster.mdx §8.1). The only documented downgrade
2412+
// here is a multi-node GATE DENIAL, below. (#13330 — this sentence said
2413+
// the opposite for as long as the silent catch below agreed with it.)
24102414
let clusterConfig: { driver: string; url?: string } | undefined;
24112415
// The gate's verdict, held for the operator-facing telemetry emitted near
24122416
// the end of boot (#12667). The gate is consulted exactly once per
@@ -2432,9 +2436,15 @@ export default class Serve extends Command {
24322436
// '@objectstack/service-cluster'` and took the whole boot down — while
24332437
// app-side code loaded the very same package fine.
24342438
const __clusterPkg: string = '@objectstack/service-cluster';
2435-
const { checkMultiNodeAllowed } = (await importFromHost(__clusterPkg)) as {
2439+
// The whole namespace, not just the gate: the DRIVER REGISTRY read
2440+
// further down has to come from this same module instance, because
2441+
// that is the instance `defineCluster()` consults (#13330).
2442+
const __clusterModule = (await importFromHost(__clusterPkg)) as {
24362443
checkMultiNodeAllowed: (requested?: number) => MultiNodeGateVerdict;
2444+
/** Optional: an app on a pre-#13330 `service-cluster` does not have it. */
2445+
listClusterDrivers?: () => string[];
24372446
};
2447+
const { checkMultiNodeAllowed } = __clusterModule;
24382448
// Ask the gate about the topology the operator actually DECLARED.
24392449
// Calling zero-arg leaves `requested` undefined, which a cap-aware gate
24402450
// has nothing to clamp against — so the licensed-overflow verdict was
@@ -2467,12 +2477,97 @@ export default class Serve extends Command {
24672477
const __capAdvisory = formatMultiNodeCapAdvisory(__gate);
24682478
if (__capAdvisory) console.warn(__capAdvisory);
24692479
// Same host-anchored resolution as the gate above — the shipped
2470-
// drivers (`-redis`, `-postgres`, …) are app-declared too. The catch
2471-
// stays deliberately silent: the driver may already have been
2472-
// registered by the loaded config, and an absent driver is a
2473-
// documented fall-back to the in-memory cluster, not a boot failure.
2474-
try { await importFromHost(`@objectstack/service-cluster-${__clusterDriver}`); }
2475-
catch { /* may already be registered by the loaded config */ }
2480+
// drivers (`-redis`, `-postgres`, …) are app-declared too.
2481+
//
2482+
// ── Why this is no longer a silent catch (#13330) ────────────────
2483+
//
2484+
// A driver package's entire contract is a load-time SIDE EFFECT:
2485+
// `registerClusterDriver('<driver>', …)` into the module-scope
2486+
// registry of `@objectstack/service-cluster`, which `defineCluster()`
2487+
// reads two statements below. Whether that side effect landed is a
2488+
// fact about THIS process, so it is read here rather than assumed.
2489+
//
2490+
// It used to be assumed. The catch was silent on two stated grounds —
2491+
// "may already be registered by the loaded config" and "an absent
2492+
// driver is a documented fall-back to the in-memory cluster" — and a
2493+
// single EE boot measured both wrong at once:
2494+
//
2495+
// • the load SUCCEEDED and the registration was invisible. The
2496+
// declared leg of `importFromHost` resolved with CommonJS
2497+
// semantics, so the driver ran as its `.cjs` build and registered
2498+
// into a SECOND instance of the registry, while the ESM Runtime
2499+
// read the first. Fixed at the seam (`@objectstack/types/node`);
2500+
// this reading is what makes any residual split audible instead
2501+
// of arriving as "not registered" one line later.
2502+
// • an absent driver falls back to nothing HERE — `clusterConfig`
2503+
// below names the driver either way, so `defineCluster()` raises
2504+
// its documented error (cluster.mdx §8.1). That is left exactly
2505+
// as it is: downgrading to in-memory instead would boot a silent
2506+
// single node for an operator who explicitly asked for a remote
2507+
// driver, and on the multi-replica deployments this matters for,
2508+
// the ADR-0010 split-brain guard throws on that downgrade anyway.
2509+
// What changes is only that the reason is no longer swallowed.
2510+
//
2511+
// Nothing below throws: every branch is a diagnosis printed ahead of
2512+
// behaviour that is unchanged.
2513+
let __driverLoadError: unknown;
2514+
try {
2515+
await importFromHost(`@objectstack/service-cluster-${__clusterDriver}`);
2516+
} catch (err) {
2517+
__driverLoadError = err;
2518+
}
2519+
// `undefined` ⇒ the app's `@objectstack/service-cluster` predates
2520+
// `listClusterDrivers`, so the registry cannot be read from here.
2521+
// That is NOT MEASURED — it is not "registered" and not "missing",
2522+
// and no branch below claims either.
2523+
const __registeredDrivers =
2524+
typeof __clusterModule.listClusterDrivers === 'function'
2525+
? __clusterModule.listClusterDrivers()
2526+
: undefined;
2527+
const __driverVisible =
2528+
__registeredDrivers === undefined
2529+
? undefined
2530+
: __registeredDrivers.indexOf(__clusterDriver) >= 0;
2531+
if (__driverVisible !== true) {
2532+
if (__driverLoadError !== undefined) {
2533+
// Resolution failures carry a kind and are already worded for an
2534+
// operator by `createHostImporter`; anything else RESOLVED and
2535+
// then crashed while evaluating. Swallowing the second is how a
2536+
// driver with a broken dependency reported as "not registered",
2537+
// sending operators to look for a package already installed.
2538+
const __kind = hostImportFailureKind(__driverLoadError);
2539+
if (__kind !== undefined) {
2540+
console.warn(
2541+
`[cluster] driver "${__clusterDriver}" was requested but could not be ` +
2542+
`loaded (${__kind}):\n${
2543+
__driverLoadError instanceof Error
2544+
? __driverLoadError.message
2545+
: String(__driverLoadError)
2546+
}`,
2547+
);
2548+
} else {
2549+
console.warn(
2550+
`[cluster] driver "${__clusterDriver}" resolved but threw while loading — ` +
2551+
`this is the driver package's own failure, not a missing package:`,
2552+
__driverLoadError,
2553+
);
2554+
}
2555+
} else if (__driverVisible === false) {
2556+
// Loaded cleanly and still not in the registry: two live
2557+
// instances of `@objectstack/service-cluster` in one process,
2558+
// which is a PHYSICAL-copy split no resolver condition can merge.
2559+
console.warn(
2560+
`[cluster] driver "${__clusterDriver}" loaded but did not register: ` +
2561+
`@objectstack/service-cluster-${__clusterDriver} evaluated without error, yet the ` +
2562+
`registry this boot reads holds [${__registeredDrivers?.join(', ') || 'nothing'}]. ` +
2563+
`Two instances of @objectstack/service-cluster are live in this process and the ` +
2564+
`driver registered into the other one — look for two physical copies (a version ` +
2565+
`skew between the app and the framework, or a bundled one). Importing ` +
2566+
`"@objectstack/service-cluster-${__clusterDriver}" from objectstack.config.ts ` +
2567+
`registers into the instance the Runtime reads.`,
2568+
);
2569+
}
2570+
}
24762571
clusterConfig = { driver: __clusterDriver, url: process.env.OS_REDIS_URL };
24772572
}
24782573
}
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #13330 — the driver registry is READABLE, and what it reads is what
5+
* `defineCluster()` consults.
6+
*
7+
* A driver package's whole contract is a load-time side effect into the
8+
* module-scope `driverRegistry` here. Until now a booting process could only
9+
* discover whether that side effect had landed by calling `defineCluster()`
10+
* and catching the throw — which constructs a real cluster on success, so it
11+
* is not a probe anyone can run first. `os serve` therefore ASSUMED the
12+
* registration, in a silent `catch`, and a shipped EE boot proved the
13+
* assumption wrong: the driver had loaded into a second, CommonJS instance of
14+
* this module, and the ESM Runtime read this one and found nothing.
15+
*
16+
* The accessor exists so that boot can read instead of assume. Its whole value
17+
* rests on agreeing with `defineCluster()` — an accessor that could drift from
18+
* the lookup it reports on would make `serve`'s diagnosis a phantom check —
19+
* so the agreement is pinned here in both directions, not just the shape of
20+
* the list.
21+
*/
22+
23+
import { describe, it, expect } from 'vitest';
24+
import type { IClusterService } from '@objectstack/spec/contracts';
25+
import { defineCluster, listClusterDrivers, registerClusterDriver } from './cluster.js';
26+
27+
/** A factory whose product is identifiable without connecting to anything. */
28+
const marker = { driver: 'fixture-marker' } as unknown as IClusterService;
29+
30+
describe('the driver registry can be read, not only written (#13330)', () => {
31+
it('CONTROL: the reader can return both answers, so an empty list is a reading', () => {
32+
// Nothing has registered yet in this module instance, and the reader is not
33+
// stuck on that answer — every assertion below depends on it moving.
34+
expect(listClusterDrivers()).toEqual([]);
35+
registerClusterDriver('custom', () => marker);
36+
expect(listClusterDrivers()).toEqual(['custom']);
37+
});
38+
39+
it('omits `memory`, which defineCluster special-cases rather than registers', () => {
40+
// A true reading of what the REGISTRY holds. Listing `memory` here would
41+
// make an empty registry look populated to the one caller that needs to
42+
// tell those apart.
43+
expect(listClusterDrivers()).not.toContain('memory');
44+
expect(defineCluster({ driver: 'memory' }).driver).toBe('memory');
45+
});
46+
47+
it('agrees with defineCluster — listed means resolvable', () => {
48+
expect(listClusterDrivers()).toContain('custom');
49+
expect(defineCluster({ driver: 'custom' })).toBe(marker);
50+
});
51+
52+
it('agrees with defineCluster — unlisted means the documented throw', () => {
53+
// The other direction. `postgres` is accepted by the schema and shipped by
54+
// nobody, which is exactly the "requested but not registered" case.
55+
expect(listClusterDrivers()).not.toContain('postgres');
56+
expect(() => defineCluster({ driver: 'postgres' })).toThrow(
57+
/Cluster driver "postgres" is not registered/,
58+
);
59+
});
60+
});

packages/services/service-cluster/src/cluster.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,29 @@ export function registerClusterDriver(
116116
driverRegistry.set(name, factory);
117117
}
118118

119+
/**
120+
* The driver names currently in this module instance's registry.
121+
*
122+
* Exported so a boot sequence can READ whether a driver package's load-time
123+
* `registerClusterDriver()` actually landed, instead of assuming it did.
124+
* `defineCluster()` consults this same `Map`, so an answer from here is an
125+
* answer about the call that comes next — which is the whole point (#13330:
126+
* `os serve` loaded a driver that registered into a SECOND, CommonJS instance
127+
* of this module and then failed one line later in `defineCluster` with
128+
* "not registered", with nothing between the two to say so).
129+
*
130+
* `memory` is deliberately absent: it is not registered, it is special-cased
131+
* inside `defineCluster`. This lists what the REGISTRY holds, so an empty array
132+
* is a true and useful reading rather than a misleading one.
133+
*
134+
* A list rather than a `has()` predicate because the caller that needs the
135+
* boolean also needs to print what WAS there when the answer is no — one call,
136+
* both readings, no way for the two to drift.
137+
*/
138+
export function listClusterDrivers(): string[] {
139+
return [...driverRegistry.keys()];
140+
}
141+
119142
// ---------------------------------------------------------------------------
120143
// Helpers
121144
// ---------------------------------------------------------------------------

packages/services/service-cluster/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
export {
2525
defineCluster,
2626
registerClusterDriver,
27+
listClusterDrivers,
2728
ComposedClusterService,
2829
type ClusterDriverFactory,
2930
type DriverFactoryConfig,

0 commit comments

Comments
 (0)