fix(server): prevent crashed platform teardown from destroying its replacement by silverbucket · Pull Request #1167 · sockethub/sockethub

silverbucket · Jul 5, 2026

Summary

Fixes the Parent Process Sudden Termination CI flake (#1166), which turned out to be a real crash-recovery race in the server, not a timing issue.

When a platform child crashes, its close event fires tens to hundreds of ms after the process dies. If a client message arrives in that window, ProcessManager.ensureProcess creates a replacement instance under the same identifier and Redis queue name. The stale instance's shutdown() then destroyed the replacement's shared resources:

queue.shutdown() paused and obliterate({force: true})d the shared-name queue, deleting the replacement's pending jobs — this is what vaporized the flaky test's sigterm job (8s of total silence in the failing run's logs)
platformInstances.delete(this.id) unconditionally evicted the replacement from the instance map, so the next message spawned yet another child — the duplicate queue-inits and duplicate platform boots visible in the failing CI logs

The ensureProcess replacement path had the same defect ordering: it constructed the replacement first and then fired void existing.shutdown(), whose delayed delete/obliterate landed on the new instance.

Changes

PlatformInstance.markReplaced() — flags an instance as superseded so its teardown leaves shared resources alone
PlatformInstance.shutdown() — closes queue connections without pause/obliterate when replaced or no longer the map-slot owner; only deletes the map entry it still owns
ProcessManager.ensureProcess() — marks a dead instance replaced before firing its async shutdown, and does both before constructing the replacement
JobQueue.disconnect() (data-layer) — closes connections without pausing or obliterating the underlying Redis queue

Verification

Unit tests added for all three behaviors (609 pass)
Standalone repro (crash the dummy platform, fire echoes every 5ms into the crash-to-close window, 3 cycles):
- before: 2–3 leaked duplicate dummy children after churn, jobs silently lost
- after: exactly 1 child in every run
Root-cause analysis with CI log timeline in platform crash-recovery race: stale instance shutdown obliterates replacement's queue and evicts it from the instance map #1166

Deliberately unchanged: jobs enqueued between a child's death and its close event (while the dead instance still legitimately owns the queue) are still obliterated with it — preserving them would risk replaying the poison job that killed the platform (BullMQ stalled-job retry).

Fixes #1166

Summary by CodeRabbit

New Features
- Added a clean queue disconnection path for teardown scenarios, without pausing or obliterating the underlying queue.
Bug Fixes
- Improved replacement lifecycle handling to prevent the active replacement instance from being removed or disrupted during shutdown.
- Enhanced teardown behavior to disconnect queue/event connections safely when instances are superseded.
Tests
- Expanded coverage for shutdown/replacement and disconnection behavior, including error propagation and lifecycle edge cases.

…placement When a platform child crashes, its close event fires tens to hundreds of ms after the process dies. A client message arriving in that window makes ProcessManager create a replacement instance under the same identifier and Redis queue name. The stale instance's shutdown() then destroyed the replacement's shared resources: queue.shutdown() paused and obliterated the shared-name queue (deleting the replacement's pending jobs), and platformInstances.delete(this.id) evicted the replacement from the map (causing duplicate child processes on the next message). - PlatformInstance.markReplaced(): flags an instance superseded by a replacement so its teardown leaves shared resources alone - shutdown() closes queue connections without pause/obliterate when the instance was replaced or no longer owns its map slot, and only deletes the map entry it still owns - ProcessManager.ensureProcess() marks a dead instance replaced before firing its async shutdown, and does so before constructing the replacement - JobQueue.disconnect(): closes connections without pausing or obliterating the underlying Redis queue Fixes the 'Parent Process Sudden Termination' CI flake: the SIGTERM test's job was enqueued right as the previous crash's close-event teardown ran, got obliterated, and no platform ever received it. Fixes #1166

coderabbitai · Jul 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 485b9be8-a6e8-455d-b6e1-ef8594bfad08

📥 Commits

Reviewing files that changed from the base of the PR and between 2f1ba19 and 55b2fc4.

📒 Files selected for processing (1)

packages/data-layer/src/job-queue.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

packages/data-layer/src/job-queue.test.ts

📝 Walkthrough

Walkthrough

Adds JobQueue.disconnect(), makes PlatformInstance.shutdown() replacement-aware, and updates ProcessManager.ensureProcess() to mark stale instances before starting teardown so replacement instances keep their shared queue and map entry.

Changes

Queue disconnect and instance replacement lifecycle

Layer / File(s)	Summary
JobQueue disconnect method `packages/data-layer/src/job-queue.ts`, `packages/data-layer/src/job-queue.test.ts`	Adds `disconnect()` to close queue/events connections and remove listeners without pausing or obliterating; tests verify the call sequence and error propagation.
PlatformInstance replacement shutdown `packages/server/src/platform-instance.ts`, `packages/server/src/platform-instance.test.ts`	Adds internal replacement-state tracking and `markReplaced()`; `shutdown()` now conditionally disconnects or shuts down the queue and only deletes `platformInstances[this.id]` when still current.
ProcessManager replacement ordering `packages/server/src/process-manager.ts`, `packages/server/src/process-manager.test.ts`	`ensureProcess()` marks a non-reusable existing instance as replaced and starts its shutdown before creating or reusing the next instance; tests cover replacement ordering, map preservation, and reuse behavior.

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

Sequence Diagram(s)

sequenceDiagram
  participant ProcessManager
  participant OldInstance as PlatformInstance
  participant NewInstance as PlatformInstance
  participant JobQueue
  participant platformInstances

  ProcessManager->>OldInstance: markReplaced()
  ProcessManager->>OldInstance: shutdown()
  ProcessManager->>NewInstance: create or reuse
  ProcessManager->>platformInstances: store replacement
  OldInstance->>JobQueue: disconnect() or shutdown()
  OldInstance->>platformInstances: delete only if still current

Possibly related PRs

sockethub/sockethub#1155: Also changes packages/server/src/process-manager.ts around instance replacement and shutdown timing in ensureProcess.

Suggested labels: kredits-1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preventing a crashed platform's teardown from destroying its replacement.
Linked Issues check	✅ Passed	The code matches `#1166` by marking replaced instances, preserving the replacement map entry, and disconnecting queues without obliterating them.
Out of Scope Changes check	✅ Passed	The added queue/instance lifecycle changes and tests are directly tied to the crash-recovery race described in the linked issue.
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/platform-replacement-shutdown-race

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.

❤️ Share

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

coderabbitai

🧹 Nitpick comments (1)

packages/data-layer/src/job-queue.test.ts (1)

294-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add error-path coverage for disconnect().

This test only covers the happy path (verifies close/removeAllListeners called, pause/obliterate not called). disconnect() has no internal try/catch, so a rejection from queue.close()/events.close() propagates directly to the caller — worth asserting that behavior, and optionally that events.close() is also invoked.

As per path instructions, **/*.test.ts: "Mock external dependencies (Redis, network calls) in tests and MUST test error paths, not just happy paths."

✅ Suggested additional assertions

     describe("disconnect", () => {
         it("closes connections without pausing or obliterating the queue", async () => {
             await jobQueue.disconnect();
             sinon.assert.called(jobQueue.queue.removeAllListeners);
             sinon.assert.called(jobQueue.queue.close);
             sinon.assert.notCalled(jobQueue.queue.pause);
             sinon.assert.notCalled(jobQueue.queue.obliterate);
+            sinon.assert.called(jobQueue.events.close);
         });
+
+        it("propagates errors from the underlying connection close", async () => {
+            jobQueue.queue.close.rejects(new Error("close failed"));
+            await expect(jobQueue.disconnect()).rejects.toThrow("close failed");
+        });
     });

🤖 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 `@packages/data-layer/src/job-queue.test.ts` around lines 294 - 303,
`jobQueue.disconnect()` only has happy-path coverage; add an error-path test
that makes `queue.close()` and/or `events.close()` reject and asserts the
rejection is propagated to the caller since there is no internal try/catch. Use
the existing `disconnect` test setup in `job-queue.test.ts` and the
`jobQueue.disconnect`, `jobQueue.queue.close`, and `jobQueue.events.close`
symbols to verify the failure behavior, while keeping the current assertions
about `pause` and `obliterate` untouched.

Source: Path instructions

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

Nitpick comments:
In `@packages/data-layer/src/job-queue.test.ts`:
- Around line 294-303: `jobQueue.disconnect()` only has happy-path coverage; add
an error-path test that makes `queue.close()` and/or `events.close()` reject and
asserts the rejection is propagated to the caller since there is no internal
try/catch. Use the existing `disconnect` test setup in `job-queue.test.ts` and
the `jobQueue.disconnect`, `jobQueue.queue.close`, and `jobQueue.events.close`
symbols to verify the failure behavior, while keeping the current assertions
about `pause` and `obliterate` untouched.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b873797-b3b4-49fc-b756-5f51788e5f42

📥 Commits

Reviewing files that changed from the base of the PR and between 55dc804 and 2f1ba19.

📒 Files selected for processing (6)

packages/data-layer/src/job-queue.test.ts
packages/data-layer/src/job-queue.ts
packages/server/src/platform-instance.test.ts
packages/server/src/platform-instance.ts
packages/server/src/process-manager.test.ts
packages/server/src/process-manager.ts

github-actions Bot added scope:server Issues related to Sockethub Server package type:fix Bug fix labels Jul 5, 2026

coderabbitai Bot reviewed Jul 5, 2026

View reviewed changes

silverbucket mentioned this pull request Jul 5, 2026

platform crash-recovery race: stale instance shutdown obliterates replacement's queue and evicts it from the instance map #1166

Closed

silverbucket self-assigned this Jul 5, 2026

silverbucket added the kredits-2 Medium contribution label Jul 5, 2026

test(data-layer): add error-path coverage for JobQueue.disconnect

55b2fc4

silverbucket merged commit 9055079 into master Jul 5, 2026
21 checks passed

silverbucket deleted the fix/platform-replacement-shutdown-race branch July 5, 2026 15:44

silverbucket mentioned this pull request Jul 6, 2026

fix(server): serialize platform replacement behind crashed instance teardown #1175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(server): prevent crashed platform teardown from destroying its replacement#1167

fix(server): prevent crashed platform teardown from destroying its replacement#1167
silverbucket merged 2 commits into
mastersockethub/sockethub:masterfrom
fix/platform-replacement-shutdown-racesockethub/sockethub:fix/platform-replacement-shutdown-raceCopy head branch name to clipboard

silverbucket commented Jul 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

silverbucket commented Jul 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

silverbucket commented Jul 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading