Relationship property access now always resolves synchronously

[kriszyp]

Restores the v4 synchronous contract for schema-defined relationship accessors. The array-of-foreign-keys resolver branch called `primaryStore.get()`, which on RocksDB opts into the async read path: a value on a block-cache hit, a Promise on a miss — an intermittent value-or-Promise leaking into user code touching `record.`. It now uses `getSync()` (matching the scalar-FK branch and the repo's prior `get`→`getSync` conversions), and the dead `hasPromises`/`Promise.all` plumbing is removed. The to-many branch and the `select()` (`returnEntry`) path were already synchronous and are unchanged.

Where to look:

• `resources/Table.ts` — the resolver in `updatedAttributes`. Two review findings applied beyond the core fix: the early-return guard is now `ids == null` returning `ids` (a stored `null` FK array previously threw `TypeError` on the `filterMissing` branch; a `null` scalar FK did a pointless store read), and loop-invariant work is hoisted out of the per-id map.
• `unitTests/resources/query.test.js` — contract test that stubs `store.get` to throw, so any resolver regression to the async path fails deterministically regardless of block-cache state (passes on both storage engines).
• `resources/DESIGN.md` — documents the invariant.

Post-review addition (from Gemini's PR review, commit 24c3f35): attribute.set on an elements (array-of-FK) attribute stored a scalar id when given a single related record — which the reader misread, and which would silently misread a single composite (array) primary key as multiple scalar ids. The setter now normalizes single records to a one-element id array, and the reader coerces scalar stored ids to an array for pre-existing data.

Deliberate tradeoff: `getSync` on a block-cache miss is a blocking read on the worker thread — the same behavior v4/LMDB had via mmap page faults.

Behavioral note: a relation whose stored FK/FK-array field is `null` now resolves to `null` (previously: `undefined` from a wasted store read on the scalar branch, or a throw on the `filterMissing` array branch).

No docs PR needed: the documentation describes relationship access as direct property access and never documented the Promise-returning behavior — this restores the documented contract.

Cross-model review: standard mode (Gemini/agy + Harper-domain adjudication + final-artifact pass) — no unresolved blockers or concerns. Local suites: unit:main 3959 passing, unit:resources green, integration:all green except `ollama-backend.test.ts` (requires a local Ollama instance; environmental). Authored by Claude (Fable).

[gemini-code-assist]

Code Review

This pull request enforces a synchronous contract for relationship accessors to align with v4 parity, replacing asynchronous store.get() calls with synchronous store.getSync() or store.getEntry() and removing the obsolete promise-handling logic. It also documents this design decision and adds a corresponding unit test. Feedback on the changes highlights a potential runtime TypeError in Table.ts if a scalar ID is stored instead of an array, which can be resolved by coercing the ID to an array before mapping.

[claude]

Reviewed; no blockers found.

[kriszyp]

Addressed the composite-ID test-coverage finding on unitTests/resources/query.test.js: added a related record with an actual array primary key (['composite', 42]) and extended the test to assert both that set() produces the nested [[...]] shape and that resolve() reads it back to that single record (not two separate lookups). Also caught and fixed a small regression the change introduced: hoisting the read-transaction acquisition out of the per-id loop meant an empty array-of-FK relationship now opened a read transaction it never used — normalized the id list and short-circuited before touching the transaction, with a new test pinning the empty case. — Claude (Sonnet), for Kris

[Devin-Holland]

Reviewed at head fb50a00. The central claim is that get() on RocksDB is a value-or-Promise and the relationship resolvers must not use it; I verified that end to end in the storage layer rather than from the description, and it holds. Two non-blocking nits, both about the new empty/null shapes rather than the fix itself.

The synchronous contract is real, and both methods the fix uses satisfy it.

• PrimaryRocksDatabase.getEntry branches on the caller's own flag: const raw = options?.async ? super.get(id, getOptions) : super.getSync(id, getOptions) (resources/PrimaryRocksDatabase.ts:123). get() is the only entry point that sets it (:147, {...options, async: true}) — precisely the value-or-Promise this PR removes. getSync (:142) goes through getEntry without the flag, and so does a bare getEntry, so returnEntry ? 'getEntry' : 'getSync' is synchronous on both branches and dropping the hasPromises/Promise.all plumbing cannot strand a Promise. That plumbing also covered the getEntry path, so this was the part worth checking rather than assuming.
• The same holds on LMDB, so getSync is not a RocksDB-only method name: handleLocalTimeForGets defines store.getSync over the synchronous getEntry and reserves async: true for store.get (resources/RecordEncoder.ts:547-563).
• The new test is a real pin rather than a tautology: stubbing store.get to throw cannot be satisfied by accident, because getSync reaches super.getSync and never routes through the instance's own get. And it runs on both storage engines in CI — test:unit:all includes test:unit:resources, and test:unit:lmdb re-runs that same glob under HARPER_STORAGE_ENGINE=lmdb (package.json:72-75), which unit-test.yml:62 invokes. All three Unit Test legs (Node 22/24/26) are green.
• The to-many branch really is untouched and was already synchronous (.asArray over searchByIndex/search, Table.ts:4855-4871), and its attribute.set is a deliberate no-op — so the setter change is scoped to the relationship.from branch, as the description says.

1. nit (confirmed) — the empty-array early return hands back the record's own FK array. if (normalizedIds.length === 0) return normalizedIds; (Table.ts:4896) returns ids itself when ids is [], so record.manyToMany === record.manyToManyIds — the resolved relation and the stored FK field are the same object. freezeRecord is a shallow Object.freeze of the record (Table.ts:164-167) and never touches the nested array, so a record.manyToMany.push(related) on an empty relation now writes a record object into the FK field, where before 2a06ca3 the [].map() handed back a throwaway array. Nothing in-tree does that, and skipping the read-transaction acquisition is the whole point of the early return — but return [] keeps the saving without the aliasing.

2. nit (confirmed) — an array-typed relation now has two different empty shapes. if (ids == null) return ids (:4891) yields null for an elements attribute, while ids: [] yields [] (the case the new test pins) and the to-many branch always yields an array. So record.manyToMany is iterable when the FK array is empty and not iterable when it is null, and consumers have to handle both. This is strictly better than what it replaced — a null FK array threw a TypeError on the filterMissing branch and gave undefined otherwise — so it is not a regression; it is just that [] is available for free by moving that guard inside the branch, after which the "a relation whose stored field is null now resolves to null" note in the description applies only to scalar FKs.

3. residual risk, not a request — the reader's tolerance closes the scalar half of the legacy-data problem but not the composite half. Array.isArray(ids) ? ids : [ids] recovers a single scalar id written by the old setter, but a single composite id written by the old setter is stored as a bare array (['composite', 42]), which cannot be distinguished from two scalar ids — the exact misread the description describes. The setter fix is the right fix and this half is irreducible; worth knowing the tolerance does not reach it, so if any array-of-FK relationship targets a composite-primary-key table and was ever set with a single record, that data stays ambiguous.

Also verified: the loop-invariant hoist is behavior-preserving (txnForContext(context).getReadTxn() moves from per-id to once, and the early return keeps it from running at all for an empty relation); filterMissing ? results.filter(exists) : results matches the old non-Promise branches exactly; and the DESIGN.md invariant as written is an accurate statement of what I traced.

Verdict: approve.

— Claude & DevAIn (Claude Opus 5)

[DavidCockerill]

Approving. The core fix is real and I verified it: moving the array-of-FK resolver off the MaybePromise get() to getSync genuinely retires the Array-or-Promise duality on the foreign-key side. That duality has already caused at least one downstream bug — central-manager#487, where a spread over the result index-keyed it — so closing it is worth having.

Approving with comments rather than blocking, because the code is sound and the one substantive item is about the design record rather than behaviour: the new DESIGN.md invariant claims more than the PR enforces. The .to resolver still goes through search(...).asArray, and I can see one route (source revalidation on a caching table) where that still hands back a Promise. That bullet is exactly what someone will grep before writing record.children.filter(...), so it's worth scoping to the FK side and naming .to as a known gap.

Two smaller notes in threads: a test label that reads as a contract it doesn't pin, and a response-shape change (null scalar FK now emits "related": null) that interacts with forceNulls and isn't mentioned in the body.

— Reviewed by DAIvid (Claude Opus 5)

[kriszyp]

Addressed @DavidCockerill's three review-comment threads (pushed in 3b7a64c, 8bed1f0):

• resources/DESIGN.md:172 — scoped the invariant to the FK side and added a "Known gap" line naming .to (source-revalidation on a caching table can still hand back a Promise via transformEntryForSelect's loadingFromSource.then(transform)). Not fixing the .to path itself here — that's the separate bug you identified; noting it as a follow-up rather than expanding this PR's scope.
• unitTests/resources/query.test.js:789 — relabeled the childrenOfSelf assertion to say what it actually observes (synchronous on a local table), with a comment explaining the store.get stub doesn't pin that branch, and pointing at the DESIGN.md gap note for the caching-table case.
• resources/Table.ts:4891 (null scalar FK → null vs undefined) — this was already covered by the PR description's "Behavioral note" (added 2026-07-30, ahead of this review round). Leaving the code as-is; forceNulls's undefined/null distinction is an existing, separate mechanism this PR doesn't otherwise touch.

Also fixed one issue an advisory pre-push pass surfaced on the existing (already-approved) resolver code: the empty array-of-FK fast path returned the record's own stored FK array by reference rather than a fresh one, so record.manyToMany.push(x) on an empty relation aliased into the stored data. Fixed + pinned with a regression test (8bed1f0).

One item from that same pass I'm not fixing here, flagging for separate triage: a legacy (pre-this-PR) single composite-key record written via the old single-record setter stores the flat shape (e.g. manyToManyIds: ['composite', 42]), which is indistinguishable from two scalar FKs at read time. Resolving it needs an explicit compat decision (versioned encoding vs. documented unsupported legacy shape) rather than a mechanical fix — Kris's call.