fix(connectors): serve catalog from a build-time manifest, stop cold-scan 503s#1013
Merged
Conversation
…scan 503s The "Add a connection" picker compiled every bundled connector on demand (esbuild + a forked metadata-extract subprocess each, run serially) the first time a cold pod served `list_connector_definitions`. On prod's 1-CPU, freshly-rolled, multi-replica app pods that scan (~12s) overran the request timeout and returned 503, so the catalog hook fell back to its empty default and rendered "No connectors found". Precompute the metadata once at build time into `dist/connectors/.catalog-manifest.json` (via build:server) and read it at runtime — 4426ms cold scan -> 3ms manifest read for the same connector set in a fresh process. Files not covered by the manifest (custom CONNECTOR_CATALOG_URIS dirs, a missing/stale manifest) still compile on demand, so the dynamic runtime path is preserved. The CLI build vendors the manifest next to its bundled connectors. Refs #1012 (separate orphaned-feed bug seen in the same logs).
📝 WalkthroughWalkthroughAdds build-time generation of a connector catalog manifest, runtime loading with fallback to on-demand extraction, vendoring the manifest into the CLI build, and soft-deletes feeds that reference connector versions without compiled or bundled code (with an integration test). ChangesConnector Catalog Manifest System
Orphan Feed Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/utils/connector-catalog.ts (1)
46-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
interfaceforExtractedConnectorCatalogMetadata(packages/server/src/utils/connector-catalog.ts, ~46-61)
export type ExtractedConnectorCatalogMetadata = { ... }defines an object shape; useexport interface ExtractedConnectorCatalogMetadata { ... }per repo TypeScript standards.Suggested change
-export type ExtractedConnectorCatalogMetadata = { +export interface ExtractedConnectorCatalogMetadata { key: string; name: string; description: string | null; version: string; auth_schema: Record<string, unknown> | null; feeds_schema: Record<string, unknown> | null; actions_schema: Record<string, unknown> | null; options_schema: Record<string, unknown> | null; mcp_config: Record<string, unknown> | null; openapi_config: Record<string, unknown> | null; favicon_domain: string | null; required_capability: string | null; runtime: Record<string, unknown> | null; login_enabled: boolean; -}; +}🤖 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/server/src/utils/connector-catalog.ts` around lines 46 - 61, Replace the exported type alias ExtractedConnectorCatalogMetadata with an exported interface of the same name and fields; keep all property names and types identical (key, name, description, version, auth_schema, feeds_schema, actions_schema, options_schema, mcp_config, openapi_config, favicon_domain, required_capability, runtime, login_enabled) but change the declaration from "export type ExtractedConnectorCatalogMetadata = { ... }" to "export interface ExtractedConnectorCatalogMetadata { ... }" to follow the repo TypeScript standard.
🤖 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 `@packages/server/scripts/build-connector-catalog-manifest.ts`:
- Around line 1-24: The script file build-connector-catalog-manifest.ts must be
moved from packages/server/scripts to packages/server/src (e.g.,
packages/server/src/build-connector-catalog-manifest.ts) so TypeScript sources
live under the package src directory; update any internal imports/relative paths
in the moved file (references to CATALOG_MANIFEST_FILENAME and
generateCatalogManifest and the connectorsDir/here constants) if their
resolution changes, and update the build:server (or equivalent) npm/bun build
task to point to the new path so the script is executed from packages/server/src
during the build step.
- Around line 26-31: The current check using existsSync(connectorsDir) logs a
warning and calls process.exit(0), which silently succeeds; change this to fail
the build by returning a non-zero exit or throwing an error instead. Update the
block around connectorsDir so that when the directory is missing you call
process.exit(1) or throw a new Error with a clear message (e.g., referencing
connectorsDir) rather than process.exit(0); ensure references to existsSync,
connectorsDir and process.exit are updated accordingly so the CI/build fails if
dist/connectors is missing.
---
Outside diff comments:
In `@packages/server/src/utils/connector-catalog.ts`:
- Around line 46-61: Replace the exported type alias
ExtractedConnectorCatalogMetadata with an exported interface of the same name
and fields; keep all property names and types identical (key, name, description,
version, auth_schema, feeds_schema, actions_schema, options_schema, mcp_config,
openapi_config, favicon_domain, required_capability, runtime, login_enabled) but
change the declaration from "export type ExtractedConnectorCatalogMetadata = {
... }" to "export interface ExtractedConnectorCatalogMetadata { ... }" to follow
the repo TypeScript standard.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03b07277-0786-4110-9804-72a34fcebac6
📒 Files selected for processing (4)
packages/cli/scripts/build.cjspackages/server/package.jsonpackages/server/scripts/build-connector-catalog-manifest.tspackages/server/src/utils/connector-catalog.ts
Comment on lines
+1
to
+24
| /** | ||
| * Build-time generator for the connector catalog manifest. | ||
| * | ||
| * Compiles every bundled connector once (esbuild + a forked metadata-extract | ||
| * subprocess each) and writes the result to dist/connectors/.catalog-manifest.json. | ||
| * The server then serves the bundled connector catalog by reading this file | ||
| * instead of recompiling every connector on demand — the cold per-pod scan that | ||
| * overran the request timeout and returned 503 on the "Add a connection" picker | ||
| * under prod's CPU limits (empty "No connectors found"). | ||
| * | ||
| * Runs after build-server-bundle.mjs (which copies the sources into | ||
| * dist/connectors). Executed under `bun` so it can import the TS catalog code. | ||
| */ | ||
| import { existsSync } from 'node:fs'; | ||
| import { writeFile } from 'node:fs/promises'; | ||
| import { dirname, join } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { | ||
| CATALOG_MANIFEST_FILENAME, | ||
| generateCatalogManifest, | ||
| } from '../src/utils/connector-catalog'; | ||
|
|
||
| const here = dirname(fileURLToPath(import.meta.url)); | ||
| const connectorsDir = join(here, '..', 'dist', 'connectors'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this TypeScript script under packages/server/src.
This new .ts source file is in packages/server/scripts, which violates the repo TS file placement rule. Move it under packages/server/src/... (and update build:server accordingly).
As per coding guidelines, **/*.ts: Place TypeScript source code in packages/*/src directory and tests in __tests__ directory within each workspace.
🤖 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/server/scripts/build-connector-catalog-manifest.ts` around lines 1 -
24, The script file build-connector-catalog-manifest.ts must be moved from
packages/server/scripts to packages/server/src (e.g.,
packages/server/src/build-connector-catalog-manifest.ts) so TypeScript sources
live under the package src directory; update any internal imports/relative paths
in the moved file (references to CATALOG_MANIFEST_FILENAME and
generateCatalogManifest and the connectorsDir/here constants) if their
resolution changes, and update the build:server (or equivalent) npm/bun build
task to point to the new path so the script is executed from packages/server/src
during the build step.
Comment on lines
+26
to
+31
| if (!existsSync(connectorsDir)) { | ||
| console.warn( | ||
| `[catalog-manifest] ${connectorsDir} missing; skipping (run build:server first).` | ||
| ); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
Fail the build when dist/connectors is missing.
Line 30 exits with code 0, which can hide a broken bundle step and ship without a manifest, reintroducing timeout-prone runtime scans.
Suggested change
if (!existsSync(connectorsDir)) {
- console.warn(
- `[catalog-manifest] ${connectorsDir} missing; skipping (run build:server first).`
- );
- process.exit(0);
+ throw new Error(
+ `[catalog-manifest] ${connectorsDir} missing; expected after build-server-bundle.mjs`
+ );
}🤖 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/server/scripts/build-connector-catalog-manifest.ts` around lines 26
- 31, The current check using existsSync(connectorsDir) logs a warning and calls
process.exit(0), which silently succeeds; change this to fail the build by
returning a non-zero exit or throwing an error instead. Update the block around
connectorsDir so that when the directory is missing you call process.exit(1) or
throw a new Error with a clear message (e.g., referencing connectorsDir) rather
than process.exit(0); ensure references to existsSync, connectorsDir and
process.exit are updated accordingly so the CI/build fails if dist/connectors is
missing.
…code Feed materialization threw "Connector '<key>' has no compiled code and no bundled source file" on every CheckDueFeeds tick when a feed resolved to a connector definition + version row that had neither persisted compiled code nor a bundled source to compile on demand (prod: connector_key 'chrome.tabs', which was never a standalone bundled connector). The feed never progressed and the error repeated every poll. Route that case through the same soft-delete-orphan path already used when no active connector_definition exists, so the orphan feed stops appearing in CheckDueFeeds (operators recover by registering code + clearing deleted_at). Closes #1012.
Member
Author
|
bug_free 80, simplicity 84, slop 12, bugs 0, 0 blockers [env] Integration suite refused DATABASE_URL db "postgres" as unsafe; failures are setup/test-db, not diff. Read diff/logs; typecheck/unit passed. Ran git diff --check, loaded catalog from dist manifest (31 defs incl google.gmail), and rebuilt manifest (31/33 files). Suggested fixes
Full verdict JSON{
"bug_free_confidence": 80,
"bugs": 0,
"slop": 12,
"simplicity": 84,
"blockers": [],
"change_type": "fix",
"behavior_change_risk": "medium",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/server/src/utils/queue-helpers.ts",
"line": 55,
"change": "Move the createSyncRun JSDoc block from lines 55-61 to immediately above createSyncRun at line 201, or delete it here; it currently documents softDeleteOrphanFeed incorrectly."
},
{
"file": "packages/server/scripts/build-connector-catalog-manifest.ts",
"line": 1,
"change": "Shorten the file header to a brief generator summary; the detailed production incident rationale is already documented beside CATALOG_MANIFEST_FILENAME."
}
],
"notes": "[env] Integration suite refused DATABASE_URL db \"postgres\" as unsafe; failures are setup/test-db, not diff. Read diff/logs; typecheck/unit passed. Ran git diff --check, loaded catalog from dist manifest (31 defs incl google.gmail), and rebuilt manifest (31/33 files).",
"categories": {
"src": 300,
"tests": 58,
"docs": 0,
"config": 0,
"deps": 2,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/server/src/utils/queue-helpers.ts (1)
71-75: ⚡ Quick winUse a named
interfacefor thefeedparameter shape.Line 74 defines an inline object shape in a TypeScript signature; extract it to a local interface for guideline compliance.
♻️ Proposed refactor
+interface OrphanFeedRef { + connector_key: string; + organization_id: string; +} + async function softDeleteOrphanFeed( sql: DbClient, feedId: number, - feed: { connector_key: string; organization_id: string }, + feed: OrphanFeedRef, reason: string ): Promise<void> {As per coding guidelines,
**/*.{ts,tsx}: Useinterfacefor defining object shapes in TypeScript files.🤖 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/server/src/utils/queue-helpers.ts` around lines 71 - 75, The inline object type for the feed parameter in softDeleteOrphanFeed should be extracted to a named TypeScript interface (e.g., declare interface OrphanFeed { connector_key: string; organization_id: string }) placed near the top of the file or above the function; then update the function signature to use that interface (softDeleteOrphanFeed(sql: DbClient, feedId: number, feed: OrphanFeed, reason: string)). Ensure the interface name is descriptive and follow existing file export/visibility conventions.packages/server/src/__tests__/integration/connectors/sync-run-orphan-feed.test.ts (1)
45-52: ⚡ Quick winReplace inline cast object shapes with named interfaces.
Lines 45 and 52 use inline object-shape casts; define named interfaces for these row shapes.
♻️ Proposed refactor
+interface FeedIdRow { + id: number; +} + +interface FeedDeletedAtRow { + deleted_at: Date | null; +} + - const feedId = Number((feed as { id: number }).id); + const feedId = Number((feed as FeedIdRow).id); ... - expect((after as { deleted_at: Date | null }).deleted_at).not.toBeNull(); + expect((after as FeedDeletedAtRow).deleted_at).not.toBeNull();As per coding guidelines,
**/*.{ts,tsx}: Useinterfacefor defining object shapes in TypeScript files.🤖 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/server/src/__tests__/integration/connectors/sync-run-orphan-feed.test.ts` around lines 45 - 52, Replace the inline object-shape casts with named interfaces: define an interface (e.g., FeedRow { id: number }) and use it for the `feed` cast where you compute `feedId` (currently `(feed as { id: number })`), and define another interface (e.g., FeedDeletedRow { deleted_at: Date | null }) and use it for the `after`/query result cast (currently `(after as { deleted_at: Date | null })`); update the two casts in this test file and keep the rest of the logic (including the `createSyncRun(feedId, {} as Env, sql)` call and assertions) unchanged.
🤖 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/server/src/__tests__/integration/connectors/sync-run-orphan-feed.test.ts`:
- Around line 45-52: Replace the inline object-shape casts with named
interfaces: define an interface (e.g., FeedRow { id: number }) and use it for
the `feed` cast where you compute `feedId` (currently `(feed as { id: number
})`), and define another interface (e.g., FeedDeletedRow { deleted_at: Date |
null }) and use it for the `after`/query result cast (currently `(after as {
deleted_at: Date | null })`); update the two casts in this test file and keep
the rest of the logic (including the `createSyncRun(feedId, {} as Env, sql)`
call and assertions) unchanged.
In `@packages/server/src/utils/queue-helpers.ts`:
- Around line 71-75: The inline object type for the feed parameter in
softDeleteOrphanFeed should be extracted to a named TypeScript interface (e.g.,
declare interface OrphanFeed { connector_key: string; organization_id: string })
placed near the top of the file or above the function; then update the function
signature to use that interface (softDeleteOrphanFeed(sql: DbClient, feedId:
number, feed: OrphanFeed, reason: string)). Ensure the interface name is
descriptive and follow existing file export/visibility conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de3d2095-b0ba-48d8-ae81-c1e475fa9fba
📒 Files selected for processing (2)
packages/server/src/__tests__/integration/connectors/sync-run-orphan-feed.test.tspackages/server/src/utils/queue-helpers.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two connector-reliability fixes from the same prod investigation (
app.lobu.ai→summaries-prod, currentmain).1. Catalog picker showed "No connectors found" (the 503)
list_connector_definitions(withinclude_installable) built the picker catalog by compiling every bundled connector on demand —esbuild+ a forked Node subprocess each, serially — the first time a cold pod served it, behind a per-pod in-memory cache.On prod's 1-CPU, freshly-Flux-rolled, multi-replica app pods, that cold scan crawled (the two expected skip-warnings logged ~12s apart) and overran the request timeout → the
manage_connectionsPOST returned 503 → the React Query hook fell back to its empty[]default → "No connectors found". It briefly worked once a pod warmed, then broke on the next roll.Fix: precompute the metadata once at build time into
dist/connectors/.catalog-manifest.json(newbuild-connector-catalog-manifest.ts, chained inbuild:server); the runtime reads it and skips compilation for covered files. Files not covered (customCONNECTOR_CATALOG_URISdirs, missing/stale manifest) still compile on demand — the dynamic runtime path is preserved. CLI build vendors the manifest. Also hardens against runtime esbuild/npm-dep differences (metadata captured in the build env).Repro (cold-cache fresh process per phase):
Real
make build-packages: bundle 0 errors + manifest 31/33 in 5.7s; runtime reads it in 3 ms; CLI vendored it; server typechecks clean.2. Orphan feed storming the logs (#1012)
Feed materialization threw
Connector 'chrome.tabs' has no compiled code and no bundled source fileon every CheckDueFeeds poll — a feed resolving to a connector definition + version row with neither compiled code nor a bundled source (prodconnector_key='chrome.tabs', never a standalone bundled connector). The feed never progressed and the error repeated indefinitely.Fix: route that case through the existing soft-delete-orphan path (already used when no active
connector_definitionexists), so the orphan feed stops appearing in CheckDueFeeds; operators recover by registering code + clearingdeleted_at. Red→green integration test added (verified against ephemeral embedded Postgres: 1 passed).Closes #1012.
Validation
make review: pi bug_free 80 / simplicity 84 / slop 12 / 0 bugs / 0 blockers. (Local integration suite couldn't run via embedded PG — its DB is namedpostgres, which the post-wipe safety guard rejects — so the #1012 test was verified manually withLOBU_ALLOW_DESTRUCTIVE_TEST_DB=1against the ephemeral embedded PG; CI'sintegrationjob runs it againstlobu_test.)