Skip to content

feat(server): fail fast on duplicate route registration - #761

Merged
gaborage merged 1 commit into
mainfrom
feature/route-duplicate-fail-fast
Jul 23, 2026
Merged

feat(server): fail fast on duplicate route registration#761
gaborage merged 1 commit into
mainfrom
feature/route-duplicate-fail-fast

Conversation

@gaborage

@gaborage gaborage commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Summary

Echo's engine is constructed with AllowOverwritingRoute: true, so two modules registering the same method + full path silently collide — last one wins, the first module's handler is dead on arrival, and nothing surfaces unless the shadowed route is exercised. For a modular framework this is exactly the class of bug the Fail Fast principle exists to catch (duplicate module names and duplicate messaging triples already fail startup; HTTP routes did not).

This PR adds per-server route-collision tracking at the framework's own registration seam and a startup sweep that aborts with one aggregate error naming every collision and both registrants:

duplicate route registration (2 conflict(s))
GET /v1/events — first: createEvent (github.com/acme/svc/modules/events), duplicate: legacyCreateEvent (github.com/acme/svc/modules/legacy)
POST /v1/orders — first: createOrder (github.com/acme/svc/modules/orders), duplicate: submitOrder (github.com/acme/svc/modules/checkout)

Design decisions

  • Per-Server tracker, not the global DefaultRouteRegistry — the registry is a package-level global; dedup there would cross-contaminate parallel tests running multiple servers. The tracker lives on Server and threads through every routeGroup (including nested Group() children).
  • Recording at both registration seams — raw RouteRegistrar.Add derives provenance locally; the typed path threads the already-built descriptor's HandlerName/Package through the unexported echoAdder seam (single implementer, zero blast radius). A shared addRoute funnel was considered and deliberately skipped as non-blocking indirection — the invariant is pinned by tests instead.
  • Startup sweep via optional interface assertion (app.checkRouteConflicts), mirroring applyGlobalMiddleware — the exported ServerRunner interface gains no method, so the apidiff gate stays green and existing test fakes keep compiling. The sweep runs synchronously in prepareRuntime before the server starts: a deterministic registration bug fails fast, not as an async post-start crash.
  • Health/ready probes are recorded explicitly in New() — they register directly on the engine (not through a routeGroup), so New() records their four method/path pairs in the tracker; a module claiming GET /health fails startup like any other collision.
  • Attribution is HandlerName + caller PackageRouteDescriptor.ModuleName is populated by no registration path (route-log attribution uses registration-order spans), so the error reports what is actually available on both typed and raw paths.
  • Rejected alternative: flipping echo to AllowOverwritingRoute: false (viable since echo v5.3.1 / Implicitly registered group routes should be allowed overwritten in default routes labstack/echo#3049) — it would require hand-constructing the router, surface as an echo-flavored panic through the ADR-034 boundary, and cannot produce provenance-rich errors.
  • Excluded by design: param-name-differing templates (/users/:id vs /users/:uid) — distinct strings; echo's radix-tree behavior governs there. Documented in wiki/startup_defaults.md.
  • No disable knob — a colliding route is always a startup-blocking bug, never a warning.

Adoption note

Apps with a latent duplicate registration will now fail startup with duplicate route registration … naming both registrants — that is the intended fail-fast on a previously silent bug. Fix by removing or renaming the colliding route. No exported API changed (additive only: Server.RouteConflicts(), server.RouteConflict, server.RouteRegistrant). A wiki/migrations.md hop atom was deliberately deferred: the file's structure keys on a release-version edge that release-please assigns post-merge; the adopt note lives in wiki/startup_defaults.md#duplicate-route-detection.

Testing

  • Tracker unit coverage: duplicate/method-disjoint/nil-tracker (TestRouteConflictTrackerRecordsDuplicate).
  • Cross-group + nested-group propagation on a real Server (TestServerRouteConflictsAcrossGroups).
  • Typed + raw registration both tracked, provenance threading pinned (TestRouteConflictTypedAndRawBothTracked).
  • Probe shadowing detected (TestRouteConflictDetectsProbeCollision).
  • Startup sweep: report/aggregate/skip-path table on a real server injected white-box (TestCheckRouteConflictsAggregatesAndSkips); mutation-verified (removing the record call or neutering the sweep fails the pinning tests).
  • Alloc guards unaffected — all new work is registration-time only; the per-request hot path (ADR-026) is untouched.

Pre-push gates: /simplify (2 applied: errors.Join aggregate matching the in-package idiom; test-table consolidation), /security-audit (clean — gosec 0, no new input surface, error carries compile-time metadata only), CodeRabbit CLI on the final diff (0 findings). make check green after every mutation.

Closes #759

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added duplicate HTTP route detection for matching methods and paths.
    • Applications now fail during startup when conflicting routes are registered, with an aggregate error identifying both handlers.
    • Detection includes nested groups and health/readiness probe collisions.
  • Documentation

    • Documented duplicate route behavior, coverage, and startup error handling.

Echo's engine hard-codes AllowOverwritingRoute: true, so two modules
registering the same method+full path silently collide (last one wins)
with no error and no warning. Add a per-Server route conflict tracker
threaded through routeGroup/addEcho and sweep it after RegisterRoutes
in app.prepareRuntime, aborting startup with an aggregate error naming
every collision and both registrants (handler name + caller package).

Health/ready probes bypass the registrar and stay excluded; param-name
-differing templates (/users/:id vs /users/:uid) are distinct strings
and are not detected, matching echo's own radix-tree behavior.

Documented in wiki/startup_defaults.md. Skipped a wiki/migrations.md
hop atom per the plan's escape hatch: the file's structure requires a
known target-version edge, which isn't knowable pre-release.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c273a5c9-4779-47cc-ade8-2aec072b27ab

📥 Commits

Reviewing files that changed from the base of the PR and between 0a189fe and ab8c189.

📒 Files selected for processing (9)
  • CLAUDE.md
  • app/lifecycle.go
  • app/lifecycle_test.go
  • server/handler.go
  • server/route_conflicts.go
  • server/route_conflicts_test.go
  • server/route_registrar.go
  • server/server.go
  • wiki/startup_defaults.md

Walkthrough

HTTP route registrations now track duplicate method-and-path collisions across typed, raw, grouped, nested, and probe routes. Application startup aggregates detected conflicts and fails before maintenance loops begin. Tests and documentation cover the behavior.

Changes

Duplicate route conflict detection

Layer / File(s) Summary
Route tracking and registration metadata
server/route_conflicts.go, server/route_registrar.go, server/handler.go
Route metadata and duplicate method-plus-full-path conflicts are tracked across typed, raw, grouped, and nested registrations.
Server wiring and startup validation
server/server.go, app/lifecycle.go
Servers track probe and registrar routes, expose conflict snapshots, and abort startup with aggregated conflict errors.
Conflict coverage and documented behavior
server/route_conflicts_test.go, app/lifecycle_test.go, wiki/startup_defaults.md, CLAUDE.md
Tests and documentation cover conflict aggregation, route sources, probe collisions, unsupported servers, and startup behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Possibly related PRs

Suggested labels: enhancement, area/server

Poem

A rabbit hops where routes align,
Tracking paths in neat design.
If two claim one method’s door,
Startup says, “Not anymore!”
Names are joined, the logs grow clear—
Safe routes burrow far from fear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely matches the main change: fail-fast handling for duplicate server route registrations.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/route-duplicate-fail-fast

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@gaborage
gaborage merged commit 7dbc7e0 into main Jul 23, 2026
25 checks passed
@gaborage
gaborage deleted the feature/route-duplicate-fail-fast branch July 23, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server: fail fast on duplicate route registration (same method+path)

1 participant