fix: a throw while removing repeat() leftovers keeps the removed row

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

@@ -183,6 +183,8 @@ 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.** (One reconciler is still exempt: a plain `.map()` array whose item TEMPLATE SHAPE changes in the same render that throws can strand a row. `repeat()` is the keyed path and is repaired.) 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). `repeat()` additionally repairs its key map so the map describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the failure was a permanently duplicated row); it is deliberately NOT a rebuild of the region, which would discard the node identity keyed reconciliation exists to preserve. `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.
+
 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.
 
 ## Task: client-only async data

packages/core/src/render-client.js

@@ -1078,12 +1078,19 @@ function clearInstance(inst, container) {
   for (const p of inst.bound) {
     if (p.kind === 'event') p.el.removeEventListener(p.name, p.dispatcher);
     if (p.kind === 'element') {
+      // Guarded for the same reason as the sibling in `disposeInstance`, and
+      // the stakes are higher here: this is the container-level teardown
+      // `render()` runs before a template swap, so a throw skips the rest of
+      // this loop AND the `replaceChildren()` below, leaving the old DOM in
+      // place with `host[INSTANCE]` never reassigned. Since `lastTarget` is
+      // cleared only after the write, every later swap of that container
+      // then throws at the same part, permanently.
       const prev = /** @type any */ (p).lastTarget;
       if (prev) {
         if (typeof prev === 'function') {
           try { prev(undefined); } catch { /* swallow */ }
         } else if (typeof prev === 'object') {
-          prev.value = undefined;
+          try { prev.value = undefined; } catch { /* swallow */ }
         }
         /** @type any */ (p).lastTarget = undefined;
         /** @type any */ (p).__lastEl = undefined;
@@ -1756,12 +1763,33 @@ function disposeInstance(inst) {
       // Unbind any active ref so the user observes the element being
       // removed (callback receives undefined / Ref.value cleared).
       // Mirrors lit-html's cleanup-on-disconnect for element parts.
+      //
+      // BOTH branches swallow, and lit is not the reason: lit's ref directive
+      // guards neither, so a throw there propagates. The reason is that a
+      // teardown has to be TOTAL. `lastTarget` is cleared only AFTER these
+      // writes, so a throw leaves the part still pointing at the ref and
+      // every later teardown of the same instance throws at the same line
+      // forever. It also aborts the rest of this loop, so the remaining
+      // parts keep their listeners and their refs bound. A teardown has no
+      // retry either (a commit has the COMMIT_FAILED sentinel and a next
+      // render; this does not), so there is nothing a propagated error could
+      // usefully repair.
+      //
+      // The object branch is the one this adds. The callback branch was
+      // already guarded here AND on the commit path (`applyElement` wraps
+      // every `nextTarget(...)` / `prevTarget(undefined)` call), so a
+      // throwing ref CALLBACK has always been swallowed everywhere. What was
+      // inconsistent is the object ref, guarded on neither. This makes the
+      // two agree on TEARDOWN, which is where the totality argument bites.
+      // It does NOT touch the commit path, so `applyElement`'s object-ref
+      // writes still propagate to the component boundary, which has a route
+      // for the error and a next render to repair it.
       const prev = /** @type any */ (p).lastTarget;
       if (prev) {
         if (typeof prev === 'function') {
           try { prev(undefined); } catch { /* swallow */ }
         } else if (typeof prev === 'object') {
-          prev.value = undefined;
+          try { prev.value = undefined; } catch { /* swallow */ }
         }
         /** @type any */ (p).lastTarget = undefined;
         /** @type any */ (p).__lastEl = undefined;
@@ -1828,20 +1856,36 @@ function reconcileRepeat(part, value) {
         state.map.delete(key);
       } else {
         if (existing) {
+          // Unmapped BEFORE the row is touched, for the reason spelled out
+          // in the leftover loop below: a key kept across a refused removal
+          // points at a half-removed row, and reusing that row later walks
+          // the removal off the end of the region. Same ordering, same
+          // trade, and the two have to agree or the invariant the catch
+          // relies on holds on one branch and not the other.
+          state.map.delete(key);
           disposeInstance(existing);
           removeBetween(existing.startNode, existing.endNode);
-          state.map.delete(key);
         }
         const { inst, frag } = buildDetached(/** @type any */ (tr));
         parent.insertBefore(frag, marker);
         newMap.set(key, inst);
       }
     }
 
-    // Remove any keys that remain in the old map.
-    for (const inst of state.map.values()) {
-      disposeInstance(inst);
-      removeBetween(inst.startNode, inst.endNode);
+    // Remove any keys that remain in the old map. The key leaves the map
+    // BEFORE its row is touched and the removal is in a `finally`, so at any
+    // throw point `state.map` holds exactly the leftovers this pass has not
+    // reached, and a row whose dispose threw still leaves the document.
+    // Iterating a snapshot keeps the delete obviously safe rather than
+    // relying on the reader knowing that deleting during a Map iteration is
+    // legal.
+    for (const [k, inst] of [...state.map]) {
+      state.map.delete(k);
+      try {
+        disposeInstance(inst);
+      } finally {
+        removeBetween(inst.startNode, inst.endNode);
+      }
     }
     state.map = newMap;
   } catch (err) {
@@ -1861,6 +1905,33 @@ function reconcileRepeat(part, value) {
     // reconcile against a truthful map, which repositions every row and
     // re-applies whatever the throw skipped.
     //
+    // That claim covers the REMOVAL loop as well as the walk, and only
+    // because the loop was written to earn it. It drops each key before
+    // touching that row and removes the nodes in a `finally`, so a throw
+    // mid-removal cannot merge `newMap` over a `state.map` still holding
+    // disposed, detached rows. That was the failure: the row the app DELETED
+    // stayed on screen, the survivors reordered, and a later render that
+    // re-added that key reinserted the detached instance. The invariant, at
+    // any throw point on either branch: every instance this pass has not
+    // destructively touched is described by exactly one of the two maps,
+    // `newMap` for the processed new keys and `state.map` for the leftovers
+    // not reached yet, which is what makes the merge below correct. The
+    // exception is the row named in the residual just below, whose removal
+    // refused part-way; that one is in neither map, by choice.
+    //
+    // The residual is a throw from `removeBetween` ITSELF, which only calls
+    // `removeChild` on nodes the renderer owns, so it takes a throwing DOM to
+    // reach. That row is already unmapped, so its remaining nodes stay in the
+    // document tracked by nothing and a later re-add of that key builds a
+    // second row beside them. Unmapping AFTER the removal instead would keep
+    // that key, and it is measurably worse rather than better: the row is
+    // half removed, its start marker gone and its end marker still in place,
+    // so the re-add hits the reuse branch and `moveRange` re-attaches the
+    // lone start marker AFTER the end marker. The next removal of that key
+    // then walks forward from a start that never reaches its end, taking the
+    // repeat part's own marker and every following sibling with it, and the
+    // region is dead for good. One untracked row beats a destroyed list.
+    //
     // Deliberately NOT a teardown-and-rebuild of the region. Rebuilding is
     // the obvious defensive move and it is measurably worse: it discards node
     // identity for every row, which is the exact cost keyed reconciliation
@@ -1884,9 +1955,16 @@ function reconcileRepeat(part, value) {
 
 /** @param {{ kind: 'repeat', map: Map<any, TemplateInstance> }} state */
 function teardownRepeat(state) {
-  for (const inst of state.map.values()) {
-    disposeInstance(inst);
-    removeBetween(inst.startNode, inst.endNode);
+  // Same delete-as-you-go shape as the leftover loop in `reconcileRepeat`,
+  // for the same reason: a throw part-way must not leave already-removed
+  // instances in the map. The trailing `clear()` stays as a no-op safety net.
+  for (const [k, inst] of [...state.map]) {
+    state.map.delete(k);
+    try {
+      disposeInstance(inst);
+    } finally {
+      removeBetween(inst.startNode, inst.endNode);
+    }
   }
   state.map.clear();
 }

packages/core/test/rendering/browser/directive-commit-throw.test.js

@@ -16,7 +16,7 @@
 import { html } from '../../../src/html.js';
 import { render } from '../../../src/render-client.js';
 import { repeat } from '../../../src/repeat.js';
-import { watch } from '../../../src/directives.js';
+import { watch, ref } from '../../../src/directives.js';
 import { signal } from '../../../src/signal.js';
 import { WebComponent } from '../../../src/component.js';
 
@@ -215,6 +215,77 @@ suite('directive commit throws (browser)', () => {
     assert.strictEqual(after[2], before[2]);
   });
 
+  test('a throwing ref unbind removes the row, and re-adding it builds a new element', () => {
+    // The identity facts linkedom cannot prove: the survivor is MOVED rather
+    // than rebuilt, and the resurrected key is a genuinely new element rather
+    // than the disposed instance handed back.
+    const boom = { set value(v) { if (v === undefined) throw new Error('ref-boom'); }, get value() { return null; } };
+    const refRows = (items) => html`<ul>${repeat(
+      items,
+      (it) => it.id,
+      (it) => html`<li><span ${it.id === 9 ? ref(boom) : ref({})}>${it.n}</span></li>`,
+    )}</ul>`;
+
+    render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container);
+    const before = [...container.querySelectorAll('li')];
+
+    render(refRows([{ id: 1, n: 'a' }]), container);
+    assert.deepEqual([...container.querySelectorAll('li')].map((li) => li.textContent), ['a']);
+    assert.strictEqual(container.querySelector('li'), before[0]);
+
+    render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'again' }]), container);
+    const after = [...container.querySelectorAll('li')];
+    assert.deepEqual(after.map((li) => li.textContent), ['a', 'again']);
+    assert.strictEqual(after[0], before[0]);
+    assert.ok(after[1] !== before[1], 'the disposed instance must not be resurrected');
+  });
+
+  test('a refused DOM removal keeps that row keyed, and does not duplicate it', () => {
+    // The ref-unbind case above cannot reach the removal loop's own shape,
+    // because the guard makes that step unable to throw at all. This drives
+    // the throw from the DOM removal instead, in a real browser, where node
+    // identity is the thing that separates "reused the row already there"
+    // from "built a second one beside it".
+    const idRows = (items) => html`<ul>${repeat(
+      items,
+      (it) => it.id,
+      (it) => html`<li>${it.n}</li>`,
+    )}</ul>`;
+
+    render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container);
+    const [liOne, liTwo, liThree] = [...container.querySelectorAll('li')];
+
+    const ul = container.querySelector('ul');
+    const origRemove = ul.removeChild.bind(ul);
+    ul.removeChild = (node) => {
+      if (node === liThree) throw new Error('rm-boom');
+      return origRemove(node);
+    };
+    throwsMatching(() => { render(idRows([{ id: 1, n: 'one' }]), container); }, /rm-boom/);
+    ul.removeChild = origRemove;
+
+    render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container);
+    const after = [...container.querySelectorAll('li')];
+
+    // Key 1 never left the map, so it is the same element. Key 2 left the map
+    // together with its row, so it MISSES and rebuilds rather than having a
+    // disposed instance handed back.
+    assert.strictEqual(after.filter((li) => li === liOne).length, 1);
+    assert.ok(!after.includes(liTwo), 'a removed row must not be resurrected');
+    assert.strictEqual(after.filter((li) => li.textContent === 'two').length, 1);
+
+    // `liThree` is the named residual: the DOM removal itself refused, so
+    // those nodes stayed, and its key was already dropped, so nothing tracks
+    // them. What the trade buys is that the region still RECONCILES, which is
+    // the assertion that matters and the one only a real browser settles.
+    assert.strictEqual(liThree.parentNode, ul);
+    render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container);
+    assert.deepEqual(
+      [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'),
+      ['one', 'two', 'four'],
+    );
+  });
+
   test('removing rows after recovery leaves nothing behind', () => {
     render(rows(good), container);
     throwsMatching(() => {