Make Router#register() failure-atomic

register() previously updated #routesByName and removed committed
routes from the trie while still resolving later entries in the same
batch.  If a later resolvePathPattern() threw (e.g. an invalid path
template), the router was left with the name map and trie out of
sync: a partially replaced route could build() to the new template
while neither the old nor the new path resolved through route().

register() now resolves and builds every pending entry locally
first, so an invalid template aborts the batch before any mutation.
The trie/map are only touched once the whole batch is known to be
valid, preserving the previous router state on failure.

Added regression tests covering a throwing entry mid-batch on both a
populated router (previous route must still resolve and build) and an
empty router (no routes may leak).

#758 (comment)

Assisted-by: Claude Code:claude-opus-4-7

Author: 2chanhaeng

packages/uri-template/src/router/router.test.ts

@@ -381,6 +381,57 @@ test("Router treats re-registration as replacement", async (t) => {
   );
 });
 
+test("Router#register() is failure-atomic", async (t) => {
+  await t.step(
+    "a throwing entry leaves the previous router state intact",
+    () => {
+      const router = new Router();
+      router.add("/old/{id}", "user");
+
+      // Batch with a valid replacement for "user" followed by an invalid
+      // template.  The invalid template makes resolvePathPattern() throw
+      // mid-batch.
+      throws(() =>
+        router.register([
+          ["/new/{id}", "user"],
+          ["/bad path", "broken"],
+        ])
+      );
+
+      // The previous "user" route must still resolve and build exactly as
+      // before the failed batch: no partial mutation may survive.
+      deepEqual(router.route("/old/1"), {
+        name: "user",
+        template: "/old/{id}",
+        values: { id: "1" },
+      });
+      equal(router.build("user", { id: "1" }), "/old/1");
+
+      // The aborted replacement and the unrelated invalid name must not leak.
+      equal(router.route("/new/1"), null);
+      equal(router.has("broken"), false);
+    },
+  );
+
+  await t.step(
+    "a throwing entry does not register any routes on an empty router",
+    () => {
+      const router = new Router();
+
+      throws(() =>
+        router.register([
+          ["/users/{id}", "user"],
+          ["foo" as Path, "relative"],
+        ])
+      );
+
+      equal(router.has("user"), false);
+      equal(router.has("relative"), false);
+      equal(router.route("/users/1"), null);
+    },
+  );
+});
+
 test("Router.from() mirrors the constructor", async (t) => {
   await t.step("no arguments", () => {
     const router = Router.from();

packages/uri-template/src/router/router.ts

@@ -81,7 +81,7 @@ interface RouteEntry {
 export default class Router {
   readonly #trie: Trie<RouteEntry>;
   readonly #routesByName: Map<string, RouteEntry>;
-  #nextIndex: number;
+  #prevIndex: number = -1;
 
   /**
    * Whether to ignore trailing slashes when matching paths.
@@ -113,7 +113,6 @@ export default class Router {
 
     this.#trie = new Trie();
     this.#routesByName = new Map();
-    this.#nextIndex = 0;
     this.trailingSlashInsensitive = options?.trailingSlashInsensitive ?? false;
 
     if (routes != null) this.register(routes);
@@ -178,7 +177,7 @@ export default class Router {
     const previous = this.#routesByName.get(name);
     if (previous != null) this.#trie.remove(previous);
 
-    const entry = createRouteEntry({ index: this.#nextIndex++, name, pattern });
+    const entry = createRouteEntry({ index: this.#index, name, pattern });
 
     this.#routesByName.set(name, entry);
     this.#trie.insert(entry);
@@ -189,28 +188,35 @@ export default class Router {
    * @param routes Iterable of `[pathOrPattern, name]` pairs to register.
    */
   register = (routes: Iterable<RouterRoute>): void => {
-    const pending = new Map<string, RouteEntry>();
-
-    for (const [pathOrPattern, name] of routes) {
-      if (!pending.has(name)) {
-        const committed = this.#routesByName.get(name);
-        if (committed != null) this.#trie.remove(committed);
-      }
+    const resolved = Array.from(routes).map(
+      ([pathOrPattern, name]): [string, RouterPathPattern] => [
+        name,
+        resolvePathPattern(pathOrPattern),
+      ],
+    );
 
-      const pattern = resolvePathPattern(pathOrPattern);
-      const entry = createRouteEntry({
-        index: this.#nextIndex++,
+    const pending = new Map<string, RouteEntry>();
+    for (const [name, pattern] of resolved) {
+      pending.set(
         name,
-        pattern,
-      });
+        createRouteEntry({ index: this.#index, name, pattern }),
+      );
+    }
 
-      this.#routesByName.set(name, entry);
-      pending.set(name, entry);
+    for (const name of pending.keys()) {
+      const committed = this.#routesByName.get(name);
+      if (committed != null) this.#trie.remove(committed);
     }
 
+    for (const [name, entry] of pending) this.#routesByName.set(name, entry);
+
     this.#trie.insertAll(pending.values());
   };
 
+  get #index(): number {
+    return this.#prevIndex++;
+  }
+
   /**
    * Resolves a path name and values from a URI, if any match.
    * @param url The URI to resolve.