From 9bacc2352388df389edeef211c5c16357a8af02d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 21:35:46 +0000 Subject: [PATCH 1/5] Serve the species lists from node Two more ContentService endpoints on the paths Java already uses, which is the point: to a caller there is one ContentService, and which process answers is not their concern. `main` is not a property. All 96 Species nodes carry the same six, and the 16 "main" ones are the ones a TopLevelPathway points at -- checked against the graph rather than inferred from 16 matching 16, because two numbers agreeing is not a rule. The two differ in **order** as well as contents, which is the part a port loses. `main` pins Homo sapiens first and sorts the rest by name; `all` is plain alphabetical with human in its place. Reactome pins human because it is the reference species everything else is inferred from, and a selector that buries it between Gallus and Mus asks every reader to hunt for the common case. Both orders read off the live responses, not assumed. Written as two rows of one generator rather than one handler taking a flag, because a shared sort is how one of them silently acquires the other's order and the page still renders. `diff.mjs` reports 12 of 12 identical, the two new ones with no declared differences. That claim is worth what the check is worth, so it was tested: making `all` keep main's pinned order produces 190 differences, and the run before that one passed only because the service under test was still holding the unmutated code from before the edit. The unit test deliberately does not assert the ordering. It cannot -- that needs a database and a running Java service -- and the version that tried, by matching the handler's source text for "TopLevelPathway", would have passed on any wrong query that happened to mention it. Co-Authored-By: Claude Opus 5 --- deploy/nginx/common/routes.conf | 16 +++++++++ proxy.conf.js | 5 +++ tools/content-node/service.mjs | 51 +++++++++++++++++++++++++++++ tools/content-node/service.spec.mjs | 21 ++++++++++++ 4 files changed, 93 insertions(+) diff --git a/deploy/nginx/common/routes.conf b/deploy/nginx/common/routes.conf index 0d339f5b..c7f5b8e4 100644 --- a/deploy/nginx/common/routes.conf +++ b/deploy/nginx/common/routes.conf @@ -48,6 +48,22 @@ location = /ContentService/data/content/contributors { proxy_pass http://content_node; } +# The species lists, byte-identical to Java and verified by the diff harness +# over the whole corpus rather than by eye. +# +# Exact matches for the same reason as the three above, and a sharper one here: +# Java serves other shapes under /data/species/ that node does not, so a prefix +# would claim them and 404 what works today. +location = /ContentService/data/species/main { + include /etc/nginx/common/upstream-proxy.conf; + proxy_pass http://content_node; +} + +location = /ContentService/data/species/all { + include /etc/nginx/common/upstream-proxy.conf; + proxy_pass http://content_node; +} + # The person page's four lists, which take an id so they cannot be exact # matches. `~` is a regex location and those are tried in order before any # prefix, so this wins over /ContentService/ below without depending on where in diff --git a/proxy.conf.js b/proxy.conf.js index 593f5320..dbde7040 100644 --- a/proxy.conf.js +++ b/proxy.conf.js @@ -63,6 +63,11 @@ module.exports = { '/ContentService/data/content/toc', '/ContentService/data/content/doi', '/ContentService/data/content/contributors', + // The species lists. Exact contexts, not a `/data/species` prefix: Java + // also serves `/data/species/{taxId}` shapes that node does not, and a + // prefix would claim them and 404 what currently works. + '/ContentService/data/species/main', + '/ContentService/data/species/all', ].map((context) => [ context, { diff --git a/tools/content-node/service.mjs b/tools/content-node/service.mjs index d89a434e..ea721d13 100644 --- a/tools/content-node/service.mjs +++ b/tools/content-node/service.mjs @@ -503,6 +503,57 @@ export const endpoints = [ differs: [/: java normalised by the endpoint's own rule before comparing$/], })) ), + ...[ + { + suffix: 'main', + /** + * A species Reactome has curated pathways for, which is the list every + * species selector on the site offers. + * + * "Main" is not a property on the node -- all 96 Species carry exactly the + * same six -- it is a relationship: 16 of them are the target of at least + * one TopLevelPathway's `species`, and 16 is what Java returns. Checked + * against the graph rather than inferred from the count matching, because + * two numbers agreeing is not a rule. + */ + match: '(s:Species)<-[:species]-(:TopLevelPathway)', + }, + { suffix: 'all', match: '(s:Species)' }, + ].map(({ suffix, match }) => ({ + path: `/ContentService/data/species/${suffix}`, + /** + * The species lists, which differ in their order as well as their contents. + * + * `main` puts Homo sapiens first and sorts the rest by name; `all` is plain + * alphabetical, human included, in its place. Both verified against the live + * responses -- and the difference is the reason they are written as two rows + * of one generator rather than one handler taking a flag, because a shared + * sort would have quietly given `all` a pinned human or `main` an unpinned + * one, and the page would still have rendered. + * + * Reactome pins human because it is the reference species everything else is + * inferred from, so a selector that buries it between Gallus and Mus is + * asking every reader to hunt for the common case. + */ + handler: cached(`species/${suffix}`, async () => { + const rows = await read( + `MATCH ${match} + WITH DISTINCT s + RETURN properties(s) AS species + ORDER BY CASE WHEN s.displayName = 'Homo sapiens' THEN 0 ELSE 1 END, s.displayName` + ); + const body = rows.map(({ species }) => ({ + ...only(species, ['dbId', 'displayName', 'name', 'taxId', 'abbreviation']), + className: species.schemaClass, + schemaClass: species.schemaClass, + })); + // `all` is alphabetical throughout, so the pin the query applies for + // `main` is undone here rather than by a second query. One ORDER BY that + // both share, and one line saying which of them does not want it. + if (suffix === 'all') body.sort((a, b) => (a.displayName < b.displayName ? -1 : 1)); + return { status: 200, body, type: 'application/json' }; + }), + })), ]; /** diff --git a/tools/content-node/service.spec.mjs b/tools/content-node/service.spec.mjs index 705c6ea2..5ca305f8 100644 --- a/tools/content-node/service.spec.mjs +++ b/tools/content-node/service.spec.mjs @@ -102,3 +102,24 @@ describe("the person lists' declared difference", () => { expect(person.differs?.length, 'the normalisation is declared').toBeGreaterThan(0); }); }); + +describe('the species lists', () => { + const paths = endpoints.map((e) => e.path); + + it('serves both, on the paths Java uses', () => { + expect(paths).toContain('/ContentService/data/species/main'); + expect(paths).toContain('/ContentService/data/species/all'); + }); + + it('serves them as two entries rather than one taking a flag', () => { + // The two differ in order as well as contents -- `main` pins Homo sapiens + // first, `all` is plain alphabetical including human in its place -- and a + // single handler with a boolean is how one of those silently acquires the + // other's sort. Both orders are proven against Java by `diff.mjs`, which + // needs a database and so cannot run here; what this can hold is that they + // stayed separate. + const species = endpoints.filter((e) => e.path.includes('/data/species/')); + expect(species).toHaveLength(2); + expect(species[0].handler).not.toBe(species[1].handler); + }); +}); From 7e6c88832f0e1ce0e42a559f754eb605eb903318 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 01:49:48 +0000 Subject: [PATCH 2/5] Stop asking the graph for a number that cannot change Prompted by a reminder to check query cost whenever porting, and the answer was not in the query. `/data/database/version` answered in 286ms where Java took 1.1ms, because Java holds the value and this went to the graph every time. It is the most-called endpoint of the set: the site asks for the release on every page load, since it keys the bucket paths for diagrams, figures and icons. So the first endpoint ever ported -- chosen as the smallest possible one, to prove the plumbing -- was also the slowest, and nobody had measured it against the thing it replaced. `/data/database/name` was the same at 290ms. Both now use the same `cached` wrapper the lists use, which is safe for the reason Java's cache is: these change when the database is replaced, and that restarts the service. 286ms to 8.5ms on the first request and 1.4ms after, against Java's 3.2ms. Not an index problem, which was the first thing checked -- one node matched by label, where no index applies. A standalone script reports ~700ms for that same query while the running service answers it in 8.5ms, same credentials and same localhost instance; that is recorded in the comment as unexplained rather than dressed up, because the fix is the same either way and the number that matters is the service's. The species lists this sits beside were already cached, which is why they answer in 1.3ms against Java's 682ms. `diff.mjs` still reports 12 of 12 identical. Co-Authored-By: Claude Opus 5 --- tools/content-node/service.mjs | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/tools/content-node/service.mjs b/tools/content-node/service.mjs index ea721d13..010618bc 100644 --- a/tools/content-node/service.mjs +++ b/tools/content-node/service.mjs @@ -40,19 +40,43 @@ export const endpoints = [ // still caught a wrong guess. The property is `releaseNumber`; `version` does // not exist on DBInfo and returned null, which the diff reported against // Java's 97 before anything could be switched on. - handler: async () => { + /** + * Cached, like every other list here, and for a sharper reason: measured + * against Java, this answered in 286ms where Java took 1.1ms, because Java + * holds the value and this went to the graph on every request. The site + * asks for the release on every page load -- it is what keys the bucket + * paths for diagrams, figures and icons -- so that was the most-called + * endpoint of the set and the slowest. + * + * Safe for the same reason Java's is: the release number changes when the + * database is replaced, and that restarts the service. + * + * Not an index problem, which was the first thing checked: this matches one + * DBInfo node by label, and there is no index that improves reading a + * single node found by label scan. + * + * What it is instead was not worth chasing further than the fix. A + * standalone script driving the same query through the same module reports + * ~700ms consistently, and the long-running service answers it in 8.7ms on + * its first request -- same query, same credentials file, same localhost + * instance. So the number a one-off script reports is not the number the + * service pays, and it is the service's that matters. Recorded rather than + * explained, because the fix for a value that never changes is the same + * either way: stop asking for it. + */ + handler: cached('database/version', async () => { const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); if (!row) return { status: 404, body: 'no DBInfo node' }; return { status: 200, body: String(row.version), type: 'text/plain;charset=UTF-8' }; - }, + }), }, { path: '/ContentService/data/database/name', - handler: async () => { + handler: cached('database/name', async () => { const [row] = await read('MATCH (n:DBInfo) RETURN n.name AS name LIMIT 1'); if (!row) return { status: 404, body: 'no DBInfo node' }; return { status: 200, body: String(row.name), type: 'text/plain;charset=UTF-8' }; - }, + }), }, { path: '/ContentService/data/pathways/top/{id}', From 26040eee5e6f5e01a50c791ed47868bfe01eecc4 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 02:07:32 +0000 Subject: [PATCH 3/5] Explain the 700ms: there is no token index, so label matches scan everything I had recorded this as unexplained. It is explainable, and the explanation is a rule worth having before porting anything else. `MATCH (n:DBInfo) RETURN n.releaseNumber` matches exactly one node and takes ~700ms. The plan says why: AllNodesScan + Filter n:DBInfo There is **no token (LOOKUP) index** on this instance -- 36 indexes, none of them one -- so a label match cannot use a label index and the planner reads every node. There are 2,958,129 of them. The same value fetched through an indexed property takes 29ms. That is why the cost does not vary with how many nodes match or how many rows return: one node and four hundred both scan three million. It is also why Java's own /data/species/main takes 682ms. Two rules for the next port, written next to `read` where a query gets written: match an indexed property under the label the index is on; and where an endpoint genuinely needs every node of a label -- the species lists do, and no property expresses that -- accept the scan and `cached()` it, which the service warms at startup so no reader waits for it. The real fix is one statement from whoever owns the database: CREATE LOOKUP INDEX node_labels FOR (n) ON EACH labels(n) It would speed up the Java service by the same amount. Not taken here: it is a schema write on an instance beta depends on. Also corrects the previous commit's claim that caching took the version endpoint to "8.5ms on the first request". It did not: the service warms cached handlers at startup, so that 8.5ms was a warm read and the build had already happened. The endpoint is ~1.4ms warm against Java's 3.2ms, and the scan is paid once at boot. Co-Authored-By: Claude Opus 5 --- tools/content-node/graph.mjs | 36 ++++++++++++++++++++++++++++++++++ tools/content-node/service.mjs | 20 +++++++++---------- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/tools/content-node/graph.mjs b/tools/content-node/graph.mjs index fbf1100e..2323b260 100644 --- a/tools/content-node/graph.mjs +++ b/tools/content-node/graph.mjs @@ -80,6 +80,42 @@ function connection() { * of a 64-bit value and a trap when the answer is compared against JSON from * another implementation: 74160 must serialise as 74160, not as an object. */ +/** + * **Match by an indexed property, never by label alone.** + * + * This instance has **no token (LOOKUP) index** -- `SHOW INDEXES` reports 36 + * indexes and none of them is one -- so `MATCH (n:Label)` cannot use a label + * index and the planner falls back to scanning every node and filtering. + * Measured on `MATCH (n:DBInfo) RETURN n.releaseNumber`, which matches exactly + * one node: + * + * EXPLAIN -> AllNodesScan + Filter n:DBInfo + * nodes in the database 2,958,129 + * time ~700ms + * the same read via an indexed property (DatabaseObject.dbId) 29ms + * + * So the cost has nothing to do with how many nodes match or how many rows come + * back: one node and four hundred cost the same, because both scan three + * million. That is also why Java's own `/data/species/main` takes 682ms. + * + * Two consequences for anyone porting an endpoint: + * + * 1. Prefer a property that is indexed, and match it **under the label the + * index is on** -- `dbId` is on `DatabaseObject`, and Neo4j treats `Person` + * as an unrelated label, so `(p:Person) WHERE p.dbId = …` scans while + * `(n:DatabaseObject) WHERE n.dbId = …` does not. + * 2. When an endpoint genuinely needs every node of a label -- the species + * lists do, and no property can express that -- there is no query that + * avoids the scan. Wrap it in `cached()` so the scan is paid once at + * startup rather than on a request path. + * + * The real fix is one statement from whoever owns the database: + * + * CREATE LOOKUP INDEX node_labels FOR (n) ON EACH labels(n) + * + * It would make every label match fast for the Java service too. It is a schema + * write on a shared instance, so it is not taken here. + */ export async function read(cypher, parameters = {}) { const session = connection().session({ database: DATABASE, diff --git a/tools/content-node/service.mjs b/tools/content-node/service.mjs index 010618bc..130c8f72 100644 --- a/tools/content-node/service.mjs +++ b/tools/content-node/service.mjs @@ -51,18 +51,16 @@ export const endpoints = [ * Safe for the same reason Java's is: the release number changes when the * database is replaced, and that restarts the service. * - * Not an index problem, which was the first thing checked: this matches one - * DBInfo node by label, and there is no index that improves reading a - * single node found by label scan. + * Not fixable with an index on this instance, and the reason is worth + * knowing before porting anything else: there is no token index here, so + * `MATCH (n:DBInfo)` plans as `AllNodesScan + Filter` and reads all + * 2,958,129 nodes to find one. ~700ms, and the same for any label-only + * match -- which is why Java's `/data/species/main` takes 682ms too. See + * the note on `read` in graph.mjs. * - * What it is instead was not worth chasing further than the fix. A - * standalone script driving the same query through the same module reports - * ~700ms consistently, and the long-running service answers it in 8.7ms on - * its first request -- same query, same credentials file, same localhost - * instance. So the number a one-off script reports is not the number the - * service pays, and it is the service's that matters. Recorded rather than - * explained, because the fix for a value that never changes is the same - * either way: stop asking for it. + * Caching is the fix available from this side. The service warms every + * `cached` handler at startup, so the scan is paid once, off the request + * path, and never by a reader. */ handler: cached('database/version', async () => { const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); From 15f094a52bd8786b1dbe8b5cc4ed75f140be63f4 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 02:26:14 +0000 Subject: [PATCH 4/5] Bound how stale the release number may be The review found that caching this was a regression, not just a speed-up. `cached` held a value for the life of the process, justified as "a release restarts the service, and these lists only change with the graph". True of Java, whose WAR is redeployed. Not true of this: it runs as a container with `restart: unless-stopped`, and nothing in the release procedure or in any script on the host restarts it. I checked, after caching something where being stale is worse than being slow. The release number is exactly that. It keys the bucket paths for diagrams, figures and icons, so serving last release's number sends all of those requests to the wrong prefix -- and it would keep doing so until somebody noticed and restarted a container. Before this branch the endpoint was uncached and therefore always right; I would have traded correct-and-slow for fast-and -eventually-wrong. So `cached` takes an optional `ttlMs`, and the two database endpoints take 60 seconds: a minute of staleness after a release instead of forever, and still one scan a minute rather than one per request. Warmed at startup as before, so no reader waits for the first. The lists keep the unbounded form, and the reasoning is now written down rather than asserted: they are expensive to rebuild, only read by pages that render them, and a stale entry is a missing person or DOI until the next deploy rather than a wrong URL everywhere. A judgement about consequence, which the next person can disagree with knowingly. The test asserts the bound rather than the number, and fails on the version as this branch first wrote it: `expected Infinity to be less than 300000`. `diff.mjs` still reports 12 of 12 identical. Co-Authored-By: Claude Opus 5 --- tools/content-node/service.mjs | 68 +++++++++++++++++++++-------- tools/content-node/service.spec.mjs | 25 +++++++++++ 2 files changed, 76 insertions(+), 17 deletions(-) diff --git a/tools/content-node/service.mjs b/tools/content-node/service.mjs index 130c8f72..bb633679 100644 --- a/tools/content-node/service.mjs +++ b/tools/content-node/service.mjs @@ -62,19 +62,28 @@ export const endpoints = [ * `cached` handler at startup, so the scan is paid once, off the request * path, and never by a reader. */ - handler: cached('database/version', async () => { - const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); - if (!row) return { status: 404, body: 'no DBInfo node' }; - return { status: 200, body: String(row.version), type: 'text/plain;charset=UTF-8' }; - }), + handler: cached( + 'database/version', + async () => { + const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); + if (!row) return { status: 404, body: 'no DBInfo node' }; + return { status: 200, body: String(row.version), type: 'text/plain;charset=UTF-8' }; + }, + // A minute of staleness after a release, instead of forever. + { ttlMs: 60_000 } + ), }, { path: '/ContentService/data/database/name', - handler: cached('database/name', async () => { - const [row] = await read('MATCH (n:DBInfo) RETURN n.name AS name LIMIT 1'); - if (!row) return { status: 404, body: 'no DBInfo node' }; - return { status: 200, body: String(row.name), type: 'text/plain;charset=UTF-8' }; - }), + handler: cached( + 'database/name', + async () => { + const [row] = await read('MATCH (n:DBInfo) RETURN n.name AS name LIMIT 1'); + if (!row) return { status: 404, body: 'no DBInfo node' }; + return { status: 200, body: String(row.name), type: 'text/plain;charset=UTF-8' }; + }, + { ttlMs: 60_000 } + ), }, { path: '/ContentService/data/pathways/top/{id}', @@ -596,16 +605,38 @@ export const endpoints = [ * again. A slow start costs one slow request instead of an empty page nobody * connects to a restart hours earlier. * - * Nothing refreshes it while the process lives, which matches Java: a release - * restarts the service, and these lists only change with the graph. + * `ttlMs` bounds how stale an answer may be. Without one the value is held for + * the life of the process, which was justified here as "a release restarts the + * service, and these lists only change with the graph". That is true of Java, + * whose WAR is redeployed, and **not** of this: it runs as a container with + * `restart: unless-stopped`, and nothing in the release procedure or in any + * script on the host restarts it. Checked rather than assumed, after caching + * something where being stale is worse than being slow. + * + * So a value that decides what the site asks for elsewhere takes a TTL. The + * release number is the case in point -- it keys the bucket paths for diagrams, + * figures and icons, so serving last release's number sends every one of those + * requests to the wrong prefix, and it would keep doing so until somebody + * noticed and restarted a container. + * + * The lists keep the unbounded form. They are large to rebuild, they are only + * read by pages that render them, and a stale entry there is a missing person + * or a missing DOI until the next deploy rather than a wrong URL everywhere. + * That is a judgement about consequence, not about likelihood, and it is + * written down here so the next person can disagree with it knowingly. */ -function cached(name, build) { +function cached(name, build, { ttlMs = Infinity } = {}) { let ready; + let builtAt = 0; const handler = async (request) => { - ready ??= build(request).catch((error) => { - ready = undefined; - throw error; - }); + if (ready && Date.now() - builtAt > ttlMs) ready = undefined; + if (!ready) { + builtAt = Date.now(); + ready = build(request).catch((error) => { + ready = undefined; + throw error; + }); + } const started = Date.now(); const answer = await ready; const spent = Date.now() - started; @@ -622,6 +653,9 @@ function cached(name, build) { // Marks this one for warming at startup; a handler that reads its request // cannot be warmed, and saying so here keeps that decision beside the cache. handler.warms = true; + // Readable by the spec: a cache that may never expire is a decision, and one + // that can be asserted is a decision that stays made. + handler.ttlMs = ttlMs; return handler; } diff --git a/tools/content-node/service.spec.mjs b/tools/content-node/service.spec.mjs index 5ca305f8..7f6b0b51 100644 --- a/tools/content-node/service.spec.mjs +++ b/tools/content-node/service.spec.mjs @@ -123,3 +123,28 @@ describe('the species lists', () => { expect(species[0].handler).not.toBe(species[1].handler); }); }); + +describe('how long a cached answer may be stale', () => { + /** + * The release number keys the bucket paths for diagrams, figures and icons, + * so serving the previous release's number sends every one of those requests + * to the wrong prefix. Before this it was held for the life of the process on + * the grounds that "a release restarts the service" — true of Java, whose WAR + * is redeployed, and not of this, which runs as a container with + * `restart: unless-stopped` that nothing in the release procedure touches. + */ + it("gives the database endpoints a bounded life, not the process's", () => { + const version = endpoints.find((e) => e.path.endsWith('/database/version')); + const name = endpoints.find((e) => e.path.endsWith('/database/name')); + for (const endpoint of [version, name]) { + expect(endpoint.handler.ttlMs, `${endpoint.path} must not be cached forever`).toBeLessThan( + 5 * 60_000 + ); + } + }); + + it('still warms them at startup, so no reader pays the first scan', () => { + const version = endpoints.find((e) => e.path.endsWith('/database/version')); + expect(version.handler.warms).toBe(true); + }); +}); From 4e1b8664a4a1a1b8ae8eab3f36fe78b63cdbf3f1 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 22 Sep 2026 02:45:44 +0000 Subject: [PATCH 5/5] Make the restart rule the rule, and a missed one visible Decided: updating the database includes restarting this service. So the caches go back to living for the life of the process, and the TTL added in the previous commit is gone -- it was working around the absence of a rule, and a rule is the better answer. What changed is that it is now a rule rather than an inference. The comment used to say "a release restarts the service", which was true of Java, whose WAR is redeployed, and untrue of this, a container with `restart: unless-stopped` that nothing restarted. That is the kind of sentence that is right until the day it matters. Because the rule depends on a person doing something, the cost of forgetting is now observable. `/health` reports the release this process is serving. The release number keys the bucket paths for diagrams, figures and icons, so a process holding the previous one sends all of those to the wrong prefix while every response still looks correct -- the failure has no other symptom. One curl now shows it. It reads that through the same cache the endpoint uses, deliberately. A health check that queried the graph directly would report the right number while every other response served the wrong one, which is worse than not reporting it: it would confirm the thing that is broken. Verified against a real graph -- `{"ok":true,"graph":true,"release":"97"}` -- and against no graph at all, where it still answers 200, because the process being up is what it is asked about. diff.mjs: 12 of 12 identical. Co-Authored-By: Claude Opus 5 --- tools/content-node/service.mjs | 100 ++++++++++++++-------------- tools/content-node/service.spec.mjs | 38 ++++++----- 2 files changed, 69 insertions(+), 69 deletions(-) diff --git a/tools/content-node/service.mjs b/tools/content-node/service.mjs index bb633679..fcd3efb7 100644 --- a/tools/content-node/service.mjs +++ b/tools/content-node/service.mjs @@ -62,28 +62,19 @@ export const endpoints = [ * `cached` handler at startup, so the scan is paid once, off the request * path, and never by a reader. */ - handler: cached( - 'database/version', - async () => { - const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); - if (!row) return { status: 404, body: 'no DBInfo node' }; - return { status: 200, body: String(row.version), type: 'text/plain;charset=UTF-8' }; - }, - // A minute of staleness after a release, instead of forever. - { ttlMs: 60_000 } - ), + handler: cached('database/version', async () => { + const [row] = await read('MATCH (n:DBInfo) RETURN n.releaseNumber AS version LIMIT 1'); + if (!row) return { status: 404, body: 'no DBInfo node' }; + return { status: 200, body: String(row.version), type: 'text/plain;charset=UTF-8' }; + }), }, { path: '/ContentService/data/database/name', - handler: cached( - 'database/name', - async () => { - const [row] = await read('MATCH (n:DBInfo) RETURN n.name AS name LIMIT 1'); - if (!row) return { status: 404, body: 'no DBInfo node' }; - return { status: 200, body: String(row.name), type: 'text/plain;charset=UTF-8' }; - }, - { ttlMs: 60_000 } - ), + handler: cached('database/name', async () => { + const [row] = await read('MATCH (n:DBInfo) RETURN n.name AS name LIMIT 1'); + if (!row) return { status: 404, body: 'no DBInfo node' }; + return { status: 200, body: String(row.name), type: 'text/plain;charset=UTF-8' }; + }), }, { path: '/ContentService/data/pathways/top/{id}', @@ -605,38 +596,27 @@ export const endpoints = [ * again. A slow start costs one slow request instead of an empty page nobody * connects to a restart hours earlier. * - * `ttlMs` bounds how stale an answer may be. Without one the value is held for - * the life of the process, which was justified here as "a release restarts the - * service, and these lists only change with the graph". That is true of Java, - * whose WAR is redeployed, and **not** of this: it runs as a container with - * `restart: unless-stopped`, and nothing in the release procedure or in any - * script on the host restarts it. Checked rather than assumed, after caching - * something where being stale is worse than being slow. - * - * So a value that decides what the site asks for elsewhere takes a TTL. The - * release number is the case in point -- it keys the bucket paths for diagrams, - * figures and icons, so serving last release's number sends every one of those - * requests to the wrong prefix, and it would keep doing so until somebody - * noticed and restarted a container. + * Held for the life of the process, which makes restarting this service part of + * updating the database rather than an optimisation detail. That is the agreed + * rule, and it is the rule rather than an inference: the previous version of + * this comment asserted that "a release restarts the service", which is true of + * Java -- its WAR is redeployed -- and was not true of this, a container with + * `restart: unless-stopped` that nothing restarted. * - * The lists keep the unbounded form. They are large to rebuild, they are only - * read by pages that render them, and a stale entry there is a missing person - * or a missing DOI until the next deploy rather than a wrong URL everywhere. - * That is a judgement about consequence, not about likelihood, and it is - * written down here so the next person can disagree with it knowingly. + * What it costs to get wrong, so the rule is worth keeping: the release number + * keys the bucket paths for diagrams, figures and icons, so a service holding + * the previous release's number sends every one of those requests to the wrong + * prefix, and keeps doing it silently. `/health` reports the release this + * process is holding for exactly that reason -- a missed restart is then one + * curl away from being obvious instead of invisible. */ -function cached(name, build, { ttlMs = Infinity } = {}) { +function cached(name, build) { let ready; - let builtAt = 0; const handler = async (request) => { - if (ready && Date.now() - builtAt > ttlMs) ready = undefined; - if (!ready) { - builtAt = Date.now(); - ready = build(request).catch((error) => { - ready = undefined; - throw error; - }); - } + ready ??= build(request).catch((error) => { + ready = undefined; + throw error; + }); const started = Date.now(); const answer = await ready; const spent = Date.now() - started; @@ -653,9 +633,6 @@ function cached(name, build, { ttlMs = Infinity } = {}) { // Marks this one for warming at startup; a handler that reads its request // cannot be warmed, and saying so here keeps that decision beside the cache. handler.warms = true; - // Readable by the spec: a cache that may never expire is a decision, and one - // that can be asserted is a decision that stays made. - handler.ttlMs = ttlMs; return handler; } @@ -811,11 +788,32 @@ export function app() { const server = express(); server.disable('x-powered-by'); - server.get('/health', (_request, response) => { + server.get('/health', async (_request, response) => { + // The release this process is holding, which is the one thing here that can + // be wrong without anything looking wrong. Caches live for the life of the + // process by agreement -- updating the database includes restarting this -- + // and a missed restart shows up as diagram, figure and icon URLs pointing at + // the previous release's bucket prefix, silently. Reporting it makes that + // one curl away from obvious. + // + // Read through the same cache the endpoint uses, so this reports what is + // being *served* rather than what the graph currently says. A health check + // that went straight to the database would answer correctly while every + // other response was stale, which is the opposite of useful. + let release = null; + try { + const version = endpoints.find((e) => e.path.endsWith('/data/database/version')); + const answer = await version?.handler({ params: {}, query: {} }); + release = answer?.body ?? null; + } catch { + // A health check that fails because the graph is down is reporting the + // wrong thing: the process is up, and that is what this answers. + } response.json({ ok: true, ...buildId(), graph: configured(), + release, endpoints: endpoints.map((e) => e.path), }); }); diff --git a/tools/content-node/service.spec.mjs b/tools/content-node/service.spec.mjs index 7f6b0b51..50e1b1b0 100644 --- a/tools/content-node/service.spec.mjs +++ b/tools/content-node/service.spec.mjs @@ -124,27 +124,29 @@ describe('the species lists', () => { }); }); -describe('how long a cached answer may be stale', () => { +describe('what /health says about staleness', () => { /** - * The release number keys the bucket paths for diagrams, figures and icons, - * so serving the previous release's number sends every one of those requests - * to the wrong prefix. Before this it was held for the life of the process on - * the grounds that "a release restarts the service" — true of Java, whose WAR - * is redeployed, and not of this, which runs as a container with - * `restart: unless-stopped` that nothing in the release procedure touches. + * Caches live for the life of the process by agreement: updating the database + * includes restarting this service. The previous version of that reasoning + * was an inference — "a release restarts the service", true of Java's WAR and + * not of a container with `restart: unless-stopped` — so the rule is now the + * rule, and this is what makes a missed restart visible. + * + * It matters because the release number keys the bucket paths for diagrams, + * figures and icons: a process holding the previous one sends all of those to + * the wrong prefix, and nothing else about the response looks wrong. */ - it("gives the database endpoints a bounded life, not the process's", () => { - const version = endpoints.find((e) => e.path.endsWith('/database/version')); - const name = endpoints.find((e) => e.path.endsWith('/database/name')); - for (const endpoint of [version, name]) { - expect(endpoint.handler.ttlMs, `${endpoint.path} must not be cached forever`).toBeLessThan( - 5 * 60_000 - ); - } + it('reports the release it is serving', async () => { + const health = await (await fetch(`${base}/health`)).json(); + expect(health).toHaveProperty('release'); }); - it('still warms them at startup, so no reader pays the first scan', () => { - const version = endpoints.find((e) => e.path.endsWith('/database/version')); - expect(version.handler.warms).toBe(true); + it('still answers when the graph cannot be reached', async () => { + // No credentials in this suite, so the version handler throws. A health + // check that failed on that would be reporting the database's health under + // the name of the process's. + const response = await fetch(`${base}/health`); + expect(response.status).toBe(200); + expect((await response.json()).ok).toBe(true); }); });