[simpleqt]

fix(tree): fall back to skipped wildcard when a param branch dead-ends

Relation to open PRs in the same area

The recently opened #4891/#4894/#4906 (trailing-slash redirect preservation when the walk rolls back to a skipped wildcard) touch the same skippedNodes rollback machinery in tree.go but fix a different behavior: they keep the TSR redirect during an existing rollback, while this PR adds the missing rollback at two dead ends that returned 404 directly. The changes are complementary; a small textual overlap in getValue is possible and trivially resolvable.

Problem

getValue (tree.go) records a skippedNode whenever it prefers a static child over a wildcard sibling, and rolls back to it when the static branch fails. Two dead ends inside the case param: block bypassed that rollback and returned not-found directly:

1. more path remains but the param node has no child to descend into (// ... but we can't);
2. the param node consumed the whole remaining path but has no handler registered (the len(n.children) == 1 TSR check falls through).

Repro

router := gin.New()
h := func(c *gin.Context) { c.String(200, "hit %s", c.FullPath()) }
router.GET("/:p0/:p1", h)
router.GET("/bc/:p1/a", h)
// GET /bc/bc -> 404 page not found   (before the fix)

The walk prefers the static bc branch, dead-ends on the handler-less :p1 node (its handler lives on the child /a), and previously gave up with 404 instead of retrying the skipped /:p0 branch. GET /bc/bc should match /:p0/:p1 with p0=bc, p1=bc — exactly like GET /x/y already does. The second dead end is reachable with the route pair /:p0/:p1/:p2 + /bc/:p1 via GET /bc/bc/2 (should match /:p0/:p1/:p2).

Root cause

The two param-case dead ends return value without consulting skippedNodes, inconsistent with the three other dead ends of the same walk (the !n.wildChild block, the path == prefix block, and the final "Nothing found" block), which all roll back to the last valid skippedNode.

Fix

Both dead ends now run the same rollback loop the other dead ends already use (pop the last skipped node, restore path/n, truncate params to paramsCount, continue walk), guarded by !value.tsr && path != "/" like the existing sites. The rolled-back node replica intentionally carries no indices, so a retry cannot re-enter the static branch or re-push the same entry; the walk therefore stays bounded and cannot loop.

Tests

TestTreeParamFallbackAfterStaticDeadEnd (tree_test.go) covers each dead end in table style with param-value assertions:

• param consumed the whole path but has no handler — routes /:p0/:p1 + /bc/:p1/a; expects GET /bc/bc to match /:p0/:p1 (and /bc/bc/a, /x/y unchanged);
• more path remains but the param node has no child — routes /:p0/:p1/:p2 + /bc/:p1; expects GET /bc/bc/2 to match /:p0/:p1/:p2 (and /bc/bc, /x/y/2 unchanged).

Red/green evidence

Base commit 0f09c3a (master, 2026-10-10), Windows/amd64, Go 1.27.1.

RED — pristine base worktree + test file only (go test -run TestTreeParamFallbackAfterStaticDeadEnd -v .):

--- FAIL: TestTreeParamFallbackAfterStaticDeadEnd (0.00s)
    --- FAIL: TestTreeParamFallbackAfterStaticDeadEnd/param_consumed_the_whole_path_but_has_no_handler (0.00s)
    --- FAIL: TestTreeParamFallbackAfterStaticDeadEnd/more_path_remains_but_the_param_node_has_no_child (0.00s)
FAIL

GREEN — with the fix (go test -run TestTreeParamFallbackAfterStaticDeadEnd -v .):

--- PASS: TestTreeParamFallbackAfterStaticDeadEnd (0.00s)
    --- PASS: TestTreeParamFallbackAfterStaticDeadEnd/param_consumed_the_whole_path_but_has_no_handler (0.00s)
    --- PASS: TestTreeParamFallbackAfterStaticDeadEnd/more_path_remains_but_the_param_node_has_no_child (0.00s)
PASS

Package-level: on this Windows machine the suite has four pre-existing failures on unchanged master (TestSaveUploadedFileWithPermission, TestSaveUploadedFileWithPermissionFailed, TestSaveUploadedFileToExistingDir, TestFileDescriptor). go test ./... reports exactly those 4 --- FAIL lines with the fix and the same 4 plus TestTreeParamFallbackAfterStaticDeadEnd without it — no other test changes behavior. go vet . passes; golangci-lint run ./... (v2.6.2, repo .golangci.yml) reports 0 issues.

Disclosure

This fix and its regression test were produced with AI assistance; the reproductions, red/green runs and lint results shown above were executed locally against master at 0f09c3a.

Checklist

• Open your pull request against the master branch.
• All tests pass in available continuous integration systems (CI runs on the PR).
• Tests are added or modified as needed to cover code changes.
• New feature documented in docs/doc.md — not applicable, bug fix.

Relation to open PRs

• fix(tree): roll back to skipped wildcards at param route dead ends #4783 targets the same fallback path. Its TestZZ4783TreeParamFallback fails on master and passes here; its gin.go hunk no longer appears necessary because master resets *skippedNodes on entry to getValue (tree.go:428, from fix: reset skipped-nodes stack on getValue entry to prevent slice overflow panic [#4818] #4819). Happy to fold this work into fix(tree): roll back to skipped wildcards at param route dead ends #4783 instead.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.66%. Comparing base (0f09c3a) to head (82a6525).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4919   +/-   ##
=======================================
  Coverage   98.65%   98.66%           
=======================================
  Files          48       48           
  Lines        3188     3210   +22     
=======================================
+ Hits         3145     3167   +22     
  Misses         43       43           

Flag	Coverage Δ
--ldflags="-checklinkname=0" -tags sonic	98.65% <100.00%> (+<0.01%)	⬆️
-tags go_json	98.58% <100.00%> (+<0.01%)	⬆️
-tags nomsgpack	98.64% <100.00%> (+<0.01%)	⬆️
go-1.26	98.34% <100.00%> (+0.01%)	⬆️
go-1.27	98.68% <100.00%> (+0.01%)	⬆️
macos-latest	98.66% <100.00%> (+<0.01%)	⬆️
ubuntu-latest	98.66% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Tales-of-zuna]

Thanks for digging into this! I reproduced both 404s on master (GET /bc/bc with /:p0/:p1 + /bc/:p1/a, and GET /bc/bc/2 with /:p0/:p1/:p2 + /bc/:p1), so the bug is real.

Heads-up, though: #4783 (open since August) already fixes these same two dead ends, with the same route pairs and a test of the same name (TestTreeParamFallbackAfterStaticDeadEnd). It also moves the rollback loop into a rollbackToSkippedNode helper, so the five call sites share one copy. It still merges cleanly with current master, and its tests fail on master's tree.go and pass with the fix.

The one difference I found is the extra path != "/" guard here. As far as I can tell it can't change the result at either site: at the first dead end, path == "/" already sets value.tsr, and at the second, end == len(path) means path can't be "/".

Since the "Relation to open PRs" section doesn't mention #4783, it might be worth looking at it. A review or 👍 there would probably help this fix land sooner than a second PR would.

[simpleqt]

Thank you for checking both setups - confirmed here as well: /bc/bc with /:p0/:p1 + /bc/:p1/a, and /bc/bc/2 with /:p0/:p1/:p2 + /bc/:p1 both return 404 on 0f09c3a and 200 with the correct FullPath at this head (82a6525), and #4783's /bc/bc/bc variant behaves the same on this branch. The package suite on this branch shows only the four pre-existing Windows upload/permission failures that master has too.

You are right about path != "/": stripping both guards leaves a diff that is byte-identical to #4783's call sites, and the tests behave the same either way - the condition is inert at both places (tree.go 500-502, 532, 540-541, 558, 564-570). I have kept the checks as written rather than remove them here, so the decision stays yours rather than being folded into this PR.

On #4783: I added it to the "Relation to open PRs" section as you suggested. Two things I verified while doing so, in case they are useful for the decision to back it rather than merge a second PR here:

• its TestZZ4783TreeParamFallback passes on this branch and fails on master with the handle mismatch, so the behaviour it targets is covered here;
• its gin.go hunk looks redundant now: TestZZ4783RouteNotAllowedDoesNotReuseSkippedNodes passes on master already, because master resets *skippedNodes on entry to getValue (tree.go:428, from fix: reset skipped-nodes stack on getValue entry to prevent slice overflow panic [#4818] #4819). That is the basis for my suggestion, not a claim about which PR should land.

Happy to rebase onto #4783, drop the parts of this branch it covers, or close this one in favour of it - whatever is least work for you.