Skip to content

Conversation

@timbrinded
Copy link
Collaborator

This pull request introduces significant improvements to the Moonwall CLI startup and test execution performance, primarily through a new startup artifact caching mechanism and enhanced retry logic for port and readiness checks. The changes also refactor logging for better maintainability and debugging.

Startup performance optimizations:

  • Added a new StartupCacheService (packages/cli/src/internal/effect/StartupCacheService.ts) that caches precompiled WASM and raw chain specs, reducing node startup time by ~10x. This service is integrated into the LaunchCommandParser and is triggered when cacheStartupArtifacts is enabled in the launch spec. [1] [2]
  • Updated the launch command flow to utilize cached artifacts, including logic to handle dev mode and chain spec generation, and to set appropriate CLI arguments for faster startup. [1] [2]

Reliability and retry logic improvements:

  • Increased the number of retry attempts and reduced the interval for node readiness, port discovery, and RPC port discovery, making startup more robust in parallel and high-contention environments. [1] [2] [3] [4]

Logging and maintainability:

  • Replaced direct console.log and chalk usage with a structured logger (createLogger from @moonwall/util) for more consistent and configurable logging throughout command parsing and port discovery. [1] [2] [3]

Test execution enhancements:

  • Added support for caching imports during test execution by introducing a .setCacheImports() option in VitestOptionsBuilder, enabling dependency optimizer for selected libraries. [1] [2]

Copilot AI review requested due to automatic review settings December 5, 2025 14:11
@timbrinded timbrinded added feature feature request performance labels Dec 5, 2025
Copilot finished reviewing on behalf of timbrinded December 5, 2025 14:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a comprehensive caching system to dramatically improve Moonwall CLI startup and test execution performance. The changes include a new startup artifact caching service, enhanced retry logic for better reliability in parallel environments, and structured logging improvements.

Key changes:

  • New StartupCacheService that caches precompiled WASM and raw chain specs for ~10x faster node startup
  • Increased retry attempts and reduced intervals for port discovery and node readiness checks to handle high-contention scenarios
  • Replaced ad-hoc console logging with structured logger throughout command parsing and port discovery

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
test/suites/papi/dev.test.ts Code formatting fixes (spacing and semicolon)
test/moonwall.config.json Enables new caching features for test environment
packages/types/src/config.ts Adds type definitions for caching configuration options
packages/types/config_schema.json JSON schema updates for new caching options
packages/cli/src/internal/effect/StartupCacheService.ts New Effect-based service for caching precompiled artifacts
packages/cli/src/internal/commandParsers.ts Integrates startup cache into launch command flow
packages/cli/src/cmds/runTests.ts Adds Vitest dependency optimizer configuration
packages/cli/src/internal/providerFactories.ts Implements metadata caching for PolkadotJS connections
packages/cli/src/lib/globalContext.ts Increases retry attempts and reduces delay for provider connections
packages/cli/src/internal/effect/NodeReadinessService.ts Adjusts retry parameters for faster convergence
packages/cli/src/internal/effect/PortDiscoveryService.ts Increases retry attempts and reduces interval
packages/cli/src/internal/effect/RpcPortDiscoveryService.ts Increases retry attempts and reduces interval
packages/cli/src/lib/runnerContext.ts Simplifies context creation with ternary operator
packages/cli/src/lib/repoDefinitions/moonbeam.ts Adds ParityDB database backend flag

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Check if using --dev flag
const hasDevFlag = this.args.includes("--dev");
// Extract chain name from --chain=XXX or --chain XXX
const existingChainName = chainArg?.match(/--chain[=\s]?(\S+)/)?.[1];
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern --chain[=\s]?(\S+) used to extract the chain name may not handle all valid chain argument formats correctly. Specifically:

  1. The pattern allows optional whitespace or =, but if whitespace is matched, it won't be consumed, causing the chain name to start with a space
  2. The pattern uses \S+ which captures any non-whitespace, but this could capture additional arguments if --chain is followed by a space and the next argument (e.g., --chain moonbase-dev --other-flag could capture moonbase-dev)

