Raise on docs tarball unpack failures (#110)

* Raise on docs tarball unpack failures

Unpack errors were being swallowed by Logger.error and never reached
Sentry, so we had no visibility into malformed publisher tarballs
(e.g. lustre 5.7.0 with mode-0 duplicate entries that hit EACCES).

Replace Hexdocs.Tar.unpack_to_dir/2 with unpack_to_dir!/2 raising
Hexdocs.Tar.UnpackError, and drop the error-swallowing branches in
queue.ex and bucket.ex. Broadway's catch logs with crash_reason
metadata, which Sentry.LoggerHandler turns into a proper exception
event.

* Address self-review: correct version reported in delete-flow exception

- bucket.ex delete/4: pass new_latest_version (the version actually
  being unpacked) instead of the version being deleted
- tar.ex: replace `unless ok?` with `if not ok?` for clarity
- tar_test.exs: pin the invalid-gzip raise to the message prefix

Author: ericmj

lib/hexdocs/bucket.ex

@@ -167,25 +167,22 @@ defmodule Hexdocs.Bucket do
 
         case Hexdocs.Store.get_to_file(:repo_bucket, key, tarball_path) do
           :ok ->
-            case Hexdocs.Tar.unpack_to_dir({:file, tarball_path},
-                   repository: repository,
-                   package: package,
-                   version: version
-                 ) do
-              {:ok, dir, files} ->
-                upload_files =
-                  list_upload_files(repository, package, new_latest_version, dir, files, :both)
-
-                paths = MapSet.new(upload_files, &elem(&1, 0))
-                update_versions = [version, new_latest_version]
-
-                upload_new_files(upload_files)
-                delete_old_docs(repository, package, update_versions, paths, :both)
-                purge_hexdocs_cache(repository, package, update_versions, :both)
-
-              {:error, reason} ->
-                Logger.error("Failed unpack #{repository}/#{package} #{version}: #{reason}")
-            end
+            {dir, files} =
+              Hexdocs.Tar.unpack_to_dir!({:file, tarball_path},
+                repository: repository,
+                package: package,
+                version: new_latest_version
+              )
+
+            upload_files =
+              list_upload_files(repository, package, new_latest_version, dir, files, :both)
+
+            paths = MapSet.new(upload_files, &elem(&1, 0))
+            update_versions = [version, new_latest_version]
+
+            upload_new_files(upload_files)
+            delete_old_docs(repository, package, update_versions, paths, :both)
+            purge_hexdocs_cache(repository, package, update_versions, :both)
 
           nil ->
             Logger.error("Failed to get tarball #{repository}/#{package} #{new_latest_version}")

lib/hexdocs/queue.ex

@@ -66,19 +66,16 @@ defmodule Hexdocs.Queue do
 
           case Hexdocs.Store.get_to_file(:repo_bucket, key, tarball_path) do
             :ok ->
-              case Hexdocs.Tar.unpack_to_dir({:file, tarball_path},
-                     repository: repository,
-                     package: package,
-                     version: version
-                   ) do
-                {:ok, _dir, files} ->
-                  update_index_sitemap(repository, key)
-                  update_package_sitemap(repository, key, package, files)
-                  Logger.info("#{key}: done")
-
-                {:error, reason} ->
-                  Logger.error("Failed unpack #{repository}/#{package} #{version}: #{reason}")
-              end
+              {_dir, files} =
+                Hexdocs.Tar.unpack_to_dir!({:file, tarball_path},
+                  repository: repository,
+                  package: package,
+                  version: version
+                )
+
+              update_index_sitemap(repository, key)
+              update_package_sitemap(repository, key, package, files)
+              Logger.info("#{key}: done")
 
             nil ->
               Logger.error("#{key}: package not found in store")
@@ -163,36 +160,33 @@ defmodule Hexdocs.Queue do
         {version, all_versions, retired_versions}
       end
 
-    case Hexdocs.Tar.unpack_to_dir(input,
-           repository: repository,
-           package: package,
-           version: version
-         ) do
-      {:ok, dir, files} ->
-        rewrite_files(dir, files)
-
-        Hexdocs.Bucket.upload(
-          repository,
-          package,
-          version,
-          all_versions,
-          retired_versions,
-          dir,
-          files
-        )
+    {dir, files} =
+      Hexdocs.Tar.unpack_to_dir!(input,
+        repository: repository,
+        package: package,
+        version: version
+      )
 
-        if Hexdocs.Utils.latest_version?(package, version, all_versions) do
-          update_index_sitemap(repository, key)
-          update_package_sitemap(repository, key, package, files)
-          update_package_names_csv(repository)
-        end
+    rewrite_files(dir, files)
 
-        elapsed = System.os_time(:millisecond) - start
-        Logger.info("FINISHED UPLOADING DOCS #{key} #{elapsed}ms")
+    Hexdocs.Bucket.upload(
+      repository,
+      package,
+      version,
+      all_versions,
+      retired_versions,
+      dir,
+      files
+    )
 
-      {:error, reason} ->
-        Logger.error("Failed unpack #{repository}/#{package} #{version}: #{reason}")
+    if Hexdocs.Utils.latest_version?(package, version, all_versions) do
+      update_index_sitemap(repository, key)
+      update_package_sitemap(repository, key, package, files)
+      update_package_names_csv(repository)
     end
+
+    elapsed = System.os_time(:millisecond) - start
+    Logger.info("FINISHED UPLOADING DOCS #{key} #{elapsed}ms")
   end
 
   defp process_search(key, repository, package, version, input, start) do
@@ -205,15 +199,16 @@ defmodule Hexdocs.Queue do
           :error when package in @special_package_names -> version
         end
 
-      case Hexdocs.Tar.unpack_to_dir(input, package: package, version: version) do
-        {:ok, dir, files} ->
-          update_search_index(key, package, version, dir, files)
-          elapsed = System.os_time(:millisecond) - start
-          Logger.info("FINISHED INDEXING DOCS #{key} #{elapsed}ms")
+      {dir, files} =
+        Hexdocs.Tar.unpack_to_dir!(input,
+          repository: repository,
+          package: package,
+          version: version
+        )
 
-        {:error, reason} ->
-          Logger.error("Failed unpack #{package} #{version}: #{reason}")
-      end
+      update_search_index(key, package, version, dir, files)
+      elapsed = System.os_time(:millisecond) - start
+      Logger.info("FINISHED INDEXING DOCS #{key} #{elapsed}ms")
     end
   end
 

lib/hexdocs/tar.ex

@@ -1,13 +1,22 @@
 defmodule Hexdocs.Tar do
   require Logger
 
+  defmodule UnpackError do
+    defexception [:repository, :package, :version, :reason]
+
+    @impl true
+    def message(%{repository: repository, package: package, version: version, reason: reason}) do
+      "Failed to unpack #{repository}/#{package} #{version}: #{reason}"
+    end
+  end
+
   def create(files) do
     files = for {path, contents} <- files, do: {String.to_charlist(path), contents}
     {:ok, tarball} = :hex_tarball.create_docs(files)
     tarball
   end
 
-  def unpack_to_dir({:file, path}, opts \\ []) do
+  def unpack_to_dir!({:file, path}, opts \\ []) do
     repository = Keyword.get(opts, :repository, "UNKNOWN")
     package = Keyword.get(opts, :package, "UNKNOWN")
     version = Keyword.get(opts, :version, "UNKNOWN")
@@ -26,14 +35,15 @@ defmodule Hexdocs.Tar do
           |> Enum.map(&Path.relative_to(&1, output_dir))
 
         files = fix_paths(repository, package, version, files)
-
-        case check_version_dirs(files) do
-          :ok -> {:ok, output_dir, files}
-          {:error, _} = error -> error
-        end
+        check_version_dirs!(repository, package, version, files)
+        {output_dir, files}
 
       {:error, reason} ->
-        {:error, inspect(reason)}
+        raise UnpackError,
+          repository: repository,
+          package: package,
+          version: version,
+          reason: inspect(reason)
     end
   end
 
@@ -55,17 +65,19 @@ defmodule Hexdocs.Tar do
     end)
   end
 
-  defp check_version_dirs(files) do
-    result =
+  defp check_version_dirs!(repository, package, version, files) do
+    ok? =
       Enum.all?(files, fn path ->
         first = Path.split(path) |> hd()
         Version.parse(first) == :error
       end)
 
-    if result do
-      :ok
-    else
-      {:error, "root file or directory name not allowed to match a semver version"}
+    if not ok? do
+      raise UnpackError,
+        repository: repository,
+        package: package,
+        version: version,
+        reason: "root file or directory name not allowed to match a semver version"
     end
   end
 

test/hexdocs/tar_test.exs

@@ -2,12 +2,12 @@ defmodule Hexdocs.TarTest do
   use ExUnit.Case, async: true
   alias Hexdocs.Tar
 
-  test "unpack_to_dir" do
+  test "unpack_to_dir!" do
     blob = Tar.create([{"index.html", "contents"}, {"foo.bar", "contents"}])
     path = Hexdocs.TmpDir.tmp_file("test-tarball")
     File.write!(path, blob)
 
-    assert {:ok, dir, files} = Tar.unpack_to_dir({:file, path})
+    assert {dir, files} = Tar.unpack_to_dir!({:file, path})
     assert File.dir?(dir)
     assert length(files) == 2
     assert "index.html" in files
@@ -19,19 +19,59 @@ defmodule Hexdocs.TarTest do
   test "invalid gzip" do
     path = Hexdocs.TmpDir.tmp_file("test-tarball")
     File.write!(path, "")
-    assert {:error, _} = Tar.unpack_to_dir({:file, path})
+
+    assert_raise Tar.UnpackError, ~r/Failed to unpack hexpm\/foo 1\.0\.0:/, fn ->
+      Tar.unpack_to_dir!({:file, path}, repository: "hexpm", package: "foo", version: "1.0.0")
+    end
   end
 
   test "do not allow root files/directories with version names" do
-    reason = "root file or directory name not allowed to match a semver version"
-
     blob = Tar.create([{"1.0.0", "contents"}])
     path = Hexdocs.TmpDir.tmp_file("test-tarball")
     File.write!(path, blob)
-    assert Tar.unpack_to_dir({:file, path}) == {:error, reason}
+
+    assert_raise Tar.UnpackError,
+                 ~r/root file or directory name not allowed to match a semver version/,
+                 fn ->
+                   Tar.unpack_to_dir!({:file, path},
+                     repository: "hexpm",
+                     package: "foo",
+                     version: "1.0.0"
+                   )
+                 end
 
     blob = Tar.create([{"1.0.0/index.html", "contents"}])
     File.write!(path, blob)
-    assert Tar.unpack_to_dir({:file, path}) == {:error, reason}
+
+    assert_raise Tar.UnpackError,
+                 ~r/root file or directory name not allowed to match a semver version/,
+                 fn ->
+                   Tar.unpack_to_dir!({:file, path},
+                     repository: "hexpm",
+                     package: "foo",
+                     version: "1.0.0"
+                   )
+                 end
+  end
+
+  test "raises on tarball with duplicate mode-0 entries" do
+    path = Hexdocs.TmpDir.tmp_file("test-tarball")
+    {:ok, tar} = :hex_erl_tar.open(String.to_charlist(path), [:write, :compressed])
+
+    for _ <- 1..3 do
+      :ok = :hex_erl_tar.add(tar, "contents", ~c"#", [{:mode, 0}])
+    end
+
+    :ok = :hex_erl_tar.close(tar)
+
+    assert_raise Tar.UnpackError,
+                 ~r/Failed to unpack hexpm\/lustre 5\.7\.0: :eacces/,
+                 fn ->
+                   Tar.unpack_to_dir!({:file, path},
+                     repository: "hexpm",
+                     package: "lustre",
+                     version: "5.7.0"
+                   )
+                 end
   end
 end