fix(router): guard against nil context in getResponseHeaderPropagation

[stakeswky]

Summary

Fixes #2530

The router panics with runtime error: invalid memory address or nil pointer dereference in getResponseHeaderPropagation when a fetch path completes with a nil context.Context. This happens because ctx.Value() is called without checking if ctx is nil first.

Changes

router/core/header_rule_engine.go

• Added nil guard for ctx parameter in getResponseHeaderPropagation()
• When ctx is nil, returns nil (no-op), which causes ApplyResponseHeaderRules to return early — matching the expected behavior

router/core/header_rule_engine_test.go

• Added regression test TestGetResponseHeaderPropagation_NilContext to verify nil context does not panic

Root Cause

As described in the issue, certain query plan variants can result in a fetch path completing with a nil context. The OnFinished hook in engine_loader_hooks.go then calls ApplyResponseHeaderRules which calls getResponseHeaderPropagation(ctx), and ctx.Value() panics on nil receiver.

Before / After

Before: Router panics → 500 error → recovery middleware catches it
After: Nil context treated as no-op → response returned normally (no header propagation applied)

Summary by CodeRabbit

• Bug Fixes

  • Improved header processing stability by adding a defensive check to safely handle missing context.

• Tests

  • Added a regression test to ensure header handling does not fail when context is absent.

[coderabbitai]

Walkthrough

Adds a nil-check in getResponseHeaderPropagation to return nil when passed a nil context, and adds a regression test ensuring the function does not panic and returns nil for a nil context.

Changes

Cohort / File(s)	Summary
Nil-Context Guard router/core/header_rule_engine.go	Adds an early nil check for the ctx parameter in getResponseHeaderPropagation, returning nil if ctx is nil to avoid a nil-pointer dereference when calling ctx.Value.
Regression Test router/core/header_rule_engine_test.go	Adds TestGetResponseHeaderPropagation_NilContext to assert that calling getResponseHeaderPropagation(nil) does not panic and returns nil.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

• fix(router): fix singleflight deduplication bugs and propagate response headers #2522: Changes to header propagation logic in router/core/header_rule_engine.go touching the same header propagation area.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 specifically describes the main change: adding a nil-guard in getResponseHeaderPropagation, which is the primary fix addressing the panic issue.
Linked Issues check	✅ Passed	The PR fully implements the objectives from issue #2530: it adds a nil-check for ctx in getResponseHeaderPropagation, ensures no panic occurs, treats nil context as a no-op, and includes a regression test.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #2530: the nil-guard addition and regression test are focused on preventing the panic when ctx is nil.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[endigma]

Hi @stakeswky, thanks for your contribution, we'll take a look.

cc @jensneuse

[stakeswky]

Thanks! Let me know if you have any questions or feedback.

[endigma]

Hi @stakeswky, would you mind updating this PR to the latest main and seeing if the test still fails? I believe we addressed the root cause of this last week.

[endigma]

Or, seeing that the test will absolutely still fail, can you retry your original issue?

[stakeswky]

Rebased onto the latest main just now. The test still fails on the unrebased branch — curious to see if the upstream fix resolves it. Let me know what you find!

[coderabbitai]

🧹 Nitpick comments (1)

router/core/header_rule_engine_test.go (1)

339-347: Consider moving the nil-return assertion outside the NotPanics closure.

Nesting assert.Nil inside assert.NotPanics is logically correct but slightly muddied: if a panic does occur, the closure is interrupted before assert.Nil runs and only NotPanics fails — the nil-return assertion silently becomes unreachable. Pulling it out makes both assertions independently visible in test output and easier to read.

♻️ Proposed refactor

-	assert.NotPanics(t, func() {
-		result := getResponseHeaderPropagation(nil)
-		assert.Nil(t, result)
-	})
+	var result *responseHeaderPropagation
+	assert.NotPanics(t, func() {
+		result = getResponseHeaderPropagation(nil)
+	})
+	assert.Nil(t, result)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router/core/header_rule_engine_test.go` around lines 339 - 347, The test
currently calls getResponseHeaderPropagation(nil) and asserts nil inside the
assert.NotPanics closure; change it so the call is still wrapped by
assert.NotPanics but the result is assigned to an outer scoped variable (e.g.
var result interface{}; assert.NotPanics(t, func(){ result =
getResponseHeaderPropagation(nil) })), and then perform assert.Nil(t, result)
outside the NotPanics closure so both the panic-check and the nil-return
assertion are reported independently; update
TestGetResponseHeaderPropagation_NilContext accordingly, referencing
getResponseHeaderPropagation and the test function name.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@router/core/header_rule_engine_test.go`:
- Around line 339-347: The test currently calls
getResponseHeaderPropagation(nil) and asserts nil inside the assert.NotPanics
closure; change it so the call is still wrapped by assert.NotPanics but the
result is assigned to an outer scoped variable (e.g. var result interface{};
assert.NotPanics(t, func(){ result = getResponseHeaderPropagation(nil) })), and
then perform assert.Nil(t, result) outside the NotPanics closure so both the
panic-check and the nil-return assertion are reported independently; update
TestGetResponseHeaderPropagation_NilContext accordingly, referencing
getResponseHeaderPropagation and the test function name.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between adf9941 and d617274.

📒 Files selected for processing (2)

• router/core/header_rule_engine.go
• router/core/header_rule_engine_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• router/core/header_rule_engine.go

[endigma]

Hi @stakeswky, are you able to recheck the actual query/plan that prompted you to create this PR?

We didn’t guard against nil in the function directly because nil context should never have been passed to this function in the first place, so the panic was justified.

I believe this test will still fail because it forces the nil context to be passed directly.

We’ve resolved the root issue now, please let us know and create a new issue if you run into any other problems.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.13%. Comparing base (9972295) to head (d617274).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
router/core/header_rule_engine.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2544       +/-   ##
===========================================
- Coverage   43.56%   34.13%    -9.43%     
===========================================
  Files        1030      225      -805     
  Lines      143745    24942   -118803     
  Branches     8941        0     -8941     
===========================================
- Hits        62617     8514    -54103     
+ Misses      79435    15356    -64079     
+ Partials     1693     1072      -621     

Files with missing lines	Coverage Δ
router/core/header_rule_engine.go	8.08% <0.00%> (-75.68%)	⬇️

... and 971 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[endigma]

Closing for now, please feel free to reopen as an issue if your problem is unresolved