Return 4xx instead of 500 for client errors in bundle endpoint

ide · ide · commit e078abd87a67 · 2026-02-24T18:56:51.000-08:00
Why
===
All errors from the `/bundle/` endpoint returned HTTP 500, including permanent failures like "package not found" (404 from npm) and "package can't be bundled" (invalid modules, unresolvable dependencies). These expected client errors inflated 5xx rates and created unnecessary Sentry noise. They should be 4xx to signal "don't retry" and to separate them from real server errors in metrics.

How
===
Two new error classes, `PackageNotFoundError` and `UnbundleablePackageError`, are thrown from the appropriate sites in `fetchMetadata` and `packageBundle`. In `bundle.ts`, `PackageNotFoundError` maps to 404 and `UnbundleablePackageError` maps to 422, with descriptive response bodies explaining the error and how to work around it. Only 500s are reported to Sentry. `fetchBundle` stores `errorName` in Redis alongside cached errors so that `UnbundleablePackageError` is preserved across the cache boundary. Log levels in `servePackage` and `fetchBundle` are downgraded to warn for client errors.

Test Plan
===
`yarn test` passes (74 tests). New unit tests in `src/__tests__/bundle.test.ts` mock `servePackage` to throw each error class and verify the status code, response body, and Sentry behavior.

