diff --git a/.changeset/fix-telemetry-arithmetic-error.md b/.changeset/fix-telemetry-arithmetic-error.md new file mode 100644 index 0000000000..f14a272029 --- /dev/null +++ b/.changeset/fix-telemetry-arithmetic-error.md @@ -0,0 +1,5 @@ +--- +"@core/sync-service": patch +--- + +Fixed ArithmeticError in `ServeShapePlug.end_telemetry_span/2` when `parse_body` halts before the telemetry span is started. Moved `parse_body` plug after `start_telemetry_span` in the pipeline. diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index e9e7431ee7..c3b8f26fb2 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -114,4 +114,5 @@ jobs: Start by reading `.claude/commands/pr-review.md`, then follow its phases to conduct the review. + allowed_bots: 'claude[bot]' claude_args: '--allowedTools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*),Bash(gh api:*),Bash(cat:*),Read,Glob,Grep"' diff --git a/packages/sync-service/lib/electric/plug/serve_shape_plug.ex b/packages/sync-service/lib/electric/plug/serve_shape_plug.ex index 91ee5bd5c3..062076e44d 100644 --- a/packages/sync-service/lib/electric/plug/serve_shape_plug.ex +++ b/packages/sync-service/lib/electric/plug/serve_shape_plug.ex @@ -12,12 +12,12 @@ defmodule Electric.Plug.ServeShapePlug do require Logger + plug :put_resp_content_type, "application/json" plug :fetch_query_params - plug :parse_body # start_telemetry_span needs to always be the first plug after fetching query params. plug :start_telemetry_span - plug :put_resp_content_type, "application/json" + plug :parse_body plug :validate_request # Admission control is applied here, but note: diff --git a/packages/sync-service/test/electric/plug/serve_shape_plug_test.exs b/packages/sync-service/test/electric/plug/serve_shape_plug_test.exs index 6241f1f264..8519dc8abe 100644 --- a/packages/sync-service/test/electric/plug/serve_shape_plug_test.exs +++ b/packages/sync-service/test/electric/plug/serve_shape_plug_test.exs @@ -1028,6 +1028,66 @@ defmodule Electric.Plug.ServeShapePlugTest do end end + describe "parse_body error handling" do + setup :with_lsn_tracker + + setup ctx do + {:via, _, {registry_name, registry_key}} = + Electric.Shapes.Supervisor.name(ctx.stack_id) + + {:ok, _} = Registry.register(registry_name, registry_key, nil) + set_status_to_active(ctx) + + patch_storage(for_shape: fn @test_shape_handle, _opts -> @test_opts end) + + :ok + end + + test "returns 400 for invalid JSON body without crashing", ctx do + conn = + Plug.Test.conn(:post, "/?offset=-1", "not valid json") + |> put_req_header("content-type", "application/json") + |> call_serve_shape_plug(ctx) + + assert conn.status == 400 + assert %{"error" => "Invalid JSON in request body"} = Jason.decode!(conn.resp_body) + end + + test "returns 400 for non-object JSON body without crashing", ctx do + conn = + Plug.Test.conn(:post, "/?offset=-1", Jason.encode!(["an", "array"])) + |> put_req_header("content-type", "application/json") + |> call_serve_shape_plug(ctx) + + assert conn.status == 400 + assert %{"error" => "Request body must be a JSON object"} = Jason.decode!(conn.resp_body) + end + + test "returns 413 for oversized body without crashing", ctx do + Repatch.patch(Plug.Conn, :read_body, fn conn, _opts -> {:more, "partial", conn} end) + + conn = + Plug.Test.conn(:post, "/?offset=-1", "body") + |> put_req_header("content-type", "application/json") + |> call_serve_shape_plug(ctx) + + assert conn.status == 413 + assert %{"error" => "Request body too large"} = Jason.decode!(conn.resp_body) + end + + test "returns 400 for body read failure without crashing", ctx do + Repatch.patch(Plug.Conn, :read_body, fn _conn, _opts -> {:error, :timeout} end) + + conn = + Plug.Test.conn(:post, "/?offset=-1", "body") + |> put_req_header("content-type", "application/json") + |> call_serve_shape_plug(ctx) + + assert conn.status == 400 + assert %{"error" => "Failed to read request body"} = Jason.decode!(conn.resp_body) + end + end + describe "stack not ready" do test "returns 503", ctx do conn =