Skip to content

fix(serve): eagerly load extension registries and surface loader errors (swamp-club#463)#1458

Merged
stack72 merged 2 commits into
mainfrom
worktree-463
May 27, 2026
Merged

fix(serve): eagerly load extension registries and surface loader errors (swamp-club#463)#1458
stack72 merged 2 commits into
mainfrom
worktree-463

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented May 27, 2026

Summary

  • Replace silent catch {} blocks in all 5 extension loader functions (loadUserModels, loadUserVaults, loadUserDrivers, loadUserDatastores, loadUserReports) with diagnostic warning logs so loader failures are visible in the serve process journal. Deno.errors.NotFound is still suppressed as the expected "no extensions directory" case.
  • Eagerly call ensureLoaded() on all extension registries (modelRegistry, vaultTypeRegistry, driverTypeRegistry, reportRegistry) during swamp serve startup, before the scheduler and webhook services start. This surfaces loading failures at startup rather than silently on first scheduled/webhook execution.

Closes swamp-club#463

Test Plan

  • deno check passes
  • deno lint passes
  • deno fmt --check passes
  • All 6325 tests pass (deno run test)
  • deno run compile succeeds
  • Manual verification: compiled binary with pulled extension (@swamp/aws/cognito), scheduled workflow — serve starts, eagerly loads extensions, scheduled run completes successfully

🤖 Generated with Claude Code

…rs (swamp-club#463)

The extension loader functions (loadUserModels, loadUserVaults, etc.) had
catch {} blocks that silently swallowed all errors. If loading failed during
a long-running swamp serve process, registries would be permanently marked
as "loaded" while empty, causing "Unknown model type" errors on scheduled
workflow execution.

Two changes:
- Replace silent catch blocks with diagnostic logging (NotFound is still
  suppressed as the expected "no extensions dir" case)
- Eagerly call ensureLoaded() on all registries at serve startup so
  failures surface immediately rather than on first execution

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR makes no user-facing UX changes. It converts five silent catch {} blocks into logger.warn calls (failures now visible in the serve process journal) and eagerly loads extension registries at swamp serve startup so failures surface immediately rather than silently on first scheduled/webhook execution. Warning message format is consistent with existing per-file failure messages. No new flags, no output format changes, no help text affected.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Code Review

Blocking Issues

  1. Missing datastoreTypeRegistry.ensureLoaded() in eager loading (src/cli/commands/serve.ts:102-107): The PR updates all 5 loader function catch blocks (loadUserModels, loadUserVaults, loadUserDrivers, loadUserDatastores, loadUserReports) but the Promise.all only eagerly loads 4 registries — datastoreTypeRegistry is missing. It has the same ensureLoaded() method and lazy-loading pattern as the other four (see src/domain/datastore/datastore_type_registry.ts:97). This means datastore extension failures will still surface silently on first scheduled/webhook execution, defeating the purpose of the change for that registry.

Fix: add datastoreTypeRegistry.ensureLoaded() to the Promise.all and the corresponding import.

Suggestions

None — the rest of the PR is clean. The NotFound suppression is the right call, the LogTape tagged template usage is correct, domain registry imports are consistent with the existing CLI pattern (e.g. workflow_run.ts, doctor_extensions.ts), and the comment on lines 100-101 explains the "why" concisely.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

CLI UX Review

Blocking

None.

Suggestions

  1. No startup confirmation of successful extension loading. When extensions load cleanly, the serve process emits nothing — users have no way to confirm extensions were picked up. A brief logger.info (e.g. "Loaded model extensions" or similar) after ensureLoaded() resolves would make it easier to distinguish "no extensions installed" from "extensions installed and loaded." Not a blocker since the existing behavior is unchanged; the PR only adds visibility into failures.

  2. Warning messages in log mode only. The new logger.warn calls surface in the serve journal but not in JSON output mode. This is consistent with how other operational warnings work in serve (e.g. the non-loopback address warning), so it's not a regression — just worth noting if a future consumer wants to monitor extension health via the JSON event stream.

Verdict

PASS — no UX regressions. This is a net improvement: extension load failures that were previously silently swallowed now appear as warnings in the serve process journal, and errors now surface at startup rather than on first scheduled/webhook execution. Message format is consistent with existing warn patterns in the same file.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Code Review

Blocking Issues

None.

Suggestions

  1. No startup confirmation of successful extension loading (src/cli/commands/serve.ts:101-109): After the Promise.all resolves, a brief logger.info (e.g. "Extension registries loaded") would let operators confirm extensions were picked up vs. "none installed." Not a blocker — current behavior only adds visibility into failures, which is the stated goal.

Notes

  • Import pattern is consistent: The domain registry imports in serve.ts (lines 33-37) follow the same pattern already established in src/cli/mod.ts (lines 75-80). These are domain-layer singletons, not libswamp internals, so the libswamp barrel import rule does not apply.
  • Error handling is correct: Deno.errors.NotFound is appropriately suppressed (expected "no extensions dir" case), all other errors are surfaced as warnings via LogTape tagged templates with bare interpolation per project conventions.
  • No fire-and-forget promises: The Promise.all is properly awaited.
  • DDD alignment: The serve command (application/CLI layer) orchestrating eager registry loading is the right responsibility boundary — it's startup wiring, not domain logic.
  • All 5 registries covered: modelRegistry, vaultTypeRegistry, driverTypeRegistry, datastoreTypeRegistry, reportRegistry — matching the 5 loadUser* functions that received catch-block improvements.
  • Second commit addressed prior review feedback: datastoreTypeRegistry.ensureLoaded() was added to the Promise.all in commit 4297f3e.

@stack72 stack72 merged commit 906c855 into main May 27, 2026
11 checks passed
@stack72 stack72 deleted the worktree-463 branch May 27, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant