fix(engine/2.0): stop rejecting valid engine postgres config - #30
Merged
Conversation
The storage subtree closed every object with `additionalProperties: false`, so the schema had to enumerate each field the engine accepts or reject it. It fell behind: `target_session_attrs` has been read by the engine for a while, and `max_conns` joins it, and both were rejected. Because the two postgres variants sit inside a `oneOf`, an unknown key fails every branch, so the error lands on the whole `storage` node instead of naming the offending field. Enumerating fields here means a second place to update whenever engine config grows, and the failure mode when it is missed is a hard reject of a config the engine runs happily. Open the subtree instead. The variants are discriminated by `required`, not by `additionalProperties`, so this does not weaken them: an embedded config still matches only the data_path variant, an external one only the host variant, and a config carrying both is still ambiguous and rejected. A typo in a required key is still caught by `required`. What is given up is catching a typo in an optional key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wdawson
force-pushed
the
wils/engine-config-pool-and-schedules
branch
from
September 1, 2026 18:15
f5db1d3 to
8bc73c5
Compare
wdawson
marked this pull request as ready for review
September 1, 2026 18:17
evantahler
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three lines. The
storagesubtree closed every object withadditionalProperties: false; this opens them.Why not just document the missing fields
That was the first version of this PR, and it was the wrong fix. Enumerating engine config fields here creates a second place to update whenever engine config grows, and the failure mode when it is missed is a hard reject of a config the engine runs happily.
It had already been missed:
target_session_attrshas been read by the engine for a while and was absent, andmax_connswould have been the second instance in a month. Documenting them fixes today's two and leaves the mechanism that produced them.The error was also unhelpful. Because the two postgres variants sit inside a
oneOf, an unknown key fails every branch, so the message lands on the wholestoragenode rather than naming the field:This does not weaken the variants
The embedded and external variants are discriminated by
required, not byadditionalProperties, so opening the objects changes nothing about which one a config matches. Verified with ajv:example.yaml, embedded variant (regression)max_connstarget_session_attrsmax_connsschedulessection of any shapepasswrod)sslmodee)What is given up
Typos in optional keys are no longer caught. Typos in required keys still are, because
requiredcatches them independently — sopasswrod: pstill fails, and only something likesslmodee: disablenow slips through to be ignored by the engine.That is the deliberate trade: the schema stops being a second source of truth that can reject valid config, and in exchange it stops catching one class of typo.
execution_loggingstill setsadditionalProperties: falseand is the next likely instance of the same problem. Left alone here rather than widened speculatively.Context
Consumer side is ArcadeAI/monorepo#3754, which adds
max_connsandschedules.max_concurrent_executionsto the engine.— Drake 🐉, Wils's agent