fix: report message handler failures instead of transport errors - #1669
Open
Endika wants to merge 1 commit into
Open
fix: report message handler failures instead of transport errors#1669Endika wants to merge 1 commit into
Endika wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
MessageProducerHandler.do_POSTsent the200 OKresponse line before callingthe handler:
Any exception raised by the handler therefore left the client with a
half-written response.
http.serverprinted a traceback to stderr and droppedthe connection, and the Pact core reported the disconnect as
Request Failed - error sending request for url (http://localhost:<port>/_pact/message).Three unrelated failures collapsed into that one misleading message:
contentsKeyError: 'contents'error sending request for urlKeyError: '<description>'error sending request for urlRuntimeErrorerror sending request for urlThis PR makes three changes:
MessageProducerHandler.do_POSTcalls the handler before responding, andreports any exception as a
500whose body carries the exception type andmessage. This also makes it consistent with
StateCallbackHandler.do_POST,which already called its handler first.
StateCallbackHandler.do_POSTcatches handler exceptions the same way.Its ordering was already correct, but an exception still dropped the
connection rather than producing a
500.Verifier.message_handlervalidates dictionary values eagerly. The casein Misleading 'error sending request for url' when message_handler dict returns raw payload (missing 'contents' key) #1665 is a configuration error that is fully knowable when the handler is
set, so it is now raised as a
TypeErroron themessage_handler(...)linein the user's own test, rather than surfacing much later as a failed
interaction. The error names the offending message and shows the expected
envelope shape:
A request for a message with no handler now also lists the messages which
do have one.
🚨 Breaking Changes
Verifier.message_handlernow raisesTypeErrorfor a dictionary value that isneither a callable,
bytes, nor aMessagedictionary. Previously such a valuewas accepted and only failed during verification.
This only rejects handlers which could never have worked, and
TypeErrorwasalready the documented behaviour for invalid handler values. Anyone affected was
already getting a failed verification; they now get a clear error earlier
🔥 Motivation
Closes #1665.
The error the user sees points squarely at the network layer, so the natural
first steps are to check IPv4/IPv6 resolution, port binding, and the ordering of
add_transport— none of which are involved. The reporter of #1665 spentroughly six hours on it, including two false-lead fixes, before spotting the
stderr traceback beneath the FFI error.
🔨 Test Plan
Added to
tests/test_server.py:test_message_post_handler_raises— a handler raisingRuntimeErrorproduces a
500whose body contains the exception message, and the handleris called exactly once.
test_callback_post_handler_raises— the same for the state callback server.Added to
tests/test_verifier.py:test_message_handler_invalid_dict_value— a dict withoutcontentsand avalue of an unsupported type both raise
TypeErroratmessage_handlertime.
test_message_handler_unknown_message— theKeyErrorfor an unknownmessage names the messages which do have a handler.
Verified locally on Python 3.14 / Linux:
pytest tests/ --ignore=tests/compatibility_suite --ignore=tests/v2—354 passed, 2 skipped.
pytest tests/compatibility_suite— 167 passed, 12 skipped, includingtest_v3_message_producerandtest_v4_message_provider, which exercisethe real FFI message-producer path.
ruff check,ruff format --checkandmypyclean.I also reproduced the original report end-to-end against a V3 message pact. The
misleading transport error is gone; the verifier now fails the interaction on a
content-type mismatch against the
500body, and the cause is visible in thelog:
🔗 Related issues/PRs
Closes #1665.
Known limitation
The Pact core still matches the
500response body against the expectedmessage body, so the verifier output reports a content-type mismatch
(
expected a body of 'application/json' but the actual content type was 'text/html;charset=utf-8') rather than the producer error itself. Surfacing the error explanation in the verifier output would require a change inpact-reference, so it is out of scope here. Happy to raise that separately if you think it is worth doing — the eager validation in point 3 above means the most common case in #1665 never reaches the FFI at all.