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

.agents/skills/webjs/references/components.md

@@ -183,7 +183,7 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i
 
 **A commit that throws leaves the directive's own state consistent, so the NEXT valid render is correct.** This matters because the corruption is otherwise silent: the renders that expose it are fully valid and log nothing after the first throw. The hole whose commit threw is marked so the next render re-applies it rather than skipping it as unchanged (its recorded value is never advanced past a throw, and would otherwise match exactly what the recovering render supplies, leaving a child region blank for good). Both list reconcilers additionally repair their own bookkeeping so it describes the DOM again, and the next render is an ordinary reconcile rather than a rebuild of the region, which would discard the node identity the reconcilers exist to preserve. `repeat()` re-unites its key map and repositions every row (the failure was a permanently duplicated row). A plain `.map()` array splices the part of its slot list the failed pass never reached back on, which matters whenever a slot is REPLACED rather than updated in place (its template shape changed, its kind changed between text, template and empty, or the array grew past its old length), since that is the branch that inserts the replacement before removing what it replaced (the failure was a stranded row that outlived even a render of an empty array). `guard()` records its new deps only once the commit succeeds, so a later render with those same deps re-renders the region instead of short-circuiting past a region the throw had blanked; `until()` advances its resolved priority only after the commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.
 
-**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it.
+**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it. Total also means a removal takes the row's own boundary markers with it, so a list that grows and shrinks all day is net zero on the nodes the renderer added, rather than accruing one invisible comment per removed row for the life of the region.
 
 Decision rules. Use `async render()` for request-time server data that should be in the first paint (the default). Add `renderFallback()` when a client re-fetch's stale content would mislead. Use `Task` / signals for genuinely client-only data (a click, viewport, live updates). For SLOW data where blocking the first byte hurts, wrap the region in `<webjs-suspense .fallback=${html\`Loading...\`}>` to stream it (the only way to show a first-paint fallback; see `client-router-and-streaming.md`). Do NOT fetch in `connectedCallback` for data knowable server-side, and do NOT prop-drill what a leaf can fetch itself.
 

packages/core/src/render-client.js

@@ -1701,16 +1701,46 @@ function nodesToFrag(nodes) {
   return frag;
 }
 
-/** @param {Node} start @param {Node} end */
+/**
+ * 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 a replacement, where there is one, is built with
+ * markers of its own. So this is a REMOVE, 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
+ * compared the end marker's live parent against `null`, so the guard could
+ * never fire and every teardown left one `wjm-e` comment in the document,
+ * unbounded for the life of the region.
+ *
+ * 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.
+ *
+ * The walk assumes the range is INTACT: it steps `nextSibling` from `start`
+ * and stops on `end`, so a range whose end no longer follows its start runs
+ * off the child list and takes the part's own marker with it. That is not
+ * something this function defends against, before or after the parent capture,
+ * and the consequence is spelled out where it bites, on `reconcileRepeat`'s
+ * catch below. The guard is the narrower promise: whatever the walk did, a
+ * marker that is somewhere else is left alone.
+ *
+ * @param {Node} start @param {Node} end
+ */
 function removeBetween(start, end) {
-  if (!start.parentNode) return;
+  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 === start.parentNode) end.parentNode?.removeChild(end);
+  if (end.parentNode === parent) parent.removeChild(end);
 }
 
 /* ================================================================

packages/core/test/directives/browser/directives-cache.test.js

@@ -22,8 +22,11 @@ import { cache } from '../../../src/directives.js';
 import { assert } from '../../../../../test/browser-assert.js';
 
 /**
- * Strip webjs marker comments (the framework injects `<!--?webjs?-->`
- * style comments around dynamic parts; tests assert plain HTML).
+ * Strip the renderer's marker comments (it brackets each instance with a
+ * `wjm-s` / `wjm-e` pair and marks each hole with a `wjm-N`; tests here assert
+ * plain HTML). Note this hides marker ACCOUNTING by construction, so it cannot
+ * be used to show markers are balanced. Assertions about that live in
+ * `rendering/marker-leak-on-teardown.test.js`, which counts them directly.
  */
 function stripExpressionComments(s) {
   return s.replace(/<!--[\s\S]*?-->/g, '');

packages/core/test/rendering/browser/marker-leak-on-teardown.test.js

@@ -0,0 +1,173 @@
+/**
+ * Real-browser assertions for the teardown that takes its own end marker.
+ *
+ * The unit suite proves the accounting under linkedom, driving the renderer
+ * with bare `render()` calls. Two things are left for a real engine. A
+ * component reaches the same teardown through the update pipeline instead, so
+ * the leak is shown on the path an app actually takes. And `removeChild` runs
+ * a custom element's `disconnectedCallback` synchronously, which means author
+ * code lands part-way through the removal walk; that is worth pinning on the
+ * engines people run rather than on a shim, and no unit case covers it.
+ *
+ * (linkedom does fire `disconnectedCallback`, so the second one is a question
+ * of fidelity rather than of capability. Do not write the stronger claim.)
+ */
+import { html } from '../../../src/html.js';
+import { MARKER } from '../../../src/html.js';
+import { repeat } from '../../../src/repeat.js';
+import { WebComponent } from '../../../src/component.js';
+
+import { assert } from '../../../../../test/browser-assert.js';
+
+let uid = 0;
+const tagName = (p) => `${p}-${uid++}`;
+
+const tick = () => new Promise((r) => queueMicrotask(() => queueMicrotask(r)));
+
+/** Count renderer bookends under `root`, at any depth. */
+function countBookends(root) {
+  let s = 0;
+  let 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 };
+}
+
+const rows = (n) => Array.from({ length: n }, (_, i) => ({ id: i, t: `row ${i}` }));
+
+suite('teardown takes its own end marker', () => {
+  test('a component churning a repeat() list stays flat on marker count', async () => {
+    const tag = tagName('marker-churn');
+    class C extends WebComponent({ items: Array }) {
+      constructor() {
+        super();
+        this.items = rows(4);
+      }
+      render() {
+        return html`<ul>${repeat(this.items, (it) => it.id, (it) => html`<li>${it.t}</li>`)}</ul>`;
+      }
+    }
+    C.register(tag);
+
+    const host = document.createElement(tag);
+    document.body.appendChild(host);
+    await host.updateComplete;
+    await tick();
+
+    try {
+      const baseline = countBookends(host);
+      assert.equal(baseline.s, baseline.e, 'baseline is paired');
+      assert.ok(baseline.e > 0, 'the list really rendered bookends');
+      const survivor = host.querySelector('li');
+
+      for (let i = 0; i < 5; i++) {
+        host.items = rows(2);
+        await host.updateComplete;
+        host.items = rows(4);
+        await host.updateComplete;
+      }
+      await tick();
+
+      const got = countBookends(host);
+      assert.equal(got.s, got.e, `bookends unpaired: ${got.s} start, ${got.e} end`);
+      assert.equal(got.e, baseline.e, 'end markers drifted from baseline');
+      assert.equal(host.querySelectorAll('li').length, 4, 'all four rows rendered');
+      assert.equal(host.querySelector('li'), survivor, 'a row that never left kept its identity');
+    } finally {
+      host.remove();
+    }
+  });
+
+  test('a disconnectedCallback running mid-walk does not derail the removal', async () => {
+    // This reds on the reverted fix like the case above (it counts bookends
+    // too), but that is not what it is for. It exists to prove the removal
+    // finishes, and the region stays renderable, while a `disconnectedCallback`
+    // runs synchronously in the middle of the walk, on the engines people
+    // actually run.
+    //
+    // The callback here writes OUTSIDE the range being torn down, which is what
+    // ordinary author code does. It deliberately does not move a node the walk
+    // is about to step onto: a range mutated underneath the walk overruns, that
+    // is true before and after this fix, and this case is not the place to
+    // claim otherwise.
+    const seen = [];
+    // A fixed tag, because a tag NAME is not a hole position in an `html`
+    // template; only attribute and child positions are.
+    const cellTag = 'marker-leak-cell';
+    class Cell extends WebComponent({ label: String }) {
+      disconnectedCallback() {
+        super.disconnectedCallback?.();
+        // Author code, running synchronously from inside `removeChild`, that
+        // writes to a container OUTSIDE the range being torn down.
+        seen.push(this.label);
+        const note = document.createElement('i');
+        note.className = 'gone';
+        note.textContent = this.label;
+        document.querySelector('#marker-sink')?.appendChild(note);
+      }
+      render() {
+        return html`<span>${this.label}</span>`;
+      }
+    }
+    Cell.register(cellTag);
+
+    const tag = tagName('marker-host');
+    class C extends WebComponent({ items: Array }) {
+      constructor() {
+        super();
+        this.items = rows(4);
+      }
+      render() {
+        return html`<ul>${repeat(
+          this.items,
+          (it) => it.id,
+          (it) => html`<li><marker-leak-cell label=${it.t}></marker-leak-cell></li>`,
+        )}</ul>`;
+      }
+    }
+    C.register(tag);
+
+    const sink = document.createElement('div');
+    sink.id = 'marker-sink';
+    document.body.appendChild(sink);
+    const host = document.createElement(tag);
+    document.body.appendChild(host);
+    await host.updateComplete;
+    await tick();
+
+    try {
+      const baseline = countBookends(host);
+      assert.equal(baseline.s, baseline.e, 'baseline is paired');
+      assert.ok(baseline.e > 0, 'the list really rendered bookends to count');
+
+      host.items = rows(1);
+      await host.updateComplete;
+      await tick();
+
+      assert.ok(seen.length > 0, 'a disconnectedCallback really ran during the removal');
+      assert.equal(host.querySelectorAll('li').length, 1, 'the shrink completed');
+      assert.equal(host.querySelector('li span').textContent, 'row 0', 'the survivor is intact');
+
+      // The region is still usable: the part marker survived the walk, so a
+      // later grow renders back into the same place.
+      host.items = rows(4);
+      await host.updateComplete;
+      await tick();
+
+      assert.equal(host.querySelectorAll('li').length, 4, 'the region still renders after the walk');
+      const got = countBookends(host);
+      assert.equal(got.s, got.e, `bookends unpaired: ${got.s} start, ${got.e} end`);
+      assert.equal(got.e, baseline.e, 'end markers drifted from baseline');
+    } finally {
+      host.remove();
+      sink.remove();
+    }
+  });
+});