diff --git a/CLAUDE.md b/CLAUDE.md index c0cf6cb..db389ba 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,29 @@ To add a new template variable: - Do not use `dependency_overrides` or dev package versions unless there is truly no stable alternative (ngdart 8.x dev is the documented exception). +## Before handing work over + +Mandatory, every time — not "when the change looks risky". Run all four from +`unpub/` and report what they said: + +``` +fvm dart pub get +fvm dart analyze +fvm dart format --output=none --set-exit-if-changed . +fvm dart test +``` + +These are exactly what CI runs (`.github/workflows/analyze-and-test.yml`). A +change is not done until they are green locally; reporting it done on anything +less is how a red CI gets handed over. + +`dart test` needs MongoDB on `localhost:27017`. CI runs the suite against +**both** versions of its matrix, and they disagree: MongoDB removed the legacy +OP_QUERY opcode in 5.1, so a mongo_dart call implemented over it passes on +5.0.6 (what the deployment runs) and is refused on 7 (what it would be upgraded +to). `.dev/docker-compose.yml` starts 5.0.6 only, so testing against it alone +is not testing the matrix — run the suite a second time against `mongo:7`. + ## Changelog and commit messages Keep both compact. Length is not evidence of care. diff --git a/unpub/CHANGELOG.md b/unpub/CHANGELOG.md index ceb8311..8c536ed 100644 --- a/unpub/CHANGELOG.md +++ b/unpub/CHANGELOG.md @@ -1,3 +1,8 @@ +## Unreleased + +### Added +- `GET /health` for monitoring: 200 while the database answers, 503 when it does not. Public, no credential needed. + ## 3.7.0 ### Added diff --git a/unpub/README.md b/unpub/README.md index c28438a..ab0a602 100644 --- a/unpub/README.md +++ b/unpub/README.md @@ -354,6 +354,36 @@ From the command line docs are enabled by default; use `--no-docs` to disable them and `--dart-executable` to point at a specific Dart SDK. +### Health check + +`GET /health` is the endpoint to point a monitor at. It answers 200 while the +server can reach its metadata store and 503 when it cannot, so a check that +reads nothing but the status code is already right. The answer is never cached. + +```json +{ "status": "ok", "checks": { "database": { "status": "ok", "latencyMs": 4 } } } +``` + +The check is a real query, not a look at the driver's connection state — a +database that has stopped answering keeps a socket that still calls itself +open. A store that does not answer within 5 seconds counts as unreachable, and +the failing check names the error by type; the full error, connection string +and all, goes to the server log. + +```json +{ + "status": "error", + "checks": { "database": { "status": "error", "error": "TimeoutException" } } +} +``` + +The route is public and stays that way with `--auth` on, so that a monitor +needs no credential. It is the same answer for every caller: nothing in it +describes what this repository holds. A package count would have been a number +anybody could poll every few seconds, and a count read on a schedule is a +publication feed for a repository whose whole point is that outsiders cannot +watch one. + ### Usage behind reverse-proxy Using in_pub behind reverse proxy(nginx or another), ensure you have necessary headers diff --git a/unpub/lib/src/app.dart b/unpub/lib/src/app.dart index f0ea155..0ec2311 100644 --- a/unpub/lib/src/app.dart +++ b/unpub/lib/src/app.dart @@ -86,6 +86,15 @@ class App { final String version; + /// How long `/health` waits for the metadata store before calling it + /// unreachable. + /// + /// Bounded because the answer is for a monitor: a database that has stopped + /// answering must end as a failed check rather than as a probe left hanging + /// until the monitor's own timeout, which reads as a network problem + /// instead of a database one. + final Duration healthProbeTimeout; + /// validate if the package can be published /// /// for more details, see: https://github.com/Innim/in_pub#package-validator @@ -104,6 +113,7 @@ class App { this.uploadValidator, this.proxy_origin, this.version = '', + this.healthProbeTimeout = const Duration(seconds: 5), HostedUrlCompat? hostedUrlCompat, }) : // Off unless one is passed. The rewrite makes this server answer @@ -499,6 +509,61 @@ class App { /// the whole table on every request. late final Router router = _$AppRouter(this); + /// What a monitor polls: is this server up, and can it still reach the + /// store it keeps every package's metadata in? + /// + /// Answers 200 when the store answered and 503 when it did not, so a check + /// that reads nothing but the status code is already right. + /// + /// The same answer for everybody, which is what makes it safe to leave + /// public — `classifyRoute` has reserved this path since before there was + /// a handler on it, and the gate resolves no credential for a public + /// route, so this handler could not tell a signed-in caller from anyone + /// else even if it wanted to. Nothing here describes what the repository + /// holds: a package count would have been a number an anonymous caller + /// could poll every few seconds, and a count polled on a schedule is a + /// publication feed for a repository whose whole point is that outsiders + /// cannot watch one. + @Route.get('/health') + Future health(shelf.Request req) async { + final watch = Stopwatch()..start(); + Object? failure; + try { + await metaStore.checkHealth().timeout(healthProbeTimeout); + } catch (e) { + // Logged in full here and named only by type in the answer: the + // message of a driver error carries the connection string, and this + // endpoint is read by anyone who asks. + print('Health check failed: $e'); + failure = e; + } + watch.stop(); + + final healthy = failure == null; + + return shelf.Response( + healthy ? HttpStatus.ok : HttpStatus.serviceUnavailable, + headers: { + HttpHeaders.contentTypeHeader: jsonContentType, + // An answer served out of a proxy's cache says only that the server + // was up when the entry was stored. + HttpHeaders.cacheControlHeader: 'no-store', + }, + body: json.encode({ + 'status': healthy ? 'ok' : 'error', + 'checks': { + 'database': { + 'status': healthy ? 'ok' : 'error', + if (healthy) + 'latencyMs': watch.elapsedMilliseconds + else + 'error': failure.runtimeType.toString(), + }, + }, + }), + ); + } + @Route.get('/api/packages/') Future getVersions(shelf.Request req, String name) async { var package = await metaStore.queryPackage(name); diff --git a/unpub/lib/src/app.g.dart b/unpub/lib/src/app.g.dart index 820cc15..5118f82 100644 --- a/unpub/lib/src/app.g.dart +++ b/unpub/lib/src/app.g.dart @@ -8,6 +8,11 @@ part of 'app.dart'; Router _$AppRouter(App service) { final router = Router(); + router.add( + 'GET', + r'/health', + service.health, + ); router.add( 'GET', r'/api/packages/', diff --git a/unpub/lib/src/meta_store.dart b/unpub/lib/src/meta_store.dart index cc421ce..97b6713 100644 --- a/unpub/lib/src/meta_store.dart +++ b/unpub/lib/src/meta_store.dart @@ -33,4 +33,24 @@ abstract class MetaStore { String? uploader, String? dependency, }); + + /// Reaches the store, for `/health`. Throws when it cannot be reached — + /// the caller turns that into what it reports. + /// + /// A round trip on purpose, rather than a look at whatever the driver + /// calls its connection state: a socket that still describes itself as + /// open is exactly what a wedged database leaves behind, and only an + /// answer that came back over the wire says the store can still be used. + /// Nothing is returned, because the probe answers one question — did the + /// store answer — and `/health` reports nothing else about it. + /// + /// Concrete rather than abstract because `MetaStore` is a documented + /// extension point (README, "Customize meta and package store"): an + /// implementation outside this repository must not stop compiling for a + /// probe it never asked for. The default asks for a single package, which + /// is the cheapest round trip this interface can express; a store that can + /// do better — [MongoStore] asks for one document's `_id` — overrides it. + Future checkHealth() async { + await queryPackages(size: 1, page: 0, sort: 'download'); + } } diff --git a/unpub/lib/src/mongo_store.dart b/unpub/lib/src/mongo_store.dart index 56424bf..f05e930 100644 --- a/unpub/lib/src/mongo_store.dart +++ b/unpub/lib/src/mongo_store.dart @@ -116,6 +116,22 @@ class MongoStore extends MetaStore { return _queryPackagesBySelector(selector); } + @override + Future checkHealth() async { + // One document's `_id` and nothing else: the smallest answer the server + // can be asked for that still proves the connection carries a query. + // + // `findOne` rather than `count`, which reads no document at all and would + // otherwise be the obvious probe: mongo_dart 0.7.4 implements `count` + // over the legacy OP_QUERY opcode, and MongoDB removed that in 5.1 — + // against a 5.1-or-newer server every call is refused with + // `Unsupported OP_QUERY command: count`, so the probe would have reported + // the database unreachable on exactly the versions this repository is + // headed for. `findOne` picks the modern OP_MSG path where the server has + // one and falls back on the legacy path where it does not. + await db.collection(packageCollection).findOne(where.fields(['_id'])); + } + @override Future> queryRecentPublications({ required int size, diff --git a/unpub/test/health_test.dart b/unpub/test/health_test.dart new file mode 100644 index 0000000..f8b1ac5 --- /dev/null +++ b/unpub/test/health_test.dart @@ -0,0 +1,238 @@ +import 'dart:async'; +import 'dart:convert'; +import 'dart:io'; + +import 'package:http/http.dart' as http; +import 'package:in_pub/in_pub.dart'; +import 'package:test/test.dart'; + +import 'auth/fake_provider.dart'; +import 'auth/memory_auth_store.dart'; + +/// `/health` is what a monitor polls, and it is reachable without signing in. +/// +/// Both halves matter. A check that cannot tell a wedged database from a +/// working one is not worth running, and an endpoint that anybody can poll +/// on a schedule must not describe what the repository holds — that is a +/// feed, not a status. +void main() { + late HttpServer server; + HttpServer? started; + late _ProbeStore store; + + Future serve({bool auth = false}) async { + await started?.close(force: true); + server = started = await App( + metaStore: store, + packageStore: _UnusedPackageStore(), + healthProbeTimeout: const Duration(milliseconds: 200), + auth: auth + ? AuthService( + config: AuthConfig( + enabled: true, + issuer: 'https://id.example.org', + clientId: 'in-pub', + clientSecret: 'secret', + publicUrl: Uri.parse('http://127.0.0.1:4000'), + secret: + utf8.encode('a-test-signing-secret-of-sufficient-length!!'), + ), + store: MemoryAuthStore(), + provider: FakeIdentityProvider(), + googleAuth: false, + ) + : null, + ).serve('127.0.0.1', 0); + } + + setUp(() => store = _ProbeStore()); + tearDown(() => started?.close(force: true)); + + Future health() => + http.get(Uri.parse('http://127.0.0.1:${server.port}/health')); + + Map body(http.Response res) => + json.decode(res.body) as Map; + + Map database(http.Response res) => + (body(res)['checks'] as Map)['database'] + as Map; + + group('with authentication off', () { + setUp(() => serve()); + + test('a store that answers is a healthy server', () async { + var res = await health(); + + expect(res.statusCode, HttpStatus.ok); + expect(body(res)['status'], 'ok'); + expect(database(res)['status'], 'ok'); + expect(database(res)['latencyMs'], isA()); + expect(store.probes, 1); + }); + + test('answers with the status and nothing about the repository', () async { + // Every key at every level, rather than a list of fields that must be + // absent: what has to hold is that nothing describing the packages + // here can be read off a public endpoint, and a field added later + // should have to be argued for in this test rather than slip in + // somewhere the assertions do not reach. + var res = await health(); + + expect(body(res).keys, ['status', 'checks']); + expect((body(res)['checks'] as Map).keys, ['database']); + expect(database(res).keys, ['status', 'latencyMs']); + }); + + test('a failing check says no more than a passing one', () async { + // The path that has something to say: it carries the reason the store + // could not be reached, which is the one place a leak would be easy. + store.failure = const _StoreDown(); + + var res = await health(); + + expect(body(res).keys, ['status', 'checks']); + expect(database(res).keys, ['status', 'error']); + }); + + test('a store that throws is a 503, not a 500', () async { + // The distinction a monitor acts on: this server is running and its + // database is not, which is neither "up" nor an unhandled error. + store.failure = const _StoreDown(); + + var res = await health(); + + expect(res.statusCode, HttpStatus.serviceUnavailable); + expect(body(res)['status'], 'error'); + expect(database(res)['status'], 'error'); + expect(database(res).containsKey('latencyMs'), isFalse); + }); + + test('a store that hangs fails the check rather than the request', + () async { + // Left unbounded this is the failure that reads as a network problem: + // the probe never comes back and the monitor blames its own timeout. + store.hang = true; + + var res = await health(); + + expect(res.statusCode, HttpStatus.serviceUnavailable); + expect(database(res)['error'], 'TimeoutException'); + }); + + test('the failure is named by type, never by its message', () async { + // A driver error spells out the connection string it failed on, and + // this endpoint is read by anyone who asks. + store.failure = + const _StoreDown('mongodb://admin:hunter2@db.internal:27017'); + + var res = await health(); + + expect(database(res)['error'], '_StoreDown'); + expect(res.body, isNot(contains('hunter2'))); + expect(res.body, isNot(contains('db.internal'))); + }); + + test('is not served from a cache', () async { + var res = await health(); + + expect(res.headers[HttpHeaders.cacheControlHeader], 'no-store'); + }); + }); + + group('with authentication on', () { + setUp(() => serve(auth: true)); + + test('answers without a session', () async { + var res = await health(); + + expect(res.statusCode, HttpStatus.ok); + expect(body(res)['status'], 'ok'); + expect(database(res)['status'], 'ok'); + }); + + test('answers the same as it does with authentication off', () async { + // The gate resolves no credential for a public route, so this handler + // cannot tell who is asking; one answer for everybody is the only + // thing it can honestly give. + var res = await health(); + + expect(body(res).keys, ['status', 'checks']); + expect(database(res).keys, ['status', 'latencyMs']); + }); + + test('a store that is down is still a 503', () async { + store.failure = const _StoreDown(); + + var res = await health(); + + expect(res.statusCode, HttpStatus.serviceUnavailable); + expect(body(res)['status'], 'error'); + }); + }); +} + +/// A metadata store whose health probe is scripted. +class _ProbeStore extends MetaStore { + Object? failure; + bool hang = false; + int probes = 0; + + @override + Future checkHealth() async { + probes++; + if (hang) return Completer().future; + if (failure case var e?) throw e; + } + + Never _unused() => throw UnimplementedError('not used by these tests'); + + @override + Future queryPackage(String name) => _unused(); + @override + Future addVersion(String name, UnpubVersion version) => _unused(); + @override + Future addUploader(String name, String email) => _unused(); + @override + Future removeUploader(String name, String email) => _unused(); + @override + Future removeVersion(String name, String version) => _unused(); + @override + void increaseDownloads(String name, String version) => _unused(); + @override + Future> queryRecentPublications({ + required int size, + }) => + _unused(); + @override + Future queryPackages({ + required int size, + required int page, + required String sort, + String? keyword, + String? uploader, + String? dependency, + }) => + _unused(); +} + +class _StoreDown implements Exception { + final String detail; + + const _StoreDown([this.detail = 'connection closed']); + + @override + String toString() => 'Store down: $detail'; +} + +class _UnusedPackageStore extends PackageStore { + Never _unused() => throw UnimplementedError('not used by these tests'); + + @override + Stream> download(String name, String version) => _unused(); + @override + Future upload(String name, String version, List content) => + _unused(); + @override + Future delete(String name, String version) => _unused(); +} diff --git a/unpub/test/unpub_test.dart b/unpub/test/unpub_test.dart index 070b722..16034e8 100644 --- a/unpub/test/unpub_test.dart +++ b/unpub/test/unpub_test.dart @@ -560,4 +560,31 @@ main() { expect(recent.map((e) => e.version.version), ['1.9.1', '1.0.9']); }); }); + + // The probe `/health` runs, against a real database rather than a fake: + // what it has to prove is that the query reaches Mongo and comes back, + // which a stubbed store cannot say anything about. + group('health check', () { + late MongoStore store; + + setUpAll(() async { + await _cleanUpDb(); + store = MongoStore(_db); + }); + + test('an open connection answers the probe', () async { + await expectLater(store.checkHealth(), completes); + }); + + test('a closed connection fails it', () async { + // The state `/health` exists to report. It has to arrive as a thrown + // error rather than as a probe that quietly succeeds, which is what + // reading a connection flag would have given. + var db = Db('mongodb://localhost:27017/dart_pub_test'); + await db.open(); + await db.close(); + + await expectLater(MongoStore(db).checkHealth(), throwsA(anything)); + }); + }); }