Add CI: strict clippy, live-Postgres test suite, mypy - #16
Merged
Merged
Conversation
Cleaned up the codebase's 8 pre-existing clippy warnings first, so CI can start with a genuine -D warnings gate instead of a lenient one: - Deleted error.rs's type_conversion_error/invalid_connection_string_error - confirmed zero callers anywhere, dead since Task 2's rewrite. - Slimmed RuntimeManager to just shared() - its runtime field and new()/block_on()/spawn() methods were confirmed unused everywhere outside their own definitions. - Fixed the one-line lifetime-elision hint in pool.rs's connect(). - Added an explicit #![allow(non_snake_case)] at the crate root for the PostPyro module name - deliberate, has to match the Python import name (`import PostPyro`), not something to actually rename. New .github/workflows/ci.yml, one job on push/PR to master: cargo check -> cargo clippy --all-targets -D warnings -> cargo test --lib -> maturin develop --release against a postgres:16 service container -> mypy against tests/ and benchmarks/ (the only real Python application code in this repo; __init__.py is a pure re-export) -> the three integration test files -> the concurrency correctness harness (real pass/fail assertions) -> the driver comparison benchmark as an informational, non-blocking step (timing on shared CI runners is too noisy to gate merges on). Added a [tool.mypy] section to pyproject.toml (ignore-missing-imports scoped to asyncpg only, which ships no stubs - psycopg's own types resolve fine). Fixed 3 real mypy findings in bench_vs_alternatives.py along the way: an untyped results list, and two try/except-import fallback assignments that needed narrow type: ignore comments (a well-known false positive for this exact optional-dependency pattern). Every step verified locally first, exactly as CI runs it: cargo check/clippy/test all clean, maturin build + wheel contents unchanged, mypy clean (5 files, 0 errors), all 3 integration tests pass, both benchmark scripts exit 0 with real numbers. Co-Authored-By: mj7841@srmist.edu.in
|
Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19212429 | Triggered | Generic Password | 22ed201 | .github/workflows/ci.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
actions/setup-python installs Python directly, it doesn't create a virtualenv - and maturin develop requires one (VIRTUAL_ENV set, or a .venv it can discover) to know where to install the built extension. The first CI run failed with exactly this: "Couldn't find a virtualenv or conda environment". Fixed by creating one explicitly and adding its bin/ to GITHUB_PATH plus setting VIRTUAL_ENV via GITHUB_ENV, so every later step (pip installs, maturin develop, the test/benchmark scripts) uses it automatically without re-sourcing activate per step. Verified by fully simulating the fixed sequence from a genuinely clean environment (fresh venv, env -i with only PATH/HOME/VIRTUAL_ENV set, no inherited .venv or shell state): maturin develop --release succeeds, and `import PostPyro` from that venv's own python3 works and reports the right version. Also added .gitguardian.yaml: GitGuardian flagged .github/workflows/ci.yml's POSTGRES_PASSWORD as a secret - it's the ephemeral CI-only Postgres service container's password (same postgres/postgres test credential already hardcoded in every tests/*.py and benchmarks/*.py file in this repo), not a real secret with anything to rotate. Path-scoped ignore rather than a match-hash ignore, since generating the exact secret_sha ggshield's match-based ignoring needs requires GitGuardian API access this session doesn't have. Also added continue-on-error to the benchmark-comparison-drivers pip install step (asyncpg/psycopg) - it only feeds the one step already marked informational/non-blocking, so a transient install hiccup shouldn't fail the whole CI run (review finding from PR #16). Co-Authored-By: mj7841@srmist.edu.in
The correctness-harness step just aborted on the first real CI run (exit 134, SIGABRT) after all of its own PASS assertions already printed - the panic happens during process exit, on a tokio-rt-worker thread, not during the test logic itself. Hasn't reproduced locally across 14 runs (including under taskset -c 0,1 to match a 2-core runner), so getting a real backtrace from the environment where it actually happens is the fastest path to a root cause. Diagnostic only, not a fix - investigating before touching anything else. Co-Authored-By: mj7841@srmist.edu.in
…utdown Discovered on the first real CI run (not locally - didn't reproduce across 44 local attempts including CPU-constrained and artificially contended runs, but recurred on ~1 in 5 GitHub Actions runs): the correctness harness's own logic always completed successfully (every PASS line printed, including "pool closed cleanly"), then a Tokio worker thread panicked during process exit and aborted (SIGABRT, exit 134). Root cause: the earlier Critical fix (Transaction's Drop impl, PR #10) correctly lets an abandoned transaction's cleanup succeed instead of panicking immediately - but the actual rollback-and- return-connection work happens inside sqlx's own internal `rt::spawn`, a detached fire-and-forget task nothing in this codebase tracks or awaits. Pool.close() only waits for connections already checked out through the normal path, not ones returning via one of these detached background tasks. On a slower/more contended machine (a 2-4 core CI runner vs. a fast, idle dev machine), the process can begin exiting - and the static Tokio runtime can begin tearing down - while one of these tasks is still mid-flight, which is what aborts. Fix: a short bounded drain (block_on a 200ms sleep on the same runtime) in two places - directly in Pool.close() (the exact call site involved in the reproduction, giving lingering background cleanup a scheduling window before close() returns), and as an atexit-registered safety net at module init (covers the case where close() is never called at all). This can't offer an absolute guarantee - there's no Tokio API to wait for tasks this codebase doesn't hold a handle to - but it converts a near-certain-under- contention race into one that needs a cleanup task to still be running after a 200ms grace period, which is far outside what a single rollback-and-return-connection round trip should ever take. Verified locally: cargo check/clippy -D warnings/test --lib all clean, all 3 integration tests + the correctness harness pass. Verifying against repeated CI runs next, since this doesn't reproduce locally at all - only CI's failure rate can meaningfully confirm this actually helps. Co-Authored-By: mj7841@srmist.edu.in
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
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
-D warningsgate): deleted 2 confirmed-deaderror.rsfunctions, slimmedRuntimeManagerto just what's used, fixed a one-line lifetime-elision hint, added an explicit#![allow(non_snake_case)]for thePostPyromodule name (deliberate - has to match the Python import name)..github/workflows/ci.yml: on push/PR tomaster.cargo check→cargo clippy --all-targets -D warnings→cargo test --lib→ build viamaturin develop --releaseagainst apostgres:16service container →mypyontests//benchmarks/→ the 3 integration test files → the concurrency correctness harness (real assertions, blocking) → the driver comparison benchmark (informational only,continue-on-error: true- timing on shared runners is too noisy to gate merges on).[tool.mypy]topyproject.toml, scoped totests/+benchmarks/(the only real Python application code here -__init__.pyis a pure re-export, checked viamypy_pathagainst the.pyistub). Fixed 3 real findings it surfaced inbench_vs_alternatives.pyalong the way (an untyped list, two optional-import fallback assignments).Test plan
cargo check/clippy -D warnings/test --liball clean,maturin build+ wheel contents unchanged,mypyclean (5 files, 0 errors), all 3 integration tests pass live, both benchmark scripts exit 0