fix: removeBetween leaves the end marker it was asked to remove

[vivek7405]

Closes #1289

Every rendered template instance is bracketed by a wjm-s / wjm-e comment pair, and removeBetween is what takes a range back out. The guard deciding whether to remove the end marker read start.parentNode AFTER the walk had already detached start, so it compared the live parent against null, could never fire, and every teardown left one orphan comment in the document. They accumulate for the life of the region and nothing collects them, so a long-lived list that churns rows grows comment nodes without limit. A 3-row repeat() cycled empty and back five times went from 3 end markers to 18.

The fix captures the parent before the walk consumes start. Two tokens of behaviour, and a docblock carrying the reasoning.

What I kept, and why

The guard stays. A marker moved under a different parent is not this region's to remove. Reaching into it would steal a node the renderer does not own and throw NotFoundError out of a teardown that has to stay total. There is a test for exactly that.

The walk is untouched. It already stepped nextSibling from start and stopped on end, and it still does. Worth being explicit that this is not a guarantee the range is safe from mutation: a range whose end no longer follows its start runs off the child list, before this change and after it, and reconcileRepeat's catch already spells out what that costs. The guard is the narrower promise, that a marker sitting somewhere else is left alone.

No clear/remove split. lit splits _$clear (keep the markers, render into them again) from removePart (destroy the part). Every caller here discards the instance, and a replacement, where there is one, is built with markers of its own (from buildDetached on the list paths, inline on the child-hole path), so a second helper would ship with zero callers. The contract is pinned in the docblock instead.

For what it is worth, the shape I landed on is what React's clearHydrationBoundary does (it takes parentInstance as a parameter captured before any removal, and its Suspense boundaries are comment-delimited too), and what Vue's removeFragment does. lit at HEAD removes only the start marker, and its own tests strip comment nodes, so it cannot see the difference.

Correction to the issue

The issue body claimed SSR emits these markers and they round-trip through hydration. That is false. packages/server/src emits no wjm-* bookends at all, and the only three creation sites are document.createComment in render-client.js. This is client-teardown-only by construction, so there is no server-side half. I have corrected the issue body.

Test plan

Unit packages/core/test/rendering/marker-leak-on-teardown.test.js, six leak cases covering every removeBetween caller (the two array cases share one, shrinking and emptying), each looping five cycles and counting comment nodes directly (the leak is invisible to textContent and querySelectorAll, which is why nothing caught it). Each asserts the pair count is balanced AND back to baseline, plus the rendered output, so none can pass by rendering nothing. A seventh case pins the guard instead and counts no markers. 7/7 pass.

Browser packages/core/test/rendering/browser/marker-leak-on-teardown.test.js, two cases. One drives the churn through a real component's update pipeline rather than a bare render(). The other puts a disconnectedCallback on every row, so author code runs synchronously part-way through the removal rather than after it, and asserts the removal still finishes and the region still renders. That second one is about fidelity rather than capability, since linkedom does fire the callback; it is pinned on the three engines people actually run.

Every leak case takes its baseline through a helper that refuses an empty or unpaired count. Without that, the counter matches on a marker suffix spelled in the test, so renaming it would leave all six comparing zero against zero and passing while observing nothing. Confirmed by mutating the suffix in the renderer: before, six stayed green; now all seven red.

Slot one case added to packages/core/test/slots/browser/record-self-heal.test.js. The fix hands the light-DOM backstop one extra removedNodes record per torn-down row, so this churns a slotted list five times and asserts the marker count, the projected set, and assignedNodes() all return to baseline.

Counterfactual, proven at 239f19c and re-checked since: reverting the two tokens reds all 6 leak cases (5 start markers against 15 end for repeat(), 4 against 34 for a same-key shape swap, 2 against 12 for a child hole), both browser cases, and the slot case, which fails alone out of 71 in that file. The 7th unit case is the guard case, green either way by design.

Two unit cases initially stayed green with the bug present. They rendered the outer shell from two separate template literal sites, so the container saw two different strings identities and rebuilt the whole instance every render, wiping the orphans with replaceChildren before the assertion could see them. Fixed in the second commit, and written up in a comment below.

Suites

suite	result
npm run test:browser (chromium)	740 pass, 0 fail, 1 skipped, 68 files
npm test	3897 pass, 7 fail
WEBJS_E2E=1 e2e	87 pass, 4 fail
website boot check, dist mode	/, /docs/error-handling, /ui, /ui/button all 200, 0 broken preloads

Every failure above is pre-existing in this worktree, and I did not take that on trust. The e2e set is byte-identical with the fix reverted (same 87/4, same four names, two of them JS-off cases where none of this code runs at all). The node failures reproduce identically on the reverted source too: the elision ones are 500s from a fixture app, and the Bun listener one needs a runtime this worktree does not resolve. An earlier e2e run showed 13 failures and was my own contamination, since I built core/dist while it was in flight and the e2e switches to dist mode when dist/ exists.

Bun: N/A, render-client.js is browser-only and never runs on the Bun server. It also matches none of the path patterns the Bun parity gate keys on.

Docs

.agents/skills/webjs/references/components.md and website/app/docs/error-handling/page.ts, one sentence each on the teardown paragraphs. Both already claimed teardown is total, and unbounded comment growth is a dimension of that a user can watch in devtools.

Notes for a reviewer

• moveRange looks similar and is NOT affected. It appends into a DocumentFragment and breaks on n === end, so it carries the whole range including the end marker. It must not be "fixed" to match.
• Insertion positions shift by one node wherever an orphan used to sit. Nothing anchored on one: nextArrayAnchor reads the START marker and guards on .parentNode, and both applyChildInnerRaw and reconcileRepeat insert before the PART marker.
• The throw path is unchanged. End removal is still last, so a mid-teardown throw leaves exactly the state the catch comments already describe, and if (!parent) return is byte-equivalent to the old first line.
• packages/core/dist/ is gitignored and the browser prefers it over src/ when present. Every test here imports src/ directly, so anyone hand-verifying in an example app wants a rebuilt dist first.

[vivek7405]

Design note: how to write a test that can actually SEE a renderer leak

Worth writing down, because it cost me two useless tests and it will cost the next person the same.

A test that drives the renderer with two separate render() call sites, even when the two templates look identical in the source, hands the container two different strings identities. The renderer treats that as a different template, rebuilds the whole instance, and createInstance ends with container.replaceChildren(...). That wipes every stray node in the container. So any accounting assertion made after such a sequence is measuring a freshly rebuilt subtree, and it stays green no matter how badly the teardown path leaks.

Concretely, this shape cannot observe the bug:

render(html`<div>${a()}</div>`, el);   // site 1
render(html`<div>${b()}</div>`, el);   // site 2, different `strings`, full rebuild

and this one can:

const view = (inner) => html`<div>${inner}</div>`;   // ONE site
render(view(a()), el);
render(view(b()), el);                                // updates in place, hits the child-hole teardown

The same trap is why the leak survived this long in the first place. It is invisible to textContent and to querySelectorAll, and the one helper in the suite that touches marker comments (stripExpressionComments in the cache tests) removes them before any assertion sees them. I left that helper alone, since stripping is what makes those assertions readable, but its docblock now says outright that it cannot be used to show markers are balanced.

The rule I would apply next time: for anything asserting on what a teardown left behind, render from a single template site and assert on comment nodes directly, then prove the assertion reds with the fix reverted before trusting it.

[vivek7405]

Went back over this one properly. The fix itself holds up: every removeBetween caller discards the instance right after, nothing anchors on the end marker afterwards, and the counterfactual is convincing on all three engines. I am happy with the terminator choice and with keeping the guard.

Two things I want changed before this goes in, both about the reasoning rather than the behaviour.

The docblock oversells why terminating on end is safe, in a way the very next paragraph contradicts. And the slot case does not hold itself to the standard the unit file's own docblock sets out, which matters because the two are asserting the same invariant and only one of them survives a rename.

Neither changes what ships.

[vivek7405]

Second pass, scoped to the docblock commit. It does not hold up, and the problem is that I was defending a design decision this change never made.

The terminator was already the end marker before any of this. Only the parent capture changed. Once I started arguing for it in prose I ended up claiming the walk is bounded by the instance's range, which it is not: it steps nextSibling and stops on end, so a range mutated underneath it overruns in exactly the way I was ascribing to the alternative. The rewrite therefore left no reason to prefer either terminator, and the sentence it replaced was the only thing that had ever made the contrast work.

Two knock-on problems from the same habit. The unit case pointed at the wrong catch block for that overrun, and the browser case said it justified the terminator when its callback only ever writes outside the torn-down range, so it proves nothing of the sort.

Cut the argument instead of patching it again.

[vivek7405]

Third pass, scoped to the docblock cleanup. The code is untouched by all of this and has been settled since the first commit; what keeps turning up is the same retracted argument surviving in places the previous cleanup did not reach.

Two of the three are in the PR description, which matters more than it looks: this squash-merges, so the description becomes the commit body on main. Merging as written would put an argument in the permanent history that the merged code explicitly says is wrong. The third is the browser suite header, fixed at the case but not at the top of the file.

I swept the whole diff for the rest of it this time rather than fixing what was pointed at. Nothing else survives.

[vivek7405]

Fourth pass. One real error of fact and two overclaims, all mine, all in prose again.

I had written that linkedom never runs disconnectedCallback, and used that as the reason the second browser case exists. It is not true. linkedom flips a reactive flag on the first customElements.define and fires the callback synchronously out of removeChild, walking the removed subtree, and this repo's own lifecycle test at packages/core/test/lifecycle/component-lifecycle.test.js already leans on it. I checked it directly before touching anything: a nested element inside a removed row fires. The case still earns its place, but on fidelity, running the three real engines, not on linkedom being incapable, and I have left a note in the file so the stronger version does not creep back.

The unit header claimed all seven cases count markers and assert rendered output. The guard case does neither, deliberately. And the non-zero baseline assertion I added two commits ago to stop a marker rename hollowing these out went into three of the four places that need it.

The code has not moved since the first commit.

[vivek7405]

Went through every remaining factual claim in the two new test files and the docblock myself, rather than waiting to be told about them one at a time. Three were wrong and are fixed in the commit above.

The docblock said a replacement instance gets its markers from buildDetached. The child-hole caller creates them inline, so that was right for most callers and wrong for the first one listed.

The unit header said one case per caller, when the two array cases share removeArrayItem.

And the guard case hedged that linkedom might not throw for a removeChild of a node under a different parent. It does throw, same as a browser. That hedge was hiding something better than it claimed: both of that case's arms discriminate, an unguarded removeChild reds the first and an unconditional end.remove() reds the second. I toggled each to confirm rather than reasoning about it.

The code is unchanged and has been since the first commit. Everything since has been me writing prose that outran what I had checked.

[vivek7405]

Fifth pass. One of these is the best finding of the review so far and it is worth stating plainly: the marker counter matches MARKER plus an s / e suffix spelled in the test file, so it only ever tracked half the marker text. Rename the other half in the renderer and all six leak cases stayed green, comparing zero against zero, satisfied by a counter that had gone blind. The rendered-output assertions do not cover it, because they catch rendering nothing, not counting nothing. Verified by mutating the suffix.

Every baseline now goes through a helper that refuses an empty or unpaired count, so the same mutation reds all seven where it used to red one. That is the guard I had added to the browser and slot cases two commits ago and, embarrassingly, called a completed sweep.

The remaining two are the same accounting error in two places: a case label naming a caller path nothing in the file actually takes, and the PR description still carrying the per-caller claim I corrected in the file but not in the body.

[vivek7405]

Sixth pass, and I am stopping the cycle here rather than running another.

Two findings, both the same shape as the last three rounds: a claim I corrected in one place and left standing in another. The slot helper's comment still credited reading MARKER with preventing a vacuous pass, which is the reasoning the previous commit disowned for the unit file, and the PR description still named buildDetached as the source of a replacement's markers when the child-hole path builds them inline. Both fixed above.

The pattern is worth naming, because it is mine and not the change's. The code has not moved since the first commit and every reviewer that has looked at it has confirmed it: the guard fires, the six callers all discard the instance, nothing anchors on the end marker, and the counterfactual reproduces exactly. What kept failing was the prose around it, in three ways. I wrote explanations broader than what I had actually checked. When one was corrected I fixed the instance I was shown instead of grepping every surface carrying the same sentence. And the docblock spent most of its length defending a terminator choice this change never made, which is argument surface that exists only to be wrong.

What I would keep from this: an explanatory comment should assert only what a reader could mechanically check, and a correction is not done until the source, the tests, and the PR body have all been grepped for the same claim.

The fixes in the commit above are unreviewed, so this stays a draft.

[vivek7405]

Review round 8: independent regression audit + reference cross-check