`scripts/test-error-codes.sh` runs an integration test against local Redis:
- 404 cold fetch: nonexistent package returns 404 with descriptive message
- 404 repeated fetch: returns 404 again (registry 404s are not cached)
- 422 cold fetch: `bcrypt@5.1.1` returns 200 pending (bundling is async)
- 422 cached fetch: after bundling fails, the retry returns 422 with `errorName` preserved in the Redis cache
- 400: core modules like `react-native` still return 400 (existing `parseRequest` behavior)
diff --git a/snackager/scripts/test-error-codes.sh b/snackager/scripts/test-error-codes.sh
@@ -0,0 +1,193 @@
+#!/usr/bin/env bash
+#
+# Integration test for HTTP error codes from the /bundle/ endpoint.
+#
+# Prerequisites:
+#   - Redis running on localhost:6379
+#   - Node.js with --openssl-legacy-provider (webpack 4 + Node 22)
+#
+# Usage: ./scripts/test-error-codes.sh
+
+set -euo pipefail
+
+PORT=3099
+REDIS_DB=2
+REDIS_URL="redis://localhost:6379/$REDIS_DB"
+PASS=0
+FAIL=0
+
+cleanup() {
+  if [ -n "${SERVER_PID:-}" ]; then
+    kill "$SERVER_PID" 2>/dev/null || true
+    wait "$SERVER_PID" 2>/dev/null || true
+  fi
+  redis-cli -n "$REDIS_DB" FLUSHDB > /dev/null 2>&1 || true
+}
+trap cleanup EXIT
+
+request() {
+  local url="http://localhost:$PORT$1"
+  local tmpfile
+  tmpfile=$(mktemp)
+  local status
+  status=$(curl -s --max-time "${2:-30}" -o "$tmpfile" -w "%{http_code}" "$url")
+  local body
+  body=$(cat "$tmpfile")
+  rm -f "$tmpfile"
+  echo "$status"
+  echo "$body"
+}
+
+assert_status() {
+  local description="$1" expected="$2" actual="$3" body="$4"
+  if [ "$actual" = "$expected" ]; then
+    echo "  PASS: $description (HTTP $actual)"
+    PASS=$((PASS + 1))
+  else
+    echo "  FAIL: $description — expected HTTP $expected, got HTTP $actual"
+    echo "  Body: $body"
+    FAIL=$((FAIL + 1))
+  fi
+}
+
+assert_contains() {
+  local description="$1" expected="$2" actual="$3"
+  if echo "$actual" | grep -qF "$expected"; then
+    echo "  PASS: $description"
+    PASS=$((PASS + 1))
+  else
+    echo "  FAIL: $description — expected body to contain '$expected'"
+    echo "  Body: $actual"
+    FAIL=$((FAIL + 1))
+  fi
+}
+
+wait_for_redis_type() {
+  local key="$1" expected_type="$2" timeout_secs="${3:-120}"
+  for i in $(seq 1 "$timeout_secs"); do
+    TYPE=$(redis-cli -n "$REDIS_DB" HGET "$key" type 2>/dev/null)
+    if [ "$TYPE" = "$expected_type" ]; then
+      return 0
+    fi
+    sleep 1
+  done
+  echo "  Timed out waiting for Redis key $key to have type=$expected_type (got $TYPE)"
+  return 1
+}
+
+echo "Starting snackager on port $PORT..."
+redis-cli -n "$REDIS_DB" FLUSHDB > /dev/null
+
+NODE_OPTIONS=--openssl-legacy-provider \
+DISABLE_INSTRUMENTATION=1 \
+REDIS_URL="$REDIS_URL" \
+IMPORT_SERVER_URL="http://localhost:$PORT" \
+AWS_ACCESS_KEY_ID=test \
+AWS_SECRET_ACCESS_KEY=test \
+S3_BUCKET=test \
+IMPORTS_S3_BUCKET=test \
+S3_REGION=us-west-1 \
+CLOUDFRONT_URL="http://localhost:$PORT/serve" \
+API_SERVER_URL=https://staging.exp.host \
+PORT="$PORT" \
+npx ts-node --require tsconfig-paths/register src/index.ts > /tmp/snackager-test.log 2>&1 &
+SERVER_PID=$!
+
+for i in $(seq 1 30); do
+  if curl -s "http://localhost:$PORT/status" > /dev/null 2>&1; then
+    break
+  fi
+  sleep 1
+done
+
+if ! curl -s "http://localhost:$PORT/status" > /dev/null 2>&1; then
+  echo "Server failed to start"
+  tail -20 /tmp/snackager-test.log
+  exit 1
+fi
+echo "Server ready (PID $SERVER_PID)"
+echo
+
+# ── 404: Package not found ───────────────────────────────────────────
+# PackageNotFoundError is thrown from fetchMetadata before fetchBundle
+# is called, so the 404 is returned synchronously on the first request.
+# The registry 404 is never cached in Redis, so repeated requests also
+# hit the registry and return 404.
+echo "── 404: Package not found ──"
+
+echo "  Cold fetch:"
+RESULT=$(request "/bundle/this-package-does-not-exist-xyz@1.0.0?platforms=ios")
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "nonexistent package returns 404" 404 "$STATUS" "$BODY"
+assert_contains "body explains the package was not found" "not found in the registry" "$BODY"
+assert_contains "body suggests checking the name and version" "Verify the package name and version" "$BODY"
+
+echo "  Repeated fetch (registry 404 is not cached):"
+RESULT=$(request "/bundle/this-package-does-not-exist-xyz@1.0.0?platforms=ios")
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "repeated request also returns 404" 404 "$STATUS" "$BODY"
+
+echo
+
+echo "  Scoped package:"
+RESULT=$(request "/bundle/@example/nonexistent@1.0.0?platforms=ios")
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "nonexistent scoped package returns 404" 404 "$STATUS" "$BODY"
+
+echo
+
+# ── 422: Unbundleable package ────────────────────────────────────────
+# UnbundleablePackageError is thrown inside fetchBundle's try block,
+# which catches the error, caches it in Redis (with errorName), and
+# returns {pending: true}. The 422 is only returned on subsequent
+# requests when the cached error is re-thrown.
+echo "── 422: Unbundleable package ──"
+
+# bcrypt imports Node.js's crypto module as a dependency; the bundler
+# installs it but still can't resolve it, triggering
+# UnbundleablePackageError ("Cannot resolve module crypto after
+# installing it as a dependency").
+echo "  Cold fetch (bundling starts in background):"
+RESULT=$(request "/bundle/bcrypt@5.1.1?platforms=ios" 120)
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "first request returns 200 pending" 200 "$STATUS" "$BODY"
+assert_contains "response is pending" '"pending":true' "$BODY"
+
+echo "  Waiting for bundling to fail..."
+wait_for_redis_type "snackager/buildStatus/1/bcrypt@5.1.1-ios" "error"
+
+echo "  Redis cache:"
+ERROR_NAME=$(redis-cli -n "$REDIS_DB" HGET "snackager/buildStatus/1/bcrypt@5.1.1-ios" errorName 2>/dev/null)
+assert_status "errorName preserved in Redis" "UnbundleablePackageError" "$ERROR_NAME" ""
+
+echo "  Cached fetch (error re-thrown from Redis):"
+RESULT=$(request "/bundle/bcrypt@5.1.1?platforms=ios")
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "unbundleable package returns 422" 422 "$STATUS" "$BODY"
+assert_contains "body includes the bundler error" "Cannot resolve module crypto" "$BODY"
+assert_contains "body explains the limitation" "cannot be bundled for the Snack runtime" "$BODY"
+
+echo
+
+# ── 400: Core module ────────────────────────────────────────────────
+# Core modules are rejected by parseRequest before servePackage is
+# called. This is existing behavior.
+echo "── 400: Core module (existing behavior) ──"
+
+RESULT=$(request "/bundle/react-native@0.73.0?platforms=ios" 15)
+STATUS=$(echo "$RESULT" | head -1)
+BODY=$(echo "$RESULT" | tail -n +2)
+assert_status "core module returns 400" 400 "$STATUS" "$BODY"
+
+echo
+
+# ── Summary ──────────────────────────────────────────────────────────
+echo "── Results: $PASS passed, $FAIL failed ──"
+if [ "$FAIL" -gt 0 ]; then
+  exit 1
+fi
diff --git a/snackager/src/__tests__/bundle.test.ts b/snackager/src/__tests__/bundle.test.ts
@@ -0,0 +1,76 @@
+import supertest from 'supertest';
+
+import createApp from '../app';
+import { PackageNotFoundError, UnbundleablePackageError } from '../errors';
+import * as servePackageModule from '../utils/servePackage';
+
+jest.mock('../utils/servePackage');
+
+const mockedServePackage = servePackageModule.default as jest.MockedFunction<
+  typeof servePackageModule.default
+>;
+
+function request(): supertest.SuperTest<supertest.Test> {
+  return supertest(createApp());
+}
+
+describe('/bundle/', () => {
+  it('returns 404 for PackageNotFoundError', async () => {
+    mockedServePackage.mockRejectedValue(
+      new PackageNotFoundError('Package "nonexistent" not found in the registry'),
+    );
+
+    const response = await request().get('/bundle/nonexistent@1.0.0?platforms=ios');
+
+    expect(response.status).toBe(404);
+    expect(response.text).toContain('not found in the registry');
+    expect(response.text).toContain('Verify the package name and version');
+  });
+
+  it('returns 422 for UnbundleablePackageError', async () => {
+    mockedServePackage.mockRejectedValue(
+      new UnbundleablePackageError('Cannot resolve module crypto after installing it as a dependency'),
+    );
+
+    const response = await request().get('/bundle/bcrypt@5.1.1?platforms=ios');
+
+    expect(response.status).toBe(422);
+    expect(response.text).toContain('Cannot resolve module crypto');
+    expect(response.text).toContain('cannot be bundled for the Snack runtime');
+  });
+
+  it('returns 500 for unexpected errors', async () => {
+    mockedServePackage.mockRejectedValue(new Error('ENOENT: no such file or directory'));
+
+    const response = await request().get('/bundle/some-package@1.0.0?platforms=ios');
+
+    expect(response.status).toBe(500);
+    expect(response.text).toContain('ENOENT');
+  });
+
+  it('does not report PackageNotFoundError to Sentry', async () => {
+    const raven = require('raven');
+    jest.spyOn(raven, 'captureException');
+
+    mockedServePackage.mockRejectedValue(
+      new PackageNotFoundError('Package "nonexistent" not found in the registry'),
+    );
+
+    await request().get('/bundle/nonexistent@1.0.0?platforms=ios');
+
+    expect(raven.captureException).not.toHaveBeenCalled();
+  });
+
+  it('does not report UnbundleablePackageError to Sentry', async () => {
+    const raven = require('raven');
+    jest.spyOn(raven, 'captureException');
+
+    mockedServePackage.mockRejectedValue(
+      new UnbundleablePackageError('Cannot resolve module crypto'),
+    );
+
+    await request().get('/bundle/bcrypt@5.1.1?platforms=ios');
+
+    expect(raven.captureException).not.toHaveBeenCalled();
+  });
+});
diff --git a/snackager/src/bundle.ts b/snackager/src/bundle.ts
@@ -3,6 +3,7 @@ import querystring from 'querystring';
 import raven from 'raven';
 
 import config from './config';
+import { PackageNotFoundError, UnbundleablePackageError } from './errors';
 import parseRequest, { BundleRequest } from './utils/parseRequest';
 import servePackage from './utils/servePackage';
 
@@ -23,10 +24,26 @@ export default async function bundle(req: Request, res: Response): Promise<void>
     res.status(200);
     res.end(JSON.stringify(result));
   } catch (e) {
-    if (config.sentry) {
-      raven.captureException(e);
+    if (e instanceof PackageNotFoundError) {
+      res.status(404);
+      res.end(
+        `${e.message}. The package name may be misspelled, the version may not exist, ` +
+          `or the package may have been unpublished. ` +
+          `Verify the package name and version on the npm registry.`,
+      );
+    } else if (e instanceof UnbundleablePackageError) {
+      res.status(422);
+      res.end(
+        `${e.message}. This package cannot be bundled for the Snack runtime because it ` +
+          `depends on modules that are unavailable in the browser or on native platforms. ` +
+          `Try using a different package that supports the target platforms.`,
+      );
+    } else {
+      if (config.sentry) {
+        raven.captureException(e);
+      }
+      res.status(500);
+      res.end(e.message);
     }
-    res.status(500);
-    res.end(e.message);
   }
 }
diff --git a/snackager/src/errors.ts b/snackager/src/errors.ts
@@ -0,0 +1,15 @@
+export class PackageNotFoundError extends Error {
+  override readonly name = 'PackageNotFoundError';
+
+  constructor(message: string, options?: ErrorOptions) {
+    super(message, options);
+  }
+}
+
+export class UnbundleablePackageError extends Error {
+  override readonly name = 'UnbundleablePackageError';
+
+  constructor(message: string, options?: ErrorOptions) {
+    super(message, options);
+  }
+}
diff --git a/snackager/src/utils/fetchBundle.ts b/snackager/src/utils/fetchBundle.ts
@@ -10,6 +10,7 @@ import packageBundle from './packageBundle';
 import uploadFile from './uploadFile';
 import getCachePrefix from '../cache-busting';
 import config from '../config';
