From 4e1f3ffdacfc5065dde6c8d4a90ac45b70fb11a7 Mon Sep 17 00:00:00 2001 From: claude-code Date: Wed, 19 Aug 2026 19:30:02 +0000 Subject: [PATCH] Fix: give the backend room to run, and stop transforming half-typed expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend container runs three JVMs — Saxon 12, Saxon 9.6 and Xalan — whose -Xmx values add up to 512 MB, inside a container limited to 256Mi. The limit was half the heap the JVMs believed they had, so the kernel was killing the container: OOMKilled twice a day on the 16th and 17th of August, five times a day on the 18th and 19th, and 81 users in 24 hours got "transform service unavailable" instead of a result. Before each kill the serial collector spends hours in continuous full GC, which is why three of the five pods sat pinned at exactly 0.5 CPU — their limit — around the clock, and why XSLT 2.0's p95 was 5.5s against 0.7s for the other two engines. Those limits were never in the chart to begin with: the backend deployment has never rendered a resources block, values.resources.backend was declared and unused, and the live numbers came from a manual kubectl patch that an ArgoCD sync would have silently dropped. So the chart now owns them, with room for what actually runs in there. The CPU request moves 10m -> 250m as well: against real usage of 500m the HPA read utilisation in the thousands of percent and the deployment sat permanently at maxReplicas, unable to signal anything. And the backend gets minReplicas 2, for the reason the frontend already learnt. The other half is the mid-keystroke problem again, one level down. Gating the automatic run on well-formedness took the error rate from 54% to 24%, but a stylesheet whose XPath is half-typed is perfectly good XML, so it still went to the backend and still came back as a compile error. A day of logs is mostly that, keystroke by keystroke: 'current()/..[@N' then '[@Na' then '[@Name', and "Required attribute 'select' is missing" 65 times on a single line. So the gate now also declines to send an expression that is provably unfinished — a required attribute that is absent, an empty value, a quote or bracket still open, a trailing token that cannot end an expression. Facts about the text, not guesses at intent: there are twice as many tests for what it must leave alone as for what it catches, including every template the app itself ships. It is skipped above 64KB, where a document was pasted rather than typed, and as before it only holds back the automatic run — "Run it anyway" is always there. Finally, the error classifier learns the rest of what was sitting in "other": Xalan's "Syntax error in ''" is the stylesheet's XPath, not the input. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018HnUwBeXv6PCTEzRpFoMSU --- backend/src/main.go | 8 + backend/src/main_test.go | 6 + charts/xslt-playground/Chart.yaml | 2 +- .../templates/backend-deployment.yaml | 1 + charts/xslt-playground/values.yaml | 19 +- frontend/src/App.jsx | 20 ++ frontend/src/App.test.jsx | 14 ++ frontend/src/lib/unfinishedExpression.js | 201 ++++++++++++++++++ frontend/src/lib/unfinishedExpression.test.js | 160 ++++++++++++++ 9 files changed, 428 insertions(+), 3 deletions(-) create mode 100644 frontend/src/lib/unfinishedExpression.js create mode 100644 frontend/src/lib/unfinishedExpression.test.js diff --git a/backend/src/main.go b/backend/src/main.go index c95d0af4..a052bbd2 100644 --- a/backend/src/main.go +++ b/backend/src/main.go @@ -173,6 +173,14 @@ func classifyTransformError(msg string) (code, class string) { "is undefined", "format-number picture", "cannot find external method", + // Second pass over what was still sitting in "other" (2026-08-19). The + // XPath ones dominate: Xalan reports a bad path expression as "Syntax + // error in ''", which is the stylesheet's XPath, not the input. + "syntax error in '", + "does not appear to be a stylesheet", + "must precede all other element children", + "has not been defined", + "a sequence of more than one item is not allowed", } switch { diff --git a/backend/src/main_test.go b/backend/src/main_test.go index 72f8d452..2aafa443 100644 --- a/backend/src/main_test.go +++ b/backend/src/main_test.go @@ -235,6 +235,12 @@ func TestClassifyTransformError(t *testing.T) { {"format-number picture", "format-number picture: Passive character must not appear between active characters in a sub-picture", "stylesheet", "COMPILE"}, {"missing java extension", "Cannot find external method 'com.example.util.DateUtil.now' (must be public).", "stylesheet", "COMPILE"}, {"stray xml declaration", `The processing instruction target matching "[xX][mM][lL]" is not allowed.`, "input_xml", "PARSE"}, + // Second pass over "other", from the 2026-08-19 log review. + {"xalan xpath syntax", `Syntax error in 'current()/..[@Name = 'Programming''.`, "stylesheet", "COMPILE"}, + {"not a stylesheet", "The supplied file does not appear to be a stylesheet", "stylesheet", "COMPILE"}, + {"misplaced xsl:import", "The xsl:import element children must precede all other element children of an xsl:stylesheet element, including any xsl:include element children.", "stylesheet", "COMPILE"}, + {"undeclared key", "Key 'person-key' has not been defined", "stylesheet", "COMPILE"}, + {"sequence where one item expected", `A sequence of more than one item is not allowed as the first argument of substring() ("a", "b", ...)`, "stylesheet", "COMPILE"}, // A limit the service imposes — a bug candidate, not the user's mistake. {"xpath operator limit", "JAXP0801002: the compiler encountered an XPath expression containing '101' operators that exceeds the '100' limit set by 'FEATURE_SECURE_PROCESSING'.", "backend", "XPATH_OP_LIMIT"}, {"truly unknown", "some unexpected failure", "other", "OTHER"}, diff --git a/charts/xslt-playground/Chart.yaml b/charts/xslt-playground/Chart.yaml index 2756a475..d740ecbc 100644 --- a/charts/xslt-playground/Chart.yaml +++ b/charts/xslt-playground/Chart.yaml @@ -1,6 +1,6 @@ apiVersion: v2 name: xslt-playground description: Helm chart for xslt-playground frontend and backend -version: 0.4.1 +version: 0.4.2 appVersion: "0.1.0" diff --git a/charts/xslt-playground/templates/backend-deployment.yaml b/charts/xslt-playground/templates/backend-deployment.yaml index 50c8a347..3dd89424 100644 --- a/charts/xslt-playground/templates/backend-deployment.yaml +++ b/charts/xslt-playground/templates/backend-deployment.yaml @@ -42,6 +42,7 @@ spec: mountPath: {{ .Values.firebase.credentialsMountPath }} readOnly: true {{- end }} + resources: {{- toYaml .Values.resources.backend | nindent 12 }} ports: - containerPort: {{ .Values.service.backend.port }} name: http diff --git a/charts/xslt-playground/values.yaml b/charts/xslt-playground/values.yaml index 6b01eecc..788e1147 100644 --- a/charts/xslt-playground/values.yaml +++ b/charts/xslt-playground/values.yaml @@ -33,7 +33,20 @@ ingress: resources: frontend: {} - backend: {} + # The backend container runs three JVMs side by side (Saxon 12, Saxon 9.6 and + # Xalan — see backend/start.sh), whose -Xmx values alone add up to 512 MB, + # before metaspace, thread stacks and the Go server. The limit has to leave + # room for all of it or the kernel kills the container mid-transform. + backend: + requests: + # Sized so the HPA reads something meaningful: with a 10m request the + # utilisation ratio is permanently in the thousands of percent and the + # deployment sits pinned at maxReplicas. + cpu: 250m + memory: 384Mi + limits: + cpu: "1" + memory: 1Gi hpa: frontend: @@ -43,7 +56,9 @@ hpa: targetCPUUtilizationPercentage: 80 backend: enabled: true - minReplicas: 1 + # Two, so a single evicted or restarting pod never leaves the app with no + # backend at all — the frontend learnt this the hard way. + minReplicas: 2 maxReplicas: 5 targetCPUUtilizationPercentage: 80 diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index a649823f..1a994b40 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -16,6 +16,7 @@ import { checkWellFormed, } from "./lib/workspaceUtils"; import { templateToWorkspace, findTemplate, STARTER_STYLESHEET } from "./lib/templates"; +import { findUnfinishedExpression } from "./lib/unfinishedExpression"; import { reviewWorkspace } from "./lib/reviewRules"; import { diffLines } from "./lib/diffUtils"; import { encodeCompact, decodeCompact, toSharePayload, fromSharePayload, saveFiddle, loadFiddle } from "./lib/shareLink"; @@ -1776,6 +1777,25 @@ export default function App() { })); return undefined; } + + // Well-formed XML is not the same as a finished expression. A stylesheet + // whose select= is still being typed parses fine and fails to compile, so + // it used to go to the backend anyway — most of what survived the check + // above is exactly that. Same deal: reported here, nothing sent, and the + // explicit run always wins. + const unfinished = findUnfinishedExpression(xsltText); + if (unfinished) { + const text = `Still being typed? ${unfinished.message}.`; + updateWorkspaceStatus(activeTab.id, (prev) => ({ + ...prev, + error: text, + errorLines: parseErrorLines(text), + isServerError: false, + notWellFormed: true, + isRunning: false, + })); + return undefined; + } } runTransform(xsltText, activeTab.version, activeTab.params, activeTab.id); diff --git a/frontend/src/App.test.jsx b/frontend/src/App.test.jsx index d7f010ae..76c0aba8 100644 --- a/frontend/src/App.test.jsx +++ b/frontend/src/App.test.jsx @@ -208,6 +208,20 @@ describe("well-formedness gate", () => { await waitFor(() => expect(fetch).toHaveBeenCalled(), { timeout: 3000 }); }, 10000); + it("does not call the backend while an expression is still being typed", async () => { + // Well-formed XML, unfinished XPath: this is what used to get through. + seedWorkspace( + '', + ); + render(); + fireEvent.pointerDown(window); + await waitFor( + () => expect(screen.getByText(/still being typed/i)).toBeInTheDocument(), + { timeout: 3000 }, + ); + expect(fetch).not.toHaveBeenCalled(); + }, 10000); + it("offers a way to run it anyway", async () => { seedWorkspace('` is +// well-formed XML and a compile error, so it went to the backend anyway. A day +// of production logs (2026-08-19) is mostly this, keystroke by keystroke: +// +// Syntax error in 'current()/..[@N' → '[@Na' → '[@Name' → '[@Name = ' +// line 15: Required attribute 'select' is missing. (65 times, one line) +// XPST0003: The expression is empty +// +// Everything below is a fact about the expression, never a guess about intent: +// an attribute that is required and absent, a value that is empty, a quote or +// bracket that is still open, or a trailing token that cannot legally end an +// expression. Anything we are not certain about runs, exactly as before — and +// like the well-formedness gate, this only holds back the automatic run, never +// an explicit one. + +const XSLT_NS = "http://www.w3.org/1999/XSL/Transform"; + +// Elements that cannot compile without the listed attribute. xsl:value-of is +// the exception: XSLT 2.0 lets a sequence constructor stand in for select, so +// it only counts as missing when the element is also empty. +const REQUIRED = { + "value-of": "select", + "for-each": "select", + "for-each-group": "select", + "copy-of": "select", + if: "test", + when: "test", +}; + +// Attributes holding an XPath expression or a match pattern. +const XPATH_ATTRS = [ + "select", + "test", + "match", + "use", + "group-by", + "group-adjacent", +]; + +// Tokens that cannot be the last thing in a complete expression. Word +// operators are only counted when something precedes them, so `select="and"` +// — a perfectly good path selecting children — is left alone. +const TRAILING_SYMBOLS = [ + "//", + "/", + "[", + "(", + ",", + "@", + "$", + "::", + ":", + "=", + "!=", + "<=", + ">=", + "<", + ">", + "+", + "-", + "|", + "!", +]; +const TRAILING_WORDS = [ + "and", + "or", + "div", + "idiv", + "mod", + "to", + "eq", + "ne", + "lt", + "gt", + "le", + "ge", + "is", + "instance", + "of", + "as", + "castable", + "cast", + "treat", + "return", + "in", + "satisfies", + "then", + "else", +]; + +// Walks the value once, tracking string literals so brackets inside a quoted +// string are not mistaken for structure. +function scan(value) { + let quote = null; + let round = 0; + let square = 0; + for (const ch of value) { + if (quote) { + if (ch === quote) quote = null; + continue; + } + if (ch === "'" || ch === '"') { + quote = ch; + } else if (ch === "(") { + round += 1; + } else if (ch === ")") { + round -= 1; + } else if (ch === "[") { + square += 1; + } else if (ch === "]") { + square -= 1; + } + } + return { openQuote: Boolean(quote), round, square }; +} + +// Returns a short reason when the expression is provably incomplete, else null. +export function describeUnfinished(value) { + const text = (value || "").trim(); + if (!text) return "is empty"; + + const { openQuote, round, square } = scan(text); + if (openQuote) return "has a quote that is still open"; + if (round > 0) return "has a ( that is never closed"; + if (square > 0) return "has a [ that is never closed"; + // Unbalanced the other way is a mistake, not an unfinished edit: let the + // processor report it properly. + if (round < 0 || square < 0) return null; + + // "/" on its own is the root pattern, not a path someone abandoned midway. + if (text === "/") return null; + + for (const token of TRAILING_SYMBOLS) { + if (text.endsWith(token)) return `stops at "${token}"`; + } + const words = text.split(/\s+/); + if (words.length > 1 && TRAILING_WORDS.includes(words[words.length - 1])) { + return `stops at "${words[words.length - 1]}"`; + } + return null; +} + +function elementLabel(el) { + return `<${el.prefix ? `${el.prefix}:` : ""}${el.localName}>`; +} + +// Above this the check is skipped entirely. It runs on every keystroke, on top +// of the well-formedness parse, and the walk is not free on a large document +// (jsdom: ~2ms at 3KB, ~15ms at 34KB, ~250ms at 100KB). It also stops being +// worth it: nobody types a 64KB stylesheet character by character — a document +// that size was pasted, and a pasted document is finished. So the cost is +// capped where the problem it solves stops existing. +const MAX_SCANNED_BYTES = 64 * 1024; + +// Scans a stylesheet for the first expression that is still being typed. +// Returns { message } or null. Callers pass text that is already known to +// parse; anything that does not parse is the well-formedness gate's business. +export function findUnfinishedExpression(xslt) { + if (!xslt || !xslt.trim()) return null; + if (xslt.length > MAX_SCANNED_BYTES) return null; + let doc; + try { + doc = new DOMParser().parseFromString(xslt, "application/xml"); + } catch { + return null; // no parser here: let the backend decide, as before + } + if (doc.querySelector("parsererror")) return null; + + // Array.from, not for..of: an HTMLCollection is only iterable by grace of + // the browsers, and this has to hold in every one of them. + const elements = Array.from(doc.getElementsByTagNameNS(XSLT_NS, "*")); + for (const el of elements) { + const required = REQUIRED[el.localName]; + if (required && !el.hasAttribute(required)) { + // A sequence constructor is a legitimate alternative to value-of/@select. + if (el.localName === "value-of" && el.childNodes.length > 0) continue; + return { + message: `${elementLabel(el)} has no ${required} expression yet`, + }; + } + for (const attr of XPATH_ATTRS) { + if (!el.hasAttribute(attr)) continue; + const value = el.getAttribute(attr); + const reason = describeUnfinished(value); + if (reason) { + const shown = value.trim(); + return { + message: shown + ? `${attr}="${shown}" on ${elementLabel(el)} ${reason}` + : `${attr} on ${elementLabel(el)} ${reason}`, + }; + } + } + } + return null; +} diff --git a/frontend/src/lib/unfinishedExpression.test.js b/frontend/src/lib/unfinishedExpression.test.js new file mode 100644 index 00000000..87f1196f --- /dev/null +++ b/frontend/src/lib/unfinishedExpression.test.js @@ -0,0 +1,160 @@ +import { describe, it, expect } from "vitest"; +import { + describeUnfinished, + findUnfinishedExpression, +} from "./unfinishedExpression"; +import { TEMPLATES, STARTER_STYLESHEET } from "./templates"; + +const wrap = (body, version = "2.0") => + `${body}`; + +describe("describeUnfinished — expressions that are still being typed", () => { + // Every one of these came off a day of production logs. + it.each([ + ["", "is empty"], + [" ", "is empty"], + ["/Shop/Category/", 'stops at "/"'], + ["/Shop/Category/@", 'stops at "@"'], + ["current()/", 'stops at "/"'], + ["../", 'stops at "/"'], + ["..//", 'stops at "//"'], + ["./@", 'stops at "@"'], + ["current()/..[", 'has a [ that is never closed'], + ["Book[@CategoryId = ", "has a [ that is never closed"], + ["concat(@a,", "has a ( that is never closed"], + ["@Name = 'Programm", "has a quote that is still open"], + ["$total +", 'stops at "+"'], + ["@a =", 'stops at "="'], + ["item[@id = $", 'has a [ that is never closed'], + ["1 to", 'stops at "to"'], + ["@a and", 'stops at "and"'], + ])("flags %j", (value, reason) => { + expect(describeUnfinished(value)).toBe(reason); + }); +}); + +describe("describeUnfinished — expressions it must leave alone", () => { + it.each([ + "/", // the root pattern, not an abandoned path + ".", + "..", + "*", + "@*", + "@id", + "node()", + "text()", + "child::*", + "//item", + "/Shop/Category", + "$total", + "count(item)", + "concat(@a, '-', @b)", + "item[@id = '3']", + "item[position() > 1]", + "current()/..", + "1 to 5", + "@a and @b", + "and", // a path selecting children is legal + "or", + "to", + "@price * 1.21", + "-1", + "@a - 1", + "substring(@a, 1, 2)", + "if ($a) then 'x' else 'y'", + "for $i in item return $i", + "@a != @b", + "'a literal with a ( in it'", + "'unbalanced ] inside a string'", + "item[1]/name", + "xs:date('2026-01-01')", + "map{'a': 1}", + ])("stays quiet on %j", (value) => { + expect(describeUnfinished(value)).toBeNull(); + }); + + it("leaves a genuinely wrong expression to the processor", () => { + // Closing more than was opened is a mistake, not a half-finished edit: the + // real error message is more useful than us guessing. + expect(describeUnfinished("item)")).toBeNull(); + expect(describeUnfinished("item]")).toBeNull(); + }); +}); + +describe("findUnfinishedExpression", () => { + it("catches the attribute that is not there yet", () => { + // "Required attribute 'select' is missing" was the single most common + // error in production: 65 hits on one line in 24 hours. + expect( + findUnfinishedExpression( + wrap(``), + ).message, + ).toBe(" has no select expression yet"); + + expect( + findUnfinishedExpression( + wrap(`x`), + ).message, + ).toBe(" has no test expression yet"); + }); + + it("allows a sequence constructor in place of value-of/@select", () => { + expect( + findUnfinishedExpression( + wrap( + ``, + ), + ), + ).toBeNull(); + }); + + it("names the attribute and the element it is on", () => { + const found = findUnfinishedExpression( + wrap(``), + ); + expect(found.message).toBe( + 'select="/Shop/Category/" on stops at "/"', + ); + }); + + it("ignores attributes that are not expressions", () => { + expect( + findUnfinishedExpression( + wrap(``), + ), + ).toBeNull(); + }); + + it("says nothing about a document that does not parse", () => { + // That is the well-formedness gate's job, and it reports it better. + expect(findUnfinishedExpression(` { + expect(findUnfinishedExpression("")).toBeNull(); + expect(findUnfinishedExpression(" ")).toBeNull(); + }); + + it("passes every stylesheet the app itself ships", () => { + // The strongest guard against a false positive: if the gate would hold back + // the starter document or any gallery template, it is wrong. + expect(findUnfinishedExpression(STARTER_STYLESHEET)).toBeNull(); + for (const template of TEMPLATES) { + expect( + findUnfinishedExpression(template.xslt), + `template ${template.id} must run`, + ).toBeNull(); + } + }); +}); + +describe("cost", () => { + it("skips a stylesheet too large to have been typed", () => { + const filler = "x".repeat(3000); + const big = wrap( + `${filler}`, + ); + expect(big.length).toBeGreaterThan(64 * 1024); + expect(findUnfinishedExpression(big)).toBeNull(); + }); +});