Correcting the header: this is the EIGHTH round on this PR, not the first. Seven preceded it on 2026-08-05. Six of those changed prose only; one (the fifth) found a real defect, the marker counter that tracked half the marker text and passed on zero against zero. The code has not moved since the first commit. This round adds the evidence no earlier round gathered, the counterfactual run in both directions over the full suite and an independent check of the lit / React / Vue claims, and it finds nothing to fix.

Verdict: no regressions found, approve. Nothing here is a must-fix. What follows is what I verified rather than took on trust.

Regression surface, enumerated

The change is observable only at teardown, so the question is what could depend on an orphan wjm-e surviving. I walked every consumer.

• All six removeBetween call sites (applyChildInner, reconcileRepeat twice, teardownRepeat, removeArrayItem, teardownChild) discard the instance immediately, so none can observe the marker afterwards.
• Insertion anchors never read a bookend. nextArrayAnchor reads the START marker and guards on .parentNode; applyChildInner and reconcileRepeat insert before the PART marker. Positions shift by one node where an orphan sat, and nothing is anchored there.
• bindPart path resolution is unaffected. Paths are computed against templateEl.content and resolved against a fresh clone, and in all three creation sites the bookends are attached AFTER bindPart runs. So no path index can move.
• The slot backstop's extra removedNodes record is inert. processBackstop starts its removal branch with state.authored.indexOf(node), and a renderer bookend is never in the authored record, so the entry falls straight through.
• The router morph improves rather than degrades. diffElementInPlace reuses comments POSITIONALLY (liveChildren[i]), so accumulated orphans in the live DOM were shifting comment and text peers against incoming SSR HTML that never had them. Fewer orphans means better alignment. Unclaimed second benefit.
• moveRange's no-op guard fires more often now that an orphan cannot sit between end and the anchor. Strictly fewer redundant DOM moves.
• No server half exists. packages/server/src emits no wjm-* at all and the only three creation sites are document.createComment in render-client.js, so the correction to the issue body is right.

Empirical counterfactual, both directions

Full node suite, same worktree, toggling only the two tokens:

source	result
fix applied	3905 tests, 3898 pass, 6 fail
fix reverted	3905 tests, 3892 pass, 12 fail

The 6 failures are byte-identical in both runs (blog integration, Bun listener, elision fixture), all environment-local and all green in CI. Reverting adds exactly the 6 leak cases and nothing else. So this change breaks nothing and the new tests are load-bearing.

Browser, all three engines with the fix: 740 / 730 / 740 passed, 0 failed. Reverted, on the two touched files: 3 failed on every engine (both browser cases plus the slot case). The tests are not vacuous.

The three reference claims, checked against the local clones

All three hold.

• React is the closest analogue and matches this shape exactly. clearHydrationBoundary(parentInstance, hydrationInstance) takes the parent as a PARAMETER captured before any removal, and uses it for both the walk (parentInstance.removeChild(node)) and the end marker (parentInstance.removeChild(nextNode)).
• Vue removeFragment walks while (cur !== end) then calls hostRemove(end) unconditionally, so it removes the end marker too. Worth noting Vue does NOT keep a cross-parent refusal; it reads the parent off the node at removal time. This PR is the stricter of the two, which I think is right given the light-DOM slot layer physically relocates nodes, and there is a test pinning it.
• lit leaks the same marker, confirmed by running it. removePart is _$clear() plus _$startNode.remove(), and _$endNode is never removed even though insertPart creates both markers for the part. On lit-html 3.3.3 under jsdom, a 3-row repeat() cycled empty and back five times went from 11 comments to 26, exactly one orphan per removed row, while the rendered output stayed correct. So the divergence from lit is real, deliberate, and in the right direction.

Gate claims, checked

• Bun parity genuinely does not apply. require-bun-parity-with-runtime-src.sh matches on serialize|/json\.js|file-storage|listener|ts-strip|action|render-server|/ssr\.js|..., and packages/core/src/render-client.js matches none of them. Substantively the module is browser-only.
• Doc sync is complete. I checked whether the scaffold ships a second copy of references/components.md that would drift. It does not: create.js copies the repo-root canonical (bundled at prepack), so editing the one file is the whole job.

Two notes, neither blocking

1. The directives-cache.test.js change is not in the PR body's test plan. It is comment-only, and it corrects a docblock that described the marker syntax wrongly, so the omission is cosmetic.
2. The body reports 3897 pass / 7 fail; I measure 3898 / 6 in this worktree. Environment drift, not a signal.