+import { UnbundleablePackageError } from '../errors';
 import { createRedisClient } from '../external/redis';
 import logger from '../logger';
 import { Package } from '../types';
@@ -114,6 +115,9 @@ export default async function fetchBundle({
       case 'error':
         logger.warn({ ...logMetadata, error: inProgress.message }, `an error occurred earlier`);
         if (!process.env.DEBUG_LOCAL_FILES) {
+          if (inProgress.errorName === 'UnbundleablePackageError') {
+            throw new UnbundleablePackageError(inProgress.message);
+          }
           throw new Error(inProgress.message);
         }
     }
@@ -290,17 +294,23 @@ export default async function fetchBundle({
     logger.info(logMetadata, `marking id as finished`);
     client.del(buildStatusRedisId);
   } catch (error) {
-    logger.error(
+    const isClientError = error instanceof UnbundleablePackageError;
+    const log = isClientError ? logger.warn.bind(logger) : logger.error.bind(logger);
+    log(
       { ...logMetadata, error },
       `unable to bundle, removing key from redis. error: ${error.message}`,
     );
     // Remove at a delay so we don't keep retrying
     client
       .multi()
-      .hmset(buildStatusRedisId, { type: 'error', message: error.message })
+      .hmset(buildStatusRedisId, {
+        type: 'error',
+        message: error.message,
+        ...(isClientError && { errorName: error.name }),
+      })
       .expire(buildStatusRedisId, 60 * 5)
       .exec();
-    if (config.sentry) {
+    if (config.sentry && !isClientError) {
       raven.captureException(error);
     }
   } finally {
diff --git a/snackager/src/utils/fetchMetadata.ts b/snackager/src/utils/fetchMetadata.ts
@@ -1,6 +1,7 @@
 import fetch from 'node-fetch';
 
 import config from '../config';
+import { PackageNotFoundError } from '../errors';
 import { RedisClient } from '../external/redis';
 import logger from '../logger';
 import { Metadata } from '../types';
@@ -83,14 +84,13 @@ export default async function fetchMetadata(
       return json;
     } catch (e) {
       logger.error({ ...logMetadata, qualified, e }, `error in parsing: ${e.toString()}`);
-      throw new Error(`Failed to parse the response for '${qualified}'`);
+      throw new Error(`Failed to parse the response for "${qualified}"`);
     }
   } catch (e) {
     logger.error({ ...logMetadata, qualified, e }, `error in fetching: ${e.toString()}`);
-    throw new Error(
-      e.name === 'NotFoundError'
-        ? `Package '${qualified}' not found in the registry`
-        : `Failed to fetch '${qualified}' from the registry`,
-    );
+    if (e.name === 'NotFoundError') {
+      throw new PackageNotFoundError(`Package "${qualified}" not found in the registry`);
+    }
+    throw new Error(`Failed to fetch "${qualified}" from the registry`);
   }
 }
diff --git a/snackager/src/utils/packageBundle.ts b/snackager/src/utils/packageBundle.ts
diff --git a/snackager/src/utils/servePackage.ts b/snackager/src/utils/servePackage.ts
