fix(connect): rm eager restart on ERROR_RETYING connection status

[pujitm]

Summary by CodeRabbit

• Bug Fixes

  • Improved connection handling to prevent unnecessary reconnection attempts during error retry states, ensuring reconnections only occur on specific failures.

• Tests

  • Added comprehensive tests to verify connection recovery, identity-based connection, logout behavior, DDoS prevention, and edge case handling for connection state changes.

[coderabbitai]

Walkthrough

A new test suite for MothershipHandler was added to verify its behavior under various connection and identity scenarios. Additionally, the connection status event handler was updated to only trigger reconnection logic on PING_FAILURE, removing ERROR_RETRYING from the triggering conditions.

Changes

File(s)	Change Summary
src/__test__/mothership.events.test.ts	Added comprehensive tests for MothershipHandler covering connection, identity, logout, and edge cases
src/mothership-proxy/mothership.events.ts	Modified event handler to trigger reconnection only on PING_FAILURE, not on ERROR_RETRYING

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant MothershipHandler
    participant MothershipController

    Client->>MothershipHandler: Connection status changes to PING_FAILURE
    MothershipHandler->>MothershipController: initOrRestart()
    MothershipController-->>MothershipHandler: Acknowledgement

Loading

Possibly related PRs

• fix(connect): mothership connection #1464: Refactors MothershipHandler to centralize connection logic and expands reconnection triggers, overlapping with the event handler changes in this PR.

Suggested reviewers

• mdatelle
• zackspear

Poem

When mothership pings begin to fail,
The handler listens, sets its sail.
No more retries on errors, just on pings,
With tests that check all sorts of things.
Connections guarded, logic tight—
The code now steers reconnections right!

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74da8d8 and e99fda9.

📒 Files selected for processing (2)

• packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts (1 hunks)
• packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

`**/*.{ts,tsx}`: TypeScript imports use .js extensions for ESM compatibility

**/*.{ts,tsx}: TypeScript imports use .js extensions for ESM compatibility

📄 Source: CodeRabbit Inference Engine (CLAUDE.md)

List of files the instruction was applied to:

• packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts
• packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts

`**/*.test.ts`: Use `mount` from Vue Test Utils for component testing Stub compl...

**/*.test.ts: Use mount from Vue Test Utils for component testing
Stub complex child components that aren't the focus of the test
Mock external dependencies and services in tests
Verify that the expected elements are rendered in component tests
Test component interactions (clicks, inputs, etc.) in tests
Check for expected prop handling and event emissions in component tests
Use createTestingPinia() for mocking stores in component tests
Use semantic queries like find('button') or find('[data-test="id"]') but prefer not to use data test IDs
Find components with findComponent(ComponentName) in tests
Use findAll to check for multiple elements in component tests
Assert on rendered text content with wrapper.text()
Assert on element attributes with element.attributes()
Verify element existence with expect(element.exists()).toBe(true)
Check component state through rendered output in tests
Trigger events with await element.trigger('click') in component tests
Set input values with await input.setValue('value') in component tests
Test emitted events with wrapper.emitted() in component tests
Mock external services and API calls in tests
Use vi.mock() for module-level mocks in tests
Specify return values for component methods with vi.spyOn() in tests
Reset mocks between tests with vi.clearAllMocks()
Use await nextTick() for DOM updates in async tests
Use flushPromises() for more complex promise chains in async tests
Always await async operations before making assertions in tests
Don't rely on Nuxt auto-imports in tests
Place all mock declarations at the top level in test files
Use factory functions for module mocks to avoid hoisting issues in test files
Don't mix mock declarations and module mocks incorrectly in test files
Avoid relying on Nuxt's auto-imports in test environment
Clear mocks between tests to ensure isolation
Remember that vi.mock() calls are hoisted

📄 Source: CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)

List of files the instruction was applied to:

• packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts

🧠 Learnings (2)

📓 Common learnings

Learnt from: pujitm
PR: unraid/api#1352
File: packages/unraid-api-plugin-generator/src/create-plugin.ts:91-112
Timestamp: 2025-04-21T18:27:36.482Z
Learning: For utility functions like file operations in api plugin generators, pujitm prefers minimal error handling rather than verbose try/catch blocks for each operation, as errors will propagate properly to higher level error handlers.

packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts (12)

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes after actions in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes by updating the store in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Check for expected prop handling and event emissions in component tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify proper error handling in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test action side effects and state changes in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Test emitted events with `wrapper.emitted()` in component tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test getter dependencies are properly mocked in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test store interactions with other stores in store tests

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test action side effects if not stubbed in store tests

Learnt from: elibosley
PR: unraid/api#1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external services and API calls in tests

🪛 Gitleaks (8.26.0)

packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts

126-126: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Build Web App
• GitHub Check: Build API
• GitHub Check: Build Unraid UI Library (Webcomponent Version)
• GitHub Check: Test API
• GitHub Check: Analyze (javascript-typescript)

🔇 Additional comments (6)

packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts (1)

33-42: LGTM! Excellent behavioral fix for retry logic.

Removing ERROR_RETRYING from the reconnection triggers is the correct approach. This prevents interference with the GraphQL client's exponential backoff mechanism while still allowing immediate reconnection attempts on ping failures.

packages/unraid-api-plugin-connect/src/__test__/mothership.events.test.ts (5)

1-11: Consider using .js extensions for TypeScript imports.

The coding guidelines specify that TypeScript imports should use .js extensions for ESM compatibility. Consider updating the import statements to include .js extensions.

-import { MinigraphStatus } from '../config/connect.config.js';
-import { EVENTS, GRAPHQL_PUBSUB_CHANNEL } from '../helper/nest-tokens.js';
-import { MothershipConnectionService } from '../mothership-proxy/connection.service.js';
-import { MothershipController } from '../mothership-proxy/mothership.controller.js';
-import { MothershipHandler } from '../mothership-proxy/mothership.events.js';
+import { MinigraphStatus } from '../config/connect.config.js';
+import { EVENTS, GRAPHQL_PUBSUB_CHANNEL } from '../helper/nest-tokens.js';
+import { MothershipConnectionService } from '../mothership-proxy/connection.service.js';
+import { MothershipController } from '../mothership-proxy/mothership.controller.js';
+import { MothershipHandler } from '../mothership-proxy/mothership.events.js';

Wait, the imports already have .js extensions. This suggestion is incorrect.

---

83-97: Excellent test coverage for the core behavioral change.

This test perfectly validates the key change in the PR - ensuring that ERROR_RETRYING states don't trigger reconnection attempts, which prevents interference with exponential backoff mechanisms.

---

122-161: Well-structured identity-based connection tests.

The tests properly validate that connections are only established when valid API keys are present, with good coverage of invalid credential scenarios.

---

197-241: Comprehensive DDoS prevention testing.

These tests effectively demonstrate that the system respects exponential backoff during network errors while still allowing immediate reconnection on ping failures. The differentiation between error types is well-tested.

---

126-126: Test data flagged as API key (false positive).

The static analysis tool flagged this as a potential API key exposure, but this is clearly test mock data ('valid-unraid-key-12345'). The value is appropriate for testing purposes.

✨ Finishing Touches

• 📝 Generate Docstrings

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1502/dynamix.unraid.net.plg