Consider using a more precise pattern:

const existingChainName = chainArg?.includes('=') 
  ? chainArg.split('=')[1]
  : this.args[this.args.indexOf(chainArg) + 1];

Or if keeping the regex approach:

const existingChainName = chainArg?.match(/--chain(?:=(\S+)|\s+(\S+))/)?.[1] || chainArg?.match(/--chain(?:=(\S+)|\s+(\S+))/)?.[2];
Suggested change
const existingChainName = chainArg?.match(/--chain[=\s]?(\S+)/)?.[1];
const existingChainName = chainArg
? (chainArg.includes("=")
? chainArg.split("=")[1]
: this.args[this.args.indexOf(chainArg) + 1])
: undefined;

Copilot uses AI. Check for mistakes.
Comment on lines +211 to +212
this.args.push("--alice");
this.args.push("--force-authoring");
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When replacing --dev with a raw chain spec, the code adds --alice and --force-authoring flags using .push() instead of overrideArg(). This could lead to duplicate flags if these arguments already exist in the args array (e.g., from default args or user-specified options).

Use the overrideArg() method consistently to prevent duplicates:

this.overrideArg("--alice");
this.overrideArg("--force-authoring");

However, note that overrideArg() may not handle flag-style arguments without = correctly (see the implementation at line 70-78), which could also cause issues.

Copilot uses AI. Check for mistakes.
return;
}

const data = JSON.stringify({ [genesisHash]: metadataHex });
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata cache implementation only stores a single genesis hash entry, overwriting any previously cached metadata from different chains. This will cause cache invalidation issues when testing multiple chains or environments.

The saveCachedMetadata function should merge with existing cache data rather than replacing it:

const saveCachedMetadata = (genesisHash: string, metadataHex: string): void => {
  const cacheDir = process.env.MOONWALL_CACHE_DIR;
  if (!cacheDir) return;

  const metadataPath = path.join(cacheDir, "metadata-cache.json");
  const lockPath = `${metadataPath}.lock`;

  try {
    // Simple lock to prevent concurrent writes
    try {
      fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL);
    } catch {
      // Another process is writing, skip
      return;
    }

    // Load existing cache and merge
    let existingCache: Record<string, string> = {};
    try {
      const existing = fs.readFileSync(metadataPath, "utf-8");
      existingCache = JSON.parse(existing);
    } catch {
      // No existing cache or parse error
    }

    const data = JSON.stringify({ ...existingCache, [genesisHash]: metadataHex });
    fs.writeFileSync(metadataPath, data, "utf-8");
    debug(`Saved metadata cache for genesis: ${genesisHash}`);
  } catch (e) {
    debug(`Failed to save metadata cache: ${e}`);
  } finally {
    try {
      fs.unlinkSync(lockPath);
    } catch {
      /* ignore */
    }
  }
};
Suggested change
const data = JSON.stringify({ [genesisHash]: metadataHex });
// Load existing cache and merge
let existingCache: Record<string, string> = {};
try {
const existing = fs.readFileSync(metadataPath, "utf-8");
existingCache = JSON.parse(existing);
} catch {
// No existing cache or parse error
}
const data = JSON.stringify({ ...existingCache, [genesisHash]: metadataHex });

Copilot uses AI. Check for mistakes.
try {
// Simple lock to prevent concurrent writes
try {
fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL);
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lock file descriptor is not being closed before deletion. When using fs.openSync, the file descriptor should be closed with fs.closeSync(fd) to prevent resource leaks.

try {
  const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL);
  fs.closeSync(fd); // Close the file descriptor
} catch {
  // Another process is writing, skip
  return;
}
Suggested change
fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL);
const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL);
fs.closeSync(fd); // Close the file descriptor to prevent resource leaks

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature feature request performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants