diff --git a/CHANGELOG.md b/CHANGELOG.md index a3998e1..0d7cdbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Fixed + +- Report the real error when a conversion fails at the socket level, instead of returning the + stale interface OID left in the reply as if it were the converted output. + ## [v0.10.2] - 2026-07-14 ### Changed diff --git a/lib/urp/pool.ex b/lib/urp/pool.ex index 60e8d83..2a41044 100644 --- a/lib/urp/pool.ex +++ b/lib/urp/pool.ex @@ -108,30 +108,48 @@ defmodule URP.Pool do |> load_input(input, io_in) |> store_output(store_opts, sink, io_out) + # Bridge.cleanup/1 only ever appends to conn.error, so this is the last + # point at which we can tell a failed conversion from a failed cleanup. + convert_error = conn.error result = conn.reply conn = Bridge.cleanup(conn) conn = %{conn | max_frame_size: default_max_frame_size, recv_timeout: default_recv_timeout} - # Stream-based input registers an XInputStream at a fixed OID cache slot. - # soffice's URP cache doesn't fully reset on reuse, producing truncated - # documents on subsequent stream loads. Discard the connection to force - # a fresh handshake. File-based I/O reuses connections normally. - reusable = not stream_input? and is_nil(conn.error) - has_result = is_binary(result) or result == :ok + case checkout_outcome(result, convert_error, conn.error, stream_input?) do + {value, :reuse} -> {value, {:ok, reset_conversion_state(conn)}} + {value, :discard} -> {value, :closed} + end + end) + end + + @doc false + @spec checkout_outcome(term(), String.t() | nil, String.t() | nil, boolean()) :: + {:ok | {:ok, binary()} | {:error, String.t()}, :reuse | :discard} + def checkout_outcome(result, convert_error, error, stream_input?) do + # Stream-based input registers an XInputStream at a fixed OID cache slot. + # soffice's URP cache doesn't fully reset on reuse, producing truncated + # documents on subsequent stream loads. Discard the connection to force + # a fresh handshake. File-based I/O reuses connections normally. + reusable = not stream_input? and is_nil(error) + + # A socket-level failure leaves conn.reply holding whatever the last call + # parsed — typically an interface OID — so the shape of the reply alone + # cannot tell output from leftovers. Only trust it if the conversion itself + # reported no error. + has_result = is_nil(convert_error) and (is_binary(result) or result == :ok) - cond do - reusable -> - {wrap_result(result), {:ok, reset_conversion_state(conn)}} + cond do + reusable -> + {wrap_result(result), :reuse} - has_result -> - # Conversion succeeded but close/cleanup failed (e.g. soffice drops - # the connection after stream→stream). Return the result, discard conn. - {wrap_result(result), :closed} + has_result -> + # Conversion succeeded but close/cleanup failed (e.g. soffice drops + # the connection after stream→stream). Return the result, discard conn. + {wrap_result(result), :discard} - true -> - {{:error, conn.error}, :closed} - end - end) + true -> + {{:error, error}, :discard} + end end defp normalize_io(:file), do: {:file, :file} diff --git a/mix.lock b/mix.lock index 7c2456f..fc035f8 100644 --- a/mix.lock +++ b/mix.lock @@ -12,7 +12,7 @@ "makeup_elixir": {:hex, :makeup_elixir, "1.0.1", "e928a4f984e795e41e3abd27bfc09f51db16ab8ba1aebdba2b3a575437efafc2", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "7284900d412a3e5cfd97fdaed4f5ed389b8f2b4cb49efc0eb3bd10e2febf9507"}, "makeup_erlang": {:hex, :makeup_erlang, "1.1.0", "835f7e60792e08824cda445639555d7bf1bbbddb1b60b306e33cb6f6db24dc74", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "1cd6780fb1dd1a03979abaed0fe82712b0625118fd5257d3ebbf73f960c73c3c"}, "mime": {:hex, :mime, "2.0.7", "b8d739037be7cd402aee1ba0306edfdef982687ee7e9859bee6198c1e7e2f128", [:mix], [], "hexpm", "6171188e399ee16023ffc5b76ce445eb6d9672e2e241d2df6050f3c771e80ccd"}, - "mint": {:hex, :mint, "1.9.2", "6e89e698d69cc29be001afd02c2cf4bae2d0994efe69a2ced7aab440d71584f9", [:mix], [{:castore, "~> 0.1.0 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: true]}, {:hpax, "~> 0.1.1 or ~> 0.2.0 or ~> 1.0", [hex: :hpax, repo: "hexpm", optional: false]}], "hexpm", "d8e952b432fdac2321f570d29a68b9eb2664dc80a7a2ef9464531a5425abf222"}, + "mint": {:hex, :mint, "1.9.3", "3337184d69179695c7a9f1714d92c11e629d36c8c037a21cf490131d3d150554", [:mix], [{:castore, "~> 0.1.0 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: true]}, {:hpax, "~> 0.1.1 or ~> 0.2.0 or ~> 1.0", [hex: :hpax, repo: "hexpm", optional: false]}], "hexpm", "5f7c9342480c069dbbc4eeac3490303c9e01870ff01a7f1d29b6107054fc1e74"}, "nimble_options": {:hex, :nimble_options, "1.1.1", "e3a492d54d85fc3fd7c5baf411d9d2852922f66e69476317787a7b2bb000a61b", [:mix], [], "hexpm", "821b2470ca9442c4b6984882fe9bb0389371b8ddec4d45a9504f00a66f650b44"}, "nimble_ownership": {:hex, :nimble_ownership, "1.0.2", "fa8a6f2d8c592ad4d79b2ca617473c6aefd5869abfa02563a77682038bf916cf", [:mix], [], "hexpm", "098af64e1f6f8609c6672127cfe9e9590a5d3fcdd82bc17a377b8692fd81a879"}, "nimble_parsec": {:hex, :nimble_parsec, "1.4.2", "8efba0122db06df95bfaa78f791344a89352ba04baedd3849593bfce4d0dc1c6", [:mix], [], "hexpm", "4b21398942dda052b403bbe1da991ccd03a053668d147d53fb8c4e0efe09c973"}, diff --git a/test/urp/pool_test.exs b/test/urp/pool_test.exs new file mode 100644 index 0000000..cd02de3 --- /dev/null +++ b/test/urp/pool_test.exs @@ -0,0 +1,45 @@ +defmodule URP.PoolTest do + use ExUnit.Case, async: true + + # Real OID observed in production when soffice aborted mid-conversion. + @stale_oid "57ee30e3f490;gcc3[0];46d08adf17bf43b189511ea4ed49b46f" + + describe "checkout_outcome/4" do + test "keeps the connection when a sink consumed the output" do + assert URP.Pool.checkout_outcome(:ok, nil, nil, false) == {:ok, :reuse} + end + + test "wraps the output bytes when no sink consumed them" do + assert URP.Pool.checkout_outcome("%PDF-1.7", nil, nil, false) == + {{:ok, "%PDF-1.7"}, :reuse} + end + + test "discards the connection after stream input even on success" do + assert URP.Pool.checkout_outcome(:ok, nil, nil, true) == {:ok, :discard} + end + + test "returns the result but discards the connection when only cleanup failed" do + assert URP.Pool.checkout_outcome(:ok, nil, "close failed", false) == {:ok, :discard} + end + + test "reports the error when the conversion produced nothing" do + assert URP.Pool.checkout_outcome(nil, "connection closed", "connection closed", false) == + {{:error, "connection closed"}, :discard} + end + + test "reports the error when a failed conversion left a stale OID in the reply" do + assert URP.Pool.checkout_outcome( + @stale_oid, + "connection closed", + "connection closed", + false + ) == + {{:error, "connection closed"}, :discard} + end + + test "reports the error when a failed conversion left stale bytes in the reply" do + assert URP.Pool.checkout_outcome(<<0x80, 0, 0, 0, 0>>, "timeout", "timeout", true) == + {{:error, "timeout"}, :discard} + end + end +end