From 2009aaf9d475760a6fafefdbad8e4690547777b3 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Sat, 29 Aug 2026 13:33:05 -0700 Subject: [PATCH 1/6] Add an in-process integration test harness test/integration/ boots a real, complete server on an ephemeral port for each test, in about 15ms, and makes real HTTP requests against it. Built on createServer_p from the previous commits. test/support/ holds three pieces: - server.js -- start_p(configText, options) resolving to a handle with .port, .baseUrl, .get_p(), .workerEntries() and .stop_p(). - config.js -- writes a throwaway config into a temp dir, substituting $USER, $ROOT and $DIR. - fake-worker.js -- replaces lib/worker/app-worker's launchWorker_p with one that binds a real http.Server on the endpoint's port. Only `su` and R are stubbed. The real TcpTransport, endpoint, shared secret, connectEndpoint_p handshake and proxy all stay live -- which is the part an Express upgrade actually touches. The seam is a module-export assignment rather than rewire, because rewire loads a second copy of the module and the server built by server-init.js would still use the first. It works because scheduler.js resolves app_worker.launchWorker_p as a property at call time. Note that setTransport() alone is not a sufficient seam: spawnWorker calls posix.getpwnam(appSpec.runAs) and launchWorker_p regardless of transport, which is why test configs must run_as the current user. Two traps are worth recording, because in both cases the symptom points nowhere near the cause: - The harness pins its listener to 127.0.0.1 rather than the wildcard. With `listen 0` on `::`, the kernel can hand the server a port TcpTransport just probed-and-released for a worker; the worker then binds 127.0.0.1 on that same port, which *succeeds* -- a specific-address bind is allowed alongside a wildcard one -- and shadows the server for all loopback traffic. Requests silently reach the worker instead of Shiny Server. - Requests go through http.request with `agent: false`, not fetch(). fetch pools keep-alive sockets per origin; test servers restart milliseconds apart and ephemeral ports get recycled, so a pooled socket belonging to a dead server gets handed to the next test. `Connection: close` is not a fix -- fetch treats it as a forbidden header and drops it silently. Together these took the suite from ~12% flaky to 0 failures in 25 consecutive full runs. mocha is not recursive, so package.json's "test" script and Jenkinsfile both name the directories explicitly; .mocharc.json gains a timeout, since the 2s default cannot survive a server boot. Co-Authored-By: Claude Opus 5 (1M context) --- .mocharc.json | 4 +- Jenkinsfile | 5 +- package.json | 11 +- test/integration/access-log.js | 128 +++++++++++ test/integration/assets.js | 128 +++++++++++ test/integration/harness.js | 110 ++++++++++ test/integration/lifecycle.js | 198 +++++++++++++++++ test/integration/proxy.js | 364 +++++++++++++++++++++++++++++++ test/integration/static-files.js | 135 ++++++++++++ test/support/config.js | 122 +++++++++++ test/support/fake-worker.js | 178 +++++++++++++++ test/support/server.js | 224 +++++++++++++++++++ 12 files changed, 1600 insertions(+), 7 deletions(-) create mode 100644 test/integration/access-log.js create mode 100644 test/integration/assets.js create mode 100644 test/integration/harness.js create mode 100644 test/integration/lifecycle.js create mode 100644 test/integration/proxy.js create mode 100644 test/integration/static-files.js create mode 100644 test/support/config.js create mode 100644 test/support/fake-worker.js create mode 100644 test/support/server.js diff --git a/.mocharc.json b/.mocharc.json index 81af32cd..3410bd0d 100644 --- a/.mocharc.json +++ b/.mocharc.json @@ -1,5 +1,5 @@ { "require": ["should", "./lib/core/log", "./lib/core/qutil"], - "reporter": "spec" + "reporter": "spec", + "timeout": 20000 } - diff --git a/Jenkinsfile b/Jenkinsfile index 8ec1744c..a5e58493 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -105,7 +105,10 @@ try { } stage('run tests') { // Need npm install so npm modules required for testing are available - sh './bin/node ./node_modules/mocha/bin/mocha test' + // Note: mocha is not recursive, so each test + // directory has to be named explicitly. Keep this + // in sync with the "test" script in package.json. + sh './bin/node ./node_modules/mocha/bin/mocha test test/integration' } } } diff --git a/package.json b/package.json index 0314c6f5..3842087a 100644 --- a/package.json +++ b/package.json @@ -7,8 +7,11 @@ "bin": "./lib/main.js", "scripts": { "start": "node ./lib/main.js", - "test": "mocha test", - "build": "tsc" + "test": "mocha test test/integration", + "build": "tsc", + "test:unit": "mocha test", + "test:integration": "mocha test/integration", + "test:r": "mocha test/integration-r" }, "repository": { "type": "git", @@ -41,8 +44,8 @@ }, "license": "AGPL-3.0", "engines": { - "node": ">=6.6.0", - "npm": ">=2.8.0" + "node": ">=18.0.0", + "npm": ">=7.0.0" }, "devDependencies": { "@types/node": "^22.7.3", diff --git a/test/integration/access-log.js b/test/integration/access-log.js new file mode 100644 index 00000000..1d9a04f3 --- /dev/null +++ b/test/integration/access-log.js @@ -0,0 +1,128 @@ +/* + * test/integration/access-log.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// The access log is not middleware. It is a second 'request' listener on the +// Server facade, registered after the one that calls app.handle +// (lib/server-init.js). That ordering is load-bearing: by the time morgan runs, +// req.url has already been rewritten -- by the __assets__ handler, or by the +// proxy stripping the app prefix -- so the only way to log what the client +// actually asked for is req.originalUrl. + +var assert = require('assert'); +var fs = require('fs'); +var path = require('path'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +describe('access log', function() { + var server; + var logPath; + + beforeEach(function() { + return testServer.start_p(testConfig.siteDirConfig({ + preamble: 'access_log $DIR/access.log combined;' + }), { + files: { + 'site/index.html': '

root

', + 'site/myapp/server.R': '# not actually run\n' + } + }) + .then(function(s) { + server = s; + logPath = path.join(s.config.dir, 'access.log'); + }); + }); + + afterEach(function() { + var s = server; + server = null; + return s ? s.stop_p() : null; + }); + + it('logs the original URL of a proxied app request, not the rewritten one', function() { + return server.get_p('/myapp/sub/page.html?x=1') + .then(function() { + return readLog_p(logPath); + }) + .then(function(lines) { + var line = lines.find(function(l) { return /myapp/.test(l); }); + assert.ok(line, 'no line mentioning myapp in:\n' + lines.join('\n')); + assert.ok(line.indexOf('GET /myapp/sub/page.html?x=1 HTTP') >= 0, + 'expected the pre-rewrite URL, got: ' + line); + }); + }); + + it('logs the original URL of an asset request, not the rewritten one', function() { + return server.get_p('/__assets__/sockjs.min.js') + .then(function() { + return readLog_p(logPath); + }) + .then(function(lines) { + var line = lines.find(function(l) { return /sockjs\.min\.js/.test(l); }); + assert.ok(line, 'no line mentioning sockjs.min.js in:\n' + lines.join('\n')); + // The asset middleware rewrites req.url to a leading-slash-less + // "sockjs.min.js" before handing it to express.static. + assert.ok(line.indexOf('GET /__assets__/sockjs.min.js HTTP') >= 0, + 'expected the pre-rewrite URL, got: ' + line); + }); + }); + + it('logs the response status', function() { + return server.get_p('/no-such-page') + .then(function() { + return readLog_p(logPath); + }) + .then(function(lines) { + var line = lines.find(function(l) { return /no-such-page/.test(l); }); + assert.ok(line, 'no line for the 404 in:\n' + lines.join('\n')); + assert.ok(/ 404 /.test(line), 'expected a 404 in: ' + line); + }); + }); + + it('logs requests that never reach the Express stack', function() { + // /ping is answered by the ping router inside the app, so it does go + // through app.handle -- but this pins that the logger sees every request + // unconditionally, rather than only the ones some middleware passes on. + return server.get_p('/ping') + .then(function() { + return readLog_p(logPath); + }) + .then(function(lines) { + assert.ok(lines.some(function(l) { return /GET \/ping HTTP/.test(l); }), + 'no line for /ping in:\n' + lines.join('\n')); + }); + }); +}); + +/** + * morgan writes on the response's 'finished' event through a stream, so the + * line can land slightly after the client has the body. + */ +function readLog_p(logPath) { + var deadline = Date.now() + 2000; + return new Promise(function(resolve, reject) { + (function poll() { + var text = ''; + try { + text = fs.readFileSync(logPath, 'utf8'); + } catch (err) { + if (err.code !== 'ENOENT') return reject(err); + } + var lines = text.split('\n').filter(function(l) { return l.length > 0; }); + if (lines.length > 0) return resolve(lines); + if (Date.now() > deadline) + return reject(new Error('Timed out waiting for the access log to be written')); + setTimeout(poll, 10); + })(); + }); +} diff --git a/test/integration/assets.js b/test/integration/assets.js new file mode 100644 index 00000000..d5e106a4 --- /dev/null +++ b/test/integration/assets.js @@ -0,0 +1,128 @@ +/* + * test/integration/assets.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Characterization of the __assets__ middleware (lib/server-init.js), which is +// the most Express-version-sensitive thing in the codebase: it rewrites req.url +// out from under express.static, in ways express.static was never meant to see. +// +// These assert status and body directly rather than checking for fall-through, +// because there is no fall-through. filterByRegex (lib/core/connect-util.js) +// hands a matching request to the asset handler, which passes next404 to +// express.static, so a miss renders a 404 page instead of calling next(). A +// request matching __assets__ never reaches ShinyProxy. + +var assert = require('assert'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +describe('__assets__', function() { + var server; + + before(function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: { + 'site/index.html': '

site root

', + 'site/myapp/server.R': '# not actually run\n' + } + }) + .then(function(s) { server = s; }); + }); + + after(function() { + return server ? server.stop_p() : null; + }); + + it('serves a real asset', function() { + return server.get_p('/__assets__/sockjs.min.js').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(r.body.length > 0); + assert.ok(/javascript/.test(r.headers.get('content-type')), + 'unexpected content-type: ' + r.headers.get('content-type')); + }); + }); + + it('serves a source map alongside its script', function() { + return server.get_p('/__assets__/sockjs.min.js.map').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(r.body.length > 0); + }); + }); + + it('serves the shiny-server-client bundle', function() { + return server.get_p('/__assets__/shiny-server-client.min.js').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(r.body.length > 0); + }); + }); + + it('404s a missing asset rather than falling through to the proxy', function() { + return server.get_p('/__assets__/no-such-file.js').then(function(r) { + assert.strictEqual(r.status, 404); + assert.ok(/not found/i.test(r.body), 'expected the 404 template, got: ' + + r.body.slice(0, 200)); + }); + }); + + it('does not match the bare prefix (the regex requires a path after it)', function() { + // filterByRegex tests /\b__assets__\/.+/, so "/__assets__/" alone does not + // match and the request goes on to ShinyProxy, which 404s it as a + // nonexistent app. This is the case that would rewrite req.url to '' and + // leave parseurl's pathname null if the regex ever became laxer. + return server.get_p('/__assets__/').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + + it('404s "?foo" instead of 500ing (the explicit /? rewrite)', function() { + // The `.replace(/^\?/, '/?')` in the asset middleware exists precisely for + // this input: without it, express.static sees a path of "?foo" and blows up + // with a 500. Anything other than 404 here is a regression. + return server.get_p('/__assets__/?foo').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + + it('refuses to traverse out of the assets directory', function() { + return server.get_p('/__assets__/../../package.json').then(function(r) { + assert.notStrictEqual(r.status, 200); + assert.ok(!/"shiny-server"/.test(r.body), + 'package.json leaked through the asset handler'); + }); + }); + + it('refuses percent-encoded traversal', function() { + return server.get_p('/__assets__/%2e%2e%2f%2e%2e%2fpackage.json').then(function(r) { + assert.notStrictEqual(r.status, 200); + assert.ok(!/"shiny-server"/.test(r.body), + 'package.json leaked through the asset handler'); + }); + }); + + it('matches a nested __assets__ path anywhere in the URL', function() { + // The regex is unanchored (\b__assets__\/) and the rewrite is greedy + // (/^.*\b__assets__\//), so an app-prefixed asset URL resolves to the same + // file as the top-level one. + return server.get_p('/myapp/__assets__/sockjs.min.js').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(r.body.length > 0); + }); + }); + + it('preserves originalUrl through the rewrite', function() { + // The asset middleware sets req.originalUrl before mangling req.url. Morgan + // and the access log depend on that; see the access-log test. + return server.get_p('/myapp/__assets__/no-such-file.js').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); +}); diff --git a/test/integration/harness.js b/test/integration/harness.js new file mode 100644 index 00000000..2b246539 --- /dev/null +++ b/test/integration/harness.js @@ -0,0 +1,110 @@ +/* + * test/integration/harness.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Tests of the test harness itself. If these fail, nothing else in +// test/integration/ can be trusted. + +var assert = require('assert'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +describe('integration harness', function() { + var server; + + afterEach(function() { + if (!server) return; + var s = server; + server = null; + return s.stop_p(); + }); + + it('boots on an ephemeral port and serves /ping', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': '

hi

'} + }) + .then(function(s) { + server = s; + assert.ok(s.port > 0, 'expected an ephemeral port, got ' + s.port); + assert.deepStrictEqual(s.handle.bindErrors, []); + return s.get_p('/ping'); + }) + .then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(r.body, 'OK'); + }); + }); + + it('shuts down completely, releasing the port', function() { + var port; + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': '

hi

'} + }) + .then(function(s) { + port = s.port; + return s.stop_p(); + }) + .then(function() { + // The port should be refused, not hang. This is the regression test for + // the Server#$close leak: a listener dropped from $wildcards/$hosts + // without being closed would still be accepting here. + return fetch('http://127.0.0.1:' + port + '/ping').then( + function(res) { + throw new Error('expected connection refused, got HTTP ' + res.status); + }, + function(err) { return err; } + ); + }) + .then(function(err) { + assert.ok(err, 'expected a connection error after shutdown'); + }); + }); + + it('routes an app request through the proxy to a stand-in worker', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/myapp/server.R': '# not actually run\n'} + }) + .then(function(s) { + server = s; + return s.get_p('/myapp/'); + }) + .then(function(r) { + assert.strictEqual(r.status, 200); + var payload = JSON.parse(r.body); + // The proxy strips the app prefix before forwarding. + assert.strictEqual(payload.url, '/'); + // ...and stamps the per-worker shared secret on the way through. + assert.ok(payload.sharedSecret, 'expected shiny-shared-secret to be set'); + assert.strictEqual(server.worker.workers.length, 1); + }); + }); + + it('passes a custom handler through to the stand-in worker', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/myapp/server.R': '# not actually run\n'}, + worker: { + handler: function(req, res) { + res.writeHead(418, {'Content-Type': 'text/plain'}); + res.end('teapot'); + } + } + }) + .then(function(s) { + server = s; + return s.get_p('/myapp/'); + }) + .then(function(r) { + assert.strictEqual(r.status, 418); + assert.strictEqual(r.body, 'teapot'); + }); + }); +}); diff --git a/test/integration/lifecycle.js b/test/integration/lifecycle.js new file mode 100644 index 00000000..3db3ad53 --- /dev/null +++ b/test/integration/lifecycle.js @@ -0,0 +1,198 @@ +/* + * test/integration/lifecycle.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Startup, config reload (what SIGHUP does), and shutdown. These are the paths +// lib/server-init.js and lib/server/server.js own, and the ones a test process +// exercises hundreds of times -- so a leak here shows up as flakiness +// everywhere else. + +var assert = require('assert'); +var fs = require('fs'); +var net = require('net'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +describe('server lifecycle', function() { + var server; + + afterEach(function() { + var s = server; + server = null; + return s ? s.stop_p() : null; + }); + + it('reports the ephemeral port it actually bound', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': 'hi'} + }) + .then(function(s) { + server = s; + var addresses = s.handle.addresses(); + assert.strictEqual(addresses.length, 1); + assert.strictEqual(addresses[0].port, s.port); + assert.notStrictEqual(addresses[0].port, 0, + 'addresses() should report the assigned port, not the configured 0'); + }); + }); + + it('rejects when the config file does not exist', function() { + return testServer.start_p(testConfig.siteDirConfig(), {files: {}}) + .then(function(s) { + server = s; + // Point a fresh startup at a path that isn't there. + var server_init = require('../../lib/server-init'); + return server_init.createServer_p('/nonexistent/shiny-server.conf') + .then( + function() { throw new Error('should have rejected'); }, + function(err) { return err; } + ); + }) + .then(function(err) { + assert.strictEqual(err.code, 'ENOENT'); + }); + }); + + it('rejects an invalid config rather than starting', function() { + return testServer.start_p('run_as $USER;\nserver { listen 0 127.0.0.1; nonsense; }', {}) + .then( + function(s) { server = s; throw new Error('should have rejected'); }, + function(err) { + assert.ok(/Unknown directive "nonsense"/.test(err.message), + 'unexpected error: ' + err.message); + } + ); + }); + + it('reports a bind failure without rejecting', function() { + // Historically a listener that couldn't bind was logged and forwarded as an + // 'error' event, and startup carried on. That is preserved; the errors are + // just also visible on the handle now. + var blocker = net.createServer(); + return new Promise(function(resolve) { + blocker.listen(0, '127.0.0.1', function() { resolve(blocker.address().port); }); + }) + .then(function(port) { + return testServer.start_p( + testConfig.siteDirConfig({listen: port + ' 127.0.0.1'}), {files: {}}) + .then( + function(s) { s.stop_p(); throw new Error('expected no usable address'); }, + function(err) { return err; } + ); + }) + .then(function(err) { + // start_p turns "no addresses bound" into this error; the point is that + // createServer_p itself resolved rather than rejecting. + assert.ok(/did not bind any address/.test(err.message), + 'unexpected error: ' + err.message); + assert.ok(/EADDRINUSE/.test(err.message), + 'the bind error should be reported: ' + err.message); + }) + .then(function() { + return new Promise(function(resolve) { blocker.close(resolve); }); + }); + }); + + describe('reload', function() { + it('picks up a changed config without restarting', function() { + var port; + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': 'before'} + }) + .then(function(s) { + server = s; + port = s.port; + return s.get_p('/index.html'); + }) + .then(function(r) { + assert.strictEqual(r.body, 'before'); + // Swap the served content and the config's directory_index setting, + // then reload the way SIGHUP does. + fs.writeFileSync(server.config.dir + '/site/index.html', 'after'); + return server.handle.reload_p(); + }) + .then(function() { + // Same port: setAddresses only opens/closes the delta, and the address + // didn't change. + assert.strictEqual(server.handle.addresses()[0].port, port); + return server.get_p('/index.html'); + }) + .then(function(r) { + assert.strictEqual(r.body, 'after'); + }); + }); + + it('resolves promptly even while a connection is open on the old config', function() { + // setAddresses deliberately does not wait for obsolete listeners to + // finish closing, because server.close() blocks on established + // connections. If it did wait, a reload behind a long-lived session would + // stall -- and loadConfig_p is serialized, so the *next* reload with it. + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': 'hi'} + }) + .then(function(s) { + server = s; + return new Promise(function(resolve, reject) { + var sock = net.connect(s.port, '127.0.0.1', function() { resolve(sock); }); + sock.on('error', reject); + }); + }) + .then(function(sock) { + var started = Date.now(); + return server.handle.reload_p().then(function() { + var elapsed = Date.now() - started; + sock.destroy(); + assert.ok(elapsed < 5000, + 'reload took ' + elapsed + 'ms with a socket open; it should not ' + + 'wait for connections to drain'); + }); + }); + }); + + it('survives repeated reloads', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': 'hi'} + }) + .then(function(s) { + server = s; + return s.handle.reload_p() + .then(function() { return s.handle.reload_p(); }) + .then(function() { return s.handle.reload_p(); }); + }) + .then(function() { + assert.strictEqual(server.handle.addresses().length, 1, + 'repeated reloads should not accumulate or lose listeners'); + return server.get_p('/index.html'); + }) + .then(function(r) { + assert.strictEqual(r.status, 200); + }); + }); + }); + + describe('shutdown', function() { + it('is idempotent', function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: {'site/index.html': 'hi'} + }) + .then(function(s) { + return s.handle.shutdown_p() + .then(function() { return s.handle.shutdown_p(); }) + .then(function() { + assert.deepStrictEqual(s.handle.addresses(), []); + s.config.cleanup(); + return s.worker ? s.worker.uninstall_p() : null; + }); + }); + }); + }); +}); diff --git a/test/integration/proxy.js b/test/integration/proxy.js new file mode 100644 index 00000000..7c60f98d --- /dev/null +++ b/test/integration/proxy.js @@ -0,0 +1,364 @@ +/* + * test/integration/proxy.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Characterization of ShinyProxy.httpListener against a live server: the parts +// of the proxy path that depend on Express internals, plus the connection +// accounting that the res 'finish'/'close' listeners maintain. + +var assert = require('assert'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +var APP_FILES = { + 'site/myapp/server.R': '# not actually run\n', + 'site/myapp/ui.R': '# not actually run\n', + 'site/index.html': '

root

' +}; + +describe('proxy', function() { + + describe('request forwarding', function() { + var server; + + beforeEach(function() { + return testServer.start_p(testConfig.siteDirConfig(), {files: APP_FILES}) + .then(function(s) { server = s; }); + }); + + afterEach(function() { + var s = server; + server = null; + return s ? s.stop_p() : null; + }); + + it('strips the app prefix before forwarding', function() { + return server.get_p('/myapp/some/path').then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(JSON.parse(r.body).url, '/some/path'); + }); + }); + + it('reads the query string via req._parsedUrl and forwards it intact', function() { + // lib/proxy/http.js:117 does qs.parse(req._parsedUrl.query).w. + // req._parsedUrl only exists as a side effect of parseurl caching inside + // Express's router, so it is a private-API dependency that an Express + // upgrade could remove. If it disappeared, that line would throw a + // TypeError, which the surrounding .fail() turns into a 500 "Invalid + // application configuration" -- so a 200 here is the real assertion. + return server.get_p('/myapp/?foo=bar&baz=1').then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(JSON.parse(r.body).url, '/?foo=bar&baz=1'); + }); + }); + + it('redirects to add the trailing slash, keeping the query string', function() { + // An app directory requested without a trailing slash never reaches the + // proxy: DirectoryRouter's onDirectory handler answers with a 301 first. + return server.get_p('/myapp?foo=bar').then(function(r) { + assert.strictEqual(r.status, 301); + assert.strictEqual(r.headers.get('location'), '/myapp/?foo=bar'); + }); + }); + + it('forwards the per-worker shared secret', function() { + return server.get_p('/myapp/').then(function(r) { + var secret = JSON.parse(r.body).sharedSecret; + assert.ok(secret, 'no shiny-shared-secret header reached the app'); + assert.strictEqual(secret, + server.worker.last().endpoint.getSharedSecret()); + }); + }); + + it('forwards the request method', function() { + return server.get_p('/myapp/', {method: 'POST', body: 'x'}).then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(JSON.parse(r.body).method, 'POST'); + }); + }); + + it('404s a URL that matches no app', function() { + return server.get_p('/nope/nothing/here').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + + it('relays the app status code and headers', function() { + return server.get_p('/myapp/').then(function() { + return server.stop_p(); + }) + .then(function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: APP_FILES, + worker: { + handler: function(req, res) { + res.writeHead(404, { + 'Content-Type': 'text/plain', + 'X-App-Header': 'from-the-app' + }); + res.end('app says no'); + } + } + }); + }) + .then(function(s) { + server = s; + return s.get_p('/myapp/'); + }) + .then(function(r) { + assert.strictEqual(r.status, 404); + assert.strictEqual(r.headers.get('x-app-header'), 'from-the-app'); + assert.strictEqual(r.body, 'app says no'); + }); + }); + }); + + describe('X-Powered-By', function() { + var server; + + before(function() { + return testServer.start_p(testConfig.siteDirConfig(), {files: APP_FILES}) + .then(function(s) { server = s; }); + }); + + after(function() { + return server ? server.stop_p() : null; + }); + + // The first middleware sets this unconditionally, and app.disable + // ('x-powered-by') stops Express from overwriting it with "Express". + ['/ping', '/', '/myapp/', '/__assets__/sockjs.min.js', '/no-such-thing'] + .forEach(function(path) { + it('is "Shiny Server" on ' + path, function() { + return server.get_p(path).then(function(r) { + assert.strictEqual(r.headers.get('x-powered-by'), 'Shiny Server'); + }); + }); + }); + }); + + describe('worker connection accounting', function() { + // ShinyProxy acquires an "http" reference before proxying and releases it + // from a res 'finish'/'close' listener. compression wraps res.end, and Node + // has changed finish/close ordering across versions, so this is worth + // pinning down with compression both on and off. + [true, false].forEach(function(compressionOn) { + describe('with compression ' + (compressionOn ? 'on' : 'off'), function() { + var server; + + beforeEach(function() { + return testServer.start_p(testConfig.siteDirConfig({ + preamble: 'http_allow_compression ' + (compressionOn ? 'on' : 'off') + ';' + }), {files: APP_FILES}) + .then(function(s) { server = s; }); + }); + + afterEach(function() { + var s = server; + server = null; + return s ? s.stop_p() : null; + }); + + it('returns httpConn to zero after the response completes', function() { + return server.get_p('/myapp/some/asset.txt', { + headers: {'Accept-Encoding': 'gzip'} + }) + .then(function(r) { + assert.strictEqual(r.status, 200); + var entries = server.workerEntries(); + assert.strictEqual(entries.length, 1); + // The cleanup runs on a res event, which can land a tick after the + // client sees the body. + return waitFor(function() { + return entries[0].data.httpConn === 0; + }, 'httpConn to return to 0, was ' + entries[0].data.httpConn); + }); + }); + + it('does not leak a pending reservation for a non-app-page request', function() { + // isAppPage() is false for a URL with a file extension, so no + // "pending" reference should ever be taken. + return server.get_p('/myapp/some/asset.txt') + .then(function() { + var entries = server.workerEntries(); + return waitFor(function() { + return entries[0].data.httpConn === 0; + }, 'httpConn to return to 0') + .then(function() { + assert.strictEqual(entries[0].data.pendingConn, 0); + }); + }); + }); + + it('reserves a pending session for a successful app page', function() { + // An app page request that succeeds is a strong hint that a SockJS + // connection is about to arrive, so the proxy holds a "pending" + // reference (released by a 45s timer if the session never shows). + return server.get_p('/myapp/') + .then(function(r) { + assert.strictEqual(r.status, 200); + var entries = server.workerEntries(); + return waitFor(function() { + return entries[0].data.httpConn === 0; + }, 'httpConn to return to 0') + .then(function() { + assert.strictEqual(entries[0].data.pendingConn, 1); + }); + }); + }); + + it('does not reserve a pending session when the app page fails', function() { + var s; + return server.stop_p() + .then(function() { + return testServer.start_p(testConfig.siteDirConfig({ + preamble: 'http_allow_compression ' + (compressionOn ? 'on' : 'off') + ';' + }), { + files: APP_FILES, + worker: { + handler: function(req, res) { + res.writeHead(500, {'Content-Type': 'text/plain'}); + res.end('boom'); + } + } + }); + }) + .then(function(started) { + server = s = started; + return s.get_p('/myapp/'); + }) + .then(function(r) { + assert.strictEqual(r.status, 500); + var entries = s.workerEntries(); + return waitFor(function() { + return entries[0].data.httpConn === 0; + }, 'httpConn to return to 0') + .then(function() { + assert.strictEqual(entries[0].data.pendingConn, 0); + }); + }); + }); + }); + }); + }); + + describe('error surface', function() { + var server; + + afterEach(function() { + var s = server; + server = null; + return s ? s.stop_p() : null; + }); + + function start_p(workerOptions) { + return testServer.start_p(testConfig.siteDirConfig(), { + files: APP_FILES, + worker: workerOptions + }) + .then(function(s) { server = s; return s; }); + } + + it('runs Express in development mode, because NODE_ENV is never set', function() { + return start_p().then(function(server) { + // Consequence: an unhandled synchronous throw anywhere in the middleware + // stack reaches finalhandler, which in development mode writes err.stack + // to the response. There is no 4-arg error-handling middleware to stop + // it. Recorded here so that adding one (or setting NODE_ENV) is a + // deliberate, visible change rather than an accident. + assert.strictEqual(process.env.NODE_ENV, undefined); + assert.strictEqual(server.handle.app.get('env'), 'development'); + }); + }); + + it('renders a 500 page, not a stack trace, when the app drops the connection', function() { + // The app accepts the request and then destroys the socket without + // answering, which is what http-proxy's 'error' event is for. That path + // is handled (error500), so the user sees a rendered page rather than a + // stack. Contrast with the finalhandler path above, which is not. + return start_p({ + handler: function(req, res) { + req.socket.destroy(); + } + }) + .then(function(server) { + return server.get_p('/myapp/'); + }) + .then(function(r) { + assert.strictEqual(r.status, 500); + assert.ok(/error has occurred/i.test(r.body), + 'expected the rendered 500 page, got: ' + r.body.slice(0, 400)); + assert.ok(!/\bat \w+ \(/.test(r.body), + 'a stack trace leaked into the error page: ' + r.body.slice(0, 400)); + }); + }); + + it('serves a 503 page when the app is at capacity', function() { + // simple_scheduler with max_requests 1 makes the second concurrent + // request throw OutOfCapacityError synchronously out of + // schedulerRegistry.getWorker, which httpListener catches by hand + // (outside the promise chain) and turns into errorAppOverloaded. + var release; + var blocked = new Promise(function(resolve) { release = resolve; }); + + return testServer.start_p(testConfig.siteDirConfig({ + locationBody: ' simple_scheduler 1;' + }), { + files: APP_FILES, + worker: { + handler: function(req, res) { + blocked.then(function() { + res.writeHead(200, {'Content-Type': 'text/plain'}); + res.end('ok'); + }); + } + } + }) + .then(function(s) { + server = s; + // First request opens a session and holds it. + var first = s.get_p('/myapp/'); + return waitFor(function() { + var entries = s.workerEntries(); + return entries.length === 1 && entries[0].sessionCount() >= 1; + }, 'the first request to occupy the only session slot') + .then(function() { + return s.get_p('/myapp/'); + }) + .then(function(r) { + release(); + return first.then(function() { return r; }); + }); + }) + .then(function(r) { + assert.strictEqual(r.status, 503); + assert.ok(/too many users/i.test(r.body), + 'expected the app-overloaded page, got: ' + r.body.slice(0, 400)); + }); + }); + }); +}); + +/** + * Polls until `predicate` is true, or fails the test after ~2 seconds. + */ +function waitFor(predicate, what) { + var deadline = Date.now() + 2000; + return new Promise(function(resolve, reject) { + (function poll() { + if (predicate()) return resolve(); + if (Date.now() > deadline) + return reject(new Error('Timed out waiting for ' + what)); + setTimeout(poll, 10); + })(); + }); +} diff --git a/test/integration/static-files.js b/test/integration/static-files.js new file mode 100644 index 00000000..ccc8c5ef --- /dev/null +++ b/test/integration/static-files.js @@ -0,0 +1,135 @@ +/* + * test/integration/static-files.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Characterization of static file serving through DirectoryRouter, which is +// where `send` is used directly. This is the code most exposed to the +// send 0.19 -> 1.x upgrade that rides along with Express 5. + +var assert = require('assert'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +describe('static files', function() { + + describe('with directory_index on', function() { + var server; + + before(function() { + return testServer.start_p(testConfig.siteDirConfig(), { + files: { + 'site/plain.txt': 'hello\n', + 'site/script.R': 'cat("hi")\n', + 'site/withindex/index.html': '

the index

', + 'site/withindex/other.txt': 'other\n', + 'site/noindex/a.txt': 'a\n' + } + }) + .then(function(s) { server = s; }); + }); + + after(function() { + return server ? server.stop_p() : null; + }); + + it('serves a plain file', function() { + return server.get_p('/plain.txt').then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(r.body, 'hello\n'); + }); + }); + + it('serves .R as text/R', function() { + // This is the one line changed for send 1.x: directory-router.js does + // `send.mime.define({'text/R': ['r']})` on send 0.19 and + // `require('mime-types').types['r'] = 'text/R'` on send 1.x. Both are + // process-global mutations; this asserts the observable result. + return server.get_p('/script.R').then(function(r) { + assert.strictEqual(r.status, 200); + // Note the uppercase charset: that is what send 0.19's mime table + // produces. If this flips to "utf-8" it means the mime lookup changed, + // which is exactly the kind of drift this test exists to catch. + assert.strictEqual(r.headers.get('content-type'), 'text/R; charset=UTF-8'); + assert.strictEqual(r.body, 'cat("hi")\n'); + }); + }); + + it('serves index.html for a directory that has one', function() { + return server.get_p('/withindex/').then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(r.body, '

the index

'); + }); + }); + + it('redirects a directory without a trailing slash', function() { + return server.get_p('/withindex').then(function(r) { + assert.strictEqual(r.status, 301); + assert.strictEqual(r.headers.get('location'), '/withindex/'); + }); + }); + + it('preserves the query string in the trailing-slash redirect', function() { + return server.get_p('/withindex?a=b').then(function(r) { + assert.strictEqual(r.status, 301); + assert.strictEqual(r.headers.get('location'), '/withindex/?a=b'); + }); + }); + + it('auto-indexes a directory with no index.html', function() { + return server.get_p('/noindex/').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(/a\.txt/.test(r.body), + 'expected an index listing containing a.txt, got: ' + r.body.slice(0, 300)); + }); + }); + + it('404s a missing file', function() { + return server.get_p('/no-such-file.txt').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + }); + + describe('with directory_index off', function() { + var server; + + before(function() { + return testServer.start_p( + testConfig.siteDirConfig({directoryIndex: 'off'}), { + files: { + 'site/noindex/a.txt': 'a\n', + 'site/withindex/index.html': '

the index

' + } + }) + .then(function(s) { server = s; }); + }); + + after(function() { + return server ? server.stop_p() : null; + }); + + it('404s a directory with no index.html', function() { + // $staticServe_p resolves null here, which the router chain treats as + // "not mine"; the request falls out the bottom of the chain as a 404. + return server.get_p('/noindex/').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + + it('still serves index.html when there is one', function() { + return server.get_p('/withindex/').then(function(r) { + assert.strictEqual(r.status, 200); + assert.strictEqual(r.body, '

the index

'); + }); + }); + }); +}); diff --git a/test/support/config.js b/test/support/config.js new file mode 100644 index 00000000..3e53f4d0 --- /dev/null +++ b/test/support/config.js @@ -0,0 +1,122 @@ +/* + * test/support/config.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Builds throwaway shiny-server.conf files in a temp directory. +// +// Config text is written with $-placeholders rather than as static fixture +// files, because two of the values can only be known at run time: the user the +// test process happens to be running as (run_as must name a user that +// posix.getpwnam can resolve and that the process is allowed to become), and +// the absolute path of the checkout. + +var fs = require('fs'); +var os = require('os'); +var path = require('path'); +var paths = require('../../lib/core/paths'); +var permissions = require('../../lib/core/permissions'); + +// The user this process is running as. Suitable for `run_as`, because +// permissions.canRunAs() always accepts it. +exports.processUser = permissions.getProcessUser(); + +exports.projectRoot = paths.projectRoot; + +/** + * Creates a temp directory with a config file in it. + * + * @param {string} text - The config file body. These substitutions are applied: + * `$USER` -> the user this process runs as, `$ROOT` -> the checkout root, + * `$DIR` -> the temp directory the config was written into (handy for + * log_dir and site_dir). + * @param {object} [files] - Extra files to create alongside the config, as a + * map of relative path to contents. Parent directories are created. + * + * @returns {object} `{path, dir, cleanup()}`. + */ +exports.write = write; +function write(text, files) { + var dir = fs.mkdtempSync(path.join(os.tmpdir(), 'shiny-server-test-')); + + var body = text + .replace(/\$USER\b/g, exports.processUser) + .replace(/\$ROOT\b/g, paths.projectRoot.replace(/\/$/, '')) + .replace(/\$DIR\b/g, dir); + + var configPath = path.join(dir, 'shiny-server.conf'); + fs.writeFileSync(configPath, body, 'utf8'); + + Object.keys(files || {}).forEach(function(relPath) { + var full = path.join(dir, relPath); + fs.mkdirSync(path.dirname(full), {recursive: true}); + fs.writeFileSync(full, files[relPath]); + }); + + return { + path: configPath, + dir: dir, + cleanup: function() { + try { + fs.rmSync(dir, {recursive: true, force: true}); + } catch (err) { + // Leaving a temp dir behind is not worth failing a test over. + } + } + }; +} + +/** + * The config most integration tests want: one ephemeral-port server serving + * `site_dir` out of a directory the caller controls. + * + * Note the listener is pinned to 127.0.0.1 rather than the default wildcard, + * and that is not cosmetic. With `listen 0` on `::`, the OS can hand the server + * an ephemeral port that TcpTransport has just probed-and-released for a worker + * (tcp.js allocates by binding port 0, reading the port, and closing again). + * The stand-in worker then binds that same port on 127.0.0.1 -- which succeeds, + * because a specific-address bind is allowed alongside a wildcard one -- and + * from then on shadows the server for all loopback traffic. The test client + * silently reaches the worker instead of Shiny Server, which shows up as + * unexplainable 404s and "Parse Error: Expected HTTP/". Binding the listener on + * 127.0.0.1 puts it in the same address space as the worker ports, so the + * kernel's allocator will not hand the same port out twice. + * + * @param {object} [opts] + * @param {string} [opts.siteDir] - Value for site_dir. Defaults to `$DIR/site`. + * @param {string} [opts.listen] - Value for the listen directive. Defaults to + * `0 127.0.0.1`. + * @param {string} [opts.directoryIndex] - Value for directory_index. Defaults + * to "on". Pass null to omit the directive entirely. + * @param {string} [opts.locationBody] - Extra directives inside `location /`. + * @param {string} [opts.serverBody] - Extra directives inside `server`. + * @param {string} [opts.preamble] - Extra top-level directives. + */ +exports.siteDirConfig = siteDirConfig; +function siteDirConfig(opts) { + opts = opts || {}; + var directoryIndex = opts.directoryIndex === undefined ? 'on' : opts.directoryIndex; + return [ + 'run_as $USER;', + opts.preamble || '', + 'server {', + ' listen ' + (opts.listen || '0 127.0.0.1') + ';', + opts.serverBody || '', + ' location / {', + ' site_dir ' + (opts.siteDir || '$DIR/site') + ';', + ' log_dir $DIR/logs;', + directoryIndex === null ? '' : ' directory_index ' + directoryIndex + ';', + opts.locationBody || '', + ' }', + '}', + '' + ].join('\n'); +} diff --git a/test/support/fake-worker.js b/test/support/fake-worker.js new file mode 100644 index 00000000..40eae2b7 --- /dev/null +++ b/test/support/fake-worker.js @@ -0,0 +1,178 @@ +/* + * test/support/fake-worker.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Stands in for an R or Python worker process, so that integration tests can +// exercise the whole request path without R installed and without paying +// process-spawn latency. +// +// The seam is lib/worker/app-worker's `launchWorker_p` export. Scheduler +// resolves it as a property at call time (see the `let app_worker` comment in +// lib/scheduler/scheduler.js), so reassigning the export is enough -- no rewire, +// which matters because rewire would produce a second copy of the module that +// the server built by lib/server-init.js would never see. +// +// Everything else stays real: the real TcpTransport allocates a real ephemeral +// port, the fake worker binds a real http.Server to it, and Scheduler's +// connectEndpoint_p really connects before the proxy really proxies. The only +// thing that does not happen is `su`-ing to another user and exec'ing R, which +// is also why the config under test must `run_as` the current user -- +// Scheduler calls posix.getpwnam(appSpec.runAs) regardless of this stub. + +var http = require('http'); +var Q = require('q'); +var app_worker = require('../../lib/worker/app-worker'); + +/** + * Replaces app_worker.launchWorker_p with one that starts an in-process HTTP + * server on the endpoint's port. + * + * @param {object} [options] + * @param {function} [options.handler] - (req, res, worker) request handler for + * the stand-in app. Defaults to a 200 that echoes the request as JSON. + * @param {function} [options.onUpgrade] - (req, socket, head, worker) handler + * for websocket upgrades against the stand-in app. + * + * @returns {object} A control object. Call `uninstall_p()` in an `afterEach`; + * it restores the real launcher and shuts down every stand-in worker. + */ +exports.install = install; +function install(options) { + options = options || {}; + var handler = options.handler || echoHandler; + + var original = app_worker.launchWorker_p; + var workers = []; + + app_worker.launchWorker_p = function(appSpec, pw, endpoint, logFilePath, workerId) { + var worker = new FakeAppWorker(appSpec, endpoint, logFilePath, workerId, + handler, options.onUpgrade); + workers.push(worker); + // Must be a Q promise: Scheduler calls .invoke('getExit_p') on it. + return worker.$listening_p.then(function() { return worker; }); + }; + + return { + /** Every stand-in worker launched since install(), in launch order. */ + workers: workers, + + /** The most recently launched stand-in worker. */ + last: function() { + return workers[workers.length - 1]; + }, + + /** Every request any stand-in worker has received, in arrival order. */ + requests: function() { + return workers.reduce(function(acc, w) { + return acc.concat(w.requests); + }, []); + }, + + uninstall_p: function() { + app_worker.launchWorker_p = original; + return Q.all(workers.map(function(w) { return w.destroy_p(); })); + } + }; +} + +/** + * Implements the slice of the AppWorker interface that Scheduler uses: + * getExit_p(), isRunning(), and kill(). + */ +function FakeAppWorker(appSpec, endpoint, logFilePath, workerId, handler, onUpgrade) { + var self = this; + + this.appSpec = appSpec; + this.endpoint = endpoint; + this.logFilePath = logFilePath; + this.workerId = workerId; + + /** Requests this stand-in app received, as {method, url, headers}. */ + this.requests = []; + + this.$exit = Q.defer(); + this.$running = true; + + this.server = http.createServer(function(req, res) { + self.requests.push({ + method: req.method, + url: req.url, + headers: req.headers + }); + handler(req, res, self); + }); + + if (onUpgrade) { + this.server.on('upgrade', function(req, socket, head) { + onUpgrade(req, socket, head, self); + }); + } + + this.$listening_p = Q.Promise(function(resolve, reject) { + self.server.once('error', reject); + // TcpTransport picked this port by binding and immediately closing a probe + // socket, so there is a small window in which someone else could take it. + // If that happens the launch fails, exactly as a real worker's would. + self.server.listen(+endpoint.getAppWorkerPort(), '127.0.0.1', function() { + resolve(); + }); + }); +} + +(function() { + this.getExit_p = function() { + return this.$exit.promise; + }; + + this.isRunning = function() { + return this.$running; + }; + + /** + * Simulates the worker process going away. Scheduler binds this as the + * AppWorkerHandle's kill function and calls it on idle timeout and shutdown. + */ + this.kill = function(force) { + if (!this.$running) + return; + this.$running = false; + var self = this; + this.server.close(function() { + self.$exit.resolve(0); + }); + this.server.closeAllConnections(); + }; + + this.destroy_p = function() { + this.kill(true); + return this.$exit.promise; + }; +}).call(FakeAppWorker.prototype); + +/** + * The default stand-in app: 200 with a JSON echo of what it received. Enough + * for tests that only care that the proxy delivered the request and relayed the + * response. + */ +exports.echoHandler = echoHandler; +function echoHandler(req, res) { + var body = JSON.stringify({ + url: req.url, + method: req.method, + sharedSecret: req.headers['shiny-shared-secret'] || null + }); + res.writeHead(200, { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(body) + }); + res.end(body); +} diff --git a/test/support/server.js b/test/support/server.js new file mode 100644 index 00000000..9173daac --- /dev/null +++ b/test/support/server.js @@ -0,0 +1,224 @@ +/* + * test/support/server.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Boots a real Shiny Server in-process on an ephemeral port and hands back +// something you can make HTTP requests against. + +require('../../lib/core/log'); +require('../../lib/core/qutil'); + +var http = require('http'); + +var server_init = require('../../lib/server-init'); +var config = require('./config'); +var fakeWorker = require('./fake-worker'); + +// Server startup is chatty at INFO. Tests that want the noise can set +// SHINY_LOG_LEVEL themselves. +if (!process.env.SHINY_LOG_LEVEL) { + logger.setLevel('ERROR'); +} + +/** + * Starts a server from the given config text. + * + * @param {string} configText - Config file body; see support/config.js#write + * for the substitutions applied. + * @param {object} [options] + * @param {object} [options.files] - Extra files to create in the config's temp + * directory, as a map of relative path to contents. + * @param {object|false} [options.worker] - Options for the stand-in worker (see + * support/fake-worker.js#install), or false to leave the real launcher in + * place so that actual R processes are spawned. + * + * @returns {Promise} A promise of the test server handle. + */ +exports.start_p = start_p; +function start_p(configText, options) { + options = options || {}; + + var cfg = config.write(configText, options.files); + var worker = options.worker === false ? null : fakeWorker.install(options.worker); + + // createServer_p resolves when the config is read and every socket has + // settled. If any of that stalls, fail with something more useful than + // mocha's bare "timeout of Nms exceeded". + var timeoutMs = options.startTimeout || 10000; + + return withTimeout_p(server_init.createServer_p(cfg.path), timeoutMs, + 'Server startup did not complete within ' + timeoutMs + 'ms (config: ' + + cfg.path + ')') + .then(function(handle) { + var addresses = handle.addresses(); + if (!addresses.length) { + throw new Error('Server did not bind any address. Bind errors: ' + + handle.bindErrors.map(function(e) { return e.message; }).join('; ')); + } + return new TestServer(handle, cfg, worker, addresses[0].port); + }) + .fail(function(err) { + // Don't leak the temp dir or the patched launcher if startup failed. + if (worker) worker.uninstall_p().eat(); + cfg.cleanup(); + throw err; + }); +} + +/** + * Rejects with `message` if `promise` hasn't settled within `ms`. Unlike + * qutil.withTimeout_p, this clears its timer, so it doesn't keep the process + * alive after a successful start. + */ +function withTimeout_p(promise, ms, message) { + var Q = require('q'); + var deferred = Q.defer(); + var timer = setTimeout(function() { + deferred.reject(new Error(message)); + }, ms); + promise.then( + function(value) { clearTimeout(timer); deferred.resolve(value); }, + function(err) { clearTimeout(timer); deferred.reject(err); } + ); + return deferred.promise; +} + +/** + * Wraps Node's lowercased header object in the case-insensitive `get` accessor + * that the Fetch API provides, so tests read the same either way. + */ +function makeHeaders(raw) { + return { + raw: raw, + get: function(name) { + var value = raw[String(name).toLowerCase()]; + return value === undefined ? null : + (Array.isArray(value) ? value.join(', ') : value); + } + }; +} + +function TestServer(handle, cfg, worker, port) { + this.handle = handle; + this.config = cfg; + this.worker = worker; + this.port = port; + this.baseUrl = 'http://127.0.0.1:' + port; +} + +(function() { + /** + * Makes a request against this server and resolves to + * `{status, headers, body}`, where `headers.get(name)` is case-insensitive. + * Redirects are never followed -- several tests are about the redirect. + * + * Deliberately built on http.request with `agent: false` rather than on + * global fetch(). fetch() pools keep-alive sockets per origin, test servers + * are torn down and restarted within a millisecond or two of each other, and + * ephemeral ports get recycled fast enough that a pooled socket belonging to + * an already-dead server gets handed to the next test -- which fails as + * "other side closed", an HTTP parse error, or (worse) a plausible-looking + * response from the *previous* test's config. `Connection: close` is not a + * workaround: fetch() treats Connection as a forbidden header and drops it + * silently. One connection per request removes the whole class of problem. + * + * @param {string} path - Server-relative, e.g. "/__assets__/sockjs.min.js". + * @param {object} [init] - {method, headers, body, timeout}. + */ + this.get_p = function(path, init) { + init = init || {}; + var url = this.baseUrl + path; + var timeout = init.timeout || 8000; + + return new Promise(function(resolve, reject) { + var req = http.request(url, { + method: init.method || 'GET', + headers: init.headers || {}, + agent: false + }, function(res) { + var chunks = []; + res.on('data', function(c) { chunks.push(c); }); + res.on('end', function() { + resolve({ + status: res.statusCode, + headers: makeHeaders(res.headers), + body: Buffer.concat(chunks).toString('utf8'), + res: res + }); + }); + res.on('error', reject); + }); + + req.setTimeout(timeout, function() { + req.destroy(new Error('Request to ' + url + ' timed out after ' + + timeout + 'ms')); + }); + req.on('error', reject); + + if (init.body) req.write(init.body); + req.end(); + }); + }; + + /** Alias, for tests that read better as `fetch`. */ + this.fetch = function(path, init) { + return this.get_p(path, init); + }; + + /** + * Every live WorkerEntry across every scheduler, so that tests can look at + * the connection counters (`entry.data.httpConn` and friends) that + * ShinyProxy's acquire/release pairs maintain. + */ + this.workerEntries = function() { + var schedulers = this.handle.schedulerRegistry.$schedulers; + return Object.keys(schedulers).reduce(function(acc, key) { + var workers = schedulers[key].$workers; + return acc.concat(Object.keys(workers).map(function(id) { + return workers[id]; + })); + }, []); + }; + + this.stop_p = function() { + var self = this; + this.$destroyConnections(); + return this.handle.shutdown_p() + .then(function() { + return self.worker ? self.worker.uninstall_p() : null; + }) + .fin(function() { + self.config.cleanup(); + }); + }; + + /** + * Hangs up every established connection. + * + * Server#destroy() only stops accepting; it deliberately leaves existing + * connections alone so that a real shutdown can let clients finish. That + * means shutdown_p(), which waits for each listener's 'close' event, would + * block until every lingering socket went away. Tests want teardown to be + * immediate and unconditional. + * + * This reaches into Server's private tables on purpose; exposing it on the + * facade would imply production code should do it, and it should not. + */ + this.$destroyConnections = function() { + var facade = this.handle.server; + var servers = Object.values(facade.$wildcards) + .concat(Object.values(facade.$hosts)); + servers.forEach(function(httpServer) { + httpServer.closeAllConnections(); + }); + }; +}).call(TestServer.prototype); From 95f6e96a940a532f4ef8e1c256c465952f67f90c Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Sat, 29 Aug 2026 13:33:22 -0700 Subject: [PATCH 2/6] Add unit tests for the code the Q and TypeScript ports will touch Independent of Express; this is the part meant to make those two refactors safe. All of it had zero coverage. - test/qutil.js -- forEachPromise_p (the router chain's control flow), map_p's sequencing (a naive Promise.all port would change I/O ordering), serialized, wrap, .eat(). - test/proxy-http.js -- ShinyProxy.httpListener's dispatch contract: the strict `appSpec === true` check, the 404/500/503 paths, and the acquire/release accounting including the _.once cleanup on both 'finish' and 'close'. - test/scheduler-introspection.js -- shutdown() and dump(), the only places that inspect a promise synchronously (Q's isFulfilled/inspect().value, which has no native equivalent and so needs a design change during the Q removal). Pinned as observable outcomes rather than as the Q idiom. - test/connect-endpoint.js -- the retry ladder and both abort paths, whose messages end up on the 500 page. - test/config-{lexer,parser,schema}.js -- the hand-written config language, which the memory bank rates as the highest-value gap. Ported and expanded from the manual.test/ scripts nothing ever ran. Two findings, characterized rather than fixed: - qutil.serialized hands a *queued* caller the previous invocation's outcome. Q's .fin() settles with the original promise's value even when its callback returns a promise, so a queued caller gets whatever ran ahead of it. Benign today only because the sole production user is loadConfig_p, whose queued caller is the SIGHUP handler, which .eat()s the result -- but a live trap for the Q removal. - connectEndpoint_p's `timeoutId` is assigned null and never set, so the clearTimeout on the success path is a no-op. What actually stops the retry loop is the !isPending() guard. Anyone tidying up the unused variable should know which one is load-bearing. Also confirms that manual.test/test-config-config.js fails because of a stale expectation, not a product bug: `run_as;` with no users is legal, because the schema declares `param String users...`. Co-Authored-By: Claude Opus 5 (1M context) --- test/config-lexer.js | 206 +++++++++++++++++ test/config-parser.js | 266 ++++++++++++++++++++++ test/config-schema.js | 328 +++++++++++++++++++++++++++ test/connect-endpoint.js | 159 +++++++++++++ test/proxy-http.js | 386 ++++++++++++++++++++++++++++++++ test/qutil.js | 382 +++++++++++++++++++++++++++++++ test/scheduler-introspection.js | 176 +++++++++++++++ 7 files changed, 1903 insertions(+) create mode 100644 test/config-lexer.js create mode 100644 test/config-parser.js create mode 100644 test/config-schema.js create mode 100644 test/connect-endpoint.js create mode 100644 test/proxy-http.js create mode 100644 test/qutil.js create mode 100644 test/scheduler-introspection.js diff --git a/test/config-lexer.js b/test/config-lexer.js new file mode 100644 index 00000000..7e219fbc --- /dev/null +++ b/test/config-lexer.js @@ -0,0 +1,206 @@ +/* + * test/config-lexer.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Ported from manual.test/test-config-lexer.js, which was a standalone script +// nothing ever ran. The config language is hand-written and entirely +// self-contained, which makes it both the cheapest thing to cover and the +// scariest thing to port to TypeScript uncovered. + +var assert = require('assert'); +var lexer = require('../lib/config/lexer'); +var TT = lexer.TT; + +// The lexer's character classes are private, so reproduce the numbering from +// lib/config/lexer.js rather than exporting it just for the test. +var c_ = 1; +var C_ALPHA = c_++; +var C_DIGIT = c_++; +var C_OPENBRACE = c_++; +var C_CLOSEBRACE = c_++; +var C_SEMICOLON = c_++; +var C_HASH = c_++; +var C_SQUOTE = c_++; +var C_DQUOTE = c_++; +var C_BACKSLASH = c_++; +var C_EOL = c_++; +var C_WS = c_++; +var C_CONTROL = c_++; +var C_OTHER = c_ + 100; + +/** + * Lexes `data` and asserts the full token stream, as + * [type, content, line, col] tuples. EOD is implicit. + */ +function assertLex(data, expected) { + var lex = new lexer.Lexer(data); + var actual = []; + var tok; + while ((tok = lex.nextToken()).type != TT.EOD) { + actual.push([tok.type, tok.content, tok.position.line, tok.position.col]); + } + assert.deepStrictEqual(actual, expected); +} + +function lexAll(data) { + return function() { + var lex = new lexer.Lexer(data); + while (lex.nextToken().type != TT.EOD) {} + }; +} + +describe('config lexer', function() { + + describe('character classification', function() { + var lex = new lexer.Lexer(''); + + var cases = [ + ['a', C_ALPHA], ['Z', C_ALPHA], + ['1', C_DIGIT], + ['{', C_OPENBRACE], ['}', C_CLOSEBRACE], + [';', C_SEMICOLON], + ['#', C_HASH], + ["'", C_SQUOTE], ['"', C_DQUOTE], + ['\\', C_BACKSLASH], + [' ', C_WS], ['\t', C_WS], ['\r', C_WS], + ['\n', C_EOL], + ['\x01', C_CONTROL], + ['?', C_OTHER] + ]; + + cases.forEach(function(c) { + it('classifies ' + JSON.stringify(c[0]), function() { + assert.strictEqual(lex.$classify(c[0]), c[1]); + }); + }); + }); + + describe('tokenizing', function() { + it('lexes a bare word', function() { + assertLex('foo', [[TT.WORD, 'foo', 1, 1]]); + }); + + it('treats punctuation other than the specials as part of a word', function() { + assertLex('foo \t b12?ar', [ + [TT.WORD, 'foo', 1, 1], + [TT.WS, ' \t ', 1, 4], + [TT.WORD, 'b12?ar', 1, 11] + ]); + }); + + it('unescapes a double-quoted string', function() { + assertLex('"hel\\"\\\'\'\\l\\\\o"', [ + [TT.WORD, 'hel"\'\'l\\o', 1, 1] + ]); + }); + + it('lexes a comment after whitespace', function() { + assertLex('foo # "hello"', [ + [TT.WORD, 'foo', 1, 1], + [TT.WS, ' ', 1, 4], + [TT.COMMENT, ' "hello"', 1, 5] + ]); + }); + + it('lexes a comment with no preceding whitespace', function() { + assertLex('foo# "hello"', [ + [TT.WORD, 'foo', 1, 1], + [TT.COMMENT, ' "hello"', 1, 4] + ]); + }); + + it('does not treat # inside a quoted string as a comment', function() { + assertLex('\'foo # "hello"\'', [ + [TT.WORD, 'foo # "hello"', 1, 1] + ]); + }); + + it('lexes an empty comment', function() { + assertLex('#\nhi', [ + [TT.COMMENT, '', 1, 1], + [TT.WS, '\n', 1, 2], + [TT.WORD, 'hi', 2, 1] + ]); + }); + + it('normalizes CRLF to LF', function() { + assertLex('foo\r\nbar', [ + [TT.WORD, 'foo', 1, 1], + [TT.WS, '\n', 1, 4], + [TT.WORD, 'bar', 2, 1] + ]); + }); + + it('allows a newline inside a quoted string and keeps line numbers right', function() { + assertLex('foo"\nbar"baz', [ + [TT.WORD, 'foo', 1, 1], + [TT.WORD, '\nbar', 1, 4], + [TT.WORD, 'baz', 2, 5] + ]); + }); + + it('lexes braces and semicolons as their own tokens', function() { + assertLex('a {b;}', [ + [TT.WORD, 'a', 1, 1], + [TT.WS, ' ', 1, 2], + [TT.OPENBRACE, '{', 1, 3], + [TT.WORD, 'b', 1, 4], + [TT.TERM, ';', 1, 5], + [TT.CLOSEBRACE, '}', 1, 6] + ]); + }); + + it('lexes empty input as nothing but EOD', function() { + assertLex('', []); + }); + }); + + describe('errors', function() { + it('rejects an unterminated quote', function() { + assert.throws(lexAll('"')); + }); + + it('rejects a trailing backslash inside a quote', function() { + assert.throws(lexAll('"\\')); + }); + + it('rejects a quote whose terminator was escaped', function() { + assert.throws(lexAll('"\\"')); + }); + + it('rejects control characters', function() { + assert.throws(lexAll('\x01')); + }); + + it('reports the position of the offending character', function() { + try { + lexAll('foo\nbar \x01')(); + assert.fail('should have thrown'); + } catch (err) { + assert.ok(err.position, 'error should carry a position'); + assert.strictEqual(err.position.line, 2); + assert.strictEqual(err.position.col, 5); + } + }); + + it('includes the path hint in the position when given one', function() { + try { + var lex = new lexer.Lexer('\x01', '/etc/shiny-server/shiny-server.conf'); + lex.nextToken(); + assert.fail('should have thrown'); + } catch (err) { + assert.ok(/shiny-server\.conf:1:1/.test(err.position.toString()), + 'unexpected position: ' + err.position.toString()); + } + }); + }); +}); diff --git a/test/config-parser.js b/test/config-parser.js new file mode 100644 index 00000000..a46c27be --- /dev/null +++ b/test/config-parser.js @@ -0,0 +1,266 @@ +/* + * test/config-parser.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// The parser turns the lexer's token stream into a tree of directives, and +// config.js then wraps that in ConfigNode, whose lookup methods (getOne, +// getValue, search) implement the inheritance rules the whole config system +// rests on. manual.test/test-config-parser.js printed a tree for a human to +// eyeball; these are actual assertions. + +var assert = require('assert'); +var config = require('../lib/config/config'); + +/** + * Reduces a ConfigNode tree to plain data, for easy structural assertions. + */ +function simplify(node) { + return { + name: node.name, + args: node.args, + children: node.children.map(simplify) + }; +} + +describe('config parser', function() { + + describe('structure', function() { + it('parses a simple directive with no args', function() { + var root = config.parse('foo;'); + assert.deepStrictEqual(simplify(root), { + name: null, args: [], children: [ + {name: 'foo', args: [], children: []} + ] + }); + }); + + it('parses arguments', function() { + var root = config.parse('run_as shiny admin;'); + assert.deepStrictEqual(root.children[0].name, 'run_as'); + assert.deepStrictEqual(root.children[0].args, ['shiny', 'admin']); + }); + + it('parses a block with children', function() { + var root = config.parse('server {\n listen 3838;\n}'); + assert.deepStrictEqual(simplify(root), { + name: null, args: [], children: [ + {name: 'server', args: [], children: [ + {name: 'listen', args: ['3838'], children: []} + ]} + ] + }); + }); + + it('parses a block with both args and children', function() { + var root = config.parse('location /foo { app_dir /bar; }'); + var loc = root.children[0]; + assert.strictEqual(loc.name, 'location'); + assert.deepStrictEqual(loc.args, ['/foo']); + assert.strictEqual(loc.children.length, 1); + }); + + it('nests arbitrarily deep', function() { + var root = config.parse('a { b { c { d 1; } } }'); + var node = root; + ['a', 'b', 'c', 'd'].forEach(function(name) { + node = node.children[0]; + assert.strictEqual(node.name, name); + }); + assert.deepStrictEqual(node.args, ['1']); + }); + + it('records depth, starting at 0 for the root', function() { + var root = config.parse('a { b; }'); + assert.strictEqual(root.depth, 0); + assert.strictEqual(root.children[0].depth, 1); + assert.strictEqual(root.children[0].children[0].depth, 2); + }); + + it('links each node to its parent', function() { + var root = config.parse('a { b; }'); + var a = root.children[0]; + assert.strictEqual(a.parent, root); + assert.strictEqual(a.children[0].parent, a); + assert.strictEqual(root.parent, null); + }); + + it('ignores comments and whitespace', function() { + var root = config.parse('# a comment\n\n foo bar; # trailing\n'); + assert.strictEqual(root.children.length, 1); + assert.deepStrictEqual(root.children[0].args, ['bar']); + }); + + it('skips empty statements', function() { + var root = config.parse(';;; foo; ;;'); + assert.strictEqual(root.children.length, 1); + assert.strictEqual(root.children[0].name, 'foo'); + }); + + it('parses an empty document into a childless root', function() { + var root = config.parse(''); + assert.strictEqual(root.name, null); + assert.deepStrictEqual(root.children, []); + }); + + it('keeps quoted arguments intact', function() { + var root = config.parse('desc "hello world; # not a comment";'); + assert.deepStrictEqual(root.children[0].args, + ['hello world; # not a comment']); + }); + + it('records the position of each directive', function() { + var root = config.parse('a;\n\n b;'); + assert.strictEqual(root.children[0].position.line, 1); + assert.strictEqual(root.children[0].position.col, 1); + assert.strictEqual(root.children[1].position.line, 3); + assert.strictEqual(root.children[1].position.col, 3); + }); + }); + + describe('syntax errors', function() { + function assertParseError(text, pattern) { + assert.throws( + function() { config.parse(text, '/tmp/test.conf'); }, + function(err) { + assert.ok(pattern.test(err.message), + 'expected /' + pattern.source + '/, got: ' + err.message); + return true; + } + ); + } + + it('rejects a directive with no terminator', function() { + assertParseError('foo bar', /Unterminated directive/); + }); + + it('rejects an unclosed scope', function() { + assertParseError('server { listen 80;', /scope was never closed/i); + }); + + it('rejects a stray closing brace', function() { + assertParseError('}', /Unexpected \} character/); + }); + + it('suggests the missing semicolon when a scope closes mid-directive', function() { + assertParseError('server { listen 80 }', /did you leave a semicolon off/); + }); + + it('annotates the error message with file, line and column', function() { + assertParseError('a;\nserver { listen 80;', /\/tmp\/test\.conf:2:1/); + }); + }); + + describe('ConfigNode lookup', function() { + var root; + + beforeEach(function() { + root = config.parse([ + 'run_as shiny;', + 'server {', + ' listen 3838;', + ' location /a {', + ' app_dir /srv/a;', + ' location /b {', + ' app_dir /srv/b;', + ' }', + ' }', + '}' + ].join('\n')); + }); + + function locationB() { + return root.children[1].children[1].children[1]; + } + + it('getOne finds a direct child', function() { + assert.strictEqual(root.getOne('run_as', false).name, 'run_as'); + }); + + it('getOne inherits from ancestors by default', function() { + // This is the mechanism behind "run_as declared once at the top applies + // everywhere below". + assert.strictEqual(locationB().getOne('run_as').name, 'run_as'); + }); + + it('getOne does not inherit when told not to', function() { + assert.strictEqual(locationB().getOne('run_as', false), null); + }); + + it('getOne prefers the nearest definition', function() { + var b = locationB(); + assert.deepStrictEqual(b.getOne('app_dir').args, ['/srv/b']); + }); + + it('getValue returns the first argument', function() { + assert.strictEqual(root.getValue('run_as'), 'shiny'); + }); + + it('getValue falls back to the default when there is no match', function() { + assert.strictEqual(root.getValue('nonexistent', 'fallback'), 'fallback'); + }); + + it('getValue falls back to the default when the directive has no args', function() { + var node = config.parse('empty;'); + assert.strictEqual(node.getValue('empty', 'fallback'), 'fallback'); + }); + + it('getAll returns only direct children', function() { + var multi = config.parse('a 1; a 2; b { a 3; }'); + assert.deepStrictEqual( + multi.getAll('a').map(function(n) { return n.args[0]; }), + ['1', '2']); + }); + + it('search finds descendants depth-first, preorder', function() { + var names = root.search(/^location$/, true).map(function(n) { + return n.args[0]; + }); + assert.deepStrictEqual(names, ['/a', '/b']); + }); + + it('search in postOrder puts nested locations first', function() { + // config-router relies on this so a nested location wins over its parent. + var names = root.search('location', false, true).map(function(n) { + return n.args[0]; + }); + assert.deepStrictEqual(names, ['/b', '/a']); + }); + + it('search honors includeSelf', function() { + var server = root.children[1]; + assert.strictEqual(server.search('server', true).length, 1); + assert.strictEqual(server.search('server', false).length, 0); + }); + + it('accepts a criteria of true or false', function() { + assert.ok(root.search(true, true).length > 5); + assert.deepStrictEqual(root.search(false, true), []); + }); + + it('accepts a function as criteria', function() { + var found = root.search(function(node) { + return node.args.length === 1 && node.args[0] === '/srv/b'; + }, true); + assert.strictEqual(found.length, 1); + assert.strictEqual(found[0].name, 'app_dir'); + }); + + it('rejects an unusable criteria type', function() { + assert.throws(function() { root.search(42, true); }, + /Unexpected criteria type/); + }); + + it('getValues returns an empty object rather than null when unmatched', function() { + assert.deepStrictEqual(root.getValues('nonexistent'), {}); + }); + }); +}); diff --git a/test/config-schema.js b/test/config-schema.js new file mode 100644 index 00000000..e8614ce8 --- /dev/null +++ b/test/config-schema.js @@ -0,0 +1,328 @@ +/* + * test/config-schema.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// The schema layer is what turns a parsed config tree into typed `values` and +// what produces every "you can't put that there" error an administrator ever +// sees. The schema itself is written in the same config language +// (config/shiny-server-rules.config), so this covers both halves: applying a +// schema to a config, and the schema language's own validation. + +var assert = require('assert'); +var config = require('../lib/config/config'); +var schema = require('../lib/config/schema'); +var paths = require('../lib/core/paths'); + +/** + * Applies `schemaText` to `configText` and returns the validated root. + */ +function apply(configText, schemaText) { + return schema.applySchema( + config.parse(configText, '/tmp/test.conf'), + config.parse(schemaText, '/tmp/schema.conf')); +} + +function assertRejects(configText, schemaText, pattern) { + assert.throws( + function() { apply(configText, schemaText); }, + function(err) { + assert.ok(pattern.test(err.message), + 'expected /' + pattern.source + '/, got: ' + err.message); + return true; + } + ); +} + +describe('config schema', function() { + + describe('argument typing', function() { + var SCHEMA = [ + 'thing {', + ' param Integer count "how many";', + ' param Boolean flag "whether";', + ' param Float ratio "how much";', + ' param String label "what";', + ' at $;', + '}' + ].join('\n'); + + it('converts each argument to its declared type', function() { + var values = apply('thing 42 on 1.5 hello;', SCHEMA).children[0].values; + // `values` comes from map.create(), so it has a null prototype -- a + // deliberate choice so that a directive named e.g. "constructor" can't + // collide with Object.prototype. Spread it before comparing. + assert.strictEqual(Object.getPrototypeOf(values), null); + assert.deepStrictEqual(Object.assign({}, values), { + count: 42, flag: true, ratio: 1.5, label: 'hello' + }); + }); + + ['true', 'yes', 'on', 'TRUE', 'On'].forEach(function(v) { + it('reads Boolean "' + v + '" as true', function() { + assert.strictEqual( + apply('thing 1 ' + v + ' 1.0 x;', SCHEMA).children[0].values.flag, + true); + }); + }); + + ['false', 'no', 'off', 'OFF'].forEach(function(v) { + it('reads Boolean "' + v + '" as false', function() { + assert.strictEqual( + apply('thing 1 ' + v + ' 1.0 x;', SCHEMA).children[0].values.flag, + false); + }); + }); + + it('rejects a non-Boolean where a Boolean is required', function() { + assertRejects('thing 1 maybe 1.0 x;', SCHEMA, /not a valid Boolean/); + }); + + it('accepts a negative Integer', function() { + assert.strictEqual( + apply('thing -5 on 1.0 x;', SCHEMA).children[0].values.count, -5); + }); + + it('accepts a hex Integer', function() { + assert.strictEqual( + apply('thing 0x10 on 1.0 x;', SCHEMA).children[0].values.count, 16); + }); + + it('accepts zero, which several directives treat as "unlimited"', function() { + assert.strictEqual( + apply('thing 0 on 1.0 x;', SCHEMA).children[0].values.count, 0); + }); + + it('rejects a non-Integer', function() { + assertRejects('thing 1.5 on 1.0 x;', SCHEMA, /not a valid Integer/); + }); + + it('rejects a non-Float', function() { + assertRejects('thing 1 on . x;', SCHEMA, /not a valid Float/); + }); + }); + + describe('arity', function() { + var SCHEMA = [ + 'thing {', + ' param String required "r";', + ' param String [optional] "o";', + ' at $;', + '}' + ].join('\n'); + + it('accepts just the required argument', function() { + var values = apply('thing a;', SCHEMA).children[0].values; + assert.strictEqual(values.required, 'a'); + assert.strictEqual(values.optional, undefined); + }); + + it('accepts the optional argument too', function() { + var values = apply('thing a b;', SCHEMA).children[0].values; + assert.deepStrictEqual([values.required, values.optional], ['a', 'b']); + }); + + it('rejects too few arguments', function() { + assertRejects('thing;', SCHEMA, /too few arguments; expected 1 to 2, found 0/); + }); + + it('rejects too many arguments', function() { + assertRejects('thing a b c;', SCHEMA, /too many arguments; expected 1 to 2, found 3/); + }); + + it('applies a declared default to a missing optional argument', function() { + var s = 'thing { param String [mode] "m" fallback; at $; }'; + assert.strictEqual(apply('thing;', s).children[0].values.mode, 'fallback'); + }); + + it('collects a vararg into an array', function() { + var s = 'thing { param String users... "u"; at $; }'; + assert.deepStrictEqual( + apply('thing alice bob carol;', s).children[0].values.users, + ['alice', 'bob', 'carol']); + }); + + it('lets a vararg match zero arguments', function() { + // This is why `run_as;` with no users parses -- the manual.test script + // that expected a rejection had gone stale, not the product. + var s = 'thing { param String users... "u"; at $; }'; + assert.strictEqual(apply('thing;', s).children[0].values.users, undefined); + }); + }); + + describe('placement', function() { + var SCHEMA = [ + 'outer { at $; }', + 'inner { at outer; }', + 'anywhere { at $ outer; }' + ].join('\n'); + + it('accepts a directive at a permitted location', function() { + assert.doesNotThrow(function() { apply('outer { inner; }', SCHEMA); }); + }); + + it('accepts a directive permitted in more than one place', function() { + assert.doesNotThrow(function() { + apply('anywhere;\nouter { anywhere; }', SCHEMA); + }); + }); + + it('rejects a directive at the root when it belongs in a scope', function() { + assertRejects('inner;', SCHEMA, /inner directive can't be used here/); + }); + + it('rejects a directive nested where it does not belong', function() { + assertRejects('outer { outer; }', SCHEMA, /outer directive can't be used here/); + }); + + it('rejects an unknown directive', function() { + assertRejects('mystery;', SCHEMA, /Unknown directive "mystery"/); + }); + + it('annotates the error with file, line and column', function() { + assertRejects('outer {\n inner;\n inner;\n}', + 'outer { at $; }\ninner { at outer; maxcount 1; }', + /\/tmp\/test\.conf:3:3/); + }); + }); + + describe('maxcount', function() { + var SCHEMA = 'once { at $; maxcount 1; }\nmany { at $; }'; + + it('accepts a directive up to its limit', function() { + assert.doesNotThrow(function() { apply('once;', SCHEMA); }); + }); + + it('rejects a directive past its limit', function() { + assertRejects('once; once;', SCHEMA, /once directive appears too many times/); + }); + + it('has no limit by default', function() { + assert.doesNotThrow(function() { apply('many; many; many;', SCHEMA); }); + }); + + it('counts per scope, not globally', function() { + var s = 'scope { at $; }\nonce { at scope; maxcount 1; }'; + assert.doesNotThrow(function() { + apply('scope { once; }\nscope { once; }', s); + }); + }); + }); + + describe('precludes', function() { + var SCHEMA = [ + 'scope { at $; }', + 'alpha { at scope; precludes beta; }', + 'beta { at scope; }' + ].join('\n'); + + it('accepts either directive alone', function() { + assert.doesNotThrow(function() { apply('scope { alpha; }', SCHEMA); }); + assert.doesNotThrow(function() { apply('scope { beta; }', SCHEMA); }); + }); + + it('rejects the two together', function() { + assertRejects('scope { beta; alpha; }', SCHEMA, + /alpha and beta directives are mutually exclusive/); + }); + }); + + describe('the schema language itself', function() { + it('rejects a rule with no "at"', function() { + assertRejects('thing;', 'thing { param String x "d"; }', + /Missing "at" directive/); + }); + + it('rejects an unknown parameter type', function() { + assertRejects('thing x;', 'thing { param Widget x "d"; at $; }', + /Unknown type "Widget"/); + }); + + it('rejects duplicate parameter names', function() { + assertRejects('thing a b;', + 'thing { param String x "d"; param String x "d"; at $; }', + /Not all param names were unique/); + }); + + it('rejects a required parameter after an optional one', function() { + assertRejects('thing a b;', + 'thing { param String [x] "d"; param String y "d"; at $; }', + /Required parameter defined after non-required/); + }); + + it('rejects a default value on a required parameter', function() { + assertRejects('thing a;', + 'thing { param String x "d" somedefault; at $; }', + /Only optional parameters can have default values/); + }); + + it('rejects an under-specified param', function() { + assertRejects('thing a;', 'thing { param String x; at $; }', + /Invalid schema specification/); + }); + }); + + describe('the real shiny-server-rules.config', function() { + var schemaText; + + before(function() { + schemaText = require('fs').readFileSync( + paths.projectFile('config/shiny-server-rules.config'), 'utf8'); + }); + + function applyReal(configText) { + return schema.applySchema( + config.parse(configText, '/tmp/test.conf'), + config.parse(schemaText, 'shiny-server-rules.config')); + } + + it('validates a minimal working config', function() { + var root = applyReal([ + 'run_as shiny;', + 'server {', + ' listen 3838;', + ' location / {', + ' site_dir /srv/shiny-server;', + ' log_dir /var/log/shiny-server;', + ' directory_index on;', + ' }', + '}' + ].join('\n')); + + var listen = root.search('listen', false)[0]; + assert.strictEqual(listen.values.port, 3838); + assert.strictEqual(typeof listen.values.port, 'number'); + }); + + it('accepts port 0, which means "pick an ephemeral port"', function() { + // Relied on by the integration harness; also the reason + // config-router.js's permission check exempts port 0. + var root = applyReal('run_as shiny;\nserver { listen 0 127.0.0.1; }'); + assert.strictEqual(root.search('listen', false)[0].values.port, 0); + }); + + it('accepts run_as with no users, thanks to its vararg', function() { + assert.doesNotThrow(function() { applyReal('run_as;'); }); + }); + + it('rejects a directive in the wrong scope', function() { + assert.throws(function() { + applyReal('listen 3838;'); + }, /can't be used here/); + }); + + it('rejects a misspelled directive', function() { + assert.throws(function() { + applyReal('run_as shiny;\nserver { listne 3838; }'); + }, /Unknown directive "listne"/); + }); + }); +}); diff --git a/test/connect-endpoint.js b/test/connect-endpoint.js new file mode 100644 index 00000000..5402e8e9 --- /dev/null +++ b/test/connect-endpoint.js @@ -0,0 +1,159 @@ +/* + * test/connect-endpoint.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// connectEndpoint_p is what decides whether a freshly launched app "came up". +// It's module-private in lib/scheduler/scheduler.js, hence rewire. Every one of +// its exit paths is user-visible: the two rejection messages are what end up on +// the 500 page. + +var assert = require('assert'); +var Q = require('q'); +var rewire = require('rewire'); + +var Scheduler = rewire('../lib/scheduler/scheduler.js'); +var connectEndpoint_p = Scheduler.__get__('connectEndpoint_p'); + +/** + * An endpoint double whose connect_p resolves false until the Nth call. + */ +function makeEndpoint(succeedOnAttempt) { + return { + attempts: 0, + connect_p: function() { + this.attempts++; + return Q.resolve(this.attempts >= succeedOnAttempt); + }, + toString: function() { return 'port 1234'; } + }; +} + +function alwaysContinue() { return true; } + +describe('connectEndpoint_p', function() { + + it('resolves true when the first attempt connects', function() { + var endpoint = makeEndpoint(1); + return connectEndpoint_p(endpoint, 5000, alwaysContinue) + .then(function(result) { + assert.strictEqual(result, true); + assert.strictEqual(endpoint.attempts, 1); + }); + }); + + it('retries until the app is listening', function() { + var endpoint = makeEndpoint(3); + return connectEndpoint_p(endpoint, 5000, alwaysContinue) + .then(function(result) { + assert.strictEqual(result, true); + assert.strictEqual(endpoint.attempts, 3); + }); + }); + + it('rejects with the user-facing timeout message once the budget is spent', function() { + // The ladder's first interval is 50ms, so with a 10ms budget the second + // attempt is already over budget. + var endpoint = makeEndpoint(Infinity); + return connectEndpoint_p(endpoint, 10, alwaysContinue) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + assert.strictEqual(err.message, 'The application took too long to respond.'); + assert.strictEqual(endpoint.attempts, 1); + } + ); + }); + + it('aborts immediately when shouldContinue() is already false', function() { + // In production shouldContinue is bound to exitPromise.isPending, so this + // is the "the process died while we were waiting for it" path. + var endpoint = makeEndpoint(Infinity); + return connectEndpoint_p(endpoint, 5000, function() { return false; }) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + assert.strictEqual(err.message, 'Connection attempt was aborted.'); + assert.strictEqual(endpoint.attempts, 0, + 'should not even try to connect once the caller has given up'); + } + ); + }); + + it('aborts partway through if shouldContinue() flips to false', function() { + var endpoint = makeEndpoint(Infinity); + var alive = true; + setTimeout(function() { alive = false; }, 60); + + return connectEndpoint_p(endpoint, 5000, function() { return alive; }) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + assert.strictEqual(err.message, 'Connection attempt was aborted.'); + assert.ok(endpoint.attempts >= 1 && endpoint.attempts < 12, + 'expected to stop early, made ' + endpoint.attempts + ' attempts'); + } + ); + }); + + it('checks the time budget before checking shouldContinue', function() { + // Ordering matters for which of the two messages the user sees when both + // conditions are true at once. + var endpoint = makeEndpoint(Infinity); + var calls = 0; + return connectEndpoint_p(endpoint, 10, function() { + calls++; + return false; + }) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + // First pass: within budget, shouldContinue false -> aborted. + assert.strictEqual(err.message, 'Connection attempt was aborted.'); + assert.strictEqual(calls, 1); + } + ); + }); + + it('stops calling connect_p once it has resolved', function() { + // KNOWN QUIRK, characterized rather than fixed: `timeoutId` in + // connectEndpoint_p is assigned null and never reassigned, so the + // `clearTimeout(timeoutId)` on the success path is a no-op and the pending + // retry timer is *not* cancelled. What stops the retry loop is the + // `!deferred.promise.isPending()` guard at the top of attemptToConnect. + // The visible consequence is only that one dead timer is left to fire (up + // to 500ms), not that extra connections are made -- but anyone "fixing" + // the unused variable should know the guard is what's load-bearing. + var endpoint = makeEndpoint(1); + return connectEndpoint_p(endpoint, 5000, alwaysContinue) + .then(function() { + assert.strictEqual(endpoint.attempts, 1); + // Wait past the first ladder interval; the queued timer fires and must + // not produce another connect_p call. + return Q.delay(null, 120); + }) + .then(function() { + assert.strictEqual(endpoint.attempts, 1); + }); + }); + + it('keeps retrying past the end of the interval ladder', function() { + // intervals has 12 entries totalling 1900ms; after that it falls back to + // maxInterval (500ms) rather than giving up. + var endpoint = makeEndpoint(14); + this.timeout(15000); + return connectEndpoint_p(endpoint, 10000, alwaysContinue) + .then(function(result) { + assert.strictEqual(result, true); + assert.strictEqual(endpoint.attempts, 14); + }); + }); +}); diff --git a/test/proxy-http.js b/test/proxy-http.js new file mode 100644 index 00000000..8c55b2f1 --- /dev/null +++ b/test/proxy-http.js @@ -0,0 +1,386 @@ +/* + * test/proxy-http.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// ShinyProxy.httpListener had no unit coverage at all -- test/proxy-events.js +// only greps http-proxy for event names. What is pinned here is its dispatch +// contract: the tri-state the router chain returns, and the two error paths +// that are handled by hand rather than by the promise chain. + +var assert = require('assert'); +var Q = require('q'); +var proxy_http = require('../lib/proxy/http'); +var OutOfCapacityError = require('../lib/core/errors').OutOfCapacity; +var AppSpec = require('../lib/worker/app-spec').AppSpec; + +describe('ShinyProxy.httpListener', function() { + + function makeAppSpec(prefix) { + return new AppSpec('/some/app', 'someuser', prefix || '/app', '/some/logs', { + appDefaults: {sanitizeErrors: false}, + templateDir: null + }); + } + + /** + * A response double that records what was written and resolves `done_p` once + * end() is called, since httpListener answers asynchronously. + */ + function makeRes() { + var deferred = Q.defer(); + var res = { + statusCode: null, + headers: null, + body: '', + finished: false, + proxySuccess: false, + $listeners: {}, + done_p: deferred.promise, + + writeHead: function(status, headers) { + this.statusCode = status; + this.headers = headers; + }, + setHeader: function() {}, + end: function(chunk) { + if (chunk) this.body += chunk; + if (!this.finished) { + this.finished = true; + deferred.resolve(this); + } + }, + on: function(event, listener) { + (this.$listeners[event] = this.$listeners[event] || []).push(listener); + }, + emit: function(event) { + (this.$listeners[event] || []).forEach(function(l) { l(); }); + } + }; + return res; + } + + function makeReq(url) { + var parts = String(url).split('?'); + return { + url: url, + method: 'GET', + headers: {}, + socket: {writable: true}, + _parsedUrl: {pathname: parts[0], query: parts[1] || null}, + paused: false, + resumed: false, + pause: function() { this.paused = true; }, + resume: function() { this.resumed = true; }, + on: function() {} + }; + } + + /** + * Builds a ShinyProxy whose router resolves to `appSpecValue` and whose + * scheduler registry behaves as `getWorker` says. + */ + function makeProxy(appSpecValue, getWorker) { + var router = { + getAppSpec_p: function() { + return Q.resolve(appSpecValue); + } + }; + var schedulerRegistry = { + getWorker: getWorker || function() { + throw new Error('getWorker should not have been called'); + } + }; + return new proxy_http.ShinyProxy(router, schedulerRegistry); + } + + it('pauses the request immediately', function() { + var proxy = makeProxy(null); + var req = makeReq('/whatever'); + var res = makeRes(); + proxy.httpListener(req, res); + // Synchronously, before the router promise has settled -- otherwise events + // could be missed while the router is being consulted. + assert.strictEqual(req.paused, true); + return res.done_p; + }); + + it('does nothing when the router returns exactly true', function() { + // "true" means the router already answered the request itself (a redirect, + // a directory index, /ping). Anything written here would corrupt that. + var proxy = makeProxy(true); + var req = makeReq('/handled'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.strictEqual(res.finished, false); + assert.strictEqual(res.statusCode, null); + }); + }); + + it('404s when the router returns a falsy value', function() { + var proxy = makeProxy(null); + var req = makeReq('/nope'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 404); + }); + }); + + [undefined, false, 0, ''].forEach(function(value) { + it('404s when the router returns ' + JSON.stringify(value), function() { + var proxy = makeProxy(value); + var req = makeReq('/nope'); + var res = makeRes(); + proxy.httpListener(req, res); + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 404); + }); + }); + }); + + it('uses a strict === true check, so a truthy non-AppSpec is not "handled"', function() { + // The check is `appSpec === true`, not `appSpec == true`. A router that + // returned 1 would fall through to the AppSpec path and be asked for a + // prefix, rather than being treated as having answered. Pinning this + // because "=== true" looks like something a refactor would relax. + var proxy = makeProxy(1); + var req = makeReq('/whatever'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + // 1 has no .prefix, so the prefix check fails and the code reaches for + // `appSpec.settings.templateDir` to render the 404 -- which throws, and + // the outer .fail() turns that into a 500. An ugly answer, but an + // unmistakable one: a truthy non-true value is emphatically not treated + // as "the router already handled it". + assert.strictEqual(res.statusCode, 500); + }); + }); + + it('does nothing if the socket has already closed', function() { + var proxy = makeProxy(makeAppSpec('/app')); + var req = makeReq('/app/'); + req.socket.writable = false; + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.strictEqual(res.finished, false); + }); + }); + + it('404s when the router returns a prefix the URL does not start with', function() { + // Defends against a buggy router; the alternative would be slicing the URL + // with a bad offset and proxying nonsense upstream. + var proxy = makeProxy(makeAppSpec('/somewhere-else')); + var req = makeReq('/app/page'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 404); + }); + }); + + it('serves a 503 when the scheduler reports OutOfCapacity', function() { + // getWorker is called outside the promise chain, in a bare try/catch, + // precisely so that a *synchronous* OutOfCapacityError can be turned into a + // 503 instead of escaping as an unhandled exception. Both halves of that + // arrangement are what this test protects. + var proxy = makeProxy(makeAppSpec('/app'), function() { + throw new OutOfCapacityError('no room'); + }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 503); + assert.ok(/too many users/i.test(res.body), + 'expected the app-overloaded page, got: ' + res.body.slice(0, 200)); + }); + }); + + it('turns any other synchronous getWorker error into a 500', function() { + var proxy = makeProxy(makeAppSpec('/app'), function() { + throw new Error('something else went wrong'); + }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 500); + }); + }); + + it('500s when the router itself rejects', function() { + var router = { + getAppSpec_p: function() { return Q.reject(new Error('bad config')); } + }; + var proxy = new proxy_http.ShinyProxy(router, {}); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 500); + }); + }); + + it('404s when getting the worker handle fails with ENOTFOUND', function() { + var err = new Error('no such app'); + err.code = 'ENOTFOUND'; + var proxy = makeProxy(makeAppSpec('/app'), function() { + return makeWorkerEntry(Q.reject(err)); + }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 404); + }); + }); + + it('500s when getting the worker handle fails for any other reason', function() { + var proxy = makeProxy(makeAppSpec('/app'), function() { + return makeWorkerEntry(Q.reject(new Error('the app failed to start'))); + }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return res.done_p.then(function() { + assert.strictEqual(res.statusCode, 500); + assert.ok(/failed to start/i.test(res.body), + 'expected the failure detail in the page, got: ' + res.body.slice(0, 300)); + }); + }); + + describe('connection accounting', function() { + it('acquires http, and pending for an app page, then releases on finish', function() { + var entry = makeWorkerEntry(Q.defer().promise); + var proxy = makeProxy(makeAppSpec('/app'), function() { return entry; }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.deepStrictEqual(entry.acquired, ['http', 'pending']); + res.proxySuccess = true; + res.emit('finish'); + assert.deepStrictEqual(entry.released, ['http']); + // A successful app page keeps its "pending" reservation, backed by a + // timer, because a SockJS session is expected imminently. + assert.deepStrictEqual(entry.pendingTimers, [45 * 1000]); + }); + }); + + it('releases the pending reservation when the app page did not succeed', function() { + var entry = makeWorkerEntry(Q.defer().promise); + var proxy = makeProxy(makeAppSpec('/app'), function() { return entry; }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + res.proxySuccess = false; + res.emit('close'); + assert.deepStrictEqual(entry.released, ['http', 'pending']); + assert.deepStrictEqual(entry.pendingTimers, []); + }); + }); + + it('cleans up only once even if both finish and close fire', function() { + // The cleanup is wrapped in _.once, and it is registered on both events + // because which of them fires (and in what order) depends on the Node + // version and on whether compression wrapped res.end. + var entry = makeWorkerEntry(Q.defer().promise); + var proxy = makeProxy(makeAppSpec('/app'), function() { return entry; }); + var req = makeReq('/app/'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + res.emit('finish'); + res.emit('close'); + assert.deepStrictEqual(entry.released, ['http', 'pending']); + }); + }); + + it('does not take a pending reservation for a non-app-page URL', function() { + var entry = makeWorkerEntry(Q.defer().promise); + var proxy = makeProxy(makeAppSpec('/app'), function() { return entry; }); + var req = makeReq('/app/style.css'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.deepStrictEqual(entry.acquired, ['http']); + res.emit('finish'); + assert.deepStrictEqual(entry.released, ['http']); + }); + }); + + it('does not take a pending reservation when a specific worker was requested', function() { + // isAppPage is `!worker && isAppPagePath(...)`: an explicit ?w= means the + // client already has a session, so no new one should be reserved. + var entry = makeWorkerEntry(Q.defer().promise); + var proxy = makeProxy(makeAppSpec('/app'), function() { return entry; }); + var req = makeReq('/app/?w=abc123'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.deepStrictEqual(entry.acquired, ['http']); + }); + }); + }); + + it('passes the parsed ?w= worker id through to the scheduler', function() { + var seen = {}; + var proxy = makeProxy(makeAppSpec('/app'), function(appSpec, pathname, worker) { + seen.pathname = pathname; + seen.worker = worker; + return makeWorkerEntry(Q.defer().promise); + }); + var req = makeReq('/app/sub/page?w=abc123&other=1'); + var res = makeRes(); + proxy.httpListener(req, res); + + return Q.delay(null, 20).then(function() { + assert.strictEqual(seen.worker, 'abc123'); + // The prefix has been stripped and the query removed by this point. + assert.strictEqual(seen.pathname, '/sub/page'); + }); + }); +}); + +/** + * A WorkerEntry double that records acquire/release calls. + */ +function makeWorkerEntry(handle_p) { + return { + acquired: [], + released: [], + pendingTimers: [], + acquire: function(type) { this.acquired.push(type); }, + release: function(type) { this.released.push(type); }, + pushPendingReleaseTimer: function(ms) { this.pendingTimers.push(ms); }, + getAppWorkerHandle_p: function() { return handle_p; } + }; +} diff --git a/test/qutil.js b/test/qutil.js new file mode 100644 index 00000000..a261e616 --- /dev/null +++ b/test/qutil.js @@ -0,0 +1,382 @@ +/* + * test/qutil.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// These helpers had no coverage at all, which is uncomfortable given that +// forEachPromise_p *is* the router chain's control flow and map_p's sequencing +// is load-bearing. They are also the code most likely to be rewritten when Q is +// replaced with native promises, so pin the semantics -- including the ones a +// naive port would get wrong. + +var assert = require('assert'); +var Q = require('q'); +var qutil = require('../lib/core/qutil'); + +describe('qutil.forEachPromise_p', function() { + // Accepts anything that isn't null/undefined, which is how the router chain + // uses it: an AppSpec (or `true`) is a hit, a falsy value means "not mine". + function acceptTruthy(x) { return !!x; } + + it('resolves with the first accepted value', function() { + var seen = []; + return qutil.forEachPromise_p( + ['a', 'b', 'c'], + function(item) { seen.push(item); return Q.resolve(item === 'a' ? 'hit' : null); }, + acceptTruthy, + 'fallback' + ) + .then(function(result) { + assert.strictEqual(result, 'hit'); + // Stops as soon as something is accepted; b and c are never tried. + assert.deepStrictEqual(seen, ['a']); + }); + }); + + it('skips rejected candidates and keeps going', function() { + var seen = []; + return qutil.forEachPromise_p( + ['a', 'b', 'c'], + function(item) { seen.push(item); return Q.resolve(item === 'c' ? 'hit' : null); }, + acceptTruthy, + 'fallback' + ) + .then(function(result) { + assert.strictEqual(result, 'hit'); + assert.deepStrictEqual(seen, ['a', 'b', 'c']); + }); + }); + + it('visits the array strictly in order', function() { + var seen = []; + return qutil.forEachPromise_p( + [1, 2, 3, 4], + function(item) { + seen.push(item); + // Resolve on a delay that is *longer* for earlier items, so a + // concurrent implementation would produce a different order. + return Q.delay(null, (5 - item) * 5); + }, + acceptTruthy, + 'fallback' + ) + .then(function() { + assert.deepStrictEqual(seen, [1, 2, 3, 4]); + }); + }); + + it('resolves with defaultValue when nothing is accepted', function() { + return qutil.forEachPromise_p( + ['a', 'b'], + function() { return Q.resolve(null); }, + acceptTruthy, + 'fallback' + ) + .then(function(result) { + assert.strictEqual(result, 'fallback'); + }); + }); + + it('resolves with defaultValue for an empty array without calling the iterator', function() { + var called = false; + return qutil.forEachPromise_p( + [], + function() { called = true; return Q.resolve('x'); }, + acceptTruthy, + 'fallback' + ) + .then(function(result) { + assert.strictEqual(result, 'fallback'); + assert.strictEqual(called, false); + }); + }); + + it('rejects the whole operation if any iterator promise rejects', function() { + var seen = []; + return qutil.forEachPromise_p( + ['a', 'b', 'c'], + function(item) { + seen.push(item); + if (item === 'b') return Q.reject(new Error('boom')); + return Q.resolve(null); + }, + acceptTruthy, + 'fallback' + ) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + assert.strictEqual(err.message, 'boom'); + // Gives up immediately; 'c' is never tried. + assert.deepStrictEqual(seen, ['a', 'b']); + } + ); + }); + + it('rejects if the iterator throws synchronously', function() { + // The try/catch around the iterator call matters: without it the throw + // would escape tryNext() and become an unhandled exception rather than a + // rejection, because tryNext is called from a promise callback. + return qutil.forEachPromise_p( + ['a'], + function() { throw new Error('sync boom'); }, + acceptTruthy, + 'fallback' + ) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { assert.strictEqual(err.message, 'sync boom'); } + ); + }); + + it('treats an accepted falsy value as a hit if accept says so', function() { + // accept() is what decides, not truthiness. The router chain relies on + // this to let `true` and an AppSpec both count as hits. + return qutil.forEachPromise_p( + ['a', 'b'], + function(item) { return Q.resolve(item === 'b' ? 0 : null); }, + function(x) { return x !== null; }, + 'fallback' + ) + .then(function(result) { + assert.strictEqual(result, 0); + }); + }); +}); + +describe('qutil.map_p', function() { + it('resolves to the results in input order', function() { + return qutil.map_p([1, 2, 3], function(n) { + return Q.resolve(n * 10); + }) + .then(function(results) { + assert.deepStrictEqual(results, [10, 20, 30]); + }); + }); + + it('runs sequentially, not concurrently', function() { + // This is the property a naive Promise.all() port would destroy. The + // callers depend on it because each step can do I/O whose ordering matters. + var inFlight = 0; + var maxInFlight = 0; + var order = []; + + return qutil.map_p([1, 2, 3, 4], function(n) { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + order.push('start ' + n); + return Q.delay(null, (5 - n) * 5).then(function() { + order.push('end ' + n); + inFlight--; + return n; + }); + }) + .then(function(results) { + assert.strictEqual(maxInFlight, 1, 'expected only one call in flight at a time'); + assert.deepStrictEqual(order, [ + 'start 1', 'end 1', + 'start 2', 'end 2', + 'start 3', 'end 3', + 'start 4', 'end 4' + ]); + assert.deepStrictEqual(results, [1, 2, 3, 4]); + }); + }); + + it('resolves to an empty array for an empty collection', function() { + return qutil.map_p([], function() { + throw new Error('should not be called'); + }) + .then(function(results) { + assert.deepStrictEqual(results, []); + }); + }); + + it('rejects and stops on the first failure', function() { + var seen = []; + return qutil.map_p([1, 2, 3], function(n) { + seen.push(n); + return n === 2 ? Q.reject(new Error('boom')) : Q.resolve(n); + }) + .then( + function() { throw new Error('should have rejected'); }, + function(err) { + assert.strictEqual(err.message, 'boom'); + assert.deepStrictEqual(seen, [1, 2], + 'later items should not be started after a failure'); + } + ); + }); +}); + +describe('qutil.serialized', function() { + // This is what guards config reload on SIGHUP: two signals in quick + // succession must not run two reloads over the same mutable object graph. + + it('does not overlap invocations', function() { + var inFlight = 0; + var maxInFlight = 0; + var completions = []; + + var work = qutil.serialized(function(label, delayMs) { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + return Q.delay(null, delayMs).then(function() { + inFlight--; + completions.push(label); + return label; + }); + }); + + // Start the slow one first; if they overlapped, 'b' would finish first. + var a = work('a', 40); + var b = work('b', 1); + + return Q.all([a, b]).then(function() { + assert.strictEqual(maxInFlight, 1); + assert.deepStrictEqual(completions, ['a', 'b']); + }); + }); + + it('runs a queued invocation even if the one ahead of it fails', function() { + var completions = []; + var work = qutil.serialized(function(label, shouldFail) { + return Q.delay(null, 5).then(function() { + completions.push(label); + if (shouldFail) throw new Error('boom-' + label); + return label; + }); + }); + + var a = work('a', true); + var b = work('b', false); + + return Q.allSettled([a, b]).then(function(states) { + // Both actually ran, in order... + assert.deepStrictEqual(completions, ['a', 'b']); + assert.strictEqual(states[0].state, 'rejected'); + assert.strictEqual(states[0].reason.message, 'boom-a'); + + // ...but see the "reports the *previous*..." test below: b's caller is + // told about a's failure, not b's success. + assert.strictEqual(states[1].state, 'rejected'); + assert.strictEqual(states[1].reason.message, 'boom-a'); + }); + }); + + // KNOWN DEFECT, characterized rather than fixed. + // + // The queueing branch is `currentPromise.fin(function() { return wrapped(...) })`. + // Q's .fin() waits for a promise returned by its callback, but it always + // settles with the *original* promise's outcome. So a caller whose invocation + // was queued is handed the outcome of whatever was running ahead of it. + // + // This is benign today only because nothing inspects the result: the sole + // production user is loadConfig_p in lib/server-init.js, whose queued caller + // is the SIGHUP handler, and that does `.eat()`. It is a live trap for the + // Q-to-async/await port, where the obvious rewrite would silently change this. + it('reports the *previous* invocation\'s outcome to a queued caller', function() { + var work = qutil.serialized(function(label) { + return Q.delay(label, 5); + }); + + var first = work('first'); + var second = work('second'); + + return Q.all([first, second]).then(function(results) { + assert.strictEqual(results[0], 'first'); + assert.strictEqual(results[1], 'first', + 'if this now reads "second", serialized() was fixed -- update this test'); + }); + }); + + it('still queues a call made from the previous call\'s own .then handler', function() { + // A corollary of the same mechanism: the internal `currentPromise = null` + // runs a tick later than the caller's .then, so a follow-up call issued + // from that handler is treated as queued rather than fresh -- and so gets + // the previous result back. + var calls = []; + var work = qutil.serialized(function(label) { + calls.push(label); + return Q.delay(label, 1); + }); + + return work('first').then(function(r1) { + assert.strictEqual(r1, 'first'); + return work('second'); + }) + .then(function(r2) { + assert.deepStrictEqual(calls, ['first', 'second'], + 'the second call must really run, whatever it resolves to'); + assert.strictEqual(r2, 'first'); + }); + }); + + it('starts genuinely fresh once the queue has fully drained', function() { + var work = qutil.serialized(function(label) { + return Q.delay(label, 1); + }); + + return work('first') + // Detach from the previous promise chain so currentPromise is really null. + .then(function() { return Q.delay(null, 10); }) + .then(function() { return work('second'); }) + .then(function(r2) { + assert.strictEqual(r2, 'second'); + }); + }); + + it('preserves `this`', function() { + var obj = { + name: 'obj', + go: qutil.serialized(function() { + return Q.resolve(this.name); + }) + }; + return obj.go().then(function(name) { + assert.strictEqual(name, 'obj'); + }); + }); +}); + +describe('qutil.wrap', function() { + it('resolves with the function result', function() { + return qutil.wrap(function(a, b) { return a + b; })(2, 3) + .then(function(result) { assert.strictEqual(result, 5); }); + }); + + it('converts a synchronous throw into a rejection', function() { + return qutil.wrap(function() { throw new Error('nope'); })() + .then( + function() { throw new Error('should have rejected'); }, + function(err) { assert.strictEqual(err.message, 'nope'); } + ); + }); + + it('preserves `this`', function() { + var obj = {name: 'obj', go: qutil.wrap(function() { return this.name; })}; + return obj.go().then(function(name) { assert.strictEqual(name, 'obj'); }); + }); +}); + +describe('promise.eat()', function() { + it('swallows a rejection so it never becomes an unhandled error', function() { + // Installed on Q.makePromise.prototype by requiring qutil. Modules under + // test assume it is already there; .mocharc.json requires qutil for that + // reason. + var rejected = Q.reject(new Error('ignored')); + assert.strictEqual(typeof rejected.eat, 'function'); + assert.strictEqual(rejected.eat(), undefined); + // Give Q a chance to report an unhandled rejection if eat() failed to + // attach a handler. + return Q.delay(null, 10); + }); +}); diff --git a/test/scheduler-introspection.js b/test/scheduler-introspection.js new file mode 100644 index 00000000..2c55f7cc --- /dev/null +++ b/test/scheduler-introspection.js @@ -0,0 +1,176 @@ +/* + * test/scheduler-introspection.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// Scheduler.shutdown() and Scheduler.dump() are the only places in the codebase +// that inspect a promise *synchronously*, via Q's isFulfilled() and +// inspect().value. Native promises have no equivalent, so both will need an +// actual design change during the Q removal -- most likely tracking the +// resolved handle on the WorkerEntry alongside the promise. +// +// The behaviour those two methods rely on is therefore pinned here in terms of +// observable outcomes ("a worker whose launch has completed gets killed; one +// still starting up does not"), so that a reimplementation can be checked +// against it without having to reproduce the Q idiom. + +var assert = require('assert'); +var Q = require('q'); +var sinon = require('sinon'); +var rewire = require('rewire'); +var { AppSpec } = require('../lib/worker/app-spec.js'); +var SimpleEventBus = require('../lib/events/simple-event-bus'); + +var Scheduler = rewire('../lib/scheduler/scheduler.js'); + +var appSpec = new AppSpec('/var/shiny-www/01_hello/', 'jeff', '', '/tmp', {}); + +describe('Scheduler introspection', function() { + var scheduler; + var killSpy; + var launchDeferred; + var exitDeferred; + + beforeEach(function() { + killSpy = sinon.spy(); + launchDeferred = Q.defer(); + exitDeferred = Q.defer(); + + Scheduler.__set__('app_worker', { + launchWorker_p: function() { + return launchDeferred.promise; + } + }); + + scheduler = new Scheduler(new SimpleEventBus(), appSpec); + scheduler.setTransport({ + alloc_p: function() { + return Q({ + getLogFileSuffix: function() { return ''; }, + ToString: function() { return 'Port 1234'; }, + toString: function() { return 'port 1234'; }, + getSharedSecret: function() { return 'secret'; }, + connect_p: function() { return Q(true); }, + free: function() {} + }); + } + }); + }); + + afterEach(function() { + // Each spawned worker arms an idle timer (5s by default) that nothing in + // these tests ever fires. Left armed, it keeps the mocha process alive for + // five seconds after the last test and then logs a confusing "Failed to + // kill process" from a worker whose test finished long ago. + Object.values(scheduler.$workers).forEach(function(entry) { + entry.close(); + }); + }); + + function completeLaunch() { + launchDeferred.resolve({ + kill: killSpy, + getExit_p: function() { return exitDeferred.promise; }, + isRunning: function() { return true; } + }); + // Let spawnWorker's promise chain run to the point where the WorkerEntry's + // promise is resolved with an AppWorkerHandle. + return Q.delay(null, 20); + } + + describe('shutdown()', function() { + it('kills a worker whose launch has completed', function() { + scheduler.spawnWorker(appSpec, null, true); + return completeLaunch().then(function() { + scheduler.shutdown(); + assert.strictEqual(killSpy.callCount, 1); + // `true` means "notify the app first" -- a graceful shutdown. + assert.deepStrictEqual(killSpy.firstCall.args, [true]); + }); + }); + + it('leaves a still-launching worker alone', function() { + // The launch promise is still pending, so isFulfilled() is false and + // inspect().value would be undefined; calling .kill on it would throw. + scheduler.spawnWorker(appSpec, null, true); + return Q.delay(null, 20).then(function() { + assert.doesNotThrow(function() { scheduler.shutdown(); }); + assert.strictEqual(killSpy.callCount, 0); + }); + }); + + it('does not throw when a worker failed to launch', function() { + scheduler.spawnWorker(appSpec, null, true); + launchDeferred.reject(new Error('R would not start')); + return Q.delay(null, 20).then(function() { + assert.doesNotThrow(function() { scheduler.shutdown(); }); + assert.strictEqual(killSpy.callCount, 0); + }); + }); + + it('swallows an error thrown by kill()', function() { + // One uncooperative worker must not stop the others from being killed. + killSpy = sinon.stub().throws(new Error('no such process')); + scheduler.spawnWorker(appSpec, null, true); + return completeLaunch().then(function() { + assert.doesNotThrow(function() { scheduler.shutdown(); }); + assert.strictEqual(killSpy.callCount, 1); + }); + }); + + it('does nothing when there are no workers', function() { + assert.doesNotThrow(function() { scheduler.shutdown(); }); + assert.strictEqual(killSpy.callCount, 0); + }); + }); + + describe('dump()', function() { + var logged; + var origLog; + + beforeEach(function() { + logged = []; + origLog = console.log; + console.log = function() { + logged.push(Array.prototype.join.call(arguments, ' ')); + }; + }); + + afterEach(function() { + console.log = origLog; + }); + + it('summarizes a launched worker', function() { + scheduler.spawnWorker(appSpec, null, true); + return completeLaunch().then(function() { + scheduler.dump(); + assert.strictEqual(logged.length, 1); + // summarizeWorker() pulls appSpec, endpoint and logFilePath off the + // resolved AppWorkerHandle. + assert.ok(/Port 1234/.test(logged[0]), + 'expected the endpoint in the dump, got: ' + logged[0]); + }); + }); + + it('reports an unresolved worker rather than throwing', function() { + scheduler.spawnWorker(appSpec, null, true); + return Q.delay(null, 20).then(function() { + scheduler.dump(); + assert.deepStrictEqual(logged, ['[unresolved promise]']); + }); + }); + + it('does nothing when there are no workers', function() { + scheduler.dump(); + assert.deepStrictEqual(logged, []); + }); + }); +}); From 6173a17775a346f8bf64570dab3cf43c59e00007 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Sat, 29 Aug 2026 13:33:39 -0700 Subject: [PATCH 3/6] Add a real-R test tier and GitHub Actions CI test/integration-r/ launches actual R processes through the real AppWorker -- the stdin handshake, the port handshake, per-worker logging, teardown. It is not part of `npm test`; run it with `npm run test:r`. It skips itself loudly when R or the shiny package is missing, so a developer without R still gets a useful signal from the rest of the suite; CI treats a skip as a provisioning failure so it can't pass vacuously. These run as the current user: AppWorker only shells out to `su` when appSpec.runAs differs from the process user, so a `run_as $USER` config exercises the real launcher without needing root. They also need an explicit bookmark_state_dir -- otherwise the worker tries to mkdir /var/lib/shiny-server/bookmarks and fails before R is ever started. .github/workflows/ci.yml runs npm ci, the tests and the license check on Linux and macOS, plus a **build-freshness check**: `tsc` followed by `git diff --exit-code lib/`. Generated .js is committed next to the .ts and the tests run against the generated files, so editing a .ts without running `npm run build` currently passes CI while changing nothing at runtime. Node is pinned from .nvmrc: master's nan (^2.18.0) does not compile against Node 24, so anything else fails in node-gyp rather than anywhere informative. The license check runs check-licenses.js and check-upstream.sh directly rather than tools/preflight.sh, which invokes ./bin/node -- a runtime that only exists after the full CMake build. docker/jenkins/Dockerfile.ubuntu-20.04 installs the shiny package; it had r-base only, so the real-R tier would have skipped itself there. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 108 +++++++++++++++ docker/jenkins/Dockerfile.ubuntu-20.04 | 7 + test/integration-r/real-app.js | 179 +++++++++++++++++++++++++ 3 files changed, 294 insertions(+) create mode 100644 .github/workflows/ci.yml create mode 100644 test/integration-r/real-app.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..414b131b --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,108 @@ +name: CI + +on: + push: + branches: [master] + pull_request: + workflow_dispatch: + +jobs: + test: + name: test (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest] + + steps: + - uses: actions/checkout@v4 + + # .nvmrc is the version the native addon (build/Release/posix.node) is + # expected to compile against; using anything else here tends to fail in + # node-gyp rather than anywhere informative. + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: npm + + # Also rebuilds src/posix.cc via node-gyp. + - name: Install dependencies + run: npm ci + + # The compiled .js next to each .ts is committed, and the tests run + # against the compiled output -- so editing a .ts without running + # `npm run build` currently passes CI while changing nothing at runtime. + # This makes that a failure. + - name: Check the committed build output is current + run: | + npm run build + git diff --exit-code -- lib/ \ + || (echo "::error::Compiled output in lib/ is stale. Run 'npm run build' and commit the result." && exit 1) + + # Unit tests plus the fast integration tier (test/integration), which + # boots a real server on an ephemeral port against a stand-in worker. + # mocha is not recursive, so both directories are named in the "test" + # script; keep that in sync with Jenkinsfile. + - name: Run tests + run: npm test + + licenses: + name: license check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # tools/check-upstream.sh fetches and compares refs. + fetch-depth: 0 + + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: npm + + - run: npm ci + + # tools/preflight.sh invokes ./bin/node, which only exists after the full + # CMake build vendors a Node runtime. Run its two steps directly instead. + - name: Check dependency licenses + run: node tools/check-licenses.js + + - name: Check for unmerged upstream changes + run: tools/check-upstream.sh + + test-r: + name: integration with real R + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: npm + + - uses: r-lib/actions/setup-r@v2 + with: + # Serves binary packages, so installing shiny takes seconds rather + # than the many minutes a source build would. + use-public-rspm: true + + - name: Install the shiny R package + run: install.packages("shiny") + shell: Rscript {0} + + - run: npm ci + + # Not part of `npm test`: this tier launches real R processes, so it is + # slower and needs R provisioned, as above. It skips itself rather than + # failing when shiny is missing -- convenient locally, useless here, so + # treat a skip as a provisioning failure. + - name: Run the real-R integration tier + run: | + set -o pipefail + npm run test:r 2>&1 | tee /tmp/r-tier.log + if grep -q "SKIPPING the real-R tier" /tmp/r-tier.log; then + echo "::error::The real-R tier skipped itself; R or the shiny package is missing." + exit 1 + fi diff --git a/docker/jenkins/Dockerfile.ubuntu-20.04 b/docker/jenkins/Dockerfile.ubuntu-20.04 index f625587f..21b744b4 100644 --- a/docker/jenkins/Dockerfile.ubuntu-20.04 +++ b/docker/jenkins/Dockerfile.ubuntu-20.04 @@ -39,6 +39,13 @@ RUN add-apt-repository "deb https://cloud.r-project.org/bin/linux/ubuntu $(lsb_r RUN apt-get update && apt-get install -y cmake r-base RUN cmake --version +# The real-R integration tier (test/integration-r, run via `npm run test:r`) +# launches actual Shiny apps, so the image needs the shiny package, not just +# r-base. Without it that tier skips itself and the build looks green while +# testing nothing. +RUN Rscript -e 'install.packages("shiny", repos = "https://packagemanager.posit.co/cran/__linux__/focal/latest")' +RUN Rscript -e 'stopifnot(requireNamespace("shiny", quietly = TRUE))' + ARG JENKINS_GID=999 ARG JENKINS_UID=999 RUN groupadd -g $JENKINS_GID jenkins && \ diff --git a/test/integration-r/real-app.js b/test/integration-r/real-app.js new file mode 100644 index 00000000..ac497e29 --- /dev/null +++ b/test/integration-r/real-app.js @@ -0,0 +1,179 @@ +/* + * test/integration-r/real-app.js + * + * Copyright (C) 2026 by Posit Software, PBC + * + * This program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +// The slow tier: a real R process, launched by the real AppWorker, serving a +// real Shiny app. Everything test/integration/ stubs out is live here -- the +// stdin handshake, the port handshake, per-worker logging, teardown. +// +// This is NOT part of `npm test`; run it with `npm run test:r`. It needs R with +// the `shiny` package installed, and it skips itself (loudly) if that is +// missing rather than failing, so that a developer without R still gets a +// useful signal from the rest of the suite. +// +// Note that these run as the current user: AppWorker only shells out to `su` +// when appSpec.runAs differs from the process user (app-worker.ts:325), so a +// `run_as $USER` config exercises the real launcher without needing root. + +var assert = require('assert'); +var child_process = require('child_process'); +var fs = require('fs'); +var path = require('path'); +var testServer = require('../support/server'); +var testConfig = require('../support/config'); + +var HELLO_APP = path.join(testConfig.projectRoot, 'test', 'apps', '01_hello'); + +/** + * True if `Rscript` exists and can load shiny. + */ +function hasShiny() { + try { + var result = child_process.spawnSync('Rscript', [ + '-e', 'quit(status = if (requireNamespace("shiny", quietly = TRUE)) 0 else 1)' + ], {timeout: 60000}); + return result.status === 0; + } catch (err) { + return false; + } +} + +describe('a real R Shiny app', function() { + // R startup plus package loading is slow, and slower still on a cold CI + // machine. app_init_timeout in the config below has to stay under this. + this.timeout(120000); + + var available = hasShiny(); + var server; + + before(function() { + if (!available) { + console.warn( + '\n SKIPPING the real-R tier: Rscript with the "shiny" package was ' + + 'not found.\n Install R and `install.packages("shiny")` to run it.\n'); + this.skip(); + } + + return testServer.start_p(testConfig.siteDirConfig({ + siteDir: '$DIR/site', + // Without an explicit bookmark_state_dir the worker tries to mkdir + // /var/lib/shiny-server/bookmarks and fails before R is ever started. + preamble: 'bookmark_state_dir $DIR/bookmarks;', + locationBody: ' app_init_timeout 90;\n app_idle_timeout 30;' + }), { + // The whole point: leave the real launcher in place. + worker: false, + files: { + 'site/hello/ui.R': fs.readFileSync(path.join(HELLO_APP, 'ui.R')), + 'site/hello/server.R': fs.readFileSync(path.join(HELLO_APP, 'server.R')), + 'bookmarks/.keep': '' + } + }) + .then(function(s) { server = s; }); + }); + + after(function() { + return server ? server.stop_p() : null; + }); + + it('starts R and serves the app page', function() { + return server.get_p('/hello/', {timeout: 110000}).then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(/; the + // asset middleware strips everything up to and including __assets__, so it + // resolves to the same file as the top-level URL would. + return server.get_p('/hello/__assets__/shiny-server.css').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(r.body.length > 0); + }); + }); + + it('injects the shiny-server client into the app page', function() { + return server.get_p('/hello/').then(function(r) { + assert.ok(/__assets__\/shiny-server\.css/.test(r.body), + 'expected the injected stylesheet link, got: ' + r.body.slice(0, 800)); + }); + }); + + it('reuses the same R process for a second request', function() { + return server.get_p('/hello/').then(function(r) { + assert.strictEqual(r.status, 200); + var entries = server.workerEntries(); + assert.strictEqual(entries.length, 1, + 'a second request should not have spawned a second R process'); + }); + }); + + it('proxies R\'s own static assets, not just the app page', function() { + // /shared/ is served by Shiny itself from inside the R process, so a + // sizeable, correctly-typed body here means the proxy is round-tripping + // real traffic to a live R process and not just serving the first page. + return server.get_p('/hello/shared/shiny.js').then(function(r) { + assert.strictEqual(r.status, 200); + assert.ok(/javascript/.test(r.headers.get('content-type')), + 'unexpected content-type: ' + r.headers.get('content-type')); + assert.ok(r.body.length > 10000, + 'expected the real shiny.js bundle, got ' + r.body.length + ' bytes'); + }); + }); + + it('404s a path the R process does not recognize', function() { + // The 404 comes from Shiny, not from Shiny Server, which is the point: + // the request reached R and R answered. + return server.get_p('/hello/session/nonexistent').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); + + it('captures the R process\'s stderr into a per-worker log file', function() { + // Asserting only that a hello-*.log exists proves nothing: the file is + // created before the worker is launched, so it is there even when R fails + // to start at all (which is exactly what happened before + // bookmark_state_dir was set). Assert the content, and that the port in + // the filename matches the port R reports listening on -- that is what + // makes it a *per-worker* log. + var logDir = path.join(server.config.dir, 'logs'); + var files = fs.readdirSync(logDir).filter(function(f) { + return /^hello-.*\.log$/.test(f); + }); + assert.strictEqual(files.length, 1, + 'expected exactly one worker log in ' + logDir + ', found: ' + files.join(', ')); + + var port = files[0].match(/-(\d+)\.log$/); + assert.ok(port, 'log filename should end in the endpoint port: ' + files[0]); + + var contents = fs.readFileSync(path.join(logDir, files[0]), 'utf8'); + assert.ok(contents.indexOf('Listening on http://127.0.0.1:' + port[1]) >= 0, + 'expected R to report listening on port ' + port[1] + '; log was: ' + + JSON.stringify(contents)); + }); + + it('still routes normally for URLs that are not the live app', function() { + // A control: this 404 comes from Shiny Server's router, not from R -- there + // is no /nosuchapp directory. It is here to show that hosting a live R + // process doesn't disturb ordinary routing. (It is also the one assertion + // in this file that would still pass if R never started, along with the + // __assets__ one above; everything else fails loudly.) + return server.get_p('/nosuchapp/').then(function(r) { + assert.strictEqual(r.status, 404); + }); + }); +}); From 27d9d1b75abcf5e67796d1a988f1255002d55404 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Sat, 29 Aug 2026 13:33:39 -0700 Subject: [PATCH 4/6] Update the testing guide for the three-tier suite Its runner-setup and coverage sections went stale the moment test/integration/ existed. Records the tier layout and the fact that mocha is not recursive (so package.json and Jenkinsfile must list directories in step), how the harness works and why its seam is a module-export swap rather than rewire, the two flakiness traps, and an honest recount of the coverage map -- including that the SockJS and WebSocket paths are now the highest-value remaining gap. Co-Authored-By: Claude Opus 5 (1M context) --- memory-bank/testingGuide.md | 136 ++++++++++++++++++++++++++++-------- 1 file changed, 107 insertions(+), 29 deletions(-) diff --git a/memory-bank/testingGuide.md b/memory-bank/testingGuide.md index 7ddd7d0f..c15fb196 100644 --- a/memory-bank/testingGuide.md +++ b/memory-bank/testingGuide.md @@ -1,21 +1,38 @@ --- title: Testing Guide -description: How Shiny Server is tested — the Mocha/Should.js/Sinon/Rewire automated suite in test/, what each test file covers and the large gaps it leaves, the bit-rotted manual.test/ scripts and load tests, the config and app fixtures plus tools/test-config.sh, Q-promise async conventions, and the traps (wrong Node ABI, no-op `.should.be.true`, root-only paths, fake timers vs. nextTick) to avoid when adding tests. +description: How Shiny Server is tested — the three tiers (unit tests in test/, the in-process integration harness in test/integration/ built on test/support/, and the real-R tier in test/integration-r/), the Mocha/Should.js/Sinon/Rewire conventions, what each file covers and the gaps that remain, the bit-rotted manual.test/ scripts, fixtures and tools/test-config.sh, Q-promise async conventions, and the traps (wrong Node ABI, ephemeral-port shadowing, socket pooling across recycled ports, no-op `.should.be.true`, root-only paths, fake timers vs. nextTick) to avoid when adding tests. --- # Testing Guide ## Runner setup -`npm test` runs `mocha test` (`package.json`, `"test": "mocha test"`). There is no -watch mode, no coverage tooling, and no linting step. CI does the same thing but -with the vendored interpreter: `Jenkinsfile:108` and `Jenkinsfile.internal:138` both -run `./bin/node ./node_modules/mocha/bin/mocha test`. +There are three tiers, and **mocha is not recursive**, so every test directory has to +be named explicitly wherever tests are invoked: -`.mocharc.json` is three lines and auto-requires three modules before any test file: +| Command | What it runs | Needs | +|---|---|---| +| `npm test` | `mocha test test/integration` — unit tests **and** the fast integration tier | nothing beyond `npm ci` | +| `npm run test:unit` | `mocha test` only | nothing | +| `npm run test:integration` | `mocha test/integration` only | nothing | +| `npm run test:r` | `mocha test/integration-r` — real R processes | R with the `shiny` package | + +`test/support/` holds the harness and is deliberately *not* a test directory, so mocha +never loads it directly. + +There is no watch mode, no coverage tooling, and no linting step. Two CI systems run +this: `.github/workflows/ci.yml` (Linux + macOS, plus a build-freshness check and the +real-R tier) and Jenkins, which uses the vendored interpreter — +`Jenkinsfile:108` runs `./bin/node ./node_modules/mocha/bin/mocha test test/integration`. +**Keep that list of directories in sync with `package.json`'s `test` script**; a new +test directory that isn't added in both places silently doesn't run. + +`.mocharc.json` auto-requires three modules before any test file, and sets a timeout +that a server boot can survive (mocha's 2s default cannot): ```json -{ "require": ["should", "./lib/core/log", "./lib/core/qutil"], "reporter": "spec" } +{ "require": ["should", "./lib/core/log", "./lib/core/qutil"], + "reporter": "spec", "timeout": 20000 } ``` Why each one is there: @@ -37,22 +54,80 @@ lines into the spec output; `SHINY_LOG_LEVEL=OFF npm test` silences that noise. ## Current real state of `npm test` -**63 passing, 0 failing, 0 pending, ~200ms** — measured on the PR #596 branch, not -on `master`. #596 fixes a macOS-only failure in `test/app-worker.js` (the `/blah` -mkdir case returns `EROFS` rather than `EACCES` on darwin), so a `master` checkout on -a Mac is expected to show one failure. Re-measure before trusting this number. Clean -otherwise, but with one caveat and one source of visual noise: +**291 passing, 0 failing, ~4s** on `master` (macOS, Node v20.17.0). `npm run test:r` +adds 8 more and takes about a second once R is warm. + +The macOS-only `test/app-worker.js` failure (the `/blah` mkdir case returns `EROFS` +rather than `EACCES` on darwin) is fixed — the assertion now accepts either errno. -- **Node ABI trap (this bit me first try).** `test/app-worker.js:19` requires +- **Node ABI trap (the first thing that bites).** `test/app-worker.js:19` requires `../build/Release/posix.node` directly, and `lib/core/fsutil.js` requires it transitively. If your shell's `node` is not ABI-compatible with whatever built `build/Release/`, mocha dies before running a single test with - `ERR_DLOPEN_FAILED ... NODE_MODULE_VERSION`. Run tests with the vendored interpreter - (`./bin/node ./node_modules/mocha/bin/mocha test`, currently Node v20.17.0, matching - `.nvmrc`), or `npm rebuild` against the Node you're using. + `ERR_DLOPEN_FAILED ... NODE_MODULE_VERSION`. Run tests with a Node matching + `.nvmrc` (currently v20.17.0), or `npm rebuild` against the Node you're using. + Note that `master`'s `nan` (^2.18.0) **does not compile against Node 24** — that + bump lives on the #596 branch — so `npm ci` under Node 24 fails in node-gyp. - The `app-worker` block prints four log4js lines mid-spec about bookmark state directories under `$TMPDIR/app-worker-test-bookmarks`. Expected, not a failure. +## The integration harness (`test/support/`) + +`test/integration/` boots a **real, complete server in-process** on an ephemeral port, +for each test, in about 15ms. Three modules make that possible: + +- **`test/support/server.js`** — `start_p(configText, options)` resolves to a test + server with `.port`, `.baseUrl`, `.get_p(path, init)`, `.workerEntries()` and + `.stop_p()`. It is built on `lib/server-init.js`'s `createServer_p`. +- **`test/support/config.js`** — writes a throwaway config into a fresh temp dir, + substituting `$USER` (the process user, which `run_as` must name), `$ROOT` (the + checkout) and `$DIR` (that temp dir). `siteDirConfig(opts)` is the shape most tests + want. +- **`test/support/fake-worker.js`** — `install()` replaces the + `lib/worker/app-worker` module's `launchWorker_p` export with one that binds a real + `http.Server` on the endpoint's port. Everything else stays live: the real + `TcpTransport`, the real endpoint and shared secret, the real + `connectEndpoint_p` handshake, the real proxy. Only `su` and R are skipped. + +Pass `worker: false` to `start_p` to keep the real launcher and spawn actual R — that +is what `test/integration-r/` does. + +### Why the seam is a module-export swap, not rewire + +`lib/scheduler/scheduler.js:28` declares `let app_worker` specifically so rewire can +reach it, and `test/scheduler.js` uses that. The integration harness **cannot**: rewire +loads a second copy of the module, and the server built by `lib/server-init.js` would +still be using the first. What makes the plain assignment work is that +`scheduler.js:176` resolves `app_worker.launchWorker_p` as a property *at call time*. + +Note also that `Scheduler.setTransport()` alone is not a sufficient seam: +`scheduler.js:171` calls `posix.getpwnam(appSpec.runAs)` and `:175` calls +`launchWorker_p` regardless of transport. That is why test configs must +`run_as $USER` — so the real `getpwnam` succeeds. + +### Two traps that produced days of "impossible" flakiness + +Both are recorded here because the symptoms point nowhere near the cause. + +- **Ephemeral-port shadowing.** `TcpTransport.alloc_p` allocates a worker port by + binding `127.0.0.1:0`, reading the port, and *closing again* (`lib/transport/tcp.js`). + If a server listening on the wildcard `::` is started in that window, the kernel can + hand it the very port that was just released; the stand-in worker then binds + `127.0.0.1:`, **which succeeds** — a specific-address bind is permitted + alongside a wildcard one — and from then on shadows the server for all loopback + traffic. Requests silently reach the worker instead of Shiny Server, showing up as + inexplicable 404s and `Parse Error: Expected HTTP/`. The harness avoids it by + pinning the listener to `127.0.0.1` (`listen 0 127.0.0.1`), putting it in the same + address space as the worker ports so the allocator won't double-assign. +- **Socket pooling across recycled ports.** `fetch()` pools keep-alive sockets per + origin. Test servers are torn down and restarted milliseconds apart and ephemeral + ports get recycled, so a pooled socket belonging to a dead server gets handed to the + next test — which then talks to the *previous* test's config. `Connection: close` is + not a fix: fetch treats `Connection` as a forbidden header and drops it silently. + The harness therefore uses `http.request` with `agent: false`, one connection per + request. It also destroys established connections at teardown, because + `Server#destroy()` deliberately does not (see `requestLifecycle.md`). + ## Testing patterns in use ### Should.js assertions @@ -153,23 +228,26 @@ message with a regex (`test/nested-locations.js:27-43`); the newer style uses | `lib/router/config-router-util.js` | **Narrow.** Only `parseApplication`. | | `lib/router/config-router.js` | **Narrow.** Only `createRouter_p` against 3 fixtures, with permission checks rewired out. | | `lib/router/squash-run-as-router.js` | **Complete** (it's tiny). | -| `lib/proxy/http.js` | **Almost none.** `test/proxy-events.js` only greps `node_modules/http-proxy` for `.emit(` calls and diffs them against `knownEvents` (`lib/proxy/http.js:61`) — a canary for upstream event churn, not a behavior test. | +| `lib/proxy/http.js` | **Moderate.** `test/proxy-http.js` pins `httpListener`'s dispatch contract against a doubled router/registry: the strict `appSpec === true` check, the 404/500/503 paths, and the acquire/release accounting. `test/integration/proxy.js` covers the same ground against a live server. `test/proxy-events.js` separately greps `node_modules/http-proxy` for `.emit(` calls and diffs them against `knownEvents` (`lib/proxy/http.js:61`) — a canary for upstream event churn, not a behavior test. | +| `lib/config/lexer.js`, `parser.js`, `config.js`, `schema.js` | **Good.** `test/config-lexer.js` (33), `test/config-parser.js` (32, incl. `ConfigNode` inheritance and `search` ordering), `test/config-schema.js` (46, incl. the real `shiny-server-rules.config`). Ported and expanded from the `manual.test/` scripts. | +| `lib/core/qutil.js` | **Good.** `test/qutil.js` — `forEachPromise_p`, `map_p` sequencing, `serialized`, `wrap`, `.eat()`. | +| `lib/server-init.js`, the Express stack | **Moderate.** `test/integration/` — `__assets__` rewriting, static/`send` behavior, the proxy path, the access log, `X-Powered-By`. | +| `lib/server/server.js` | **Narrow.** Exercised by every integration test's startup and teardown; the `$close` leak has a direct regression test in `test/integration/harness.js`. | | Third-party behavior guards | `test/http-proxy.js` (http-proxy must send `Connection: close` upstream), `test/config-router-util.js:166-184` (`fs.fchmod` must accept a string mode). Both exist because a silent upstream change would break production. | **Zero automated coverage**, roughly in descending order of how much a test would be worth: -- `lib/config/lexer.js`, `parser.js`, `config.js`, `schema.js` — the entire hand-written - config language. Only the (stale) `manual.test/` scripts touch it. **This is the - highest-value gap**: the manual scripts already contain usable assertions that could - be ported to mocha in an afternoon. -- `lib/proxy/sockjs.js`, `lib/proxy/multiplex.js`, `lib/proxy/errorcode.js`, and all of - `ShinyProxy`'s actual request handling. +- `lib/proxy/sockjs.js`, `lib/proxy/multiplex.js`, `lib/proxy/errorcode.js` — **now the + highest-value gap.** The SockJS and WebSocket paths are the only major traffic route + with no coverage at either tier. `test/support/fake-worker.js` already accepts an + `onUpgrade` handler, so the harness is ready for it. - `lib/router/directory-router.js`, `local-config-router.js`, `user-dirs-router.js`, and the combinators in `router.js` (`CompositeRouter`, `PrefixFilterRouter`, `RestartRouter`, - `RedirectRouter`) — pure-ish functions that would be easy to test. + `RedirectRouter`) — covered end-to-end by `test/integration/`, but not unit-tested; + they are pure-ish functions that would be easy to test directly. - `lib/transport/tcp.js`, `unix-socket.js`; `lib/worker/app-worker-handle.js`, `run-as.js`. -- `lib/main.js`, `lib/server/server.js`, `lib/core/permissions.js`, `fsutil.js`, +- `lib/main.js` (the CLI wrapper), `lib/core/permissions.js`, `fsutil.js`, `connect-util.js`, `url-util.js`, `python.ts`, `shutdown.js`. - `src/launcher.cc`, `src/posix.cc` — the native code is exercised only incidentally. @@ -199,10 +277,10 @@ have bit-rotted. | Script | Purpose | State (verified) | |---|---|---| -| `test-config-lexer.js` | Top-level `assert()`s over the config lexer's character classification and tokenization. | **Runs clean.** The best porting candidate. | -| `test-config-parser.js` | Parses two snippets and `console.log`s the AST for eyeballing. Its "assertions" at lines 9-11 are bare expressions that assert nothing. | Runs; output is for humans. | -| `test-config-config.js` | Config + schema validation, incl. good/bad fixtures under `manual.test/config/`. | **Fails.** Asserts `bad2.config` (`run_as;`) is rejected for "too few arguments", but the schema now declares `param String users...` (`config/shiny-server-rules.config:6`), so zero args is legal. Stale expectation, not a product bug. | -| `test-serialized.js` | Demonstrates `qutil.serialized()` by interleaving sleeps; verify by reading the printed ordering. Takes ~10s. | Runs clean. | +| `test-config-lexer.js` | Top-level `assert()`s over the config lexer's character classification and tokenization. | **Ported** to `test/config-lexer.js`. Kept for reference only. | +| `test-config-parser.js` | Parses two snippets and `console.log`s the AST for eyeballing. Its "assertions" at lines 9-11 are bare expressions that assert nothing. | **Superseded** by `test/config-parser.js`. | +| `test-config-config.js` | Config + schema validation, incl. good/bad fixtures under `manual.test/config/`. | **Fails**, and it is the script that is wrong: it asserts `bad2.config` (`run_as;`) is rejected for "too few arguments", but the schema declares `param String users...` (`config/shiny-server-rules.config:6`), so zero args is legal. `test/config-schema.js` pins the correct behaviour ("lets a vararg match zero arguments"). **Superseded.** | +| `test-serialized.js` | Demonstrates `qutil.serialized()` by interleaving sleeps; verify by reading the printed ordering. Takes ~10s. | **Superseded** by `test/qutil.js`, which also pins the queued-caller defect below. | | `test-proxy.js` | Stands up a `ShinyProxy` on :8001. | **Dead.** Requires `lib/worker/worker-registry` and `router.AutouserRouter`, neither of which exists anymore. | | `test-worker-registry.js`, `test-worker-registry-leak.js` | Worker registry smoke test / memory-leak logger. | **Dead** — same missing `worker-registry` module; the leak script also wants `webkit-devtools-agent` and hardcodes `/Users/jcheng/...`. | | `loadtest.js` | Real load generator: N concurrent sessions, each fetching the static asset set plus a websocket session. Usage: `./bin/node manual.test/loadtest.js [session-count]` (default 200). Requires a **running** Shiny Server hosting `01_hello`. The websocket `init` message is hardcoded for that app; retarget by capturing a new init frame from Chrome devtools. `SHINY_SERVER=false` at the top switches it to a bare Shiny process. | Should work; needs a live server. | From 02512250b29534c0a8ac850d3b60745078201499 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Sat, 29 Aug 2026 13:41:38 -0700 Subject: [PATCH 5/6] Install shiny where a worker can actually see it, and make the guard honest The real-R job failed on CI while passing locally. app-worker.ts launches R with a deliberately scrubbed environment -- HOME, LANG and PATH only -- so the worker never sees R_LIBS_USER, which is where r-lib/actions installs by default. `Rscript` in the job could load shiny; the worker could not, and crash-looped one process per request (there is no spawn backoff), leaving five worker logs behind. Install into the site library instead, which R finds unconditionally, and verify it afterwards using `env -i` with the same three variables a worker gets. The Jenkins image already gets this right by accident: its Docker build runs as root, where .libPaths()[1] is the site library. Two changes so this cannot recur silently: - hasShiny() now probes with the worker's environment rather than an inherited one. Before, the skip guard could pass while every test failed with a 500 -- the worst of both worlds, since it neither ran nor announced itself. - The app-page assertion prints the whole response body on failure. It is a rendered 500 carrying the tail of the worker's console log, i.e. R's actual complaint; truncating it made this failure pure archaeology. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 21 ++++++++++++++++++--- test/integration-r/real-app.js | 23 ++++++++++++++++++++--- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 414b131b..3fef399e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,9 +88,24 @@ jobs: # than the many minutes a source build would. use-public-rspm: true - - name: Install the shiny R package - run: install.packages("shiny") - shell: Rscript {0} + # NOT into R_LIBS_USER, which is where setup-r would put it by default. + # lib/worker/app-worker.ts launches the R worker with a deliberately + # scrubbed environment -- HOME, LANG and PATH only -- so a worker never + # sees R_LIBS_USER and would fail to load shiny even though `Rscript` in + # this job can. Install into the site library, which R finds + # unconditionally. (The Jenkins image gets this for free: its Docker + # build runs as root, where .libPaths()[1] is already the site library.) + - name: Install the shiny R package where a worker can find it + env: + R_LIBS_USER: "" + run: | + SITE=$(Rscript -e 'cat(.Library.site[1])') + echo "Installing into $SITE" + sudo mkdir -p "$SITE" + sudo chmod a+w "$SITE" + Rscript -e 'install.packages("shiny", repos = if (nzchar(Sys.getenv("RSPM"))) Sys.getenv("RSPM") else "https://cloud.r-project.org")' + # Prove it resolves with the same bare environment a worker gets. + env -i HOME="$HOME" PATH="$PATH" Rscript -e 'stopifnot(requireNamespace("shiny", quietly = TRUE)); cat("worker-visible shiny:", as.character(packageVersion("shiny")), "\n")' - run: npm ci diff --git a/test/integration-r/real-app.js b/test/integration-r/real-app.js index ac497e29..1e6b86cc 100644 --- a/test/integration-r/real-app.js +++ b/test/integration-r/real-app.js @@ -34,13 +34,26 @@ var testConfig = require('../support/config'); var HELLO_APP = path.join(testConfig.projectRoot, 'test', 'apps', '01_hello'); /** - * True if `Rscript` exists and can load shiny. + * True if `Rscript` exists and can load shiny *as a worker would see it*. + * + * The environment matters. app-worker.ts launches R with only HOME, LANG and + * PATH, so a shiny installed somewhere only R_LIBS_USER points at is invisible + * to a worker. Checking with an inherited environment would let this skip guard + * pass while every test then failed with a 500 -- which is exactly what + * happened on CI, where r-lib/actions installs into R_LIBS_USER by default. */ function hasShiny() { try { var result = child_process.spawnSync('Rscript', [ '-e', 'quit(status = if (requireNamespace("shiny", quietly = TRUE)) 0 else 1)' - ], {timeout: 60000}); + ], { + timeout: 60000, + env: { + HOME: process.env.HOME, + LANG: process.env.LANG, + PATH: process.env.PATH + } + }); return result.status === 0; } catch (err) { return false; @@ -87,7 +100,11 @@ describe('a real R Shiny app', function() { it('starts R and serves the app page', function() { return server.get_p('/hello/', {timeout: 110000}).then(function(r) { - assert.strictEqual(r.status, 200); + // On failure this is a rendered 500 whose body carries the tail of the + // worker's console log -- i.e. R's actual complaint. Surface all of it, + // because without it a CI failure here is pure archaeology. + assert.strictEqual(r.status, 200, + 'expected 200, got ' + r.status + '. Response body:\n' + r.body); assert.ok(/ Date: Sat, 29 Aug 2026 13:43:12 -0700 Subject: [PATCH 6/6] Create R_HOME/site-library before installing shiny into it .Library.site is NA on setup-r's R -- it ships without a site library -- so the previous attempt resolved to the read-only base library and the install failed outright. R searches R_HOME/site-library unconditionally once it exists, and needs no environment variable to do so, which is what a worker's scrubbed environment requires. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3fef399e..ecaf425b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,12 +99,18 @@ jobs: env: R_LIBS_USER: "" run: | - SITE=$(Rscript -e 'cat(.Library.site[1])') - echo "Installing into $SITE" - sudo mkdir -p "$SITE" - sudo chmod a+w "$SITE" - Rscript -e 'install.packages("shiny", repos = if (nzchar(Sys.getenv("RSPM"))) Sys.getenv("RSPM") else "https://cloud.r-project.org")' + # R searches R_HOME/site-library unconditionally, with no environment + # variable needed -- but setup-r's R ships without one, so + # .Library.site is NA and a plain install.packages() falls back to the + # read-only base library. Create it. + export SITE_LIB="$(Rscript -e 'cat(R.home())')/site-library" + echo "Installing into $SITE_LIB" + sudo mkdir -p "$SITE_LIB" + sudo chmod a+w "$SITE_LIB" + Rscript -e 'install.packages("shiny", lib = Sys.getenv("SITE_LIB"), repos = if (nzchar(Sys.getenv("RSPM"))) Sys.getenv("RSPM") else "https://cloud.r-project.org")' # Prove it resolves with the same bare environment a worker gets. + # If this fails the job stops here, rather than after six confusing + # 500s from a crash-looping worker. env -i HOME="$HOME" PATH="$PATH" Rscript -e 'stopifnot(requireNamespace("shiny", quietly = TRUE)); cat("worker-visible shiny:", as.character(packageVersion("shiny")), "\n")' - run: npm ci