refactor(daemon): extract fake server test fixtures#296
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDaemon test fixtures are extracted into standalone Python and POSIX shell files. Rust tests embed and materialize these assets, validate RustFS and gateway templates, and add contract coverage for fixture CLI, lifecycle, protocol, and filesystem behavior. ChangesDaemon fixture extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FixtureContractTest
participant RustFixtureHarness
participant FixtureProcess
participant LoopbackServices
FixtureContractTest->>RustFixtureHarness: materialize fixture and set executable
RustFixtureHarness->>FixtureProcess: run CLI with test arguments
FixtureProcess->>LoopbackServices: bind ports and serve fixture protocol
FixtureContractTest->>LoopbackServices: probe readiness and filesystem effects
FixtureProcess-->>RustFixtureHarness: return exit status and output
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — this PR extracts daemon fake-server programs into checked-in fixture assets while preserving the tests' CLI, protocol, lifecycle, and supervision contracts.
- Extract Managed Resource fixtures — MySQL, Mailpit, Postgres, Redis, and RustFS test programs now live under
crates/daemon/test-fixtures/managed-resources/and are embedded withinclude_str!. - Protect translated CLI contracts —
fixture_contracts.rsand fiveinstasnapshots cover parser compatibility, filesystem effects, readiness, and stable failure output for wrappers translated to Python. - Preserve gateway process topology — FrankenPHP shell parents retain signal traps, child waits, and exit propagation while sibling Python assets host the extracted HTTP and TLS server bodies.
- Extract supervisor ownership coverage — the macOS direct-Python ownership fixture is materialized from a standalone source without changing its supervised executable path.
- Document fixture requirements and design —
CONTRIBUTING.mdrecords thepython3prerequisite, and the design and implementation plan capture the extraction boundary and verification contract.
GPT Sol ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 249488c824
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,154 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
When this fixture is launched as the recorded ProcessSpec.command and python3 comes from a shim such as pyenv/asdf, ps reports the wrapper before the script (for example, bash ... pyenv-exec python3 <fixture>). live_process_matches_spec only recognizes the executable token or token 1 as the script path, so later supervisor.adopt(_recorded) calls stop recognizing the still-running RustFS/Mailpit/etc. fixture; repeated reconciliation can then treat the listener as non-PV or clean up pid/port records without stopping the process. Keeping a shell parent for adopted fixtures or broadening ownership verification avoids environment-dependent test failures and leaks.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
crates/daemon/test-fixtures/gateway/fake-frankenphp-server.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider explicitly closing the file or using
Path.read_text.Although this is a test fixture and the file will be closed during garbage collection or exit, it is better practice to use a context manager or
pathlibto avoid unclosed file handles.♻️ Proposed refactor
-config = open(sys.argv[1], encoding="utf-8").read() +from pathlib import Path +config = Path(sys.argv[1]).read_text(encoding="utf-8")🤖 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 `@crates/daemon/test-fixtures/gateway/fake-frankenphp-server.py` at line 12, Update the configuration read in the fake FrankenPHP server fixture to use pathlib.Path.read_text or an explicit context manager, ensuring the file handle is closed deterministically while preserving the existing UTF-8 decoding and command-line path behavior.
🤖 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 `@crates/daemon/test-fixtures/managed-resources/postgres.py`:
- Around line 278-282: Add daemon_threads = True to the Server class so
PostgreSQL request-handler threads are daemonized and cannot keep the fixture
alive after SIGTERM. Preserve the existing allow_reuse_address setting and
server initialization.
In `@crates/daemon/tests/fixture_contracts.rs`:
- Around line 353-356: The run_fixture helper in
crates/daemon/tests/fixture_contracts.rs (lines 353-356) must enforce a deadline
for fixture subprocesses, killing and reaping any child that times out; update
the direct MySQL invocation at lines 98-104 to use run_fixture so it receives
the same timeout behavior.
---
Nitpick comments:
In `@crates/daemon/test-fixtures/gateway/fake-frankenphp-server.py`:
- Line 12: Update the configuration read in the fake FrankenPHP server fixture
to use pathlib.Path.read_text or an explicit context manager, ensuring the file
handle is closed deterministically while preserving the existing UTF-8 decoding
and command-line path behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78ac554a-2da6-4552-abe7-fc7961ae46d8
⛔ Files ignored due to path filters (5)
crates/daemon/tests/snapshots/fixture_contracts__fake_mailpit_fixture_cli_ignores_extra_arguments.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/fixture_contracts__mailpit_fixture_cli_preserves_shell_contract.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/fixture_contracts__mysql_fixture_cli_preserves_shell_contract.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/fixture_contracts__postgres_fixture_cli_preserves_shell_contract.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/fixture_contracts__rustfs_fixture_cli_preserves_shell_contract.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
CONTRIBUTING.mdcrates/daemon/src/managed_resources/mysql_tests.rscrates/daemon/src/managed_resources/tests.rscrates/daemon/test-fixtures/gateway/fake-frankenphp-hangs-on-port-server.pycrates/daemon/test-fixtures/gateway/fake-frankenphp-hangs-on-port.sh.incrates/daemon/test-fixtures/gateway/fake-frankenphp-server.pycrates/daemon/test-fixtures/gateway/fake-frankenphp.shcrates/daemon/test-fixtures/managed-resources/fake-mailpit.pycrates/daemon/test-fixtures/managed-resources/mailpit-fast-exit.pycrates/daemon/test-fixtures/managed-resources/mailpit-unready.shcrates/daemon/test-fixtures/managed-resources/mailpit.pycrates/daemon/test-fixtures/managed-resources/mysql.pycrates/daemon/test-fixtures/managed-resources/postgres-initdb.shcrates/daemon/test-fixtures/managed-resources/postgres-unready.shcrates/daemon/test-fixtures/managed-resources/postgres.pycrates/daemon/test-fixtures/managed-resources/redis-server.pycrates/daemon/test-fixtures/managed-resources/rustfs.py.incrates/daemon/test-fixtures/supervisor/owned-python-runtime.pycrates/daemon/tests/fixture_contracts.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/supervisor_foundation.rsdocs/superpowers/plans/2026-07-15-daemon-test-fixtures.mddocs/superpowers/specs/2026-07-15-daemon-test-fixtures-design.md
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the eight commits since the prior Pullfrog review harden fixture subprocess and threaded-handler shutdown behavior without changing production paths or fixture CLI contracts.
- Bounded fixture-contract commands — Added deadline polling and bounded kill-and-reap cleanup for fixture subprocesses, including a regression that verifies timed-out children are gone.
- Daemonized fixture request handlers — Set
daemon_threads = Trueon the custom MySQL, PostgreSQL, fake Mailpit, and MailpitThreadingMixInservers so blocked clients cannot hold fixture processes open. - Synchronized lifecycle regressions — Added MySQL and PostgreSQL idle-client shutdown tests that wait for an opt-in
PV_FIXTURE_HANDLER_STARTEDmarker before sendingSIGTERM. - Aligned fixture documentation — Updated the extraction design and implementation plans to record the accepted bounded-cleanup and handler-synchronization behavior.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/daemon/tests/fixture_contracts.rs (1)
483-535: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
run_fixture_commandcan spuriously time out on verbose fixtures. Pollingtry_wait()whilestdout/stderrare piped leaves both streams undrained until exit is already observed, so a child that fills the pipe buffer can block inwrite()and never reachSome(_). Drain output concurrently, or movewait_with_output()off the polling path and enforce the deadline from outside.🤖 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 `@crates/daemon/tests/fixture_contracts.rs` around lines 483 - 535, Update run_fixture_command so piped stdout and stderr are drained while the child runs, preventing verbose fixtures from blocking on full pipes; use concurrent readers or move wait_with_output off the polling path while retaining deadline enforcement and kill_and_reap_child cleanup on timeout or errors.
🤖 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 `@docs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md`:
- Around line 341-344: Update the timeout assertion in the fixture command
regression test to allow both polling overshoots: retain
FIXTURE_SHUTDOWN_TIMEOUT and increase the poll-interval allowance to at least 2
* FIXTURE_COMMAND_POLL_INTERVAL, with any existing scheduling margin preserved.
- Around line 479-492: Update wait_for_path to wait for the marker file’s
complete expected contents, such as “started\n”, rather than returning when
path_exists first succeeds. Continue polling until the contents match, while
preserving the existing timeout and error behavior.
- Around line 497-505: Make the PostgreSQL marker-placement contract match the
implementation: in the PostgreSQL Handler.handle flow, place the
PV_FIXTURE_HANDLER_STARTED write-and-close block immediately before the initial
read_startup(self.request), after any statements and portals initialization, or
revise the documentation wording if handler-entry synchronization is the
intended contract. Keep the MySQL placement requirement unchanged.
---
Outside diff comments:
In `@crates/daemon/tests/fixture_contracts.rs`:
- Around line 483-535: Update run_fixture_command so piped stdout and stderr are
drained while the child runs, preventing verbose fixtures from blocking on full
pipes; use concurrent readers or move wait_with_output off the polling path
while retaining deadline enforcement and kill_and_reap_child cleanup on timeout
or errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 396cc12a-cbe2-46c2-a1be-b2c422f0ff82
📒 Files selected for processing (8)
crates/daemon/test-fixtures/managed-resources/fake-mailpit.pycrates/daemon/test-fixtures/managed-resources/mailpit.pycrates/daemon/test-fixtures/managed-resources/mysql.pycrates/daemon/test-fixtures/managed-resources/postgres.pycrates/daemon/tests/fixture_contracts.rsdocs/superpowers/plans/2026-07-15-daemon-test-fixtures.mddocs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.mddocs/superpowers/specs/2026-07-15-daemon-test-fixtures-design.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/daemon/test-fixtures/managed-resources/fake-mailpit.py
- crates/daemon/test-fixtures/managed-resources/mailpit.py
- docs/superpowers/plans/2026-07-15-daemon-test-fixtures.md
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the thirteen commits since the prior Pullfrog review complete the fixture-runner and signal-lifecycle hardening while preserving the extracted fixtures' historical behavior.
- Made fixture command execution backpressure-safe —
run_fixture_commandnow captures output through temporary files, requires complete handler markers, and uses a realistic timeout budget. - Preserved deterministic signal outcomes — MySQL, PostgreSQL, Redis, fake Mailpit, Mailpit, and RustFS now close their listeners orderly before reproducing the received
SIGTERMorSIGINTstatus. - Hardened process cleanup — process-group cleanup avoids signaling a recycled PGID after its original leader is reaped, and the RustFS allocation-failure regression stops its recorded fixture runtime.
- Expanded lifecycle regression coverage — the fixture contracts exercise all six server producers under both signals, PostgreSQL's shutdown race, verbose output, complete markers, and stale process-group cleanup.
- Aligned fixture sources and documentation — the FrankenPHP config read now closes explicitly, and the correction plan records the final lifecycle contracts.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md (1)
557-562: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a POSIX loop here.
{1..20}is Bash-only, so these commands won’t perform 20 runs undersh; switch to a portable loop or note Bash as a prerequisite. Also applies to: 1030-1041.🤖 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 `@docs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md` around lines 557 - 562, Replace the Bash-specific {1..20} iteration syntax in both fixture test loops with a POSIX-compatible loop that executes each command 20 times. Apply the same change to the corresponding loops around the later referenced section, preserving the existing cargo nextest commands and test filters.
🤖 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 `@crates/daemon/tests/fixture_contracts.rs`:
- Around line 400-402: Update the postgreSQL fixture status check around
status.success() so the test requires termination specifically by the injected
SIGTERM, rejecting both successful exits and other signals or exit statuses.
Preserve the existing bail message for unexpected termination while validating
status.signal() equals Signal::TERM.as_raw().
In `@docs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md`:
- Around line 1023-1045: Add an explicit ShellCheck validation command to the
Final Verification checklist, covering all extracted checked-in .sh fixtures and
shell templates alongside the existing Rust and Python checks. Use a recursive
file selection that includes both fixture scripts and templates.
- Around line 1042-1050: Update the final verification commands around git diff
--name-only, git ls-files --others, and git status --short to compare their
output against the documented expected file set and clean-state expectations,
failing when unexpected files or uncommitted changes are present. Preserve the
existing diff and snapshot checks, but replace these print-only commands with
explicit test or comparison assertions.
---
Outside diff comments:
In `@docs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md`:
- Around line 557-562: Replace the Bash-specific {1..20} iteration syntax in
both fixture test loops with a POSIX-compatible loop that executes each command
20 times. Apply the same change to the corresponding loops around the later
referenced section, preserving the existing cargo nextest commands and test
filters.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b843693e-85db-4d4b-b1dc-3d99f9398b76
⛔ Files ignored due to path filters (2)
crates/daemon/tests/snapshots/fixture_contracts__fixture_command_captures_verbose_output_without_pipe_backpressure.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/fixture_contracts__fixture_handler_marker_requires_complete_contents.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
crates/daemon/src/managed_resources/tests.rscrates/daemon/test-fixtures/gateway/fake-frankenphp-server.pycrates/daemon/test-fixtures/managed-resources/fake-mailpit.pycrates/daemon/test-fixtures/managed-resources/mailpit.pycrates/daemon/test-fixtures/managed-resources/mysql.pycrates/daemon/test-fixtures/managed-resources/postgres.pycrates/daemon/test-fixtures/managed-resources/redis-server.pycrates/daemon/test-fixtures/managed-resources/rustfs.py.incrates/daemon/tests/fixture_contracts.rsdocs/superpowers/plans/2026-07-16-daemon-fixture-review-corrections.md
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/daemon/test-fixtures/managed-resources/redis-server.py
- crates/daemon/test-fixtures/gateway/fake-frankenphp-server.py
- crates/daemon/test-fixtures/managed-resources/rustfs.py.in
- crates/daemon/src/managed_resources/tests.rs
| if !status.success() && status.signal() != Some(Signal::TERM.as_raw()) { | ||
| bail!("PostgreSQL fixture exited unexpectedly after injected SIGTERM: {status}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require termination by the injected SIGTERM.
This condition also accepts exit code 0, allowing signal-preservation regressions to pass.
Proposed fix
- if !status.success() && status.signal() != Some(Signal::TERM.as_raw()) {
+ if status.signal() != Some(Signal::TERM.as_raw()) {
bail!("PostgreSQL fixture exited unexpectedly after injected SIGTERM: {status}");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !status.success() && status.signal() != Some(Signal::TERM.as_raw()) { | |
| bail!("PostgreSQL fixture exited unexpectedly after injected SIGTERM: {status}"); | |
| } | |
| if status.signal() != Some(Signal::TERM.as_raw()) { | |
| bail!("PostgreSQL fixture exited unexpectedly after injected SIGTERM: {status}"); | |
| } |
🤖 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 `@crates/daemon/tests/fixture_contracts.rs` around lines 400 - 402, Update the
postgreSQL fixture status check around status.success() so the test requires
termination specifically by the injected SIGTERM, rejecting both successful
exits and other signals or exit statuses. Preserve the existing bail message for
unexpected termination while validating status.signal() equals
Signal::TERM.as_raw().
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the final two commits tighten the PostgreSQL shutdown regression and make the documented verification procedure portable and failure-sensitive.
- Required the injected PostgreSQL signal status —
postgres_fixture_shutdown_is_deterministic_after_sigtermnow rejects successful exits and any signal other thanSIGTERM. - Made repetition commands POSIX-compatible — the correction plan now uses explicit
whileloops instead of Bash brace expansion. - Strengthened final verification — the plan now runs ShellCheck across all fixture shell sources and asserts the expected diff, untracked-file, and worktree state.
GPT Sol ｜ 𝕏
1 participant
Summary
instasnapshotspython3test prerequisite and the final fixture-contract implementation planWhy
The embedded multi-language scripts were difficult to read, review, and maintain. Standalone fixtures make the simulated daemon behavior explicit and reusable without changing production behavior or broadening the test scope.
Behavior and scope
100644assets and become executable only when materialized by testspv-releasechangesValidation
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo nextest run --workspace --all-features --locked— 1,009 passed, 6 skippedNote
A rare passing nextest
LEAKlabel was stress-investigated and traced to its 200 ms capture-EOF observation window under parallel macOS scheduling, not a surviving fixture process. Serialized/isolated runs and runs with a 1-second window were clean, and immediate process inspection found no current-run descendants.Summary by CodeRabbit
Tests
Documentation
python3-on-PATHprerequisite for applicable daemon tests.