Skip to content

census(#14423 step 1): loadMany consumers, per-loader keyed-read cost, C3/C4 re-measured - #15033

Merged
os-musk merged 4 commits into
mainfrom
claude/issue-14423-loadmany-consumer-census
Sep 3, 2026
Merged

census(#14423 step 1): loadMany consumers, per-loader keyed-read cost, C3/C4 re-measured#15033
os-musk merged 4 commits into
mainfrom
claude/issue-14423-loadmany-consumer-census

Conversation

@os-musk

@os-musk os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Part of #14423

Step 1 (census) of the maintainer ruling on #14423, comment
5528592646.
Measurement only — no shipped package behaviour changes, skip-changeset applies.
The identity fix itself (step 2) is a separate, later PR that reads this one.

The ruling this executes (verbatim)

The governance audit's third source and the router's third rung read the same
identity: the key the store holds an item under (#14205's rule), never body.name.
Concretely, the audit's metadata-plane source moves off the unkeyed
loadMany('action') onto a keyed read — listNames('action') plus by-name load /
loadDiagnosed, or loadManyKeyed where a loader offers it — so the C2 / C6
divergence (a body without name, served by load, dropped by the audit) cannot
occur. … Census first (S, measurement): every loadMany consumer in the tree, and
per shipped loader whether listNames / loadManyKeyed is available and what it
costs … C3 … and C4 … are re-measured against the chosen shape in the same census; if
keying alone does not close them, the census says which mechanism does.

Full detail, tables, and re-runnable measurement scripts:
docs/audits/2026-09-loadmany-consumer-census.md.

Answer in one line

Keying alone (listNames() + by-name load/loadDiagnosed) closes C2 and C6 but does
not close C3 or C4, confirmed by running the real production code, not by inference.
Zero of today's 4 production loadMany consumers already key by the store key, so a
loadMany return-shape change is the more invasive of the two shapes the ruling names.
Recommendation for step 2: fix the audit's own declaration-discovery strategy (probe
already-known registered-handler names directly, mirroring what the router and the
registry rung already do) rather than widen loadMany's published return shape — this is
the only shape measured here that also closes C3 and C4, and it touches none of the four
production consumers. Clause-②: no, provisionally, under that recommendation.
unboundDeclarations BEFORE count: 0, reproducibly, for the population the fix can
move.

1. Every loadMany consumer (full table in the doc)

4 production consumers of the published IMetadataService.loadMany(), across 2 files —
all 4 depend on body.name for identity, none already keys by the store key:

  • packages/objectql/src/plugin.ts:2193 (resyncAuthoredHooksNow) — keys a Map by the
    hook's own name; a nameless hook is silently excluded.
  • packages/objectql/src/plugin.ts:2586 (resyncAuthoredActionsNow) — keys by
    object-owner-plus-name; a nameless action is silently excluded.
  • packages/objectql/src/plugin.ts:2675 (loadMetadataFromService, boot sync) — falls
    back to the body's own name field whenever id is absent; a nameless, id-less item
    registers under the key undefined.
  • packages/metadata/src/plugin.ts:1274 (MetadataPlugin._loadFromFileSystem, boot
    load) — the sharpest of the four: a nameless item is never registered at all (an
    if guard on the body's own name skips the whole registration).

Plus 1 internal, package-private fallback (metadata-manager.ts:1131,
admitLoaderItems) that already prefers the loader-level keyed method and only falls
back to body.name keying for a loader that cannot produce keys — precedent, in the
same file, for the direction step 2 is choosing between.

2. Per shipped loader — keyed-read availability and measured cost

Loader listNames loadManyKeyed Cost of listNames + N-by-name-load vs plain loadMany
FilesystemLoader yes (separate glob, names only) yes (same walk as loadMany) measured ratio 0.99-1.88 across repeats of 50 items — sub-2x, local I/O noise
DatabaseLoader yes (own query + own cache) yes (shares loadMany's query and cache — zero extra cost, empirically confirmed) measured real N+1: 1 list-query + N point-queries ({"find":1,"findOne":5} for 5 items) vs loadMany's constant 1
MemoryLoader yes (Map keys) yes (Map iteration) zero, by construction — no I/O of any kind
RemoteLoader yes, but implemented as loadMany plus a map, i.e. already as expensive as loadMany no — the only shipped loader without it (wire format carries bodies only) worst of the four: 1 + N HTTP round trips, strictly worse than loadMany's 1

DatabaseLoader.loadManyKeyed() costing nothing extra over loadMany() (same shared
query and cache) is the strongest cost argument in this census, and it argues for
whichever step-2 shape can reach that method rather than for the listNames-plus-N-loads
shape the ruling names as the default candidate.

3. C3 and C4 re-measured against the keyed shape

Reused the exact doubles the pinned fixture (packages/runtime/src/action-governance-scope-divergence.test.ts,
PR #14741, unmodified this round) uses, driven against the real, built @objectstack/metadata
and @objectstack/core in a new, separate probe script — not a change to the fixture.

  • C3 (plural read throws, by-name read answers): NOT closed. MetadataManager.listNames()
    has no per-loader try/catch (unlike loadMany()/list()), so it throws under the same
    double the fixture uses; the audit's own outer catch swallows it, landing at the exact
    same net outcome as today (standalone set becomes empty either way). Swapping the read
    method relocates where the failure is swallowed; it does not change whether it is
    swallowed. The mechanism that would close it, measured in the same script: probe the
    already-known registered-handler name directly (loadDiagnosed), skipping enumeration
    entirely — this never calls the failing list path at all.
  • C4 (scope boundary): NOT closed, for an unrelated reason. The audit's own
    ctx.getService('metadata') throws before any read method — loadMany, listNames,
    or load/loadDiagnosed — is ever invoked. No choice of read method reaches this; the
    fix is a different accessor (reaching the scoped instance somehow), a materially larger
    change than the identity fix, and (per the prior measurement round) a shape no in-repo
    composition of metadata actually registers today.

4. unboundDeclarations BEFORE count

0, reproducibly (scripts/audits/14423-unbound-declarations-before-count.mjs) — a
structural fact, not a sampling accident: collectEngineActionDeclarations filters out
any standalone action whose body has no name BEFORE reconcileActionRegistrations (which
computes unboundDeclarations) ever sees it, so a nameless orphan cannot reach that
warning today under any input. A 5-nameless-plus-1-named control run reports exactly 1
(the named one), confirming the harness finds orphans in general and specifically drops
only the nameless ones. Any nameless orphan the identity fix newly admits is therefore
NET NEW to this warning's population, not a replacement.

5. Clause-② determination

No, under the recommendation in section 1 above — an audit-side fix using already
published IMetadataService members (load/loadDiagnosed) adds no exported symbol and
no new key on any published payload. Reverses to yes, per the mechanical floor, if
step 2 instead widens loadMany's published return shape or adds a new public
loadManyKeyed member to IMetadataService.

Out-of-scope finding, reported not filed

RemoteLoader.list() maps a nameless body's own name field into the returned name list,
so a nameless remote item pollutes MetadataManager.listNames()'s result with the value
undefined rather than being dropped or keyed cleanly. Unrelated to #14423 (this loader
cannot produce keys either way) and noticed incidentally while building the loader table
above. A targeted duplicate-search hit the fleet's shared rate limit twice in a row, so
per the no-blind-filing rule this is reported here for the PM to file rather than risked
as a duplicate.

Gates (25 commands derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, no path arguments, re-derived on the final commit 0d536d67e)

24 of 25 green; the 25th (node scripts/check-test-completeness.mjs) exits 3 by its own
design when run locally with no saved turbo run test log — its own text says this is
NOT MEASURED, not a finding, and that CI supplies the log it needs. node scripts/pm/check-governed-merges.mjs --test
on the final 5-file list: not governed, ordinary queue landing applies.

  • check-ci-filter-parity — OK (135 declared globs covered)
  • check-closing-keyword-parity — OK
  • check-comment-mask-corpus — OK (5837 files, 0 disagreements)
  • check-cross-package-test-inputs (direct + pnpm) — OK
  • check-self-test-wired — OK
  • check-shard-attestation — OK
  • check-test-completeness — NOT MEASURED (exit 3, no local test log; CI supplies it)
  • check-whole-set-label-write — OK
  • lint check:doc-formula-expressions — OK
  • spec check:empty-state / check:liveness / check:strictness-ledger / check:variant-docs — OK
  • check:agent-test-spelling / bash32-floor / cli-command-ids / doc-authoring / entry-guard / merge-driver / nul-bytes / parse-guard / pnpm-filter-targets / refd-timer-probe / watch-hint-literal — all OK

Full logs captured in the scratchpad during the run; happy to paste any one in full on
request.


🤖 Generated with Claude Code

https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68


Generated by Claude Code

Census-only, per maintainer ruling (issue #14423 comment 5528592646):
enumerates every loadMany consumer, measures each shipped loader's
listNames()/loadManyKeyed() availability and cost, re-measures C3/C4
from the pinned action-governance-scope-divergence fixture against a
keyed-read shape, and establishes the unboundDeclarations BEFORE count.
No shipped package behaviour changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
@os-musk os-musk added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 3, 2026 — with Claude
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation labels Sep 3, 2026
@os-musk
os-musk marked this pull request as ready for review September 3, 2026 18:27
@os-musk
os-musk enabled auto-merge September 3, 2026 18:27

os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review — PASS; ready and armed. Two nits recorded, neither blocking.

An independent reviewer (not the author) re-derived every load-bearing claim rather than confirming it. All 35 check runs on head 0d536d67e are green or skipped, including Lint & Repo Gates.

The scope fence holds completely. git diff --stat a4e4d2d78...0d536d67e = five files, 704 insertions, 0 deletions, all additions. packages/objectql/src/action-governance.ts, plugin.ts and packages/runtime/src/action-governance-scope-divergence.test.ts each have zero diff, checked per path — and the pinned fixture was re-run (1 passed / 6 tests) and still narrates itself as "MEASUREMENT, not a fix" / "pins the current divergence". No behaviour change shipped, exactly as the ruling required for step 1.

⭐ The two negatives — the reason this census matters — both reproduce

C3 is not closed by keying, and the mechanism is a missing try/catch, quoted side by side from packages/metadata/src/metadata-manager.ts: loadMany() (:2666-2684) and list() via admitLoaderItems (:1163-1172) each wrap the per-loader call, while listNames() (:1584-1588) has none at allawait loader.list(type) bare in the loop. The reviewer ran the real production code and got the same JSON the census recorded: loadMany{threw:false, standaloneLength:0}; listNames{threwInsideListNames:true, errorMessage:"connect ECONNREFUSED …", standaloneLengthAfterOuterSwallow:0}.

C4 is not closed either, for a different reason — it is upstream of any read method. packages/core/src/kernel.ts:129-139 throws Service 'metadata' is async - use await because PluginLoader.getServiceInstance (:136-137) reads only serviceInstances, never scopedServices (:118-119), while hasService (:267-268) is true for the registered SCOPED factory. Reproduced: the audit's lookup throws, getServiceAsync('metadata','env_a') resolves fine.

⇒ The ruling's own named default — move the audit to listNames + by-name — closes C2/C6 and leaves C3 and C4 standing. That is the census doing exactly what the ruling asked of it: "if keying alone does not close them, the census says which mechanism does."

Everything else re-measured, not accepted

Three of the four probes reproduced identically (the C3/C4 probe byte-for-byte; DatabaseLoader {find:1,findOne:0} for loadManyKeyed and for loadMany alike, {find:1,findOne:5} for listNames+5×loadDiagnosed; the unbound-declarations control firing at 5 nameless → 0, 1 named → 1). The consumer enumeration was re-derived independently and lands on the same 13 files and the same 4 production consumers, all body.name-dependent, at the same line numbers. The loader inventory is the same 4 shipped classes with every cited list()/loadManyKeyed() line confirmed. The unboundDeclarations BEFORE = 0 is confirmed structurally at action-governance.ts:241 and :253, both gates inside collectEngineActionDeclarations (:231-261) and strictly before reconcileActionRegistrations computes the count (:205-212) — reproduced by two independent paths. The instruments fail loudly rather than recording a false zero (proved accidentally: one threw ERR_MODULE_NOT_FOUND when run before the build).

Gates: 25 commands derived off merge base a4e4d2d78 with no path arguments — 24 exit 0, one (check-test-completeness) exit 3 NOT MEASURED by its own text. check-governed-merges --test: NOT governed. content/docs/releases/ diff empty. skip-changeset correct.

Two nits, recorded rather than fixed

1 — the FilesystemLoader timing band is wider than the document quotes, and this one must travel to step 2. The doc records 0.99–1.88 over five runs of 50 items; the reviewer's own five runs on the same contended box gave 0.55, 1.10, 1.26, 1.31, 1.66 — dipping below 1.0, i.e. the keyed path measured faster at least once. That does not undermine the qualitative reading (sub-2×, local-I/O noise, categorically unlike DatabaseLoader's exact linear N+1) — if anything the sub-1.0 sample strengthens it. ⚠️ But the quoted range must not be read as a stable bound, and a magnitude claim on this loader would need more repetitions at larger N. This caveat is carried into the step-2 decision block so the number cannot be weighed as if it were tight.

2 — a stale ref in the document header. docs/audits/2026-09-loadmany-consumer-census.md:3 says Base: origin/main 77a532d67; the real merge base used everywhere else — PR body, dispatch-gates.mjs, the reviewer's own git merge-base — is a4e4d2d78, of which 77a532d67 is an ancestor (a mid-session snapshot the header was not updated after two rebuilds). ⛔ Deliberately not sent back for a fix round: no measurement depends on it, the reviewer re-ran all four probes against the real final build, and a CI cycle for a header line is not worth the queue slot. Recorded here so the next reader of that document knows which ref its numbers belong to.

Landing. Ready, auto-merge armed (method: MERGE, enabled at 18:27:20Z). Part of #14423 — ⛔ not a closing keyword, because step 2 is still owed on that card and its shape is now a maintainer decision: the ruling's default (A) leaves C3 and C4 open, the census proposes a B the ruling never considered (probe already-known registered-handler names via the already-published optional loadDiagnosed member, packages/spec/src/contracts/metadata-service.ts:861 — closes C2/C3/C6, clause-② no, touches none of the four production consumers), and C (changing loadMany's published return shape) is clause-② yes and breaks all four. ⛔ This seat does not pick among them.


Generated by Claude Code

@os-musk
os-musk added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit 82291fb Sep 3, 2026
37 checks passed
@os-musk
os-musk deleted the claude/issue-14423-loadmany-consumer-census branch September 3, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants