Conversation
|
|
lahma
changed the title
lahma
changed the title
lahma
marked this pull request as ready for review
July 29, 2026 08:09
Contributor
PR Summary by QodoUpdate Jint 4.0.3 → 4.15.2, rebuild event envelope, add validation isolation
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Contributor
Code Review by Qodo
1. Missing EnumConversionMode import
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| _bodyRaw = @event.Data; | ||
| _metadataRaw = @event.Metadata; | ||
| _linkMetadataRaw = @event.PositionMetadata; | ||
| EventType = @event.EventType ?? ""; |
Contributor
There was a problem hiding this comment.
1. eventtype falls back to empty 📘 Rule violation ≡ Correctness
EventEnvelope silently substitutes an empty string when @event.EventType is null, which can mask malformed events and route handler selection incorrectly. Prefer making null explicit (nullable type + handling) or fail fast with a throw if EventType is required.
Agent Prompt
## Issue description
`EventEnvelope` sets `EventType = @event.EventType ?? ""`, silently masking null values. This conflicts with the compliance requirement to avoid silent fallback defaults for required values.
## Issue Context
If `ResolvedEvent.EventType` can be null, the type should reflect that (`string?`) and callers should handle it explicitly. If it should never be null, the code should fail fast (e.g., `?? throw new InvalidOperationException(...)`) and use the same non-null value when populating the JS `eventType` property.
## Fix Focus Areas
- src/KurrentDB.Projections.JavaScript/Services/Interpreted/JintProjectionStateHandler.cs[905-936]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
lahma
marked this pull request as draft
July 29, 2026 09:03
Contributor
Author
|
All three findings addressed. Two were real; the first is a parity question rather than a new decision. 1. I checked whether the coalesce could just be dropped. It cannot: Nor is Both halves of the parity were verified, since the CLR property and the JS-visible property are fed separately: the dispatch key stays 2. Envelope presence semantics changed. Agreed, and fixed with conditional layout variants. The old conditions were reconstructed from the removed code rather than assumed, and they are narrower than they look.
One deviation is kept deliberately because a fixed layout cannot express it: the old envelope materialized a parsed member as a side effect of the first read of it, so
3. BigInt serialization now throws. Agreed, and reverted to the old behaviour. The blast radius is the point — an already-running projection with a BigInt in its state would fault at serialization and halt checkpointing on upgrade, which is not an acceptable way to discover the change. Option 1 from the suggestion, with the exact old format verified from the removed serializer first ( Two consequences, both documented in the PR description: round-tripped state turns a BigInt into a string, the same gotcha the existing The branch has also been repackaged from eleven commits into three, since most of the history was version churn while upstream fixes landed mid-review. |
lahma
marked this pull request as ready for review
July 29, 2026 09:07
| // reflection + boxing, because a converter must be offered every CLR value before it becomes a | ||
| // JsValue, so one converter for one enum cost the compiled member-read lane for every property | ||
| // and field read on every wrapped object in the engine. | ||
| options.Interop.EnumConversion = EnumConversionMode.String; |
Contributor
There was a problem hiding this comment.
1. Missing enumconversionmode import 🐞 Bug ≡ Correctness
JintEngineFactory.CreateEngine assigns `options.Interop.EnumConversion = EnumConversionMode.String` but the file no longer imports the namespace that provides EnumConversionMode, causing a compile error (e.g., `CS0103: The name 'EnumConversionMode' does not exist in the current context`). This breaks builds for any project referencing KurrentDB.Scripting.
Agent Prompt
## Issue description
`src/KurrentDB.Scripting/JintEngineFactory.cs` uses `EnumConversionMode.String` but does not import/qualify `EnumConversionMode`, which is defined in Jint's interop namespace. This introduces a deterministic compile-time failure.
## Issue Context
A prior `using Jint.Runtime.Interop;` was removed in this file, but the code still references `EnumConversionMode`.
## Fix
Choose one:
1. Re-add the import:
- `using Jint.Runtime.Interop;`
2. Or fully qualify the enum:
- `options.Interop.EnumConversion = Jint.Runtime.Interop.EnumConversionMode.String;`
## Fix Focus Areas
- src/KurrentDB.Scripting/JintEngineFactory.cs[4-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit addf0e9 |
This was referenced Jul 29, 2026
Merged
…interop defaults 4.0.3 was released 2024-09-23; this crosses seventeen releases. Every Jint API this repository touches is present and signature-identical at 4.15.3, so the bump itself is mechanical. What follows are the behaviour deltas that were audited against our usage rather than assumed. Two interop defaults changed in 4.14.0, and both are inherited deliberately: * Options.Interop.ArrayConversion now defaults to LiveView instead of Copy, so a CLR T[] crossing into script becomes a live fixed-size view (Array.isArray false, push/pop/length writes throw TypeError). No CLR array crosses the boundary anywhere in our own usage: the projections engine hands its handlers nothing but JsValues, JsRecord -- the only type we project with FromObjectWithType -- has no array member at any level, and the one SetValue call passes `new object()`. That used to be a claim backed by reading every call site. 4.15.1 counts the conversions per engine, so InteropArrayConversionAuditTests asserts it instead: mapping a record and running a filter and a field selector over it, including a selector that walks a JSON array in the payload, converts no CLR array under either mode. The third case is a positive control -- without it the other two would pass equally well if the counters never incremented, which is the failure mode a diagnostic-backed assertion invites. Scope, stated honestly: this covers the scripting engines, because JsRecord is the only object graph we project and therefore the only place an array could appear. The counters also exclude an array crossing under a non-array declared type (IReadOnlyList<T>), which honours that contract through the ordinary wrapper lane -- so it asserts "no array conversion", not "no array-shaped value". * Options.Interop.CacheRecentObjectWrappers now defaults to true. JsRecordEvaluator already creates exactly one wrapper and mutates the target behind it, so the cache changes nothing there. It does help the JsonNode dictionary lane, which previously allocated a fresh ObjectWrapper at every level on every access; the ring is bounded, Engine.Dispose() releases it, and no filter or selector mutates the record, so stable wrapper identity is not observable to them. Also audited and not applicable to us: Error.prototype.stack became an accessor (4.9.3), Function.prototype.toString stopped returning source text (4.11.0 -- JsFunctionValidator reads FunctionDeclaration, not toString), static CLR members left instance wrappers (4.1.0), writes to read-only CLR members became a strict-mode TypeError (4.15.0 -- every projected member has a setter), and TimeoutInterval now replaces instead of accumulating (we call it once). Two deltas do reach us and are behaviour changes rather than bugs: JSON parsing was aligned with the JSON grammar in 4.14.0, so `-09` and `1.` are now rejected as they are in V8 while raw U+2028/U+2029 in strings are now accepted; and Options.Culture stopped driving toLocaleString/localeCompare in 4.6.0, which now route through Intl. The scripting engines additionally drop a hand-written IObjectConverter in favour of Options.Interop.EnumConversion. The converter existed so that record.schema.format read as "Json" rather than 1, but a registered converter must be offered every CLR value before it becomes a JsValue, so its presence disabled the compiled member-read lane for every wrapped member in the engine -- record.id, record.sequence, record.timestamp and the rest all fell back to reflection and boxing. EnumConversionMode.String produces the member name, falling back to the numeric value for a value with no name, which is exactly what `Enum.GetName(...) ?? e.ToString()` computed. JsSchemaFormat is the only enum in the model and all its values are named, so the rendering is unchanged. 4.15.3's Options.AddImmutableCrossing was assessed here and declined. It memoizes reads through a wrapper whose target the host promises not to mutate, which is exactly the redundancy this path has -- a four-field index re-walks record.value.customer.country once per selector. But the root of every such walk is JsRecord, which Remap mutates in place for every event, along with its Schema and Position children, so the promise is false at the root and the memo could only be rooted one level in, at the per-event JsonNode documents. Those are reachable and writable from user-supplied filter scripts, and a wrong promise here would surface as a wrong value written to a DuckDB index column and served to queries -- silent and persisted. The redundancy is better removed than memoized: fusing the field selectors into a single call eliminates the repeated walks outright, and parsing the payload into native JS values removes the wrapper from the path altogether. Blast radius for reviewers: Kurrent.Surge.Core carries its own transitive Jint 4.0.3 reference and builds its own engines for the connector record transformer and SQL reducer. Central package management lifts it to 4.15.3 without recompilation, and because it predates the option it cannot have pinned ArrayConversion. The audit test cannot reach those engines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e with Jint Three changes to the projection engine that only make sense together, since they all concern the per-event path: how the envelope is built, how state is serialized, and how often the execution constraint is checked. ## The envelope EventEnvelope was a custom ObjectInstance subclass. A host subclass carries none of the engine's storage flags, so it reaches no member-read inline cache at all: every event.streamId in a handler was a virtual call into a property dictionary, on an object rebuilt for every event. It is now a plain JsObject built from a shared JsObjectLayout, so envelopes share a hidden class and a handler's reads stay monomorphic across events even though the object itself is new each time. This needed Jint 4.15.1. A layout's slots held values, so a member that must parse a JSON document could only be a raw descriptor, which deopts the object to the dictionary representation and forfeits the point. AddLazy declares a slot whose factory runs on the first read that observes its value, so body, data, metadata and linkMetadata stay lazy on an object that is hidden-class throughout. The two names for one document share a memo, so a handler reading both parses once. There are two layout variants because the previous key set was not fixed, and projections may branch on which members exist. Reconstructing the old conditions exactly: metadataRaw and linkMetadataRaw were assigned unconditionally, and a null string converts to JS null rather than undefined, so metadata and linkMetadata were always present -- carrying null when their document was absent, which is distinct from being absent. EnsureBody additionally required IsJson, and body and data always appeared together sharing one descriptor. IsJson is therefore the only condition, and two layouts cover it exactly. Each variant is still one shared hidden class; a projection handling both JSON and non-JSON events goes polymorphic at its read sites, which is the honest cost of the key sets genuinely differing and still far cheaper than reaching no cache at all. The shaping is what the whole change is for, and it is invisible from script -- the representations behave identically and differ only in speed -- so when_shaping_the_event_envelope asserts it through Engine.Advanced.HasSharedShape rather than leaving it as a one-time observation. JsObject.Create falls back to the property dictionary quietly and correctly when it cannot shape an object, and two of the triggers depend on what the engine has already built rather than on anything visible at the call site, so the assertion is worth having. It needs the envelope type visible to the test assembly; InternalsVisibleTo to a test project is the convention already used by KurrentDB.Core, Api.V2 and Connectors. One deviation remains and cannot be expressed with a fixed layout: the old envelope materialized a parsed member as a side effect of the first read, so "body" in event answered false before anything had read event.body and true afterwards. The answer is now the same before and after. It describes the event rather than the reading history. Layout slots are configurable/enumerable/writable where the old descriptors were non-writable and non-configurable, so a handler assigning to event.streamId used to silently do nothing and now mutates the envelope -- a per-event object discarded when the handler returns. Object.freeze would restore the old attributes but drops the object to the dictionary representation, undoing the reason for the change. event.eventType is no longer round-tripped out of the JavaScript property table to key the CLR handler dictionary; it is read off the record, as are isJson and bodyRaw. ResolvedEvent.EventType is null when the resolved event carries no event record, and its declaring project has nullable reference types disabled, so the string annotation says nothing. The removed CLR getter coalesced to "" through AsString(...) ?? "", so "" is the dispatch key an untyped event has always produced; the coalesce is kept to preserve that rather than to decide something new, and the JS-visible property is still fed the raw value, so it stays null there exactly as before. ## State serialization Deletes ~215 lines of hand-written JSON and routes state, emitted event bodies and log output through Jint.Native.Json.JsonSerializer -- the implementation behind JSON.stringify, which has been publicly reachable all along. Compatibility was not assumed: when_serializing_state already asserted that our output was byte-identical to Jint's built-in serializer for every object, array, value, string and number case it covers plus the big_state.json production shape, and those assertions pass unchanged. Two deviations the old serializer had are preserved deliberately. Top-level undefined, and now also functions, serialize as "null" rather than as "no JSON representation", because every caller treats the result as a JSON document. And a BigInt still serializes as a JSON string of its digits: JSON.stringify has no representation for one and throws, which for a projection already running with a BigInt in its state would mean faulting at serialization and halting checkpointing on upgrade. It is restored through BigInt.prototype.toJSON, which JSON.stringify consults before deciding a value has no representation, and which is installed only on the projection engines -- the scripting engines never ran the string-writing serializer and have nothing to preserve. Deliberately not a replacer passed to Serialize: a replacer is invoked for every node of every state document on the per-event path, whereas toJSON costs nothing until a BigInt is actually present. Two consequences: round-tripped state turns a BigInt into a string, the same gotcha the "created" comment describes for dates; and a projection's own JSON.stringify(123n) now succeeds where it used to throw, which is strictly more lenient and confined to projection engines. Everything else the swap changes is a correctness gain: a cycle raises a TypeError instead of running off the end of a fixed 64-entry stack with an IndexOutOfRangeException, state nested deeper than 64 levels serializes at all, toJSON is honoured, and non-enumerable and symbol-keyed own properties are excluded per spec rather than emitted or throwing. The direction of the data decided which overload to adopt. 4.15.0 added Serialize(JsValue, IBufferWriter<byte>) for callers that emit UTF-8, but every consumer downstream -- PartitionState, EmittedDataEvent -- wants a string, and PartitionState even re-parses it with Newtonsoft. The old code produced UTF-8 and transcoded it straight back, so the string overload is the fit: it removes an encode and a decode per event, and the 1 MB ArrayBufferWriter each handler allocated up front (one per partition worker under the V2 engine) goes with it. ## The execution constraint Jint splits registered constraints into an "exact" partition checked before every statement and an "amortized" one checked every N. Before 4.15.0 the split was a type test against Jint's own sealed constraints, so a user-derived Constraint could only be exact. TimeConstraint qualifies as amortizable: Check() reads a wall clock and nothing else -- it does not count its own invocations, and it does not budget a quantity that can grow without bound between two checks, the two cases where amortizing would change what is measured. Being exact cost a virtual Check() with two TimeSpan materializations per statement and, since 4.13.0, also disarmed the interpreter's tight-loop lanes, so any projection folding over an array paid full per-statement bookkeeping. Only detection latency changes, and it stays bounded because Jint re-checks whenever control returns from host code. The ten explicit Constraints.Reset() calls were reviewed at the same time and deliberately kept: Engine.Call/Execute do reset around themselves, but GetSourceDefinition() runs no script at all, and Load/LoadShared call JsonParser.Parse, which checks constraints from inside its own loops without going through ExecuteWithConstraints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two places compile user-supplied JavaScript on a static engine that lives as long as the process: workspace filter rules, and index function validation. Both take a lock, because a Jint engine is not thread safe, and neither resets anything afterwards. Evaluating an expression can define globals. A filter is validated as (<expr>), and a comma expression both defines a global and produces the function that passes validation -- (g = 1, r => true). On these engines that global then outlives the request, is visible to every later validation, and is never collected. Jint 4.15.0 added a global snapshot for exactly this reuse pattern: capture the configured global surface once, restore it after each use. JsValidationEngine pairs it with the lock the two sites already had. The restore itself is Jint's own WithRestoredGlobals, added in 4.15.3 -- the helper is precisely the try/finally this had hand-written, and the reason it exists is that getting it wrong is easy and silent: a restore outside the lock, or not in a finally, means the validation that throws is the one that leaves state behind. The lock stays ours, because an engine is single-threaded and that helper is a finally, not a sandbox. Each site keeps its own engine configuration; only the lifecycle is shared. It is worth being precise about what this is: Jint documents the snapshot as a configuration-reuse primitive, not an isolation boundary. It reverses global bindings, not mutations of built-in prototypes, so it bounds accumulation rather than making hostile input safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Jint has verifiers for the contracts a host makes when it implements one of the engine's extension points -- that TryGetOwnPropertyValue agrees with GetOwnProperty, that a type claiming ordinary access semantics really has them, that ProbeOwnProperty agrees with the descriptor. They exist because the engine trusts those hooks on its hot paths and cannot afford to re-verify them there, so a violation is otherwise silent: a key disappears from every enumeration, or a read resolves on the prototype for a property that exists. Until 4.15.3 they were compiled out of the shipped package, so reaching them meant cloning Jint and building it in Debug -- which is exactly what this branch had to do earlier, by hand, dropping a locally built Jint.dll over the one in a test output directory. 4.15.3 gates them on an AppContext switch instead, so the Release package on NuGet can run them. A module initializer per test assembly is what makes it stick: the flag behind the switch is read once at type initialization, so it has to be set before the first use of any Jint type, and a module initializer runs before any test in the assembly. Production is unaffected -- with the switch unset the guards fold away entirely, which is why it is set here and nowhere else. Each assembly gets a positive control alongside it, a host object that claims every name as its own while GetOwnProperty reports them all absent. Without one, both suites would report exactly the same green whether the verifiers were running or silently inert, and a switch that must be set before first use is easy to render inert by accident. In Release the verifiers throw rather than writing a diagnostic, so a violation is a visible failure. The projections suite is where this currently has something to check: the definition-phase DSL object is a host ObjectInstance subclass and reads against it are verified. The scripting suite has no host subclass today, so there it guards a future one -- which is the case where the positive control earns its keep most clearly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lahma
changed the title
lahma
marked this pull request as draft
July 29, 2026 11:25
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.
Updates Jint from 4.0.3 (released 2024-09-23) to 4.15.3, and adopts the parts of the newer embedding surface that fit what this repository already does. Draft, because a few commits are judgement calls the maintainers should make rather than inherit, and each is flagged below.
Qodo's bot review findings are addressed — see the section at the end.
What changed
Three commits, one per area of the codebase, each independently reviewable:
EnumConversionModereplacing the oneIObjectConverter, and the array-conversion audit testJsonSerializerreplacing ~215 hand-written lines, and the amortizable execution constraintJsValidationEnginefor the two process-lifetime validation enginesAppContextswitch per test assembly, each with a positive controlNet: +770 / −461 across 16 files.
The second commit groups three changes because they share the per-event path and were measured together; the amortizable constraint sits with it rather than with the bump because it lives in the projection handler. The fourth is separate because it is test-harness policy spanning two assemblies and belongs to none of the other three.
The headline: the envelope is now a shaped object
EventEnvelopewas a customObjectInstancesubclass. A host subclass carries none of the engine's storage flags, so it reaches no member-read inline cache at all: everyevent.streamIdin a handler was a virtual call into a property dictionary, on an object rebuilt for every event.It is now a plain
JsObjectbuilt from a staticJsObjectLayout, so every envelope in an engine shares one hidden class and a handler's reads stay monomorphic across events even though the object itself is new each time.This was not expressible when the branch started. A layout's slots held values, so a member that must parse a JSON document could only be a raw descriptor — which deopts the object to the dictionary representation and forfeits the point. That is exactly why the envelope was a subclass, and the branch first worked around it with
TryGetOwnPropertyValueon a still-subclassed envelope. Jint 4.15.1'sJsObjectLayout.Builder.AddLazydeclares a slot whose factory runs on the first read that observes its value, sobody,data,metadataandlinkMetadatastay lazy on an object that is hidden-class throughout. The two names for the same document share one memo, so a handler reading both parses once.Consequences worth reviewing:
'body' in eventandObject.keys(event)answer for them without parsing anything — only reading a value parses. The key set still matches the old envelope exactly:body/dataappear together and only for JSON events,metadata/linkMetadataalways appear and carrynullwhen their document is absent. That needs two layout variants; see the review findings below.when_enumerating_the_event_envelopepins it.event.streamIdused to silently do nothing and now mutates the envelope — a per-event object discarded when the handler returns.Object.freezewould restore the old attributes exactly, but it drops the object to the dictionary representation and would undo the reason for the change, so this is a deliberate trade rather than an oversight. This is the change most worth a second opinion.Two things fall out.
TryGetOwnPropertyValueis gone — it existed to make a host subclass cheap to read, and there is no longer a host subclass on this path. Andevent.eventTypeis no longer round-tripped out of the JavaScript property table to key the CLR handler dictionary; it is read straight off the record, as areisJsonandbodyRaw.Why the version moved twice mid-branch
Adopting an API surface is a good way to find out where it does not fit, and the gaps this branch hit were reported upstream rather than worked around permanently. Seven of them shipped while the branch was open, which is why the pin moved from 4.15.0 to 4.15.3:
JsObjectLayout.Builder.AddLazy— 4.15.1LiveViewaudit was proseGetInteropConversionDiagnostics— 4.15.1PropertyFlaghad no name for the combinations a host buildsnull, but the values span's element type was non-nullable, forcingnull!ReadOnlySpan<JsValue?>— 4.15.2AppContextswitch, live in the shipped Release package — 4.15.3Engine.Advanced.WithRestoredGlobals— 4.15.3Engine.Advanced.HasSharedShape— 4.15.3Two of these close loops this branch opened by name. The
null!one landed the same week it was reported and removed four null-forgiving operators from the envelope. The verifier one is larger: this PR earlier had to clone Jint, build it in Debug and hand-copyJint.dllover the one in a test output directory to reach the checks that guard host contracts — the report said that made the documented checker unreachable for anyone who was not already a Jint contributor. 4.15.3 gates them onAppContext.SetSwitch("Jint.EnableHostContractVerification", true)instead, so the shipped Release package runs them; both JavaScript test assemblies now enable it in a module initializer, each with a positive control proving the switch actually took effect. One further gap remains open by choice rather than oversight:Engine.Advanced.GetPropertyAccessSemanticsalso shipped, but by then the type it would have protected had stopped being a host type — see the verdicts below.The branch went through an intermediate shape worth knowing about when reading the diff: on 4.15.0 the envelope stayed a host subclass and used
TryGetOwnPropertyValueto make its reads cheap. That was verified against a Debug build of Jint, whose verifier checks the hook's contract on every read — it passed, and the verifier was confirmed live by deliberately breaking the hook and watching the run fail.AddLazythen made the subclass unnecessary, so the hook is gone and none of it survives in the final diff.The 4.14.0 interop defaults
4.14.0 changed two defaults. Both were audited against our usage and both are inherited deliberately rather than pinned back.
Options.Interop.ArrayConversion:Copy→LiveView. A CLRT[]crossing into script is now a live fixed-size view:Array.isArrayreturnsfalse(whileinstanceof Arraystaystrue), andpush/pop/lengthwrites throwTypeError.Originally this was answered by reading every call site. It is now asserted (
InteropArrayConversionAuditTests): 4.15.1 counts array conversions per engine, and mapping a record then running a filter and a field selector over it — including a selector that walks a JSON array in the payload — converts no CLR array under either mode. A third case is a positive control, so the assertion cannot pass by the counters simply never incrementing.Scope, stated honestly: the test covers the scripting engines, because
JsRecordis the only object graph this repository projects into script and therefore the only place a CLR array could appear. The projections engine hands its handlers nothing butJsValues. And the counters deliberately exclude an array crossing under a non-array declared type (IReadOnlyList<T>), which honours that contract through the ordinary wrapper lane — so this asserts "no array conversion", not "no array-shaped value".Options.Interop.CacheRecentObjectWrappers:false→true.JsRecordEvaluatoralready creates exactly one wrapper and mutates the target behind it, so the cache is a no-op there. Where it does bite is theJsonNodedictionary lane, where a freshObjectWrapperwas previously allocated at every level on every access — an improvement, not a hazard: the ring is bounded,Engine.Dispose()releases it, and no filter or selector mutates the record.Other deltas across the 17 releases
Audited and not applicable:
Error.prototype.stackbecame an accessor (4.9.3 — we never read it);Function.prototype.toString()stopped returning source text (4.11.0 —JsFunctionValidatorreadsFunctionDeclaration); static CLR members left instance wrappers (4.1.0 —JsRecordhas none); writes to read-only CLR members became a strict-modeTypeError(4.15.0 — every projected member has a setter);TimeoutIntervalnow replaces instead of accumulating (we call it once); saturated constraint sentinels register nothing (we use none).Audited and applicable — behaviour changes, not bugs:
-09and1.are now rejected as in V8, while raw U+2028/U+2029 in strings and escaped control characters in keys are now accepted. Persisted state or event bodies containing those malformed number forms parsed before and now raiseSyntaxError.Options.Culture(4.6.0) stopped drivingtoLocaleString/localeCompare; they route throughIntl, andInvariantCultureresolves to the Intl locale"en". No script in our tests uses them.Blast radius outside this repository
Kurrent.Surge.Corecarries its own transitive Jint 4.0.3 reference and builds its own engines forJintRecordTransformerandJintSqlReducer. Central package management lifts it to 4.15.3 without recompilation, and because it predates the option it cannot have pinnedArrayConversion. The audit test above cannot reach those engines — it asserts for the ones this repository constructs. Worth a look from whoever owns Surge.Per-feature verdicts
Adopted
Constraint.IsAmortizableTimeConstraint.Check()reads a wall clock and nothing else — it neither counts its own invocations nor budgets a quantity that can grow unboundedly between checks. Being in the exact partition cost a virtualCheck()per statement and, since 4.13.0, disarmed the interpreter's tight-loop lanes.Options.Interop.EnumConversion = StringIObjectConverterwhose mere presence disabled the compiled member-read lane for every wrapped member in the engine.JsonSerializer(string overload)JsObjectLayout+AddLazyCapture/RestoreGlobalSnapshotEngine.Advanced.GetInteropConversionDiagnosticsLiveViewaudit into an assertion.PropertyFlagnamed combinationsAllForbiddenforlog,NonEnumerablefor the functions a projection calls.The ten explicit
Constraints.Reset()calls were reviewed and kept:Engine.Call/Executereset around themselves, butGetSourceDefinition()runs no script andLoad/LoadSharedcallJsonParser.Parse, which checks constraints from inside its own loops without going throughExecuteWithConstraints.Skipped, with reasons
JsonSerializer.Serialize(IBufferWriter<byte>)string;PartitionStateeven re-parses it with Newtonsoft. The old code produced UTF-8 and immediately transcoded back, so the string overload is the fit.JsonParser.Parse(ReadOnlySpan<byte>)ResolvedEventdecodes eagerly into a publicstringfield used all over V1, andbodyRawis script-visible. InKurrentDB.Scriptingit is a real opportunity —JsRecord.Value/Propertiesdeserializeevt.Data.Spaninto aJsonNodetree that then crosses as anObjectWrapper— but that is a model change with its own script-visible surface.Engine.Advanced.GetPropertyAccessSemanticsExotic↔Ordinarywhen aGetoverride moves. The envelope is now an in-boxJsObject, for which Jint documents the answer as an internal classification that may be refined in any release. The only remaining host subclass,InterpreterRuntime, is definition-phase only and not reachable from a test project.TryGetOwnPropertyValueCaptureGlobalSnapshotfor projection handlers_state, the installed globals. Nothing to reset between events.AddObjectConverter(converter, params Type[])EnumConversionModeremoves the converter outright.Options.AddLazyGlobalemit/linkTo/linkStreamTo/copyToare installed afterExecute(source)precisely so the definition phase cannot see them.Prepared<Script>/ReferencedGlobalsPrepareScriptis used nowhere, so the V2 partition fan-out reparses per partition and on every restart. Real win, own change.ArrayLikeObject,JsObjectShapeJsObjectShapedescribes singleton prototypes, not per-item records.NullPropagatingReferenceResolverOptionsinline with a freshTimeConstraint.Options.AddImmutableCrossing(4.15.3)PropertyDescriptor.CreateLazy(4.15.3)ObjectInstancesubclass; our lazy members are layout slots, and there is no host subclass left on this path.Serializer compatibility
when_serializing_statealready asserted that our output was byte-identical to Jint's built-in serializer — for every object, array, value, string and number case it covers, plus thebig_state.jsonproduction fixture. Those assertions pass unchanged after the swap. That is the compatibility oracle, and this repository wrote it.Two deliberate deviations were pinned by their own tests:
undefined. The old serializer wrote"null"; Jint reports "no JSON representation". Kept as"null", because every caller treats the result as a JSON document. This now also covers functions, which the old serializer rendered as an object of their non-enumerable members.JSON.stringifyhas no representation for a BigInt and throws, so the projection engines installBigInt.prototype.toJSONto restore the old rendering byte for byte. See the review findings below for why, and for the two consequences.Everything else the swap changes is a correctness gain: a cycle raises a
TypeErrorinstead of running off a fixed 64-entry stack withIndexOutOfRangeException; state nested deeper than 64 levels serializes at all;toJSONis honoured; non-enumerable and symbol-keyed own properties are excluded per spec rather than emitted (or, for symbols, throwing).Measurements
Projection event processing, end to end. One operation = 100
ProcessEventcalls, each building the event envelope, invoking a handler that reads several members, and serializing the resulting state — the loop a running projection actually executes.Read the delta for what it is: it spans fifteen Jint releases plus this migration, measured from the merge base to the tip of this branch. It is not the effect of any one commit; it is what an upgrader gets.
BenchmarkDotNet v0.15.2, default job,
[MemoryDiagnoser], .NET 10.0.10, AMD Ryzen 9 5950X, quiet machine, both sides run serially from an identical harness compiled against each.The pin has since moved to 4.15.3; the table is left as measured rather than re-run, since nothing in 4.15.2 or 4.15.3 touches these paths beyond a nullable annotation, a probe-lane fast path, and additions this PR adopts only in tests.
StdDev was under 1.3% of the mean on every row and one outlier was removed per row; no row was multimodal, so no re-pairing was needed. The body-parse row improves less in time because the JSON parse it adds is work neither version avoids — the envelope and serialization around it are what got cheaper. Gen1 collections also disappear: the before side promoted on both rows, the after side reports none.
The allocation figures were independently confirmed by a deterministic probe (
GC.GetAllocatedBytesForCurrentThreadover the same harness, byte-identical across repeated runs on both sides): 380,713 → 183,913 B/op and 549,898 → 270,778 B/op, agreeing with BenchmarkDotNet at ≈−51%. That is ≈1,968 B and ≈2,791 B saved per event.Allocation, state serialization in isolation (
big_state.json, 210,509 chars, 500 iterations, deterministic probe):Steady state only; excludes the 1 MB
ArrayBufferWriterthe old serializer allocated once per handler — i.e. per partition worker under the V2 engine.The benchmark harness itself is deliberately not part of this PR: it has to compile unchanged against both sides to be a valid A/B, which makes it scaffolding rather than something the repository should carry.
Gates
Full solution build,
dotnet build -c Release KurrentDB.slnx: 0 errors, and the only warnings are pre-existing (GitInfo not finding a fork point for a branch named offmaster, and unused protobuf imports).KurrentDB.Projections.Management.TestsKurrentDB.Projections.JavaScript.TestsKurrentDB.Projections.V2.TestsKurrentDB.Kontext.TestsKurrentDB.SecondaryIndexing.TestsBoth JavaScript test assemblies now run with Jint's host-contract verifiers enabled, so those totals are green with the checks on.
Not run locally: anything requiring Docker or a live cluster.
While the envelope was still a host subclass, the projections suites were additionally run against a Debug build of Jint, whose verifier checks the
TryGetOwnPropertyValuecontract on every read; they passed, and the verifier was confirmed live by deliberately breaking the hook and watching the run fail. That is no longer needed — the envelope is an in-boxJsObjectwith no host-implemented read contract to verify. What replaced it as the check iswhen_enumerating_the_event_envelope, which pins each layout variant's observable key set and order.Why
AddImmutableCrossingwas declined4.15.3 added it, and this branch's own friction report is where it came from — the walk
record.value.customer.countryre-wrapping every node on every access. Having asked for it, the honest answer on inspection is that it does not fit here.The memo is rooted on a wrapper, and the root of every such walk is
JsRecord.Remapmutates that object in place for each event — along with itsSchemaandPositionchildren — so the promise is simply false at the root and the type cannot be declared. The memo could only be rooted one level in, at the per-eventJsonNodedocuments, which genuinely are built fresh per event and never mutated by us.That is where it stops being worth it. Those documents are reachable and writable from user-supplied filter scripts, and while Jint evicts a key's memo on a write through the wrapper, a wrong promise on this path would surface as a wrong value written to a DuckDB index column and served to queries — silent and persisted. A stale read in an indexing path is a worse outcome than a missed optimization, and nothing here needs the win badly enough to take that trade.
The redundancy it would paper over is also better removed than memoized: a four-field index makes one JS call per field, each re-walking the record from scratch, so fusing the selectors into a single call eliminates the repeated walks outright. And the deeper fix is the one the envelope rework already applies on the projections side — parsing the payload into native JS values, so there is no wrapper in the path at all. The friction turned out to want that, not a memo.
Review findings addressed
Qodo's bot review raised three. All three are resolved; two were real.
1.
EventTypefalls back to""(rule violation). Kept, and made explicit.ResolvedEvent.EventTypereally is null whenever the resolved event carries no event record, and its declaring project has nullable reference types disabled, so thestringannotation asserts nothing — dropping the coalesce would not have been safe. The removed CLR getter read the value back out of the property table throughAsString(...) ?? "", so""is the dispatch key an untyped event has always produced. Making it throw instead would be a real behaviour change on a path that previously dispatched, which is worse than the style violation. Both halves of the parity were checked: the CLR dispatch key stays"", and the JS-visibleevent.eventTypeis still fed the raw value, so it staysnullthere exactly as before. A comment at the assignment now states this.2. Envelope presence semantics changed (real — fixed). The fixed layout made
body/dataexist on non-JSON events, and a projection distinguishing them with('body' in event)would have silently changed meaning. The old conditions were reconstructed from the removed code rather than guessed:metadataRawandlinkMetadataRawwere assigned unconditionally and a null string converts to JSnull, notundefined, sometadataandlinkMetadatawere always present carryingnullwhen absent; onlyEnsureBodyadditionally requiredIsJson, andbody/dataalways appeared together.IsJsonis therefore the sole condition, so two layout variants reproduce the old key sets exactly — not the eight a general treatment would need. Each variant is still one shared hidden class, so the cache win survives; a projection handling both kinds of event goes polymorphic at its read sites, which is the honest cost of the key sets genuinely differing.One deviation is kept deliberately because a fixed layout cannot express it: the old envelope materialized a parsed member as a side effect of the first read, so
'body' in eventanswered false before anything had readevent.bodyand true afterwards. The answer is now the same before and after — it describes the event, not the reading history.when_enumerating_the_event_envelopenow pins the key set and order per variant, covering a JSON event, a non-JSON event, and an event with no metadata documents.3. BigInt serialization now throws (real — fixed). The blast radius is the point: an already-running projection with a BigInt in its state would fault at serialization and halt checkpointing on upgrade. The old output format was verified from the removed serializer (
WriteStringValue(value.ToString())— quoted decimal digits) and is restored exactly, throughBigInt.prototype.toJSON, whichJSON.stringifyconsults before deciding a value has no representation. Installed on the projection engines only; the scripting engines never ran that serializer. Not a replacer passed toSerialize, because a replacer is invoked for every node of every state document on the per-event path whereastoJSONcosts nothing until a BigInt is present. Two consequences: round-tripped state turns a BigInt into a string, the same gotcha thecreatedcomment describes for dates; and a projection's ownJSON.stringify(123n)now succeeds where it used to throw — strictly more lenient, and confined to projection engines. The test pins the string output again.Adjacent findings, not changed here
Noticed while auditing; none is in this PR.
JsFunctionValidatoruses a barenew Engine()— no timeout, noDisableStringCompilation— toEvaluateuser-supplied text, so(function(){}) + (function(){while(1){}})()hangs the caller.JintEngineFactory.CreateEngine()would fix it, but it is alsoStrict(), which could reject validators accepted today.LinkStreamTohas an emptyif (parameters.Length == 3) { }, then readsparameters.At(4)inside a secondif (parameters.Length == 3).Atreturnsundefinedout of range andUndefined.AsObject()throws, solinkStreamTo(a, b, meta)always throws.LoadCurrentSharedStateassigns_state = jsValuein its non-bi-state branch, overwriting the main state with the shared state.LinkTouses|instead of||in aTryGetValuechain.LimitMemory,MaxStatementsorLimitRecursion. Untrusted projection JavaScript is bounded by wall clock alone.partitionBythat readse.bodyparses the body twice.UserIndexProcessor.TryExtractFieldValuesmakes one JS call per field, each re-walkingrecord.value.…: a 4-field index runs 5 JS calls per event.JsRecord.Remapallocates two closures per record capturingevtandoptionseven when unused, plus aGuid.ToStringand a reflectiveEnum.Parse.