Fix TempDir leak in embedded Postgres tests#155
Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change updates the embedded PostgreSQL test utility to return and retain ownership of the temporary directory used for the database instance. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Util as test-util::start_embedded_postgres
participant PG as Embedded PostgreSQL
participant Temp as TempDir
participant Server as TestServer
Test->>Util: Call start_embedded_postgres(setup)
Util->>Temp: Create TempDir
Util->>PG: Start embedded PostgreSQL with TempDir path
Util-->>Test: Return EmbeddedPg (db_url, PG, TempDir)
Test->>Server: Pass db_url, PG, TempDir to launch
Server->>Server: Store TempDir for resource management
Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuidePreserve TempDir handle for embedded Postgres in test utilities to prevent directory leaks by returning the handle from start_embedded_postgres and storing it in PostgresTestDb and TestServer. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test-util/src/lib.rs (1)
77-84: Drop-order bug –_tempis dropped beforepg
TempDiris declared beforepg, so it is dropped first whenTestServeris destroyed (Rust drops fields in declaration order).
IfPostgreSQL’sDropimplementation (executed afterwards) still references the data directory, the directory will already be gone, risking IO errors or silent inconsistencies.Move
_tempafterpgto guarantee the directory outlives the database instance:pub struct TestServer { child: Child, port: u16, db_url: String, - _temp: Option<TempDir>, #[cfg(feature = "postgres")] pg: Option<PostgreSQL>, + _temp: Option<TempDir>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-util/src/lib.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (actions)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: coverage
🔇 Additional comments (3)
test-util/src/lib.rs (3)
133-141: Double runtime creation guard is solid – LGTMThe
try_current/newfallback cleanly handles both async and sync contexts and now propagatesTempDircorrectly.
Nice work.
245-246: Correct placement inPostgresTestDbKeeping
_tempafterpgensures the directory lives longer than the Postgres process – good attention to detail.
446-456: Happy path looks correctCall-site propagation of
(db_url, pg, temp)matches the new launch signature; compile-time features gate the alternatives cleanly.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Rename the private
_tempfields inPostgresTestDbandTestServerto_temp_dirso it’s immediately clear they hold aTempDirhandle. - Consider replacing the
(String, PostgreSQL, TempDir)tuple return fromstart_embedded_postgreswith a small struct to improve readability and avoid positional confusion. - Add a brief inline comment next to the
TempDirfields explaining that they’re kept alive to prevent automatic cleanup during test execution.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rename the private `_temp` fields in `PostgresTestDb` and `TestServer` to `_temp_dir` so it’s immediately clear they hold a `TempDir` handle.
- Consider replacing the `(String, PostgreSQL, TempDir)` tuple return from `start_embedded_postgres` with a small struct to improve readability and avoid positional confusion.
- Add a brief inline comment next to the `TempDir` fields explaining that they’re kept alive to prevent automatic cleanup during test execution.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-util/src/lib.rs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: coverage
- GitHub Check: build-test (postgres)
🔇 Additional comments (1)
test-util/src/lib.rs (1)
109-117: 👍 Switched totempfile::Builderwith prefix – clearer diagnosticsThe change implements the earlier nitpick; log directories will now be
prefixed withmxd-pg, which is useful when inspecting/tmp.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
PostgresTestDbandTestServerTesting
cargo fmt --allcargo clippy -- -D warningsRUSTFLAGS="-D warnings" cargo testhttps://chatgpt.com/codex/tasks/task_e_6851b94c35448322b8ee0881a04629f1
Summary by Sourcery
Retain the temporary directory handle for embedded Postgres instances to prevent premature cleanup of the data directory in tests.
Bug Fixes:
TempDirhandle.Enhancements:
start_embedded_postgresto return theTempDirand updatePostgresTestDbandTestServerto store it.Summary by CodeRabbit