Skip to content

test: stabilize POSIX descendant containment - #177

Merged
mohanagy merged 1 commit into
developmentfrom
fix/176-posix-descendant-test
Jul 22, 2026
Merged

test: stabilize POSIX descendant containment#177
mohanagy merged 1 commit into
developmentfrom
fix/176-posix-descendant-test

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • replace the cold Node-on-Node POSIX containment fixture with a direct, test-only POSIX provider fixture that creates a real descendant and retains inherited streams;
  • gate only the initial test timeout until the descendant is demonstrably running, then invoke the same runner timeout callback while real termination and force-kill timers remain active;
  • observe and drain the pending command in every readiness-failure path so the test cannot leave an unhandled rejection behind.

Root cause

The production runner starts its timeout immediately after spawning. The former fixture had to cold-start a Node provider, cold-start a second Node descendant, and asynchronously write the descendant PID. Under host scheduling pressure the valid timeout cleanup could kill the group before that record existed, so the test failed before it had observed the expected command rejection.

Safety and behavior

This does not change production source, configured timeout values, Windows coverage, shell: false, or POSIX process-group containment. The new fixture is copied to a private test sandbox and made executable there; it is invoked directly through the existing shell-free runner path.

Validation

  • focused descendant tests: 15 repeated runs, all passing
  • npm test
  • npm run test:core
  • npm run test:coverage
  • npm run lint
  • npm run typecheck
  • npm run build
  • npm run smoke:cli
  • npm run check:pack
  • npm run test:package

Refs #176

Summary by CodeRabbit

  • Bug Fixes

    • Improved process cleanup when provider commands time out, are cancelled, or exit while descendant processes remain active.
    • Ensured retained descendant processes are force-terminated reliably on POSIX systems.
  • Tests

    • Added POSIX coverage for descendant-process handling.
    • Improved test determinism for timeout and termination scenarios.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a POSIX descendant-provider fixture and updates descendant termination tests with POSIX polling, command observation, deterministic timeout control, and process cleanup assertions.

Changes

POSIX descendant termination testing

Layer / File(s) Summary
POSIX descendant provider fixture
tests/fixtures/posix-descendant-provider.sh
Adds fixture modes for recording descendant PIDs, early provider exits, persistent streams, readiness signaling, and SIGTERM recording.
Polling and timeout control
tests/secret-providers.test.ts
Adds POSIX PID/condition polling, command outcome observation, executable installation, and held-timeout helpers.
Descendant termination test integration
tests/secret-providers.test.ts
Updates non-Windows termination tests to use the POSIX fixture, trigger deterministic timeouts, assert failures, and clean up processes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Possibly related PRs

  • mohanagy/miftah#121 — Both PRs coordinate descendant readiness, termination, and orphan checks in tests/secret-providers.test.ts.

Poem

A rabbit hops where descendants run,
Recording each PID beneath the sun.
Timers pause, then gently spring,
Stubborn streams surrender their string.
POSIX tests now cleanly sing.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change to POSIX descendant containment.
Description check ✅ Passed The description covers summary, rationale, safety, and validation with concrete results, though it uses a custom security heading instead of the template heading.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/176-posix-descendant-test

Comment @coderabbitai help to get the list of available commands.

@mohanagy
mohanagy marked this pull request as ready for review July 22, 2026 07:30
@mohanagy

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mohanagy
mohanagy merged commit f8cd4ed into development Jul 22, 2026
21 of 23 checks passed
@mohanagy
mohanagy deleted the fix/176-posix-descendant-test branch July 22, 2026 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant