Replace private psycopg2/Bolt API usage with public interfaces for he…#83
Merged
Conversation
…alth and shutdown
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces the Slack Bolt server startup with a stdlib ChangesBolt HTTP server and shutdown updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperscout/shutdown.py (1)
55-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose both HTTPServer sockets after joining their threads.
shutdown()stopsserve_forever(), but the listening socket stays open. Addserver_close()for bothhealth_serverandbolt_serverso their ports are released cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/paperscout/shutdown.py` around lines 55 - 63, After calling shutdown and joining the threads in the shutdown flow, also close the listening sockets so the ports are released cleanly. Update the shutdown logic around the health_server and bolt_server handling in shutdown() to call server_close() for each server after their corresponding _join_thread() completes, keeping the existing exception logging intact.
🧹 Nitpick comments (1)
tests/test_shutdown.py (1)
90-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test still doesn't prove the pool is actually drained.
Issue
#80asks for a post-shutdown “zero active connections” check. A plainMagicMockonly provescloseall()was called, so this test still passes if checked-out connections remain leaked. Please add an assertion against real pool state (for example viapool_status(pool)or a lightweight fake that tracks active leases).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_shutdown.py` around lines 90 - 105, The shutdown test only verifies that shutdown_services() calls pool.closeall(), so it does not validate the required zero-active-connections state. Update test_shutdown_services_closes_pool to assert the pool is fully drained after shutdown by checking real pool status via pool_status(pool) or by using a lightweight fake pool that tracks checked-out leases. Keep the existing shutdown_services and pool.closeall assertions, but add a post-shutdown assertion that confirms no active connections remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_bolt_server.py`:
- Around line 24-43: Update the bolt_server_url fixture teardown so it fully
releases the HTTP server resources: in the fixture that uses
create_bolt_http_server, call server.server_close() after server.shutdown() to
close the listening socket, and replace the current thread.join(timeout=2)
cleanup with a join followed by an assertion that the serve_forever thread is no
longer alive. Use the existing bolt_server_url fixture, create_bolt_http_server,
and the thread object to locate the change.
---
Outside diff comments:
In `@src/paperscout/shutdown.py`:
- Around line 55-63: After calling shutdown and joining the threads in the
shutdown flow, also close the listening sockets so the ports are released
cleanly. Update the shutdown logic around the health_server and bolt_server
handling in shutdown() to call server_close() for each server after their
corresponding _join_thread() completes, keeping the existing exception logging
intact.
---
Nitpick comments:
In `@tests/test_shutdown.py`:
- Around line 90-105: The shutdown test only verifies that shutdown_services()
calls pool.closeall(), so it does not validate the required
zero-active-connections state. Update test_shutdown_services_closes_pool to
assert the pool is fully drained after shutdown by checking real pool status via
pool_status(pool) or by using a lightweight fake pool that tracks checked-out
leases. Keep the existing shutdown_services and pool.closeall assertions, but
add a post-shutdown assertion that confirms no active connections remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05e6b3a1-bac4-466c-b4cb-9b3f10800990
📒 Files selected for processing (7)
src/paperscout/__main__.pysrc/paperscout/bolt_server.pysrc/paperscout/db.pysrc/paperscout/shutdown.pytests/test_bolt_server.pytests/test_main_health_merge.pytests/test_shutdown.py
Collaborator
Author
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_shutdown.py (1)
90-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe pool-close test does not verify the leak is fixed.
This only checks that
closeall()was invoked on aMagicMock. Issue#80asks for a shutdown test that proves the pool reaches zero active connections; this test would still pass if checked-out connections were leaked. Please switch this to a stateful fake or integration-style pool test and assert the active count is0aftershutdown_services().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_shutdown.py` around lines 90 - 105, The shutdown pool test currently only asserts that shutdown_services() calls closeall() on a MagicMock, which does not prove connections are actually released. Replace the MagicMock pool in test_shutdown_services_closes_pool with a stateful fake or integration-style pool that tracks checked-out connections, then verify after shutdown_services() completes that the pool’s active connection count is 0. Keep the existing shutdown_services() and pool.closeall() flow, but strengthen the assertion to validate real connection cleanup rather than just method invocation.
🧹 Nitpick comments (1)
tests/test_bolt_server.py (1)
18-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid the free-port probe race in this fixture.
_find_free_port()releases the socket before the server binds, so another process or parallel test can grab that port and make this suite flaky. Let the server bind to port0and read back the assigned port instead.♻️ Proposed fix
-import socket @@ -def _find_free_port() -> int: - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("127.0.0.1", 0)) - return s.getsockname()[1] - - `@pytest.fixture`() def bolt_server_url(): @@ - port = _find_free_port() - server = create_bolt_http_server(app, port, bind_host="127.0.0.1") + server = create_bolt_http_server(app, 0, bind_host="127.0.0.1") + port = server.server_address[1]Also applies to: 33-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_bolt_server.py` around lines 18 - 21, The free-port helper in _find_free_port is causing a race because the socket is closed before the server starts, so update the test setup to stop probing for a port and instead let the Bolt server bind to port 0 directly. Then, in the server startup/fixture code that uses _find_free_port and the Bolt server launch path, read back the actual assigned port from the bound socket/server object and use that value for the client connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/paperscout/shutdown.py`:
- Around line 55-59: The Bolt server shutdown path in shutdown and bolt_server
handling only calls shutdown(), which stops the loop but leaves the listening
socket open. Update the shutdown logic around bolt_server to call server_close()
after a successful shutdown() so the port is released during teardown, and keep
the existing exception logging in place for failures.
---
Outside diff comments:
In `@tests/test_shutdown.py`:
- Around line 90-105: The shutdown pool test currently only asserts that
shutdown_services() calls closeall() on a MagicMock, which does not prove
connections are actually released. Replace the MagicMock pool in
test_shutdown_services_closes_pool with a stateful fake or integration-style
pool that tracks checked-out connections, then verify after shutdown_services()
completes that the pool’s active connection count is 0. Keep the existing
shutdown_services() and pool.closeall() flow, but strengthen the assertion to
validate real connection cleanup rather than just method invocation.
---
Nitpick comments:
In `@tests/test_bolt_server.py`:
- Around line 18-21: The free-port helper in _find_free_port is causing a race
because the socket is closed before the server starts, so update the test setup
to stop probing for a port and instead let the Bolt server bind to port 0
directly. Then, in the server startup/fixture code that uses _find_free_port and
the Bolt server launch path, read back the actual assigned port from the bound
socket/server object and use that value for the client connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb63cc85-da3f-48b6-a189-71b6b577369b
📒 Files selected for processing (7)
src/paperscout/__main__.pysrc/paperscout/bolt_server.pysrc/paperscout/db.pysrc/paperscout/shutdown.pytests/test_bolt_server.pytests/test_main_health_merge.pytests/test_shutdown.py
wpak-ai
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pool_status()with only publicgetconn/putconnfor DB pool health checks (with pinned minimum version note).app.start()+ privateapp._development_server._servershutdown with an ownedHTTPServerthat dispatches via publicApp.dispatch().shutdown_services()usingbolt_server.shutdown(), matching the health server pattern.Why
Private slack-bolt and psycopg2 internals created undocumented version coupling: minor dependency bumps could break health checks or graceful shutdown without clear test failures.
Test plan
pytest tests/test_db.py tests/test_shutdown.py tests/test_bolt_server.py tests/test_main_health_merge.py413 passed)Related issue
Summary by CodeRabbit