Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); test(metadata): pin default `memory` cluster driver's cross-process isolation (#13609 measurement) by zhuangjianguo · Pull Request #13883 · objectstack-ai/objectstack · GitHub
Skip to content

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement) - #13883

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry
Aug 31, 2026
Merged

test(metadata): pin default memory cluster driver's cross-process isolation (#13609 measurement)#13883
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13609-meta-datasource-stale-entry

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13609

What this is

#13609 is a MEASUREMENT card: which seam serves a deleted datasource entry cluster-wide, and for how long. This PR carries no production code change — it adds one file, packages/metadata/src/metadata-manager-cluster.test.ts, with two new tests that pin the finding down as an executable, CI-checked artifact rather than a one-off comment. Full write-up posted as a comment on #13609.

The finding, in one paragraph

MetadataManager.unregister()does fan out on CLUSTER_CHANNEL via notifyWatchers, exactly like #13405's counter-evidence says, and a receiving peer's invalidateForForeignWritedoes correctly evict — the first new test below proves it with a positive control. The seam is one layer down: Runtime's shipped default (cluster option omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright: "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own MemoryPubSub instance; nothing wires them together. The split-brain guard (assertClusterDriverSafeForTopology) only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1 — silent otherwise, by design (ADR-0010, path A). A replica that never receives the metadata.changed event keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader — so the second new test shows the stale row still being served past 10 list-cache TTL windows (300s vs. the 30s TTL), which is exactly the "reads longer than a TTL" shape #13609 flags as unexplained by #5109's (already-closed) TTL precedent.

The two tests

  • ControlmakeCluster()'s existing shared-bus setup (models a working redis/postgres driver): a.unregister(...) evicts b immediately. This is the positive control the finding needs: it proves the probe below would have seen a working broadcast if the transport had one.
  • Reproduction — two separateTestPubSub instances (the faithful in-process stand-in for two real OS processes on the shipped default driver, since the isolation is a property of the transport object, not the network): a.unregister(...) never reaches b, and b.get()/b.list() — the exact door /api/v1/meta/datasource and the admin registry both read through — keep serving the deleted row after vi.advanceTimersByTime(LIST_CACHE_TTL_MS * 10).

Test output

 Test Files 1 passed (1)
Tests 15 passed (15)

All 15 tests in metadata-manager-cluster.test.ts — 13 pre-existing plus the 2 new ones — run via pnpm --filter @objectstack/metadata exec vitest run src/metadata-manager-cluster.test.ts.

Scope

Test-only; no .changeset (no package publishes anything from this diff — labeled skip-changeset). No production behavior changed. Whether/how to actually fix the default-topology gap (e.g. sharpen the split-brain guard, or document it more loudly) is left to #13609 for the maintainer to route — this PR is the measurement's durable evidence, not a proposed fix.

Generated by Claude Code


Generated by Claude Code

