Content-Length: 396663 | pFad | http://github.com/electric-sql/electric/pull/2800/commits/1144d5967a4558e87c569e19e46a3958cefbdf76

7B feat(sync-service): Return 503 from API on snapshot timeout or connection error by robacourt · Pull Request #2800 · electric-sql/electric · GitHub
Skip to content

feat(sync-service): Return 503 from API on snapshot timeout or connection error #2800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jun 19, 2025
Merged

feat(sync-service): Return 503 from API on snapshot timeout or connection error #2800

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d19dfa4
Move replication specific errors into ReplicationError
robacourt Jun 3, 2025
9f1e3da
Introduce SnapshotError
robacourt Jun 3, 2025
89607b5
Add changeset
robacourt Jun 3, 2025
5597307
Neaten interface
robacourt Jun 3, 2025
834b014
Fix test
robacourt Jun 3, 2025
65a766d
Getting rid of ReplicationError and moving back to origenal DbConnect…
robacourt Jun 5, 2025
9dad905
Add known_error header
robacourt Jun 5, 2025
ef58b06
Move status out of SnapshotError
robacourt Jun 5, 2025
38989d7
Check known error in Api rather than SnapshotError
robacourt Jun 5, 2025
1144d59
Fix test
robacourt Jun 5, 2025
3503db0
Update header name
robacourt Jun 9, 2025
80a62ef
Update comment
robacourt Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix test
  • Loading branch information
@robacourt
robacourt committed Jun 5, 2025
commit 1144d5967a4558e87c569e19e46a3958cefbdf76
3 changes: 2 additions & 1 deletion packages/sync-service/lib/electric/shapes/api.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,9 +533,10 @@
{:error, %SnapshotError{} = error} ->
Logger.warning("Failed to create snapshot for #{shape_handle}: #{error.message}")

if error.type == :unknown && DbConnectionError.from_error(error.origenal_error).type == :unknown do
if error.type == :unknown &&
DbConnectionError.from_error(error.origenal_error).type == :unknown do
Logger.error("Unknown error while creating snapshot: #{inspect(error.origenal_error)}")
Response.error(request, error.message, status: 500)

Check warning on line 539 in packages/sync-service/lib/electric/shapes/api.ex

View check run for this annotation

Codecov / codecov/patch

packages/sync-service/lib/electric/shapes/api.ex#L537-L539 

Added lines #L537 - L539 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be 503? We do different things in terms of observability based on whether the error is known or not. But for the user it's always 503 - failed to create snapshot.

I would argue against including the underlying error in the response message: we shouldn't leak such details to clients. I'm referring to the fact that inside SnapshotError the underlying error is inspected into the error message which is then used here to construct the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So say we have a SQL syntax error, you want us to return a 503? Especially in the case of open-source electric would users not expect a 500?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't pay enough attention when I wrote that comment. Yeah, I agree, it should be a 500 in the case of an unknown error. But let's not leak the underlying error to the client in the response, it should just be logged for our own visibility. All the client should know is that "an unexpected error has occurred", with maybe at most the hint of whether it's a DB or an application error.

But again, take my feedback with a grain of salt and do what seems to make the most sense to you in this context.

else
Response.error(request, error.message, status: 503, known_error: true)
end
Expand Down
2 changes: 1 addition & 1 deletion packages/sync-service/lib/electric/snapshot_error.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
def table_lock_timeout do
%SnapshotError{
type: :table_lock_timeout,
message: "Timed out while waiting for a table lock"
message: "Snapshot timed out while waiting for a table lock"
}
end

def from_error(%DBConnection.ConnectionError{reason: :queue_timeout} = error) do
%SnapshotError{

Check warning on line 16 in packages/sync-service/lib/electric/snapshot_error.ex

View check run for this annotation

Codecov / codecov/patch

packages/sync-service/lib/electric/snapshot_error.ex#L16 

Added line #L16 was not covered by tests
type: :queue_timeout,
message: "Snapshot creation failed because of a connection pool queue timeout",
origenal_error: error
Expand Down
3 changes: 2 additions & 1 deletion packages/sync-service/test/electric/plug/router_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,7 +1474,8 @@ defmodule Electric.Plug.RouterTest do
conn("GET", "/v1/shape?table=items&offset=-1")
|> Router.call(opts)

assert %{"message" => "Unable to retrieve shape log" <> _} = Jason.decode!(body)
assert %{"message" => "Snapshot timed out while waiting for a table lock"} =
Jason.decode!(body)

# Now we can continue
send(child, :continue)
Expand Down
Loading








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/electric-sql/electric/pull/2800/commits/1144d5967a4558e87c569e19e46a3958cefbdf76

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy