FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

plugins: isolate an activate() failure instead of crash-looping the server (#645) by melvincarvalho · Pull Request #646 · JavaScriptSolidServer/JavaScriptSolidServer · GitHub

Repository navigation

Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .js  (5) .md  (1) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
1 change: 1 addition & 0 deletions bin/jss.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ program
.option('--no-mcp', 'Disable MCP server')
.option('-q, --quiet', 'Suppress log output')
.option('--plugin <module[@prefix]>', 'Mount an app plugin (repeatable; prefix must start with /). Appends to config-file plugins (#594)', (value, previous) => previous.concat([value]), [])
.option('--plugin-strict', 'Fail startup when a plugin\'s activate() throws, instead of skipping it with a 503 under its prefix (#645)')
.option('--log-level <level>', 'Log level: error, warn, info, debug (default: info)')
.option('--print-config', 'Print configuration and exit')
.action(async (options) => {
Expand Down
15 changes: 15 additions & 0 deletions docs/configuration.md
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
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,21 @@ routed through the same upgrade path as the built-in realtime features, so
plugins never attach their own `'upgrade'` listener. Return
`{ deactivate }` to run teardown (state saves, timers) on server close.

**When a plugin fails** ([#645](https://github.com/JavaScriptSolidServer/JavaScriptSolidServer/issues/645)):
a plugin that cannot be *loaded* — unresolvable module, no `activate`
export, invalid prefix, duplicate id — fails startup loudly. A plugin whose
`activate()` *throws* is treated differently, because the usual cause is a
dependency that is not up yet (a database, a node's RPC) — the normal state
right after a reboot. By default it is skipped: the error is logged with
the plugin id, every request under its prefix answers `503` with
`Retry-After: 30` and a JSON body naming the plugin and the reason, the
other plugins and the pod itself start normally, and the skipped plugins
are listed in a `plugins: N active, M skipped` warning and on
`fastify.pluginFailures` (`[{ id, prefix, module, error }]`). Routes the
plugin registered before throwing stay in place. Re-activating it needs a
server restart once its dependency is back. Set `pluginStrict: true`
(`--plugin-strict`, `JSS_PLUGIN_STRICT=true`) to fail startup instead.

A plugin whose protocol **pins absolute paths** outside its prefix can
claim them with `api.reservePath(path)`:

Expand Down
6 changes: 6 additions & 0 deletions src/config.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ export const defaults = {
// MCP (Model Context Protocol) server — pod as a tool surface for agents (#490)
mcp: false,

// Plugins (#206): fail the boot when a plugin's activate() throws, instead
// of skipping it with a 503 under its prefix (#645)
pluginStrict: false,

// Logging
logger: true,
quiet: false,
Expand Down Expand Up @@ -209,6 +213,7 @@ const envMap = {
JSS_MONGO_URL: 'mongoUrl',
JSS_MONGO_DATABASE: 'mongoDatabase',
JSS_MCP: 'mcp',
JSS_PLUGIN_STRICT: 'pluginStrict',
};

/**
Expand Down Expand Up @@ -255,6 +260,7 @@ const BOOLEAN_KEYS = new Set([
'pay',
'mongo',
'mcp',
'pluginStrict',
'idp',
'notifications',
'logger',
Expand Down
95 changes: 84 additions & 11 deletions src/plugins.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,25 @@
* api.ws.route('/myapp/ws', (socket, req) => wss.emit('connection', socket, req));
*
* activate() may return { deactivate() {} }; deactivate runs on server
* close (world saves, timer teardown). A plugin that fails to load fails
* the boot loudly — the operator wrote the config, and a server silently
* missing an app is worse than one that refuses to start.
* close (world saves, timer teardown).
*
* Failure policy (#645). A plugin that cannot be LOADED — bad entry,
* unresolvable module, no activate export, invalid prefix, duplicate id —
* fails the boot loudly: the operator wrote the config, and a server
* silently missing an app is worse than one that refuses to start. A
* plugin whose activate() THROWS is a different animal: the usual cause is
* a dependency that isn't up yet (a database, a node's RPC, a remote
* service) — exactly the state after a host reboot, when the pod comes
* back under a supervisor before the things its plugins talk to. Killing
* the whole server for that turns one unavailable dependency into a crash
* loop that takes every other plugin and plain pod serving down with it.
* So by default an activate() failure is isolated: logged with the plugin
* id, the plugin skipped, a 503 placeholder mounted under its prefix, and
* the boot continues. The failure is recorded in ctx.pluginFailures and
* summarized at the end of the load. Operators who would rather not start
* at all than start without an app opt back in with pluginStrict
* (--plugin-strict / JSS_PLUGIN_STRICT). Re-activation of a skipped plugin
* needs a restart: Fastify's route table is sealed once listening.
*/

import fs from 'fs';
Expand Down Expand Up @@ -186,9 +202,27 @@ export function pluginId(spec) {
* @param {Array} entries options.plugins, verbatim
* @param {object} ctx { appPaths, root, log }
*/
/**
* Errors the plugin api raises for a misuse of itself — a malformed
* reservePath, a bad mountApp prefix, a path two plugins both claim — are
* operator/author mistakes, not an unavailable dependency. They stay fatal
* even in the default lenient mode (#645): marked here, rethrown by the
* activate() catch below.
*/
const CONFIG_ERROR = Symbol('jss.plugin.configError');
function configError(message) {
const err = new Error(message);
err[CONFIG_ERROR] = true;
return err;
}

export async function loadPlugins(fastify, entries, ctx) {
const log = makePluginLog(ctx.log);
const seenIds = new Set();
// activate() failures land here (#645) unless ctx.strict; the caller
// may pass its own array to read the outcome back.
const failures = ctx.pluginFailures ?? (ctx.pluginFailures = []);
let activeCount = 0;
// reservePath claims (#602), shared across all entries of this load so
// two plugins reserving the same pinned path fail the boot with both
// names — loud beats the silent-loser outcome witnessed with webfinger
Expand Down Expand Up @@ -292,22 +326,22 @@ export async function loadPlugins(fastify, entries, ctx) {
// rather than an opaque TypeError from .map deep in activate.
const options = opts && typeof opts === 'object' ? opts : {};
if (options.methods !== undefined && !Array.isArray(options.methods)) {
throw new Error(`plugin ${id}: reservePath { methods } must be an array, got ${JSON.stringify(options.methods)}`);
throw configError(`plugin ${id}: reservePath { methods } must be an array, got ${JSON.stringify(options.methods)}`);
}
// Validate AFTER normalization: '//' or '/ ' normalize to '',
// and an empty string in appPaths would match every URL in the
// WAC hook — a one-call total WAC bypass.
const key = typeof p === 'string' ? p.trim().replace(/\/+$/, '') : '';
if (!key.startsWith('/') || key.length < 2) {
throw new Error(`plugin ${id}: reservePath needs an absolute path, got ${JSON.stringify(p)}`);
throw configError(`plugin ${id}: reservePath needs an absolute path, got ${JSON.stringify(p)}`);
}
// A reservation is a pathname, not a URL: '?'/'#' don't belong.
// A query in the string would be escaped into the matcher (tying
// the exemption to that exact query), and fragments never reach
// request.url — both silently produce a reservation that matches
// nothing real.
if (key.includes('?') || key.includes('#')) {
throw new Error(`plugin ${id}: reservePath must be a pathname without '?' or '#', got ${JSON.stringify(p)}`);
throw configError(`plugin ${id}: reservePath must be a pathname without '?' or '#', got ${JSON.stringify(p)}`);
}
// Track by the shape, not the raw string: '/:user/did.json' and
// '/:acct/did.json' exempt the same URLs, so they're the same
Expand All @@ -318,7 +352,7 @@ export async function loadPlugins(fastify, entries, ctx) {
return; // idempotent re-claim — the matcher is already installed
}
if (holder) {
throw new Error(`plugin ${id}: path '${key}' is already reserved by plugin '${holder}'`);
throw configError(`plugin ${id}: path '${key}' is already reserved by plugin '${holder}'`);
}
reservations.set(claim, id);
// Read-only by default for BOTH lanes: a reserved path is
Expand Down Expand Up @@ -370,18 +404,18 @@ export async function loadPlugins(fastify, entries, ctx) {
// the handler runs and any body-reading app hangs forever.
async mountApp(handler, opts = {}) {
if (typeof handler !== 'function') {
throw new Error(`plugin ${id}: mountApp(handler) needs a (req, res) function`);
throw configError(`plugin ${id}: mountApp(handler) needs a (req, res) function`);
}
// Any provided prefix must validate — same rule as entry.prefix: a
// falsy normalization silently falling back to the entry prefix
// would mount the app (and WAC-exempt it) somewhere unexpected.
const secondary = normalizePrefix(opts.prefix);
if (opts.prefix !== undefined && !secondary) {
throw new Error(`plugin ${id}: mountApp invalid prefix ${JSON.stringify(opts.prefix)} (must start with '/'; omit to use the entry prefix)`);
throw configError(`plugin ${id}: mountApp invalid prefix ${JSON.stringify(opts.prefix)} (must start with '/'; omit to use the entry prefix)`);
}
const mountPrefix = secondary || prefix;
if (!mountPrefix) {
throw new Error(`plugin ${id}: mountApp needs a prefix (entry.prefix or opts.prefix)`);
throw configError(`plugin ${id}: mountApp needs a prefix (entry.prefix or opts.prefix)`);
}
if (mountPrefix !== prefix && !ctx.appPaths.includes(mountPrefix)) {
ctx.appPaths.push(mountPrefix); // exempt a secondary mount too
Expand Down Expand Up @@ -447,8 +481,19 @@ export async function loadPlugins(fastify, entries, ctx) {
try {
result = await activate(api);
} catch (err) {
throw new Error(`plugin ${id}: activate() failed: ${err.message}`);
if (ctx.strict || err?.[CONFIG_ERROR]) {
throw new Error(`plugin ${id}: activate() failed: ${err.message}`);
}
// Isolate (#645): this plugin is out, the server is not. Routes it
// registered before throwing stay (Fastify has no unregister), so
// the placeholder below only claims what is still free under the
// prefix — a half-activated plugin may answer some paths itself.
log.error({ err }, `plugin ${id}: activate() failed: ${err.message} — skipped; restart the server once its dependency is up (pluginStrict to fail the boot instead)`);
failures.push({ id, prefix, module: spec.module, error: err.message });
if (prefix) mountUnavailable(fastify, id, prefix, err.message, log);
continue;
}
activeCount += 1;
if (result && typeof result.deactivate === 'function') {
fastify.addHook('onClose', async () => {
try {
Expand All @@ -460,4 +505,32 @@ export async function loadPlugins(fastify, entries, ctx) {
}
log.info(`plugin ${id} active${prefix ? ` at ${prefix}` : ''}`);
}
if (failures.length) {
log.warn(`plugins: ${activeCount} active, ${failures.length} skipped after activate() failed: ${failures.map((f) => f.id).join(', ')}`);
}
}

/**
* 503 placeholder for a plugin whose activate() threw (#645). Claims the
* prefix and its subtree for every method so clients get a clear, retryable
* answer instead of a 404 (or, worse, the pod's WAC-exempt fallthrough —
* the prefix is already in appPaths). The plugin may have registered some
* of these shapes before failing; Fastify refuses duplicates, so each claim
* is best-effort and a collision just leaves the plugin's own route in place.
*/
function mountUnavailable(fastify, id, prefix, reason, log) {
const handler = async (request, reply) => {
reply.code(503).header('retry-after', '30').send({
error: 'plugin unavailable',
plugin: id,
reason,
});
};
for (const url of [prefix, `${prefix}/*`]) {
try {
fastify.all(url, handler);
} catch (err) {
log.warn(`plugin ${id}: could not mount 503 placeholder at ${url}: ${err.message}`);
}
}
}
13 changes: 11 additions & 2 deletions src/server.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const __dirname = dirname(fileURLToPath(import.meta.url));
* @param {boolean} options.webidTls - Enable WebID-TLS client certificate auth (default false)
* @param {boolean} options.pay - Enable HTTP 402 paid /pay/* routes (default false)
* @param {Array} options.plugins - App plugins to load (#206): [{ module, prefix, config, id }].
* @param {boolean} options.pluginStrict - Fail listen() when a plugin's activate() throws; default false skips it with a 503 under its prefix (#645).
* Each module's activate(api) runs at startup; prefix is WAC-exempted via appPaths.
* See src/plugins.js for the api surface.
* @param {number} options.payCost - Cost per request in satoshis (default 1)
Expand Down Expand Up @@ -127,6 +128,10 @@ export function createServer(options = {}) {
// appPaths. The WAC hook reads the array per request, so pushes made
// during plugin activation are honored.
const pluginEntries = Array.isArray(options.plugins) ? options.plugins : [];
// Plugins skipped after activate() threw (#645): [{ id, prefix, module, error }].
// Exposed as fastify.pluginFailures so an operator (or a status plugin)
// can see what is missing without grepping the boot log.
const pluginFailures = [];
// Parameterized reservations from api.reservePath (#602): compiled
// matchers for path shapes like /:user/did.json that literal appPaths
// prefixes cannot express. Same per-request read as appPaths.
Expand Down Expand Up @@ -330,6 +335,7 @@ export function createServer(options = {}) {

// Attach server config to requests
// Raw request body for the application/json parser to stash (#565).
fastify.decorate('pluginFailures', pluginFailures); // (#645)
fastify.decorateRequest('rawBody', null);
fastify.decorateRequest('connegEnabled', null);
fastify.decorateRequest('notificationsEnabled', null);
Expand Down Expand Up @@ -443,14 +449,17 @@ export function createServer(options = {}) {
}

// Load app plugins (#206). Deferred into a register scope so the dynamic
// imports and async activation run during fastify's startup; a failing
// plugin fails listen() rather than leaving a half-configured server.
// imports and async activation run during fastify's startup. A plugin
// that cannot be loaded fails listen(); one whose activate() throws is
// skipped with a 503 under its prefix unless pluginStrict (#645).
if (pluginEntries.length) {
fastify.register(async (instance) => {
const { loadPlugins } = await import('./plugins.js');
await loadPlugins(instance, pluginEntries, {
appPaths,
appPathPatterns,
strict: !!options.pluginStrict,
pluginFailures,
root: options.root || process.env.DATA_ROOT || './data',
log: fastify.log,
// api.serverInfo inputs (#601). ?? keeps an explicit port 0 —
Expand Down
63 changes: 61 additions & 2 deletions test/plugins.test.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
* WebSocket endpoint through ws.route (#588), private storage under the
* data root's dot-guard, config pass-through, and deactivate on close.
* Failure paths (missing module, no activate export, bad prefix) must fail
* listen() loudly rather than boot a server silently missing an app.
* listen() loudly rather than boot a server silently missing an app. A
* plugin whose activate() throws is isolated instead (#645): skipped with a
* 503 under its prefix, the rest of the server up — unless pluginStrict.
*/

import { describe, it, before, after, afterEach } from 'node:test';
Expand Down Expand Up @@ -67,13 +69,23 @@ export async function activate(api) {
}
`;

async function startWith(plugins) {
const THROWING_PLUGIN = `
export async function activate(api) {
// Registers one route, then fails the way a plugin whose dependency is
// down does: a rejected fetch during activation.
api.fastify.get(api.prefix + '/early', async () => ({ early: true }));
throw new Error('fetch failed');
}
`;

async function startWith(plugins, extra = {}) {
await fs.emptyDir(TEST_DATA_DIR);
server = createServer({
logger: false,
forceCloseConnections: true,
root: TEST_DATA_DIR,
plugins,
...extra,
});
await server.listen({ port: 0, host: '127.0.0.1' });
const address = server.server.address();
Expand All @@ -89,6 +101,7 @@ describe('plugin loader (#206)', () => {
originalDataRoot = process.env.DATA_ROOT;
await fs.emptyDir(FIXTURE_DIR);
await fs.writeFile(path.join(FIXTURE_DIR, 'fixture-plugin.js'), FIXTURE_PLUGIN);
await fs.writeFile(path.join(FIXTURE_DIR, 'throwing-plugin.js'), THROWING_PLUGIN);
});

afterEach(async () => {
Expand Down Expand Up @@ -217,6 +230,52 @@ describe('plugin loader (#206)', () => {
);
});

it('a plugin whose activate() throws is skipped with a 503, not a dead server (#645)', async () => {
await startWith([
{ module: `${FIXTURE_DIR}/throwing-plugin.js`, prefix: '/broken' },
{ module: `${FIXTURE_DIR}/fixture-plugin.js`, prefix: '/game' },
]);
// The healthy sibling loaded after the failure is up...
const ok = await fetch(`${baseUrl}/game/echo`);
assert.strictEqual(ok.status, 200);
// ...and so is the pod itself.
const pod = await fetch(`${baseUrl}/.well-known/openid-configuration`);
assert.notStrictEqual(pod.status, 503);
// The broken prefix answers 503 at the root and below, every method.
for (const [url, method] of [['/broken', 'GET'], ['/broken/a/b', 'GET'], ['/broken/x', 'PUT']]) {
const res = await fetch(baseUrl + url, { method });
assert.strictEqual(res.status, 503, `${method} ${url}`);
assert.strictEqual(res.headers.get('retry-after'), '30');
const body = await res.json();
assert.strictEqual(body.error, 'plugin unavailable');
assert.strictEqual(body.plugin, 'throwing-plugin');
assert.match(body.reason, /fetch failed/);
}
// A route the plugin managed to register before throwing is left alone.
const early = await fetch(`${baseUrl}/broken/early`);
assert.strictEqual(early.status, 200);
// The outcome is readable off the server.
assert.deepStrictEqual(
server.pluginFailures.map(({ id, prefix, error }) => ({ id, prefix, error })),
[{ id: 'throwing-plugin', prefix: '/broken', error: 'fetch failed' }],
);
});

it('pluginStrict restores the fail-fast boot for a throwing activate() (#645)', async () => {
await fs.emptyDir(TEST_DATA_DIR);
server = createServer({
logger: false,
forceCloseConnections: true,
root: TEST_DATA_DIR,
pluginStrict: true,
plugins: [{ module: `${FIXTURE_DIR}/throwing-plugin.js`, prefix: '/broken' }],
});
await assert.rejects(
server.listen({ port: 0, host: '127.0.0.1' }),
/plugin throwing-plugin: activate\(\) failed: fetch failed/,
);
});

it('a throwing ws handler closes that socket but not the server', async () => {
await startWith([
{ module: `${FIXTURE_DIR}/fixture-plugin.js`, prefix: '/game' },
Expand Down

Back | FazBrowse Home | New Git URL