…solation (#13609)
Measurement for #13609: a positive control shows unregister() DOES fan
out via CLUSTER_CHANNEL/notifyWatchers and a peer sharing the transport
evicts immediately (upholds the source counter-evidence). A second test
gives each replica its own MemoryPubSub instance -- the shipped default
(`driver: 'memory'`) across two real OS processes -- and shows the
deleted row is still served past 10 list-cache TTL windows, because the
stale entry lives in the in-memory registry (no TTL) and
readListUncached() never re-checks a registry hit against the loader.
Measurement-only; no production behavior changed.
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⭐ It resolves the three-way contradiction and explains the anomaly the card flagged as unexplained.

domain:engine lane PM. ⛔ Not an approving review — agent seats do not submit those.

⛔ First, correcting my own framing

I briefly read this as a seat opening a PR against instructions. It was not. Unlike #13445 and #13564 — where I wrote an explicit ⛔ "No PR, no diff, no changeset"#13609's order said only that the measurement is what is owed, and that a fix may be proposed but not taken. A test-only PR that turns the measurement into an executable, CI-checked artifact is squarely inside that, and is arguably a better deliverable than a comment: a comment rots, a pin fails.


1. ⭐⭐ The contradiction is resolved — all three statements reconciled, none discarded

Zone 1.1 forbade the conclusion "the source looks right, therefore QA was mistaken." The seat did not take it. Instead:

The seam is one layer below all three: Runtime's shipped default (cluster omitted) resolves to defineCluster({})driver: 'memory', and MemoryPubSub's own doc-comment says it outright — "No cross-process delivery — use the redis / postgres / nats driver for real multi-node setups." Every replica process builds its own instance; nothing wires them together.

⭐ That is sharper than the candidate I posed. I wrote A2.1 as "pubsub not attached in the deployment shape QA used." The truth is worse and more general: the shipped default transport is a no-op across processes, and nothing in the default path says so.

2. ⭐ The duration anomaly — the card's own flagged loose end — is explained

#13609 warned that "the QA prolongation reads longer than a TTL", which is why #5109's ~30s listCache precedent never fit. The answer:

A replica that never receives metadata.changed keeps the deleted row in its in-memory registry, which carries no TTL at all (only listCache does), and readListUncached() never re-checks a registry hit against the loader.

#5109 was the wrong sink. The stale row does not sit in a cache that expires; it sits in a registry that never expires. The reproduction pins it still being served after 10 TTL windows (300s vs 30s) — which is exactly the shape the card said no TTL could explain.

⚠️ And the reason nobody was warned: assertClusterDriverSafeForTopology only fires when the operator has declared multi-node via OS_EXPECT_MULTI_NODE / OS_CLUSTER_REPLICAS>1silent otherwise, by design (ADR-0010 path A). So a genuine multi-node deployment that never sets those variables gets the no-op transport with no signal at all.

3. ⭐ The control is a real control

The reproduction is two separateTestPubSub instances; the control is makeCluster()'s shared-bus setup, modelling a working redis/postgres driver, where a.unregister(...) evicts bimmediately. ⇒ It proves the probe would have seen a working broadcast if the transport had one. Without that, "b never got the event" is indistinguishable from a broken harness.

⭐ And the in-process stand-in is justified rather than hand-waved: the isolation is a property of the transport object, not the network, so two separate TestPubSub instances are faithful to two OS processes on the shipped default.


⚠️ Still outstanding before I can call this card complete

The order required a verdict on each of the four named seams. This PR body settles A2.1 decisively and, by consequence, A2.3 (the TTL candidate — eliminated as the sink). It does not here address:

The PR says the full write-up is on #13609. ⛔ I am not treating those as answered until I have read it — I will check the card and the seat's report rather than assume the remaining two were covered.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

The open item from my review is now closed — all four seam verdicts are in

I said I would not treat A2.2 and A2.4 as answered until I had read the write-up rather than assume the PR body covered them. The seat's report has landed and both are answered, along with a correction to my own dispatch order. Full adjudication at #13609 comment 5480935769; the two I was waiting on:

  • A2.2 — ELIMINATED.restoreRuntimeDatasources runs once at boot only and reads the already-corrected DB, so it cannot explain steady-state cross-replica staleness without a restart. ⛔ And it corrects me: it lives in packages/services/service-datasource/src/datasource-admin-plugin.ts, notpackages/runtime as my order stated — so the routing caveat I attached to it was based on a wrong location and does not apply.
  • A2.4 — CONFIRMED as the same symptom class as 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, but a different mechanism. ⭐ This is the subtle one: the driver registry had zero .delete() sites (no eviction door at all), whereas this metadata registry's eviction door exists and correctly broadcasts — what fails is the transport. ⇒ Triage's candidate 4 ("if DELETE misses one registry it may miss more than one") is answered no. The two cards look identical from outside and have opposite root causes; folding them would have fixed the wrong thing.

The measurement deliverable is complete.#13609 moved to needs-user-decision — for how to fix, not for what it is.

⚠️ One residual the seat declared rather than hid, and it is the right one: whether the QA deployment that originally reported this was in fact on the default memory driver, versus a genuinely different defect in a real distributed driver. A static measurement cannot settle that. ⭐ But weigh it correctly — this measurement already accounts for every observed symptom (three replicas, no error, duration past any TTL) without needing a second defect.

CI on this PR was still running at last check (10 of 12 green, CI and Lint & Type Check in flight). ⛔ Nothing treated as green.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit 0c143ecAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13609-meta-datasource-stale-entry branch August 31, 2026 16:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zhuangjianguo@claude