fix(tests): stop leaked RabbitMQ test container on process exit#1573
fix(tests): stop leaked RabbitMQ test container on process exit#1573ytallo wants to merge 1 commit into
Conversation
The shared RabbitMQ test container was held in a static OnceCell, but Rust does not run destructors on static items at process exit, so ContainerAsync::drop never fired and the container was never stopped. Record the container id and register a libc::atexit hook that runs `docker rm -f <id>` synchronously on exit. libc is already a dev-dep, so no new dependencies.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRabbitMQ test helper now stores container ID in a global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/tests/common/rabbitmq_helpers.rs`:
- Around line 25-35: Ensure the atexit handler is registered only after
CONTAINER_ID is set and assert that libc::atexit(stop_rabbitmq_container)
returns 0 (e.g. assert_eq!(..., 0, "failed to register RabbitMQ cleanup hook"))
so failures surface at init time; inside stop_rabbitmq_container, avoid using
eprintln! (unsafe during shutdown) — either suppress docker rm output or write
diagnostics using a low-level write to fd 2 if you must report errors, and check
the std::process::Command::status() result for non-zero and handle it (e.g. emit
minimal low-level error or ignore on success) instead of silently discarding it;
reference CONTAINER_ID, stop_rabbitmq_container, and libc::atexit when making
these changes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5aede9a0-750b-48ff-a5c2-69390a64e56a
📒 Files selected for processing (1)
engine/tests/common/rabbitmq_helpers.rs
| extern "C" fn stop_rabbitmq_container() { | ||
| let id = match CONTAINER_ID.lock() { | ||
| Ok(mut guard) => guard.take(), | ||
| Err(_) => return, | ||
| }; | ||
| if let Some(id) = id { | ||
| let _ = std::process::Command::new("docker") | ||
| .args(["rm", "-f", &id]) | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .status(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full file to understand the context
cd engine/tests/common && wc -l rabbitmq_helpers.rs && cat -n rabbitmq_helpers.rsRepository: iii-hq/iii
Length of output: 5862
🏁 Script executed:
# Also check if there's a git diff available to see what changed
git diff HEAD -- engine/tests/common/rabbitmq_helpers.rsRepository: iii-hq/iii
Length of output: 36
🏁 Script executed:
# Check what changed in the file around lines 2, 18-37, 60-67
git log --oneline -1 -- engine/tests/common/rabbitmq_helpers.rsRepository: iii-hq/iii
Length of output: 127
🏁 Script executed:
# Get the actual patch
git show HEAD -- engine/tests/common/rabbitmq_helpers.rs | head -100Repository: iii-hq/iii
Length of output: 4055
🌐 Web query:
atexit handler safety constraints libc async-safe functions eprintln
💡 Result:
atexit handlers in libc have strict safety constraints: they must terminate normally without calling exit again (undefined behavior), avoid longjmp, and cannot rely on consistent global state since they run during process shutdown after static destructors in some cases (e.g., Rust). They are NOT required to be async-signal-safe by POSIX; only _exit and _Exit are signal-safe among termination functions, and stdio functions like printf are unsafe in signal handlers due to buffering and locks. Using eprintln! (Rust's stderr println macro) in an atexit handler is unsafe: - It accesses global Stderr state via Lazy/OnceCell, which may be deinitialized during shutdown, causing panics like "cannot access stderr during shutdown". - println!/eprintln! use thread-local RefCell-wrapped stdout/stderr locks; concurrent calls (e.g., from signal interrupting print) panic on BorrowMutError. - In Rust+C interop, libc::atexit handlers run after Rust's at_exit queue, during/after TLS/static destruction, making std::io access unreliable. Safe alternatives in atexit handlers: - Use low-level write(2) to fd 2 (stderr), which is async-signal-safe. - libc::printf may work if no Rust runtime interference, but not guaranteed async-safe. - Avoid heap allocation, locks, or complex logic. For Rust programs, prefer std::process::exit over libc::exit to run Rust cleanup; for signal safety, use crates like signal-hook and async-signal-safe primitives. No source confirms eprintln! safe in atexit; evidence shows failures.
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/atexit.html
- 2: https://manned.org/atexit.3
- 3: https://stackoverflow.com/questions/35980148/why-does-an-atexit-handler-panic-when-it-accesses-stdout
- 4: https://manpages.debian.org/trixie/manpages/signal-safety.7.en.html
- 5: https://man7.org/linux/man-pages/man7/signal-safety.7.html
- 6: Fix abort-on-eprintln during process shutdown rust-lang/rust#69955
- 7: https://www.man7.org/linux/man-pages/man3/atexit.3.html
- 8: https://eelis.net/c++draft/support.start.term
- 9: https://devdoc.net/c/glibc_2.26/Cleanups-on-Exit.html
- 10: https://stackoverflow.com/questions/44865737/println-panics-when-called-concurrently
Handle initialization and cleanup errors explicitly, but use safe atexit patterns.
The new exit-cleanup path silently discards both libc::atexit() registration status and docker rm exit codes. While the implementation is new (not a regression), it should surface failures to prevent silent leaks.
However, the suggested fix has two critical issues:
-
eprintln!is unsafe in atexit handlers. It accesses global Stderr state which may be deinitialized during process shutdown, causing panics. Use low-levelwrite(2)to stderr fd 2 instead, or avoid output altogether. -
Suggested ordering is backwards. The proposed diff registers the atexit handler before setting
CONTAINER_ID, so the handler would run with no ID to clean up.
Safer approaches:
- Assert that atexit registration succeeded:
assert_eq!(libc::atexit(stop_rabbitmq_container), 0, "failed to register RabbitMQ cleanup hook");(now runs at init time, fails early and explicitly) - For the docker removal, either suppress output if success is expected, or use low-level
write(2)if diagnostics are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/tests/common/rabbitmq_helpers.rs` around lines 25 - 35, Ensure the
atexit handler is registered only after CONTAINER_ID is set and assert that
libc::atexit(stop_rabbitmq_container) returns 0 (e.g. assert_eq!(..., 0, "failed
to register RabbitMQ cleanup hook")) so failures surface at init time; inside
stop_rabbitmq_container, avoid using eprintln! (unsafe during shutdown) — either
suppress docker rm output or write diagnostics using a low-level write to fd 2
if you must report errors, and check the std::process::Command::status() result
for non-zero and handle it (e.g. emit minimal low-level error or ignore on
success) instead of silently discarding it; reference CONTAINER_ID,
stop_rabbitmq_container, and libc::atexit when making these changes.
1 participant
Summary
static OnceCell, and Rust does not run destructors onstaticitems at process exit, soContainerAsync::dropnever fired —cargo testwould leave arabbitmqcontainer running after the suite finished.libc::atexithook that runsdocker rm -f <id>synchronously on exit.libcis already a dev-dependency, so no new deps.Test plan
cargo test -p engine --test rabbitmq_queue_integrationand confirmdocker psshows no leftoverrabbitmqcontainer after the run.Summary by CodeRabbit