Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for listening on Unix domain sockets to the SQLite REST server, providing an alternative to TCP-based networking. The implementation allows users to specify a socket path via the --http-socket flag, with proper setup, logging, and cleanup of socket resources.
Key Changes
- Added
--http-socketCLI flag to enable Unix socket listening as an alternative to TCP (takes precedence over--http-addr) - Implemented socket lifecycle management including directory creation, stale socket cleanup, listener management, and graceful shutdown
- Added comprehensive test coverage for Unix socket functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server.go | Core implementation adding SocketPath field to ServerOptions, listener field to dbServer struct, and modified Start() method to support both TCP and Unix socket listening with proper cleanup |
| server_socket_test.go | New test file validating Unix socket server functionality including socket creation, HTTP communication over Unix domain sockets, and basic query operations |
| README.md | Documentation update with example usage of the new --http-socket flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| close(done) | ||
| } |
There was a problem hiding this comment.
After closing the done channel, the test immediately ends without waiting for the server to complete its shutdown sequence. This could lead to a race condition where the test cleanup (t.TempDir() cleanup) happens before the server finishes shutting down and cleaning up the socket. Consider adding a small delay or a synchronization mechanism to ensure the server has completed its shutdown before the test exits.
| return | ||
| } | ||
|
|
||
| if err := os.RemoveAll(server.socket); err != nil { |
There was a problem hiding this comment.
Using RemoveAll to clean up a socket file is dangerous and could accidentally delete an entire directory if the socket path is misconfigured. Use os.Remove instead, which will fail safely if the path is a directory.
| if err := os.RemoveAll(server.socket); err != nil { | |
| if err := os.Remove(server.socket); err != nil && !errors.Is(err, os.ErrNotExist) { |
| close(done) | ||
| } |
There was a problem hiding this comment.
The test doesn't verify that the Unix socket file is properly cleaned up after the server shuts down. Consider adding an assertion after closing the done channel and allowing some time for cleanup to verify that the socket file no longer exists, ensuring the cleanup logic in the Start method works correctly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| if err := os.RemoveAll(server.socket); err != nil { |
There was a problem hiding this comment.
Using os.RemoveAll on a socket path could be dangerous if the path points to a directory. Consider using os.Remove instead, which will fail safely if the path is a directory. This prevents accidentally deleting an entire directory structure if misconfigured.
| if err := os.RemoveAll(server.socket); err != nil { | |
| if err := os.Remove(server.socket); err != nil && !errors.Is(err, os.ErrNotExist) { |
|
|
||
| server.logger.Info("shutting down server") | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) | ||
| defer cancel() |
There was a problem hiding this comment.
The listener is not explicitly closed during shutdown. Although server.Shutdown will close the listener, explicitly closing it ensures proper cleanup and makes the code more maintainable. Add a check and close the listener before or after the Shutdown call.
| defer cancel() | |
| defer cancel() | |
| if server.listener != nil { | |
| if err := server.listener.Close(); err != nil && !errors.Is(err, net.ErrClosed) { | |
| server.logger.Error(err, "failed to close listener") | |
| } | |
| } |
| return | ||
| } | ||
|
|
||
| l, err := net.Listen("unix", server.socket) |
There was a problem hiding this comment.
The socket file permissions (0666 by default for Unix sockets) could be a security concern. Consider setting explicit restrictive permissions after creating the socket using os.Chmod to limit access to the socket file (e.g., 0600 for owner-only or 0660 for owner and group).
| } | ||
| server.listener = l | ||
|
|
||
| go server.server.Serve(l) |
There was a problem hiding this comment.
The error returned by server.Serve is not captured or logged. While Serve is called in a goroutine, consider capturing and logging the error for better observability, especially to distinguish between expected shutdown (http.ErrServerClosed) and unexpected errors.
| } | ||
| server.listener = l | ||
|
|
||
| go server.server.Serve(l) |
There was a problem hiding this comment.
The error returned by server.Serve is not captured or logged. While Serve is called in a goroutine, consider capturing and logging the error for better observability, especially to distinguish between expected shutdown (http.ErrServerClosed) and unexpected errors.
1 participant
Summary
--http-socketflagTesting
Codex Task