Stop reporting stale replies as successful conversions - #6
Merged
Conversation
The cond deciding what convert/3 returns and whether the connection is reusable was buried inside the checkout closure, where it could only be exercised against a live soffice. Move it to checkout_outcome/3 so the decision can be tested directly. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A socket-level failure (soffice aborting, a recv timeout) is rescued into
conn.error by Bridge.recv_reply/1 and Bridge.send_frame/2, but neither
clears conn.reply. It keeps whatever the previous call parsed, which in
the middle of store_document_write/2 is an interface OID from
queryInterface. The has_result guard only looked at the shape of the
reply, so a failed conversion came back as {:ok, "<oid>"} and the real
error was dropped.
Callers then get a CaseClauseError on a value that looks nothing like
their requested output, and the actual failure is unrecoverable:
** (CaseClauseError) no case clause matching:
{:ok, "57ee30e3f490;gcc3[0];46d08adf17bf43b189511ea4ed49b46f"}
(urp 0.10.2) lib/urp.ex:370: URP.do_convert/2
Capture conn.error before Bridge.cleanup/1, which only ever appends to
it, and trust the reply only when the conversion itself reported no
error. Cleanup failures after a successful conversion still return the
output, as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mix hex.audit fails on EEF-CVE-2026-59249 against mint 1.9.2, pulled in as a dev-only transitive dependency of req. Unrelated to the rest of this branch, but it turns CI red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
When soffice dies mid-conversion,
URP.Pool.convert/3returns{:ok, "<some interface OID>"}instead of an error, and the real failure is lost.Bridge.recv_reply/1andBridge.send_frame/2rescue socket errors intoconn.errorbut leaveconn.replyalone, so it still holds whatever the previous call parsed. In the middle ofstore_document_write/2that is an interface OID fromqueryInterface. Thehas_resultguard only checked the shape of the reply (is_binary(result) or result == :ok), which a stale OID satisfies just as well as real output bytes.Callers see it as a case clause error on a value that looks nothing like what they asked for:
The fix is to capture
conn.errorbeforeBridge.cleanup/1(which only ever appends to it) and trust the reply only when the conversion itself reported no error. A cleanup failure after a successful conversion still returns the output, as before.a45f645fixed the sibling case where a bootstrap desktop OID leaked out of an early failure, but the same reply-shape guard is still what decides here.The first commit is a pure extraction of the decision into
checkout_outcome/4so it can be tested without a live soffice; the second is the behaviour change. Both new tests fail on the first commit and pass on the second. The third commit bumpsmintto clear an unrelatedmix hex.auditadvisory that was turning CI red.To reproduce against a real soffice, pass a
recv_timeoutshort enough to die mid-pipeline but long enough to clear the first few calls:One thing this does not address: the surfaced message is currently
"no match of right hand side value: {:error, :timeout}", becauserecv_exact/3matches on{:ok, payload} = :gen_tcp.recv(...). Worth a proper raise, but leaving it out of this change.