fix: removeBetween leaks its end marker on every removal

[vivek7405]

Problem

removeBetween never removes the end marker it is asked to remove, so every instance torn down through it leaves one orphan wjm-e comment node behind. They accumulate for the life of the region, and nothing ever collects them.

The cause is an ordering bug in the function itself (packages/core/src/render-client.js:1705):

function removeBetween(start, end) {
  if (!start.parentNode) return;
  let n = start;
  while (n && n !== end) {
    const next = n.nextSibling;
    n.parentNode?.removeChild(n);
    n = next;
  }
  if (end.parentNode === start.parentNode) end.parentNode?.removeChild(end);
}

The loop removes start on its FIRST iteration, so by the time the last line runs, start.parentNode is null. The guard compares end.parentNode === null, which is false for an attached end marker, and the marker is skipped. The guard was presumably meant to check "are these two still in the same parent", and it silently became "never remove the end marker".

Measured against main at cea65748, under linkedom, rendering a 3-row repeat() and then cycling it empty and back five times:

after 3 rows,              wjm-e markers: 3
after 5 add/remove cycles: wjm-e markers: 18   (rows: 3)

Three orphans per cycle, unbounded. A long-lived list that churns rows (a feed, a filtered table, a live-updating list) grows comment nodes without limit. It is invisible in rendered output and in textContent, which is why no test caught it.

Found while implementing #1268, and deliberately left out of that PR as orthogonal to the mid-throw bookkeeping work it was fixing.

---

The decided fix

Capture the parent BEFORE the loop consumes start, and compare against that. The behavioural delta is two tokens. Everything else below is the reasoning that has to survive into the source comment.

/**
 * Remove a template instance's whole range, its bookend markers INCLUDED.
 *
 * Every caller discards the instance right after (the map entry or slot that
 * held it is dropped, and any replacement gets fresh markers from
 * `buildDetached`), so this is a REMOVE and never lit's clear-and-reuse. A
 * caller that wants to keep the bookends and render into them again needs its
 * OWN function, because the two want opposite answers for the end marker.
 *
 * `parent` is read BEFORE the walk because the walk removes `start` on its
 * first iteration, which nulls `start.parentNode`. Reading it afterwards left
 * one `wjm-e` comment in the document per teardown, unbounded for the life of
 * the region (#1289).
 *
 * The terminator stays `end` ITSELF rather than an `end.nextSibling` stop
 * sentinel captured up front. `removeChild` runs a custom element's
 * `disconnectedCallback` synchronously, so a sentinel pointing at a sibling
 * this region does not own can be detached or moved mid-walk, and the walk
 * would then run off the end of the child list and take the part's own marker
 * with it. `end` is renderer-created and reachable only through the instance.
 *
 * The `end.parentNode === parent` comparison is a refusal, not a formality. A
 * marker moved under a different parent is not this region's to remove, and
 * `parent.removeChild(end)` on it throws NotFoundError from inside a teardown
 * that has to stay total.
 *
 * @param {Node} start @param {Node} end
 */
function removeBetween(start, end) {
  const parent = start.parentNode;
  if (!parent) return;
  let n = start;
  while (n && n !== end) {
    const next = n.nextSibling;
    n.parentNode?.removeChild(n);
    n = next;
  }
  if (end.parentNode === parent) parent.removeChild(end);
}

Why this form and not the two alternatives

Not an unconditional end.remove(). The guard is a refusal with a job: a marker that has been moved under a different parent is not this region's to reach into.

Not lit's pre-#4975 sentinel form (const stop = end.nextSibling; while (n !== stop)). end.nextSibling is arbitrary DOM (the next row's start marker, the part's own marker, author markup), and removeChild runs disconnectedCallback synchronously in a real browser. A callback that moves or detaches the sentinel turns the walk into "remove everything to the end of the parent", which is exactly the dead-region outcome reconcileRepeat's catch comment at L1929 says must stay unreachable. end is renderer-created and reachable only through the instance, so no author code can hold it.

Precedent agrees with the chosen form. React's clearHydrationBoundary (ReactFiberConfigDOM.js:1239) is the closest structural analog, since its Suspense boundaries are comment-delimited: it takes parentInstance as a PARAMETER captured before any removal and removes the end comment explicitly. Vue's removeFragment (runtime-core/src/renderer.ts:2300) walks exclusively then removes the end anchor. Svelte's remove_effect_dom is inclusive by construction. Nobody re-reads .parentNode off an already-detached node. lit at HEAD removes only the start marker and its own tests strip comment nodes, so lit HEAD is not the precedent to copy here.

No clear/remove split

lit splits _$clear (keep the markers, render into them again) from removePart (destroy the part). WebJs has no clear-and-reuse caller: all six sites discard the instance immediately and any replacement gets fresh markers from buildDetached, so a second helper would ship with zero callers. Pin the contract in the JSDoc instead, as above. Keep the name removeBetween, which is quoted verbatim in two long catch comments whose claims stay true; renaming inflates a one-line correctness fix.

---

Already verified: do not redo this

The old body asked the implementer to confirm nothing depends on the stray end marker surviving. That is done, and the answer is no. The fix was applied in a scratch worktree off origin/main and measured, so start from this rather than re-deriving it.

Empirical (linkedom, patched vs unpatched, same script):

scenario	unpatched wjm-e	patched	rendered output
repeat(), 5 add/remove cycles	4 to 19	4 to 4	identical
plain .map() array, 5 cycles	4 to 19	4 to 4	identical
array shrink/grow sequence	11	4	identical
nested repeat() in array, 5 cycles	6 to 16	6 to 6	identical
cache() park/restore x5	2	2	identical, node identity preserved
template-shape swap x5	2	2	identical

The full Node suite produced the identical failure set patched and unpatched (all fresh-worktree environment noise in the blog and website suites).

Static:

• No insertion anchor uses an end marker. nextArrayAnchor (L2168) reads arrayItemFirstNode, which is startNode, and guards on .parentNode. applyChildInnerRaw and reconcileRepeat insert before the PART marker. moveRange carries both bookends together.
• Slot bookkeeping is untouched. processBackstop (slot.js:891) and reconnectSweep (slot.js:1043) skip renderer output via instanceOwns (slot.js:1095), and a stray sits between the HOST instance's own bookends, so strays are "owned" and never enter state.authored. Post-fix they appear in removedNodes, are not in authored, and are skipped.
• The fix creates no new "start detached, end attached" state. End removal is still last, so a mid-teardown throw leaves exactly the state the catch comments at L1922 and L2146 already reason about. Both stay accurate: if (!parent) return is byte-equivalent to if (!start.parentNode) return, and the "start marker gone and its end marker still in place" sentence describes the rejected alternative in the throw path. Post-fix that state stops being a success outcome and becomes exclusively a throw-path state, which makes both comments more precise, not less.
• Hydration is immune twice over. The comment walk at render-client.js:577-584 does Number(txt.slice(MARKER.length)), which is NaN for wjm-s / wjm-e, so parts[NaN] is undefined and the guard skips it.

Correction to this issue's original landmine list: the claim that "SSR emits these markers too (they round-trip through hydration)" is FALSE. packages/server/src emits no wjm-* bookends at all, and the only three creation sites are document.createComment at render-client.js:937, :1669, and :1729. The change is client-teardown-only by construction, so there is no server-side half to look for.

---

Implementation notes

Where to edit: packages/core/src/render-client.js:1705. The two-token change plus the JSDoc above is the whole source diff.

Every caller inherits the behaviour change: applyChildInnerRaw L1652, reconcileRepeat L1867 (shape-mismatch branch) and L1887 (leftover loop), teardownRepeat L1957, removeArrayItem L1992 (reached from reconcileArray and teardownArray), teardownChild L2222.

Landmines:

• moveRange (L2192) is NOT affected and must not be "fixed" to match. It appends into a DocumentFragment and breaks on n === end, so it carries the whole range including the end marker. Leave it alone.
• Keep the diff to a single token on the last line so the counterfactual revert is unambiguous.

---

Tests

Both new files import MARKER from src/html.js:36 rather than hardcoding 'wjm-' (marker-valid-attr-name.test.js already owns that fact). Each defines this helper at module scope; two consumers does not justify a shared util module:

/** Count renderer bookends under `root`, at any depth. */
function countBookends(root) {
  let s = 0, e = 0;
  const walk = (n) => {
    for (const c of n.childNodes) {
      if (c.nodeType === 8) {
        if (c.data === `${MARKER}s`) s++;
        else if (c.data === `${MARKER}e`) e++;
      } else walk(c);
    }
  };
  walk(root);
  return { s, e };
}

Every case asserts BOTH s === e (pairing) and equality against a baseline captured after the first render (accounting), PLUS the rendered output, so no test can pass by rendering nothing.

