test: make automatic restart recovery deterministic

[mohanagy]

Fixes #123

The automatic-restart test now gates the replacement fixture before its crash-marker preflight. It removes the crash marker before releasing the gate, proving the first retry is the recovered process and never observes stale crash state. A fixture observation marker makes the old release order deterministically reproduce restartCount 2.

Validation:

• Focused regression: passed five consecutive runs
• npm run lint
• npm run typecheck

Known separate diagnostic: a full upstream-manager test-file run reproduced the existing #122 fixture-startup cascade; it is documented on #122 and is not caused by this narrow test-ordering change.

Summary by CodeRabbit

• Tests
  • Improved coverage for automatic upstream recovery and failed startup handling.
  • Added support for recording and asserting “crash observed” artifacts during recovery flows.
  • Strengthened checks related to capacity reservation release after an unsuccessful startup, including verifying expected observed-crash markers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cf5290df-d1dd-4066-8134-41494683a754

📥 Commits

Reviewing files that changed from the base of the PR and between cbf6939 and 6958b97.

📒 Files selected for processing (1)

• tests/upstream-manager.test.ts

---

📝 Walkthrough

Walkthrough

The fake upstream now records observed tool-call crashes. Upstream-manager tests add filesystem coordination for restart timing and verify crash observation during failed startup and automatic recovery scenarios.

Changes

Upstream crash and recovery validation

Layer / File(s)	Summary
Observed crash marker support tests/fixtures/fake-upstream.mjs, tests/upstream-manager.test.ts	The fixture writes an optional observed marker for tool-call crashes, and the failed-startup test configures and verifies that marker.
Recovery timing and restart coordination tests/upstream-manager.test.ts	The bounded backoff test coordinates crash and restart gates, waits for restart readiness, and asserts that recovery did not observe an unintended crash.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• mohanagy/miftah#51: Earlier upstream crash and recovery lifecycle tests extended by this change.

Poem

A rabbit watched the crash bell ring,
Then marked the moment—small bright thing.
Gates held fast while restarts grew,
Recovery hopped cleanly through.
“No phantom crash!” the bunny sings.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	❌ Error	The PR adds regression and fixture work, but no change removing the duplicate runAutomaticRestart call is shown.	Include the upstream-process-manager fix that removes the second runAutomaticRestart invocation and keep the regression proving one timer firing causes one restart attempt.
Description check	⚠️ Warning	The description omits the required Security impact section and most of the template's validation checklist.	Add the missing Summary and Security impact sections, and expand Validation to cover the required commands and checklist items with results.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and matches the PR's focus on making automatic restart recovery deterministic.
Out of Scope Changes check	✅ Passed	The changes stay focused on deterministic restart recovery tests and supporting fixtures, which match the linked regression goal.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/123-deterministic-auto-restart

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

---

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

[mohanagy]

The failed Windows Node 24 job is unrelated to #123: all core contracts passed, then the packaged-artifact contract failed in npm run test:cli with Audit journal is unavailable after 47s. I opened #153 with the exact run/job evidence and will not alter this restart-recovery PR or retry CI as a substitute for diagnosing that separate boundary.

[mohanagy]

Current head cbf6939 addresses the independent local Fable review findings.

• The automatic-recovery test now holds the restarted child past the old 1s startup bound and uses a test-only 5s bound. This removes a second timing assumption while the dedicated 200ms hung-startup test keeps bounded-startup coverage. TDD proof: it failed first with restartCount: 2, then passed after the scoped change.
• The crash-observation assertion now has a positive control in the existing failed-start/capacity test. TDD proof: it failed when the fixture write was temporarily removed, then passed after restoration.
• Focused tests/upstream-manager.test.ts (26/26), lint, typecheck, and diff check pass.

CodeRabbit is rate-limited on this head (Review rate limited) and has no review or thread to dismiss. Per maintainer policy, the local Claude review is the fallback; I will wait for all real current-head CI jobs before merging. Separate intermittent full-suite manifestations were recorded under existing #117 and #122, with no unrelated code mixed into this PR.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@tests/upstream-manager.test.ts`:
- Around line 520-525: Replace the fixed delay(1_100) in the restart regression
test with a bounded wait for the expected restart-count or lifecycle signal,
ensuring the retry has occurred before removing crashPath and restartGatePath.
Reuse the existing waitFor or established restart-state signal, and preserve the
subsequent cleanup sequence.
- Line 508: Update the test setup around TEST_CRASH_ON_CALL_TOOL_OBSERVED_PATH
and the recovery assertions to clear the initial crash marker after verifying
the expected first-call failure, or use a distinct marker for recovery. Ensure
the recovery toBe(false) assertion checks only whether the recovery attempt
triggered the crash fixture, while preserving the initial crashPath expectation.

🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 041f1e36-e04f-45ef-a100-8b476385504a

📥 Commits

Reviewing files that changed from the base of the PR and between b667f79 and cbf6939.

📒 Files selected for processing (2)

• tests/fixtures/fake-upstream.mjs
• tests/upstream-manager.test.ts

Dismissed under maintainer-approved CodeRabbit rate-limit exception. Both original threads are addressed in fe7394b and 6958b97; GitHub reports zero unresolved review threads. Current-head CodeRabbit re-review is Review rate limited. The exact rationale, focused validation, and local Fable boundary review are documented on this PR; real CI remains required.

[mohanagy]

@coderabbitai review

Please run a fresh review of current head 6958b971001f1bd67d44dc18737e49570dd308b1. The prior two findings were addressed in fe7394b and 6958b97; the earlier re-review was rate-limited.

[coderabbitai]

@mohanagy: Starting a fresh review of the current head 6958b971001f1bd67d44dc18737e49570dd308b1.

✅ 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.