Repository navigation
fix(event_handler): clear context when route resolution raises - #8539
leandrodamascena merged 2 commits into
Conversation
resolve() and resolve_async() only cleared the routing context after a response was built, so an exception escaping a route handler left the previous invocation's context in place, including the cached Request. A warm Lambda container could then hand that stale Request to the next invocation. Clear the context in a finally block instead. The ALB response size validation no longer needs its own clean-up on error, and the JWT middleware test now expects the whole context to be gone after a failing handler rather than only the claims.
powertools-for-aws-oss-automation
Bot
added
the
size/M
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
leandrodamascena
left a comment
leandrodamascena
left a comment
There was a problem hiding this comment.
Nice catch @vyrnsynx, and thanks @rtandy for the detailed report! I was able to reproduce the stale Request across invocations and confirm the fix. I also checked that exception handlers, middlewares and not_found still see the context during resolution, so this only changes what was already a bug. Approving!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8539 +/- ##
===========================================
- Coverage 96.91% 96.91% -0.01%
===========================================
Files 317 317
Lines 15961 15959 -2
Branches 1411 1411
===========================================
- Hits 15469 15467 -2
Misses 349 349
Partials 143 143 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
073b5ba
into
aws-powertools:develop
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
2 participants
Issue number: closes #8538
Summary
Changes
resolve()andresolve_async()only cleared the routing context after the response was built, so when a route raised an unhandled exception the context (including the cached_request) carried over to the next invocation.finallyblock in bothresolve()andresolve_async().except Exception: self.clear_context()in the ALB response size validation, sinceresolve()/resolve_async()now handle it. The existing ALB tests still check the context is empty on each error path.Requestinjection across two failing invocations.test_failed_handler_cannot_leak_claims_into_later_invocationsexpectedapplication_valueto still be in the context after the handler raised. It now expects the context to be empty. I left the JWT middleware's ownfinallythat popsclaimsas it is.User experience
Before: after an unhandled exception, the next invocation on the same warm container could get the previous
Requestfromapp.request,Requestinjection orDepends(), plus any data added withappend_context. The repro in #8538 fails on its second case.After: the context is cleared after every invocation, whether the route returns or raises, as the docs say. The #8538 repro passes.
Tested locally with the full
pytest -m "not perf" --ignore tests/e2e, thetest_with_only_required_packagesandtest_with_auth_required_packagesnox sessions,ruff format --check,ruff check,mypyandty.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.