Unit: new file packages/core/test/rendering/marker-leak-on-teardown.test.js

A new file rather than the "teardown / clear paths" section at render-client.test.js:473: that file is the general renderer suite, while the folder already runs one-bug-one-file for named invariants (marker-valid-attr-name.test.js, strict-setattr-hydration.test.js, attr-mixed-later-hole.test.js, directive-commit-throw.test.js). Copy the two-phase before() from render-client.test.js:5-21 verbatim (globals first, then a dynamic import() of src/); it is load-bearing, because render-client.js reads document at module scope.

Six cases, one per distinct call site, each looping 5 cycles so a per-teardown leak is unmistakable rather than off by one:

#	Case	Call site	Asserts
U1	repeat() grown to 4, shrunk to 2, five times	L1887 leftover loop	bookends back to baseline (this carries the counterfactual), row texts, AND that a row which never left is the same node object
U2	repeat() where the same key's templateFn alternates template shape	L1867 same-key replace	bookends constant, row content
U3	plain .map() array grown and shrunk, five times	L1992 removeArrayItem	bookends back to baseline, item texts
U4	child hole alternating between two template shapes	L1652 applyChildInnerRaw	bookends constant
U5	child hole alternating between a single template and a repeat()	L2222 teardownChild plus L1957 teardownRepeat	bookends constant, both renderings correct
U6	after a render, move one row's wjm-e into a foreign element, then shrink the list	the guard itself	render() does not throw, and the comment is STILL in the foreign element

U6 pins the decision rather than the fix: it reds under an unconditional end.remove() (the comment is stolen out of a foreign parent) and under an unguarded parent.removeChild(end) (NotFoundError). Its docstring must state two things: the walk's over-removal on a desynced range is the pre-existing documented residual and is deliberately out of scope here, and linkedom may not throw on a foreign removeChild, so the survival assertion is the environment-independent arm.

Browser: new file packages/core/test/rendering/browser/marker-leak-on-teardown.test.js

Required by the project rule that a unit test is necessary but not sufficient for a browser-facing change, and substantively justified: linkedom never runs disconnectedCallback, so it cannot exercise the teardown re-entrancy the terminator decision rests on. Follow browser/directive-commit-throw.test.js (bare suite / test globals, import { assert } from '../../../../../test/browser-assert.js', direct src/ imports).

• B1: a real WebComponent rendering a repeat() list, five grow/shrink cycles driven with await el.updateComplete. Bookends return to baseline, s === e, row texts correct. The same leak proven through the component update pipeline rather than a bare render().
• B2: rows contain a custom element whose disconnectedCallback writes to the DOM. The shrink completes, surviving rows and the part marker are intact, a subsequent grow renders correctly, bookends balanced. Its docstring must say this one does NOT red on the reverted one-liner; it exists to prove the fix is safe under synchronous author code running mid-walk.

No test is engineered to red under the sentinel form. That form is not shipping, and a test whose failure depends on which node end.nextSibling happened to be is a maintenance liability. That argument lives in the source comment.

Slots: extend packages/core/test/slots/browser/record-self-heal.test.js

One test inside the existing suite, placed after the shrink test at L103-131, reusing ensureFixedShell(), tagName(), tick() and that test's component shape:

a list churning inside a slot leaves no bookends behind and does not perturb the record

Five grow/shrink cycles inside the slotted shell, then assert (i) the wjm-e count under the <slot> is back to baseline, (ii) the projected .item set is correct, (iii) slot.assignedNodes().length and the host's authored record length are unchanged from baseline. Assertion (iii) is the slot-specific value and the reason this is not a duplicate of B1: the fix strictly increases the removedNodes records the backstop sees (one extra comment per removed row), and processBackstop (slot.js:891) drops them via indexOf(node) === -1. This pins that empirically.

One scoped tweak in a file the PR does not otherwise touch

packages/core/test/directives/browser/directives-cache.test.js:24-30. Keep stripExpressionComments (a faithful lit port, and the strip is what keeps those assertions readable) but fix its docstring, which is currently wrong: it says the framework injects <!--?webjs?--> style comments, while the real markers are wjm-s / wjm-e / wjm-N. Add one sentence saying the helper hides marker accounting by construction, so marker-count assertions belong in marker-leak-on-teardown.test.js. Two lines, and it stops the next person proving marker health with a helper that cannot see it.

Counterfactual procedure

Commit the fix plus tests FIRST, then git stash push -- packages/core/src/render-client.js, run node --test packages/core/test/rendering/marker-leak-on-teardown.test.js (expect U1 red), then git stash pop and re-run green. Never git checkout the source while the fix is uncommitted, and never sed-neuter the guard. Date the claim to the commit it was proven at in the PR body.

Layers deliberately skipped

• e2e: nothing here is a network, navigation, or streaming property.
• Bun: render-client.js is browser-only (it reads document at module scope) and never runs on the Bun server. Confirmed against the gate: .claude/hooks/require-bun-parity-with-runtime-src.sh:62 matches paths on serialize|json|file-storage|listener|ts-strip|action|render-server|ssr|conditional-get|websocket|node-version|csrf|auth|session|cors|crypto|compression|body-limit|dev|stream, and packages/core/src/render-client.js matches none, so the hook will not fire and WEBJS_BUN_VERIFIED=1 is not needed.
• slots/browser/slot.test.js: marker accounting is renderer-internal and identical in light and shadow modes.

---

Docs: one sentence each, no escape hatch

.claude/hooks/require-docs-with-src.sh:59 blocks a commit staging packages/*/src with no doc surface. Do NOT reach for WEBJS_NO_DOC_GATE=1 here, because a real documented claim moves. Both surfaces carry a teardown paragraph written by #1274 and #1284, and this leak is a dimension of teardown completeness a user can observe in devtools as unbounded comment growth:

• .agents/skills/webjs/references/components.md, the paragraph beginning "Teardown is total as well."
• website/app/docs/error-handling/page.ts, the mirrored paragraph beginning "Tearing content back out is covered too".

Add one sentence to each, to the effect that a removal also takes the row's own boundary markers, so a list that grows and shrinks all day is net zero on the nodes the renderer added rather than accruing one comment per removed row for the life of the region. Same two-surface pattern #1284 and #1285 used. Invariant 11 binds both edits.

---

Risks considered, and their disposition

• R1 Stale packages/core/dist/. dist/webjs-core-browser.js exists on disk and is gitignored, so it never shows in the diff, and the browser prefers dist/ over src/ when present. Every test here imports src/ directly, so the suite proves the fix, but anyone hand-verifying in an example app against this checkout gets the old bundle until npm run build:dist --workspace=@webjsdev/core runs.
• R2 Slot backstop record volume rises monotonically. One extra removedNodes entry per torn-down row, each an O(|authored|) indexOf in slot.js:891. authored holds a host's authored children, so it is small and this is not a regression, but name it now rather than rediscover it later as "the backstop got slower after fix: removeBetween leaks its end marker on every removal #1289".
• R3 Teardown re-entrancy is newly observable. The end.parentNode === parent read now happens after every disconnectedCallback in the range has run. A callback that reparents an ancestor's children now decides whether the marker is removed. The guard refuses correctly; the sentinel form would over-remove. B2 covers this.
• R4 Insertion positions shift by one node wherever a stray used to sit. Covered by the "rendered output identical" measurements above. Worth stating in the PR so a reviewer knows it was considered rather than lucky.
• R5 The client router morph gets strictly better. reconcileChildren (router-client.js:3651-3660) reuses any comment positionally and rewrites nodeValue, so strays currently shift live-child indices. Fewer strays means better alignment. No action.

---

Verification

1. node --test packages/core/test/rendering/marker-leak-on-teardown.test.js green, then the counterfactual toggle (U1 red on revert, green after restore).
2. node scripts/run-node-tests.js green, compared against a baseline run on unpatched origin/main in the same worktree (a fresh worktree's first run produces environment noise in the blog and website suites).
3. WEBJS_BROWSERS=chromium npx wtr over the two touched browser files, then the full npm run test:browser before flipping the PR out of draft.
4. WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs plus the website boot check, since this touches packages/core.

Acceptance criteria

• removeBetween removes the end marker when it shares the captured parent
• Repeated add/remove cycles on a repeat() list leave a stable comment-node count rather than a growing one
• The same holds for a plain .map() array, which reaches removeBetween via removeArrayItem
• Surviving rows keep their DOM identity (this is teardown-only and must not perturb reconciliation)
• A light-DOM slot host with a churning list still assigns slotted content correctly, and its authored record is unchanged
• A counterfactual proves the new test reds when the one-line fix is reverted
• Tests cover both the unit and browser layers
• The two teardown doc paragraphs state that a removal takes the row's boundary markers too