Skip to content

Clear the request ID on request_finished, rather than on the way "out" of the middleware#81

Merged
j4mie merged 1 commit into
masterfrom
delete-request-id-on-request-finished
Mar 12, 2026
Merged

Clear the request ID on request_finished, rather than on the way "out" of the middleware#81
j4mie merged 1 commit into
masterfrom
delete-request-id-on-request-finished

Conversation

@j4mie

@j4mie j4mie commented Mar 12, 2026

Copy link
Copy Markdown
Member

#75 refactored RequestIDMiddleware and changed when local.request_id is cleared. Before that change, cleanup only happened on the LOG_REQUESTS=True path, so with the default LOG_REQUESTS=False the request ID remained available after the middleware returned. That behaviour was not documented and appears to have been an unintended side effect of where the cleanup lived, since request-ID lifetime should not depend on whether request logging is enabled. It wasn't quite a bug, but it was certainly a bit odd. After #75 the request_id was cleared unconditionally.

However, issue #80 showed that some users relied on that "bug", as they are reading log_request_id.local.request_id from outer WSGI wrapper code after django_application(...) returned (in that case, to copy it into a uWSGI log variable).

This change moves cleanup to Django’s request_finished signal. That keeps the request ID available for the rest of the request/response lifecycle, including outer WSGI wrapper code that runs immediately after the Django application returns, while still ensuring the value is cleared at the end of the request instead of leaking indefinitely.

…d rather than on the way "out" of the middleware
@dabapps-robot dabapps-robot self-requested a review March 12, 2026 11:02

@dabapps-robot dabapps-robot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall — the change makes sense, and moving the cleanup to request_finished feels like a good way to preserve the request ID for the full response lifecycle without leaving it around indefinitely. The test also captures the intended behaviour nicely.

One small thing I’d consider: giving the signal receiver a dispatch_uid. Because this receiver is connected at import time, a dispatch_uid helps protect against duplicate registrations in environments where modules may be imported more than once (eg autoreload / some test setups).

Something along these lines:

@receiver(request_finished, dispatch_uid="log_request_id_clear_request_id")
def _clear_request_id(**kwargs):
    ...

Other than that, this looks ready to merge to me.

@j4mie

j4mie commented Mar 12, 2026

Copy link
Copy Markdown
Member Author

@dabapps-robot I think that's an edge case, and the receiver is so lightweight that it doesn't really matter if it's called multiple times.

@j4mie

j4mie commented Mar 12, 2026

Copy link
Copy Markdown
Member Author

@RealOrangeOne do you want to have a look at this as you authored #75? 🙂

@j4mie j4mie merged commit 9d0dc32 into master Mar 12, 2026
11 checks passed
@j4mie j4mie deleted the delete-request-id-on-request-finished branch March 12, 2026 11: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.

3 participants