Give the driver registry an eviction door, so a deleted datasource stops draining /ready - #13829
Give the driver registry an eviction door, so a deleted datasource stops draining /ready#13829claude[bot] wants to merge 8 commits into
Conversation
…n door, so a deleted datasource stops draining /ready (#13578) The ObjectQL driver registry had a `registerDriver` door and no counterpart, so nothing could ever leave it. `DELETE /api/v1/datasources/:name` emptied the admin door while `GET /api/v1/ready` kept naming the deleted datasource's driver — the probe reports whatever `checkDriversHealth()` finds in that registry — leaving a process restart on every replica as the only recovery. `IObjectQLEngine` gains `unregisterDriver(name)`. The registry owns the invariant rather than each caller, because removal moves three pieces of private engine state that a caller can reach none of: the `drivers` map, the `defaultDriver` NAME (a stale one answers with a driver that is gone), and the datasource def, which has no removal door of its own. Wired into the three lifecycle paths that already funnel through teardown: datasource delete / pool teardown, failed-start rollback, and engine destroy. Eviction is per-replica, symmetric with how registration already works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
…iver-registry-eviction
…om seven members to eight `unregisterDriver` widens the seam the datasource connection service drives the engine through, and the roster pin exists so that widening is a decision written down rather than a side effect of editing the type. Restated deliberately, with a return-type pin: the eviction door answers `boolean` so an idempotent caller can tell a removal from a no-op. Part of #13578 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
…ne.ts insertion Pure line rot: `unregisterDriver` lands above every cited elevation-read site in packages/objectql/src/engine.ts, shifting all 11 anchors by the method's length. Rewritten by the gate's own `--fix`; no census row's meaning changes. Part of #13578 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
📓 Docs Drift CheckThis PR changes 3 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 129 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin a98849848c47b27f087216d466a00a5f33edfefc && git checkout a98849848c47b27f087216d466a00a5f33edfefc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bcc9189e6e080d47428eccf9cf46548d95cf9ddc 3780e19e74cc59250f25d65eb5d1f3f7dd9215a4 && git checkout -B drift-repro bcc9189e6e080d47428eccf9cf46548d95cf9ddc && git merge --no-ff 3780e19e74cc59250f25d65eb5d1f3f7dd9215a4
node scripts/docs-audit/affected-docs.mjs --json bcc9189e6e080d47428eccf9cf46548d95cf9ddc
|
PM review — ACCEPT on substance. Two questions routed to the contract reviewer, and ⛔ not enqueued pending it.
1. ⭐ A2.2 falsified — the seat asked me to confirm its reading. Confirmed: the card stands, no re-filing.The seat measured that The seat's reading is right, and here is the test I applied to it. The card's claim is "datasource DELETE does not evict the stuck driver from the driver registry". That claim was confirmed independently and mechanically: ⭐ And the seat did the thing that makes the falsification safe rather than merely honest: it fixed the real population and additionally closed the failed-start window the card imagined, so nothing the card asked for was dropped on the way. Rolling the registration back makes "failed ⇒ not registered" true by construction rather than by the current arrangement of the lines — that is the durable version of the property. 2. Clause ② overruled upward to YES/YES — accepted, and I was wrongI dispatched this NO/NO. The seat is right on both limbs: the diff touches 3. ⛔ Two errors in my dispatch order, corrected on the recordBoth caught by the seat, both mine:
⭐ The second one could have produced a false green, and the seat pre-empted it: the behavioural pin reads both envelopes ( 4. What I checked myself
|
Docs-drift rows re-verified by hand — all three clean. ⛔ Not a clean bill of health for the whole corpus.The bot listed 3 hand-written pages for implementation-accuracy re-verification. Checked each against what this diff actually changes (a deleted datasource stops being named by
⭐ The row worth naming is the third: it is a prefix match, not a real hit — Also swept, though the bot did not list it: ⛔ CI at head Generated by Claude Code |
Green at
|
A same-session precedent for the open semver question (Q1)⛔ Not a re-post of the blocker — new information bearing on the one substantive question I routed to the contract reviewer in comment 5479642323. Q1 was: A sibling PR from the same lane, this session, has now graded a comparable change the other way. #13870 (#13576) installs a new
⇒ ⭐ Same session, same lane, comparable contract impact —
⛔ Still not mine to decide, and ⛔ still not a reason to hold or release the PR on my own judgment. Handing the reviewer a precedent instead of a hedge. Everything else unchanged: green at Generated by Claude Code |
…iver-registry-eviction
…main The page is routed to `merge=os-regen` in .gitattributes, so the merge took one side whole with zero conflict markers — here, this branch's side — silently dropping main's own re-anchor after #13843 grew packages/rest/src/rest-server.ts. Regenerated with the gate's own --fix, which recomputes every anchor from the actual source positions. Verified rather than assumed: 65 table rows in, 65 out; with line numbers normalised the page is identical to main's apart from the objectql/src/engine.ts anchors; and every one of those shifts matches this branch's two insertion hunks exactly (+66 for sites between them, +75 for sites after the destroy() change). No row deleted, none reworded. Part of #13578 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
Merge conflict resolved — and it was a silent one, not a textual oneMerged The conflict was not where git said it was
The pre-commit hook then said the same thing independently, which is the reassuring part: Regenerated with the repo's own tooling, Why this was NOT the "stop and report" caseYou asked me to stop rather than guess if both sides changed the same logic and picking either loses behaviour. I checked that specifically before regenerating, and it does not apply here:
Re-verification at
|
| Commit | What it did | @objectstack/spec |
|---|---|---|
7ce02eb09d |
created the contract, 27 members | minor |
8425c17ccc |
added five optional members | minor |
52954c0ac4 |
changed one member's return type | minor |
⭐ The middle row is the decisive one: members that were all optional — breaking nobody, by construction — still took minor. This PR adds a required member, which is strictly more impactful, and grades it patch.
Two further measurements:
- The surface is genuinely public.
packages/spec/src/contracts/index.tsdoesexport * from './objectql-engine.js', and./contractsis a published export path — so an external implementer, or any structural assignment toIObjectQLEngine, is reachable and would break at compile time. - I still cannot measure whether an external implementer exists. In-repo,
ObjectQLis the only one. Recorded as NOT MEASURED, as before.
My recommendation: minor. The honest counter-argument is that this interface's own header describes it as a consumer view with one intended implementation, grown whenever a consumer needs a member — so under a strict implementer reading it could essentially never take a patch. That argument has real force, but it loses to the precedent: the repo has already answered this question three times on this file, including for a change weaker than this one. The cost of grading up is one version digit; the cost of grading down is a silent compile break with no changelog line pointing at it.
It is a one-word edit (patch to minor on the @objectstack/spec line of .changeset/driver-registry-eviction.md, plus a BREAKING banner matching #13870's shape). Say the word and I will make it — I have left it alone because a contract reviewer is queued on exactly this.
Generated by Claude Code
…nner `IObjectQLEngine.unregisterDriver` is a REQUIRED member on a published interface: additive for consumers, compile-breaking for any third-party implementer. Regraded from patch to minor to match this contract's own precedent — the three prior changes to it all took minor, including one that added five members that were ALL optional and so broke nobody by construction. A required member grading below that is inconsistent. Banner shape verified against #13870 rather than assumed: that changeset does pair a `minor` bump with a `**BREAKING**` line citing the launch-window convention. A strict-semver reading would say `major`; that reading is recorded as an open question for the maintainer in the PR body rather than acted on here, since uniform in-repo precedent is the operative convention and overruling it is not this PR's call. Part of #13578 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
Contract review (Clause ②) — REWORKReviewed at head What this means procedurally
Two things to change, both small:
Re-review is cheap. Both items are edits, not re-architecture; ping the triage seat when they land and this goes back into the review queue at the current head. The open Generated by Claude Code |
…w raises Adding the banner turned `check-adr-0087-registration` red, correctly: a declared-breaking changeset must state in writing whether the change needs a ledger entry. Answered with the category built for this exact shape — a published runtime TypeScript interface with no metadata surface behind it, so `objectstack migrate meta` has nothing to rewrite and the compiler is the channel that reaches consumers. Part of #13578 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
|
REWORK items landed — ready for re-review at
|
| Commit | What it did to IObjectQLEngine / IDataEngine |
@objectstack/spec |
|---|---|---|
7ce02eb09d |
created the contract, 27 members | minor (you verified) |
8425c17ccc |
added five members that were all optional | minor |
52954c0ac4 |
changed one member's return type | minor |
⭐ 8425c17ccc is the one that matters most: members that were all optional — breaking nobody by construction — still took minor. A required member grading below that would be internally inconsistent, which is what makes minor the floor here on this file's own evidence rather than by analogy to #13870.
Still open, deliberately
The minor-vs-major question is recorded in the PR body as a maintainer question, not acted on: a strict-semver reading says major; precedent on this file is 3-for-3 for minor; whether any external implementer exists is NOT MEASURED. As you noted, it does not block this re-review.
State
⛔ Still draft. ⛔ needs:contract-review still on both carriers. ⛔ No ready flip, no auto-merge. 61 gates re-run at 3780e19e74 on a clean tree: 60 green, 1 (check-test-completeness) structurally NOT MEASURED locally — exit-code-identical to the pre-edit round. Your note that check runs must be re-confirmed green at the current head still stands; that is the enqueue step's reading, not one I take here.
Generated by Claude Code
Fixes #13578
The ObjectQL driver registry had a
registerDriverdoor and no counterpart,so nothing could ever leave it.
DELETE /api/v1/datasources/:nameemptied theadmin door while
GET /api/v1/readykept naming the deleted datasource'sdriver, with a process restart on every replica as the only recovery.
The lifecycle enumeration
The card asked for every path that can leave an orphan driver instance, walked
from the registry's lifecycle rather than from the observed example. Traced on
origin/maineb717a12:DELETE(removeDatasource→tryUnregisterPool→DatasourceConnectionService.disconnect)unregisterDriver, after the close.disconnectAll→ samedisconnect)ObjectQL.destroy())checkDriversHealth()by pinging pools it had just closed.attemptConnectcatch)try. A throw after it returnedfailed-degradedwhile leaving a live entry: a datasource the admin list calls failed whose driver the probe still pings.handle.connect(), so a driver that throws on start was never registered. Measured, not assumed — see A2.2 below.updateDatasource→tryRegisterPool)attemptConnectshort-circuits withalready-registeredwhen the name is held, so an update never rebuilds the driver: the OLD instance, built from the OLD config, stays live and registered.Where eviction belongs, and why
The registry owns its own liveness — the second horn of the card's fork,
and triage's default, but for a load-bearing reason rather than by preference.
Removing a driver is not one deletion but three pieces of private engine
state that must move together, and a caller can reach none of them:
drivers— the MapcheckDriversHealth()iterates, and so the one/readyreports. The entry datasource DELETE does not evict the stuck driver from the data-engine driver registry — /ready keeps naming a datasource that no longer exists, recoverable only by process restart #13578 watched survive a
DELETE.defaultDriver— a name, not a reference. Dropping the entry alone leavesthe default pointing at a driver that is gone, and
getDefaultDriverName()answers with a name nothing backs — worse than the leak, because callers treat
that answer as a live routing target.
datasourceDefs— has aregisterDatasourceDefdoor and no removal door atall, so a def outliving its driver keeps judging writes for a datasource that
no longer exists.
Only (1) is visible from outside. "Every future lifecycle path remembers to clear
three maps in the right order" is a rule with nowhere to live where it would be
read. One primitive owns the invariant; every path calls it once.
Two deliberate non-responsibilities, both pinned: eviction does not disconnect
the pool (an adopted host-owned instance outlives this kernel, ADR-0062 D5), and
does not clear
unavailableDatasources(that map has its own door, and on thefailed-start path the mark is written after the eviction).
Cluster propagation
Measured rather than inherited from #13405. The driver registry has no cluster
broadcast in either direction: no datasource create or delete emits a cluster
event, and each replica populates its own registry at boot from the shared
datasource records (
rehydratePools). So eviction being per-replica issymmetric with registration, not the create-broadcasts/delete-doesn't asymmetry
#13405 records on the
/api/v1/meta/datasourcemetadata registry — adifferent registry with a different propagation story. Adding a broadcast for
delete alone would make delete more cluster-aware than create.
that served the
DELETErecovers immediately; the others keep the stuck driveruntil they restart. Closing that needs a broadcast channel this registry does not
have — design surface, not a defect fix — so it is filed rather than improvised.
Not the reporting side
⛔
packages/runtime/src/http-dispatcher.tsis untouched. It only reports theregistry's contents at
/ready; repairing the report would hide the defect. The#13408 readiness-drain semantics are likewise untouched and not re-decided here.
Verification
packages/runtime/src/registry-eviction-readiness.test.ts)— the real
ObjectQLengine, the realDatasourceConnectionService.disconnect(),and the real
HttpDispatcher/readyhandler, with no doubles for any of thethree.
packages/runtimeis the only package that depends on all three.Asserts
/readystops naming an evicted datasource, with a positive control(a second stuck datasource is still named, the healthy one still routable) so a
fix that emptied the registry could not pass.
disconnect()turns all 4 ofthose tests red. Mutation proven on disk (anchor count 1 to 0, marker injected,
blob
52c03022vsHEAD116bba65),service-datasourcerebuilt, andablation-dist-preflight --absentconfirming the artifact the suite actuallyconsumes no longer carries it — those imports resolve through
dist/, notsrc(both pairs are in
KNOWN_UNALIASED_TEST_IMPORTS). Restore leg re-verified:git diff HEADempty, blob back to116bba65, rebuilt, preflight PRESENT.packages/objectql/src/engine-driver-eviction.test.ts,funnel + rollback pins in
service-datasource's connection-service suite.ConnectionEngineLikeis
Partial<…>, so a fake missing the member would have made the optional call ano-op and every eviction assertion a vacuous pass.
ConnectionEngineLikeroster pin moved from seven members to eight,deliberately and with the reason recorded — it is a
tsc --noEmitassertion thatexists so widening the seam is a written decision, not a side effect.
Verified at final commit
3259302525(clean tree):pnpm --filter @objectstack/objectql test— 251 files, 4331 passedpnpm --filter @objectstack/service-datasource test— 28 files, 600 passedregistry-eviction-readiness+http-dispatcher.ready— 31 passedtypecheckgreen forobjectql,service-datasource,spec,runtimescripts/pm/dispatch-gates.mjs) — re-run after merging main; see the resolution comment for the current reading (61 ran, 60 green).The other three (
check-dev-prereqs,check-test-completeness,check:dual-build-cjs-loads) each printPREREQUISITE NOT MET— they need awhole-workspace build and state that nothing was measured. Recorded as NOT
MEASURED, not as passes.
check-system-context-census --fixre-anchored 11 line citations incontent/docs/permissions/system-context.mdx: pure line rot, since the newmethod sits above every cited elevation-read site in
engine.ts.packages/objectqlandpackages/runtimetypechecks exclude*.test.ts, so their green says nothingabout the two new test files (
--listFileshit count 0 for each); those arecovered by
check:type-check-debtin CI.service-datasource's typecheck doesinclude its
__tests__(hit count 1), which is what makes the roster pin real.Clause-②: yes— path limb (packages/spec/src/contracts/objectql-engine.ts) andcontent limb (a new member on a published contract widens the public surface).
This overrules the dispatch's NO/NO upward: the fix is contract-first, because
having the consumer probe an undeclared method would be exactly the tolerant
consumer-side fallback the repo forbids.
Open question for the maintainer — is
minorthe right grade, ormajor?Not a defect report and not a blocker: the changeset ships
@objectstack/specasminorwith a**BREAKING**banner (verified at head3780e19e74), and thissection records the reading that was NOT taken, so the decision is visible rather
than buried.
major.unregisterDriver(name: string): booleanis a required member added to a published interface on a
17.xpackage(
@objectstack/specis at17.2.0, lockstep 17.x).The surface is genuinely public, measured not assumed:
packages/spec/src/contracts/index.tsdoesexport * from './objectql-engine.js'and
./contractsis a published export path — so an external implementer, or anystructural assignment to
IObjectQLEngine, breaks at compile time.minor.7ce02eb09d(created the contract, 27 members),
8425c17ccc(added five members that wereall optional, breaking nobody by construction), and
52954c0ac4(changed onemember's return type) each graded
@objectstack/specminor. Uniform precedentwas treated as the repo's operative convention; overruling it upward to
majoris a maintainer call, not one taken inside this PR.
IObjectQLEngineexists is NOT MEASURED.In-repo,
ObjectQLis the only one. If the true count is zero thepractical impact is zero and
minoris comfortably right; nothing available frominside this repo can answer it for third parties.
⇒ If the maintainer reads the published-surface fact as decisive over the in-repo
precedent, this should be
majorand the one-line regrade is all it takes.Out-of-scope findings filed
already-registeredshort-circuits the reconfigure path, so the OLD pool stays live and the admin UI reports success #13804 — datasource update never rebuilds its driver (already-registeredshort-circuits the reconfigure path). The enumeration's one knowingly-uncovered
orphan path; fixing it is a behavioural decision, not a mechanical repair.
propagation in either direction.
Generated by Claude Code