Skip to content

fix(rbac): compile endpoint patterns once at load, not per rule per request (#3979) - #4047

Open
akshat-kumar-singhal wants to merge 13 commits into
gofr-dev:developmentfrom
akshat-kumar-singhal:fix/rbac-matcher-race-and-allocs-3979
Open

fix(rbac): compile endpoint patterns once at load, not per rule per request (#3979)#4047
akshat-kumar-singhal wants to merge 13 commits into
gofr-dev:developmentfrom
akshat-kumar-singhal:fix/rbac-matcher-race-and-allocs-3979

Conversation

@akshat-kumar-singhal

Copy link
Copy Markdown
Contributor

Stacked on #3934. The racy call sits in code that PR rewrites, so this branch is built on top of it.
Only the last commit — fix(rbac): compile endpoint patterns once at load, not per rule per request — is
under review here; the rest belong to #3934 and will disappear from this diff once it merges.

Description:

Fixes items 1 and 2 of #3979: the RBAC matcher's data race, its unbounded router growth, and its
per-request allocation cost. Item 3 of that issue (multi-role JWT claim arrays) is independent and
needs a semantics decision first, so it is left for a separate PR.

matchMuxPattern built a route per rule, per request, on the Config's shared muxRouter:

route := router.NewRoute().Path(pattern)

mux.Router.NewRoute appends to the router it is called on (mux@v1.8.1/mux.go:279-284), and
config.muxRouter is created once at load and shared by every request. So each pattern test both
mutated shared state without synchronization and left its entry behind permanently.

Each pattern is now compiled once at load, onto a router of its own, and stored on endpointRule.
Route.Match only reads the route and writes into the caller's RouteMatch, so a route that is never
written to again is safe to share across goroutines. The shared muxRouter is gone.

A per-scan matchContext carries the request and a single reused RouteMatch. Without it one
RouteMatch escaped per rule scanned, so cost still grew with config size.

This does not change which requests match which rules. #3979 suggests reading
mux.CurrentRoute(r).GetPathTemplate() instead. That works, but it compares config paths against
registered route templates rather than request paths — the breaking change #3935 defers to its own
item 3 and argues should not be the default, since a rule like /admin/{path:.*} deliberately spans
many registered routes today. That belongs in #3935 on its own merits, not smuggled into a bug fix.

Root cause analysis:

  • Symptom. Under concurrency, -race reports a race on mux.(*Router).NewRoute reached through
    rbac.matchMuxPattern. Separately, a long-lived process accumulates router entries without bound
    (0 → 114 routes after 100 requests, per RBAC: per-request pattern matching races and grows the router without bound; multi-role JWTs authorize nothing #3979), and authorizing one request costs hundreds to
    thousands of allocations, on the hot path of every route in the application.
  • Root cause. Config is documented as read-only after initialization — getExactEndpoint and
    GetEndpointPermission both carry the comment "Config is read-only after initialization, so no mutex
    is needed." matchMuxPattern broke that invariant: NewRoute is a mutating call, and it was being
    made on a shared field on every request. The pattern compilation it performed is fully determined by
    the config, so it never needed to happen at request time at all.
  • Why it wasn't caught. The existing suites drive requests sequentially, and the package had no
    -race concurrency test and no benchmark. A sequential test cannot observe either failure: the race
    needs two goroutines, and the growth is invisible without counting routes or allocations.
  • Fix. Compile at load onto a per-pattern router; store the compiled *mux.Route on the rule; reuse
    one RouteMatch per scan. matchesHTTPMethod is deleted — endpointRule.matchesMethod was its only
    caller and allocated a one-element slice per rule per request to reach it, and its list handling is
    unreachable now that buildEndpointRules expands methods into one rule each.
  • Prevention. Three regression tests added, described below. The concurrency tests fail on the base
    branch with exactly the stack RBAC: per-request pattern matching races and grows the router without bound; multi-role JWTs authorize nothing #3979 reported.

Verification:

TestConfig_resolve_concurrent and TestMiddleware_concurrent drive 50 goroutines × 20 requests
through the resolver and through the mounted middleware. Both fail -race on the base branch and pass
here; go test -race -count=2 ./pkg/gofr/rbac/ is clean.

BenchmarkConfig_resolve (Apple M4), before → after:

rules in config before after
6 36,730 ns/op, 1,064 allocs/op 927 ns/op, 6 allocs/op
21 131,730 ns/op, 3,924 allocs/op 1,026 ns/op, 6 allocs/op
51 373,669 ns/op, 9,684 allocs/op 1,717 ns/op, 6 allocs/op

TestConfig_resolve_allocations guards that flatness. It compares a small config against larger ones
rather than asserting an absolute count: the race detector adds three allocations, so a bound tuned
without -race fails CI with it and one tuned for -race is slack. The property the fix establishes
is that per-request cost does not scale with rule count, so that is what the test measures.

Package coverage is 94.3%.

Breaking Changes (if applicable):

None. Matching semantics are unchanged, and no exported API is touched — muxRouter,
matchMuxPattern, matchesEndpointPattern and matchesHTTPMethod are all unexported.

Additional Information:

No new dependencies. endpoint_matcher_test.go shrinks because TestMatchesHTTPMethod is removed with
the function it covered (superseded by TestEndpointRule_matchesMethod) and TestMatchMuxPattern is
retargeted at compilePattern as TestCompilePattern, which also gains cases for literal paths,
empty paths and patterns mux cannot compile.

Three pre-existing noctx lint findings remain in endpoint_matcher_test.go (httptest.NewRequest
rather than NewRequestWithContext). They are present on development, untouched here, and part of the
mechanical golangci-lint backlog.

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

akshat-kumar-singhal and others added 7 commits August 16, 2026 01:00
…deterministically

An endpoint declared with "methods": ["*"] — or with methods omitted, which
defaults to the same — never matched any request, so the route it was written
to guard was not guarded. Keys were stored under the declared method (*:/path)
while lookups only ever probed the request's method (GET:/path).

Both entry points now resolve through one ordered rule list: exact-path lookups
stay O(1), and overlapping patterns resolve most-specific-first (literal >
variable > catch-all, explicit method > "*") instead of by Go map iteration
order, which varied per request. Patterns are compiled at load, so a constraint
mux cannot parse fails the config instead of silently never matching.

Unmatched routes still pass through; changing that is tracked separately.

Fixes gofr-dev#3808
…le patterns instead of failing

Review round on gofr-dev#3934.

pathSpecificity scored {id} and {id:[0-9]+} identically, so sort.SliceStable fell
through to declaration order and the same two entries swapped in the config enforced
a different permission - the order-sensitivity most-specific-wins exists to avoid.
A constrained variable admits a strict subset of what a free one admits, so it gets
its own level between segVariable and segLiteral.

The load-time mux compile check no longer aborts startup. GoFr logs and stays up on
a config defect (gofr-dev#2378); a bugfix should not reverse that. The check moves to
logUncompilablePattern, which names the pattern at error level and states that the
endpoint is not enforced. Brace-balance validation keeps its existing fatal behaviour.

matchEndpoint is deleted - it was reachable only from its own tests, rebuilt and
re-sorted the rule list per call and skipped the exact-key fast path, which is the
duplicate-matcher shape this PR exists to remove. Its eight tests become one
table-driven test against config.resolve.

Docs: the fourth specificity level, the two cases where ordering falls back to
declaration order, the method-drop-out gotcha in the worked example, and the
uncompilable-pattern log line in Troubleshooting.
…il-open paths

pathSpecificity split the pattern on "/" before checking braces, so the "/" inside a
constraint like {p:[a-z/]+} shattered the segment into fragments that each scored as a
literal - the loosest pattern in a config ranked as the most specific one. Split at brace
depth zero instead, and treat any constraint containing "/" as a catch-all: it spans
segments exactly as {p:.*} does. The test is deliberately over-broad ({id:[0-9]+/} is
scored as a catch-all too); it only ever moves a pattern down the ordering, so a rule can
lose to a narrower one but never shadow it.

Two pre-existing fail-open paths, both reproduced against development first:

- storeEndpointMapping only ever set publicEndpointsMap[key], never cleared it, so a
  public entry followed by a protected one for the same (method, path) left the protected
  endpoint in one map and isPublic=true in the other - the route stayed unauthenticated.
  A duplicate declaration now overwrites in full, public flag and permissions included.
- An equal-specificity tie fell through to declaration order, letting a public rule
  shadow an overlapping protected one. Ties now prefer the rule that requires
  permissions; declaration order decides only between rules of the same public-ness.

Tests: table tests for pathSpecificity and compareSpecificity (including the antisymmetry
sort.SliceStable needs), duplicate declarations in both orders, protected-beats-public in
both orders, and same-depth ordering cases that reach the ordered scan rather than being
answered by the exact-key fast path. Every fix was mutation-verified by reverting it.

Docs: the "ordering is not decided" section now states the tie-break and the conservative
catch-all rule, and a new section covers duplicate declarations.
…equest (gofr-dev#3979)

matchMuxPattern called router.NewRoute().Path(pattern) on the Config's shared
muxRouter for every rule of every request. mux.Router.NewRoute appends to the
router it is called on, so each pattern test mutated shared state without
synchronization and left the route behind permanently: a data race under any
concurrency, and a router growing by one entry per rule per request for the
life of the process.

Each pattern is now compiled once at load onto a router of its own and stored
on endpointRule. Route.Match only reads the route, so the result is safe to
share. A per-scan matchContext holds the request and one reused RouteMatch,
keeping per-request allocations flat rather than growing with config size
(1,064 -> 6 allocs at 6 rules; 9,684 -> 6 at 51).

Does not change which requests match which rules.
@akshat-kumar-singhal
akshat-kumar-singhal force-pushed the fix/rbac-matcher-race-and-allocs-3979 branch from f2e4033 to 2f38826 Compare August 21, 2026 10:10
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.

1 participant