diff --git a/appview/lib/atvouch/tap_handler.ex b/appview/lib/atvouch/tap_handler.ex index a52ba8c..e7dcd5f 100644 --- a/appview/lib/atvouch/tap_handler.ex +++ b/appview/lib/atvouch/tap_handler.ex @@ -401,22 +401,28 @@ defmodule Atvouch.TapHandler do comment = Atvouch.Tangled.CommentBuilder.build_comment(author_did, author_handle, maintainer_routes, handle_map) - case Atvouch.Tangled.Client.post_comment(repo_handle, repo_name, pull_number, comment) do - :ok -> - now = DateTime.utc_now() |> DateTime.to_iso8601() - - Atvouch.BotComment.create(%{ - repo_at_uri: repo_at_uri, - pull_number: pull_number, - author_did: author_did, - commented_at: now - }) - - Logger.info("Posted vouch comment on #{repo_handle}/#{repo_name}##{pull_number}") - :ok + # Record the comment first to prevent races between duplicate events + now = DateTime.utc_now() |> DateTime.to_iso8601() - {:error, reason} -> - Logger.warning("Failed to post comment on #{repo_handle}/#{repo_name}##{pull_number}: #{inspect(reason)}") + case Atvouch.BotComment.create(%{ + repo_at_uri: repo_at_uri, + pull_number: pull_number, + author_did: author_did, + commented_at: now + }) do + {:ok, _} -> + case Atvouch.Tangled.Client.post_comment(repo_handle, repo_name, pull_number, comment) do + :ok -> + Logger.info("Posted vouch comment on #{repo_handle}/#{repo_name}##{pull_number}") + :ok + + {:error, reason} -> + Logger.warning("Failed to post comment on #{repo_handle}/#{repo_name}##{pull_number}: #{inspect(reason)}") + :ok + end + + {:error, %Ecto.Changeset{} = changeset} -> + Logger.debug("Comment already recorded for #{repo_handle}/#{repo_name}##{pull_number}, skipping (#{inspect(changeset.errors)})") :ok end end diff --git a/appview/test/atvouch/pull_handler_test.exs b/appview/test/atvouch/pull_handler_test.exs index 289add9..af74f0e 100644 --- a/appview/test/atvouch/pull_handler_test.exs +++ b/appview/test/atvouch/pull_handler_test.exs @@ -433,6 +433,186 @@ defmodule Atvouch.PullHandlerTest do refute_receive {:tangled_comment, _}, 1_000 end + test "duplicate race: pre-existing bot_comment record prevents crash and duplicate comment", %{ + tap_port: tap_port, + state_agent: state_agent + } do + repo_at_uri = setup_vouch_graph() + + # Pre-insert a BotComment to simulate a race where another process already recorded it + {:ok, _} = + Atvouch.BotComment.create(%{ + repo_at_uri: repo_at_uri, + pull_number: 1, + author_did: "did:plc:author", + commented_at: "2026-03-19T10:00:00Z" + }) + + Atvouch.Test.FakeTangledServer.set_pulls_html( + state_agent, + "repoowner.test", + "testrepo", + pulls_html("repoowner.test", "testrepo") + ) + + {:ok, _pid} = + Atvouch.Tap.Socket.start_link( + uri: "ws://localhost:#{tap_port}/channel", + handler: Atvouch.TapHandler, + password: "123", + name: :"pull_race_test_#{tap_port}" + ) + + assert_receive {:ws_connected, ws_pid}, 5_000 + + Atvouch.Test.FakeTapServer.send_event(ws_pid, %{ + "id" => 720, + "type" => "record", + "record" => %{ + "action" => "create", + "did" => "did:plc:author", + "rev" => "3mgmqjki6sz2n", + "collection" => "sh.tangled.repo.pull", + "rkey" => "3pullrace", + "record" => %{ + "$type" => "sh.tangled.repo.pull", + "title" => "Race condition PR", + "createdAt" => "2026-03-19T10:00:00Z", + "target" => %{ + "repo" => repo_at_uri, + "branch" => "main" + } + }, + "cid" => "bafypullrace", + "live" => true + } + }) + + # Should process without crashing + assert_receive {:ws_message, %{"type" => "ack", "id" => 720}}, 10_000 + + # Should NOT post a comment since it's already recorded + refute_receive {:tangled_comment, _}, 1_000 + end + + test "same pull number on different repos each get their own comment", %{ + tap_port: tap_port, + state_agent: state_agent + } do + # Set up first repo (reuses setup_vouch_graph) + repo_at_uri_1 = setup_vouch_graph() + + # Set up second repo with its own membership + {:ok, _} = Atvouch.Identity.create(%{did: "did:plc:repoowner2", handle: "repoowner2.test"}) + + repo_at_uri_2 = "at://did:plc:repoowner2/sh.tangled.repo/3def456" + + {:ok, _} = + Atvouch.Membership.create( + %{ + repo_at_uri: repo_at_uri_2, + source_did: "did:plc:repoowner2", + repo_did: "did:plc:repoowner2", + repo_name: "otherrepo", + remote_created_at: "2026-03-01T00:00:00Z", + received_at: "2026-03-01T00:00:00Z" + }, + ["did:plc:maintainer1"] + ) + + # Set up pulls pages for both repos + Atvouch.Test.FakeTangledServer.set_pulls_html( + state_agent, + "repoowner.test", + "testrepo", + pulls_html("repoowner.test", "testrepo") + ) + + Atvouch.Test.FakeTangledServer.set_pulls_html( + state_agent, + "repoowner2.test", + "otherrepo", + pulls_html("repoowner2.test", "otherrepo") + ) + + {:ok, _pid} = + Atvouch.Tap.Socket.start_link( + uri: "ws://localhost:#{tap_port}/channel", + handler: Atvouch.TapHandler, + password: "123", + name: :"pull_multi_repo_test_#{tap_port}" + ) + + assert_receive {:ws_connected, ws_pid}, 5_000 + + # Send pull event for first repo + Atvouch.Test.FakeTapServer.send_event(ws_pid, %{ + "id" => 730, + "type" => "record", + "record" => %{ + "action" => "create", + "did" => "did:plc:author", + "rev" => "3mgmqjki6sz2n", + "collection" => "sh.tangled.repo.pull", + "rkey" => "3pullmulti1", + "record" => %{ + "$type" => "sh.tangled.repo.pull", + "title" => "PR on repo 1", + "createdAt" => "2026-03-19T10:00:00Z", + "target" => %{ + "repo" => repo_at_uri_1, + "branch" => "main" + } + }, + "cid" => "bafypullmulti1", + "live" => true + } + }) + + assert_receive {:ws_message, %{"type" => "ack", "id" => 730}}, 10_000 + assert_receive {:tangled_comment, comment1}, 10_000 + assert comment1.handle == "repoowner.test" + assert comment1.rkey == "testrepo" + assert comment1.number == 1 + + # Drain + drain_messages() + + # Send pull event for second repo (same pull number #1) + Atvouch.Test.FakeTapServer.send_event(ws_pid, %{ + "id" => 731, + "type" => "record", + "record" => %{ + "action" => "create", + "did" => "did:plc:author", + "rev" => "3mgmqjki6sz2n", + "collection" => "sh.tangled.repo.pull", + "rkey" => "3pullmulti2", + "record" => %{ + "$type" => "sh.tangled.repo.pull", + "title" => "PR on repo 2", + "createdAt" => "2026-03-19T10:00:00Z", + "target" => %{ + "repo" => repo_at_uri_2, + "branch" => "main" + } + }, + "cid" => "bafypullmulti2", + "live" => true + } + }) + + assert_receive {:ws_message, %{"type" => "ack", "id" => 731}}, 10_000 + assert_receive {:tangled_comment, comment2}, 10_000 + assert comment2.handle == "repoowner2.test" + assert comment2.rkey == "otherrepo" + assert comment2.number == 1 + + # Both repos should have their own dedup records + assert Atvouch.BotComment.exists?(repo_at_uri_1, 1) + assert Atvouch.BotComment.exists?(repo_at_uri_2, 1) + end + defp drain_messages do receive do _ -> drain_messages()