Skip to content

.pr_agent_accepted_suggestions

Jump to bottom
qodo-merge-bot edited this page Apr 30, 2026 · 37 revisions
                     PR 5608 (2026-04-30)                    
[correctness] Misleading date-range example
Misleading date-range example The date-range example computes `append_time` via `epoch_ms(created_at)` even though `created_at` is documented as `INT64`, then filters using string dates on the computed alias, which is inconsistent and likely to produce errors or surprising results when run.

Issue description

The date-range query example is inconsistent with the documented type of created_at (INT64) and mixes an epoch conversion with date-string filtering on a derived alias.

Issue Context

This is an “Examples” code block; readers will copy/paste it. Prefer an example that clearly converts created_at to a timestamp (or filters on created_at directly) and uses standard SQL comments.

Fix Focus Areas

  • docs/server/features/queries/flightsql.md[33-53]
[correctness] Invalid JSON operator usage
Invalid JSON operator usage The complex example uses `data::json->>'Address'->>'Country'`, which applies a JSON operator (`->>`) to the text result of a prior `->>`, and the user-defined index examples use `data->>` without casting even though `data` is documented as `VARCHAR`.

Issue description

The examples should consistently cast data to JSON before using ->/->> operators, and the nested JSON access should use correct navigation (e.g., -> for object access then ->> for text extraction).

Issue Context

The same page says data is VARCHAR and must be cast to JSON to use arrow navigation operators.

Fix Focus Areas

  • docs/server/features/queries/flightsql.md[33-63]
  • docs/server/features/queries/flightsql.md[76-90]
[maintainability] Missing 'to' in sentence
Missing 'to' in sentence The Introduction sentence is missing a word (“required to query”), which reduces clarity for readers configuring FlightSQL authentication requirements.

Issue description

The sentence “Authentication ... is required query ...” is missing “to”, making it ungrammatical.

Issue Context

This is user-facing documentation and is likely to be copy/pasted or read quickly; fixing the grammar improves comprehension.

Fix Focus Areas

  • docs/server/features/queries/flightsql.md[18-22]
                     PR 5607 (2026-04-29)                    
[correctness] Invalid OAuth key syntax
Invalid OAuth key syntax The new OAuth setting is documented as `OAuth::DisableCodeChallengeMethodsSupportedValidation`, but the docs’ configuration conventions use YAML nesting (`OAuth:`) and `__` for nesting in env/CLI; `::` is inconsistent and will likely mislead users into setting a non-functional key.

Issue description

The docs introduce a new OAuth setting using OAuth::DisableCodeChallengeMethodsSupportedValidation, but the documentation’s established configuration patterns use YAML nesting (OAuth:) and __ for nesting in env/CLI. The :: syntax is inconsistent and likely wrong.

Issue Context

In the OAuth docs, configuration is shown under an OAuth: section and options are listed as direct property names (e.g., DisableCodeChallengeMethodsSupportedValidation). The configuration guide documents nesting via __.

Fix Focus Areas

  • docs/server/quick-start/whatsnew.md[64-73]
  • docs/server/security/user-authentication.md[366-415]
  • docs/server/configuration/configuration.md[86-98]
                     PR 5606 (2026-04-29)                    
[reliability] Bearer token crashes auth
Bearer token crashes auth BasicHttpAuthenticationProvider now accepts the "Bearer" scheme and then calls Convert.FromBase64String without handling invalid input, so a non-base64 Bearer token can throw and fail the request with an unhandled exception instead of returning 401/Unauthenticated. This exception is not caught by AuthenticationMiddleware (it only suppresses cancellation), so it propagates out of the request pipeline.

Issue description

BasicHttpAuthenticationProvider now treats Authorization: Bearer <token> as a base64-encoded username:password. If <token> is not valid base64 (e.g., JWT/opaque bearer tokens, malformed input), Convert.FromBase64String throws and the request fails with an unhandled exception.

Issue Context

This is newly more likely after the PR because the provider now accepts the Bearer scheme, but still uses a non-Try* base64 decode. Authentication middleware only suppresses cancellation exceptions, so format errors will propagate.

Fix Focus Areas

  • src/KurrentDB.Core/Services/Transport/Http/Authentication/BasicHttpAuthenticationProvider.cs[14-46]
  • src/KurrentDB.Core/Services/Transport/Http/AuthenticationMiddleware.cs[28-37]

Suggested change

  • Update TryDecodeCredential to:
    • return false when value is null/empty
    • catch FormatException (and possibly ArgumentNullException) from base64 decoding, returning false
    • optionally use Convert.TryFromBase64String(...) to avoid exceptions entirely
  • (Optional but recommended) add/extend tests to cover:
    • Authorization: Bearer not_base64 should not throw; it should return false (allowing other providers/anonymous handling)
    • Authorization: Bearer <valid-base64-user:pass> should authenticate equivalently to Basic
[correctness] Handshake response can stall
Handshake response can stall FlightSqlServer.Handshake reads until the request stream completes before writing any response, so clients that send a handshake request but don’t complete/half-close the stream promptly may see the handshake response delayed or never received until cancellation/deadline. This can break or severely slow JDBC tooling that expects a prompt handshake response.

Issue description

FlightSqlServer.Handshake drains the entire request stream before sending response headers and the handshake response. For a bidirectional streaming RPC, some clients may send a request and wait for the server response without completing the stream, which delays the response indefinitely (or until cancellation/deadline).

Issue Context

The PR adds this Handshake override to support JDBC tooling by echoing Basic credentials as a Bearer token in response headers.

Fix Focus Areas

  • src/KurrentDB.SecondaryIndexing/FlightSql/FlightSqlServer.cs[41-61]

Suggested change

  • Read at most the first request message (e.g., a single await requestStream.MoveNext(...)) and then immediately:
    • set response headers (if needed)
    • write the FlightHandshakeResponse
  • If additional client messages are possible, consider ignoring them without blocking the initial response (or document/validate expected client behavior).
                     PR 5600 (2026-04-28)                    
[correctness] Webhook connector not discoverable
Webhook connector not discoverable The PR adds the `Kurrent.Connectors.Webhook` package, but the runtime only creates/validates connectors that are explicitly registered in `ConnectorCatalogue`, which currently contains no Webhook connector types. Any config that sets `ConnectorOptions.InstanceTypeName` to a webhook connector will fail validation and/or throw during connector creation.

Issue description

Kurrent.Connectors.Webhook is added as a dependency, but webhook connectors still cannot be created/validated because connector discovery is hard-coded to ConnectorCatalogue and webhook types aren’t registered there.

Issue Context

Connector creation (SystemConnectorsFactory), validation (SystemConnectorsValidation), and data protection (ConnectorsMasterDataProtector) all depend on ConnectorCatalogue.TryGetConnector(...). If the webhook connector types are meant to be supported, they must be added to the catalogue along with validator and data protector types (and any required entitlements).

Fix Focus Areas

  • src/Connectors/KurrentDB.Connectors/Infrastructure/Connect/Components/Connectors/ConnectorCatalogue.cs[20-58]
  • src/Connectors/KurrentDB.Connectors/Infrastructure/Connect/Components/Connectors/ConnectorDataProtectors.cs[16-99]
  • src/Connectors/KurrentDB.Connectors/Infrastructure/Connect/Components/Connectors/ConnectorsMasterDataProtector.cs[69-76]
  • src/Connectors/KurrentDB.Connectors/Infrastructure/Connect/Components/Connectors/SystemConnectorsValidation.cs[11-22]

Implementation notes

  • If webhook support is intended: add the webhook connector type(s) from Kurrent.Connectors.Webhook into the Items dictionary, pointing to their validator and a new ...ConnectorDataProtector that marks any secrets (tokens, headers, etc.) as sensitive.
  • If webhook support is not intended yet: remove the added PackageReference and central PackageVersion entry to avoid a misleading/unused dependency.
                     PR 5592 (2026-04-23)                    
[observability] Cache size metric lags
Cache size metric lags `PartitionStateCache.Count` is maintained via increments on `Set` and decrements in the async eviction callback, so it can temporarily exceed the true bounded cache size. This value is exposed as `PartitionStateCacheSize` in HTTP stats, potentially misleading operators/alerts expecting it to be ≤ `MaxPartitionStateCacheSize`.

Issue description

The PartitionStateCacheSize surfaced to operators is backed by a lagging application-level counter that can temporarily exceed the configured capacity due to asynchronous eviction callbacks.

Issue Context

Tests already acknowledge eviction is asynchronous and avoid asserting Size <= capacity.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[73-84]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionStateCache.cs[37-69]
  • src/KurrentDB.Projections.V2.Tests/Integration/PartitionStateCacheEvictionTests.cs[194-214]

Suggested fix

Choose one:

  1. If RandomAccessCache exposes an internal count/current size, use that instead of the manual _count for Size.
  2. If not available, adjust the operator surface to avoid implying a hard bound:
    • rename the stat to indicate it’s approximate (and/or document it explicitly in HTTP stats output), or
    • clamp/report min(Count, capacity) plus report capacity separately.
  3. Update test comments/assertions if the metric semantics change.
[maintainability] `ProjectionSubsystemOptions` booleans unnamed
`ProjectionSubsystemOptions` booleans unnamed Several updated `ProjectionSubsystemOptions(...)` call sites pass boolean arguments positionally (`true`/`false`) instead of using named arguments, reducing clarity and increasing risk of misordered parameters. This violates the convention requiring named boolean arguments at call sites where it improves readability.

Issue description

Boolean arguments in ProjectionSubsystemOptions(...) calls are passed positionally, which is unclear and easy to break when adding/reordering parameters.

Issue Context

The PR added a new trailing parameter, so these call sites were touched and are now in-scope to align with the named-boolean-arguments convention.

Fix Focus Areas

  • src/KurrentDB.Projections.JavaScript.Tests/Integration/ProjectionRuntimeScenario.cs[30-30]
  • src/KurrentDB.Projections.Management.Tests/ClientAPI/specification_with_standard_projections_runnning.cs[43-46]
[maintainability] `ExpectedRevision` hardcoded `-2`
`ExpectedRevision` hardcoded `-2` The new integration test hardcodes `ExpectedRevision = -2` ("Any") instead of using a named constant/helper, which obscures intent and embeds protocol-specific magic numbers. This violates the no-magic-numbers convention where a named representation exists.

Issue description

The test uses ExpectedRevision = -2 to mean "Any", which is a magic number.

Issue Context

There is a named representation available in the codebase (AnyStreamRevision.Any.ToInt64()), which preserves intent and avoids encoding protocol constants inline.

Fix Focus Areas

  • src/KurrentDB.Projections.V2.Tests/Integration/PartitionStateCacheEvictionTests.cs[48-52]
[reliability] Drain uses canceled token
Drain uses canceled token Partition processors are intentionally run with `CancellationToken.None` to drain and checkpoint, but the `loadPersistedState` callback uses the engine cancellation token (`ct`), so a cache miss during shutdown can throw `OperationCanceledException` and abort draining/checkpoint completion. Bounded caches increase cache misses, making this shutdown race more likely than before.

Issue description

On shutdown, partitions are designed to keep draining (partitionCt = CancellationToken.None), but persisted state loads still honor the canceled engine token. With bounded caches, a cache miss can happen during drain and cause OperationCanceledException, faulting a partition task and potentially preventing a final checkpoint from completing.

Issue Context

  • Partitions drain to preserve correctness/checkpointing on stop.
  • Cache eviction increases the probability of LoadPartitionState needing to call loadPersistedState during drain.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[116-133]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[59-78]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[276-291]

Suggested fix

  1. Change the loadPersistedState lambda passed to PartitionProcessor to use CancellationToken.None (or the same partitionCt) instead of the engine ct.
  2. Optionally add defensive handling in PartitionProcessor.LoadPartitionState:
    • Catch OperationCanceledException from loadPersistedState and retry once with CancellationToken.None (to preserve the drain semantics).
  3. Add/adjust a test that cancels the engine while partitions are draining and verifies the engine still reaches a final checkpoint without faulting due to persisted-state reads.
[maintainability] `MaxPartitionStateCacheSize` uses `100_000`
`MaxPartitionStateCacheSize` uses `100_000` The new `MaxPartitionStateCacheSize` option and multiple updated call sites hardcode `100_000` rather than using a named constant/default, reducing readability and making future changes error-prone. This violates the no-magic-numbers convention for domain defaults.

Issue description

100_000 is hardcoded as the default/argument for MaxPartitionStateCacheSize, which violates the project convention to avoid magic numbers for domain defaults.

Issue Context

The codebase already uses named defaults like Opts.MaxProjectionStateSizeDefault. The new cache-size default should follow the same pattern so that changing the default is centralized.

Fix Focus Areas

  • src/KurrentDB.Core/Configuration/ClusterVNodeOptions.cs[573-577]
  • src/KurrentDB.Projections.JavaScript.Tests/Integration/ProjectionRuntimeScenario.cs[30-30]
[correctness] Restart test replays with state
Restart test replays with state `evicted_partition_state_is_recovered_from_stream` starts engine2 from `TFPos(0,0)` while also loading the latest persisted partition state from the `-state` stream, which can apply earlier events on top of a later snapshot and yield incorrect state. This does not match the production restart path, where `CoreProjectionV2` starts the engine from the last persisted checkpoint position.

Issue description

PartitionStateCacheEvictionTests.evicted_partition_state_is_recovered_from_stream starts the second engine from TFPos(0,0) even though it also relies on loading the latest persisted partition snapshot from the -state stream. This can cause earlier events (before the snapshot) to be applied on top of the snapshot, producing incorrect state and making the test inconsistent with the real restart flow.

Issue Context

In production, CoreProjectionV2 reads the last checkpoint event and starts the engine from that checkpoint. The -state stream snapshot is implicitly tied to that checkpoint position.

Fix Focus Areas

  • src/KurrentDB.Projections.V2.Tests/Integration/PartitionStateCacheEvictionTests.cs[306-323]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/CoreProjectionV2.cs[187-209]

Suggested fix

  1. Parse the checkpoint position written by engine1 (from the checkpoint stream event payload), and start engine2 from that TFPos.
  2. Alternatively, drive the test via the same harness/path that CoreProjectionV2 uses (read checkpoint then start), rather than constructing ProjectionEngineV2 directly for the recovery scenario.
  3. Keep the assertion that p1 reaches count=2, but ensure engine2 processes only events after the checkpoint that corresponds to the persisted -state snapshot.
                     PR 5587 (2026-04-20)                    
[performance] Decodes dropped event payloads
Decodes dropped event payloads The new event-type filter calls ConvertToProjectionEvent before checking whether the event type is handled, which decodes Data/Metadata to UTF8 strings even for events that are immediately discarded. This adds avoidable CPU/allocation overhead on streams with many filtered-out event types.

Issue description

The event-type filter allocates a ProjectionResolvedEvent (including UTF8-decoding data/metadata) before checking whether the event will be handled.

Issue Context

coreEvent.Event.EventType is already available and used in the same method for system-event checks, so the handled-type check can be done before calling ConvertToProjectionEvent.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[155-177]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[265-280]

Implementation notes

  • Check handledEventTypes.Contains(coreEvent.Event.EventType) before calling ConvertToProjectionEvent(coreEvent).
  • Only construct ProjectionResolvedEvent (and decode payload) when the event will be dispatched (or when it must be inspected for delete handling).
[correctness] Silent fallback for `Events`
Silent fallback for `Events` When `AllEvents` is `false`, `_config.SourceDefinition.Events ?? []` silently treats a missing `Events` list as empty, which can cause the projection to drop all events without surfacing a configuration bug. Required dependencies/config should fail fast (e.g., `?? throw`) instead of using silent fallbacks.

Issue description

RunReadLoop silently falls back to an empty event-type list via _config.SourceDefinition.Events ?? [] when AllEvents == false, which can drop all events without reporting a configuration error.

Issue Context

When a projection declares specific event types (AllEvents == false), Events should be treated as required configuration. Per compliance, missing required config should fail fast.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[148-151]
[correctness] Checkpoint stalls on skipped
Checkpoint stalls on skipped In ProjectionEngineV2.RunReadLoop, when an event type is not declared the code breaks out of the EventReceived case before updating lastLogPosition/bytesProcessed, so checkpoints may not advance past skipped events (especially if the tail contains only skipped types). This can cause repeated re-reading of skipped events after restart and prevent persisting EOF progress for those streams/categories.

Issue description

ProjectionEngineV2.RunReadLoop currently breaks on filtered-out event types before updating lastLogPosition/bytesProcessed. This prevents checkpoint markers from advancing past skipped events (regression vs V1 semantics), especially when the end of the read contains only skipped types.

Issue Context

  • Skipped events are intentionally not dispatched to partitions, but the engine still needs to advance its notion of the last seen TF position so it can checkpoint beyond them.
  • Today lastLogPosition is only updated after DispatchEvent, and the final checkpoint injection only happens when eventsProcessed > 0.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[155-217]

Implementation notes

  • Ensure lastLogPosition advances to logPosition for every EventReceived (even when the event is filtered out).
  • Ensure skipped events contribute to the “unhandled bytes” accounting (so CheckpointUnhandledBytesThreshold can trigger progress).
  • Update the final checkpoint condition to also inject when only skipped/unhandled events were seen since the last checkpoint (e.g., if (eventsProcessed > 0 || bytesProcessed > 0) or if (lastLogPosition != checkpoint)).
                     PR 5562 (2026-03-23)                    
[reliability] V2 engine stop race
V2 engine stop race CoreProjectionV2 cancels and disposes the engine CTS without awaiting the ProjectionEngineV2 task, then clears `_engine`. A rapid Stop/Start can run multiple engines concurrently, causing overlapping checkpoint/emitted writes and inconsistent projection state.

Issue description

StopEngine() cancels the engine but does not wait for it to stop, so restarting can overlap engine instances.

Issue Context

ProjectionEngineV2 performs async draining/checkpointing and waits for partition tasks in its finally; callers must not start a second engine instance until the first has completed.

Fix Focus Areas

  • Store and await the engine run task during Stop/Kill/Suspend (ideally off the projection worker thread if deadlock risk exists).
  • Gate Start so it cannot run until prior engine completion is observed.

Fix Focus Areas (code locations)

  • src/KurrentDB.Projections.V2/Services/Processing/V2/CoreProjectionV2.cs[215-236]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/CoreProjectionV2.cs[278-285]
                     PR 5554 (2026-03-16)                    
[reliability] Stale OTLP options value
Stale OTLP options value `OpenTelemetryLogger.OtlpOptions` is not cleared when `OtlpLogsEnabled()` is false, so if the method is called once with logs enabled and later called with logs disabled, `OtlpOptions` can remain non-null and stale. This violates the property's own documented contract ("null if log export is disabled").

Issue description

OpenTelemetryLogger.OtlpOptions is a static property documented as "null if log export is disabled", but the implementation never clears it on the early-return path when logs export is disabled.

Issue Context

Even if runtime reconfiguration is uncommon, this can cause confusing behavior in the same process (e.g., tests, integration harnesses, or repeated logger initialization) where a stale non-null value remains.

Fix Focus Areas

  • Set OtlpOptions = null; before returning when !configuration.OtlpLogsEnabled().

  • Optionally: set OtlpOptions = null; at the start of the method to ensure a clean slate, then set it when enabled.

  • src/KurrentDB.Logging/OpenTelemetryLogger.cs[21-30]

[security] Leaky public OTLP options
Leaky public OTLP options `OpenTelemetryLogger` exposes the resolved `OtlpExporterOptions` (including `Headers`) via a public static property, making auth headers easily accessible to any in-process code and increasing the chance they get logged/serialized accidentally. This is a new security exposure introduced by the PR.

Issue description

OpenTelemetryLogger.OtlpOptions is a public static reference to the resolved OtlpExporterOptions, which includes Headers (often API keys/tokens). This creates a new in-process secret exposure surface and makes accidental logging/serialization more likely.

Issue Context

The property appears to be introduced primarily to support tests asserting the resolved effective configuration.

Fix Focus Areas

  • Make the property internal (or remove it) and expose only what tests need (e.g., endpoint + protocol, or a sanitized copy with headers removed/masked).

  • If tests need access from another assembly, prefer InternalsVisibleTo for the test project.

  • Avoid returning a mutable reference that other code can modify.

  • src/KurrentDB.Logging/OpenTelemetryLogger.cs[15-57]

  • src/KurrentDB.Common.Tests/OpenTelemetry/OpenTelemetryLoggerTests.cs[1-87]

                     PR 5544 (2026-03-07)                    
[correctness] Engine v2 always faults
Engine v2 always faults EngineVersion=2 is externally selectable, but the selected V2ProjectionProcessingStrategy throws in CreateProcessingPhases, so projection creation will fault immediately in the existing V1 CoreProjection pipeline.

Issue description

EngineVersion=2 can be requested through the management API, but the code path still constructs a v1 CoreProjection which always calls CreateProcessingPhases(). The v2 strategy currently throws, so projections will fault immediately.

Issue Context

This is user-triggerable via gRPC create options, and will lead to immediate projection failures when engine v2 is selected.

Fix Focus Areas

  • src/KurrentDB.Projections.Management/Services/Grpc/ProjectionManagement.Create.cs[60-71]
  • src/KurrentDB.Projections.V1/Services/Processing/Strategies/ProcessingStrategySelector.cs[22-35]
  • src/KurrentDB.Projections.V1/Services/Processing/V2/V2ProjectionProcessingStrategy.cs[50-63]
  • src/KurrentDB.Projections.V1/Services/Processing/CoreProjection.cs[121-132]
[correctness] StateHandler shared concurrently
StateHandler shared concurrently ProjectionEngineV2 passes the same IProjectionStateHandler instance into multiple PartitionProcessor tasks, causing unsafe concurrent access and state corruption (especially for JintProjectionStateHandler).

Issue description

ProjectionEngineV2 starts N partition processors concurrently but passes the same IProjectionStateHandler instance to all of them. Projection state handlers maintain internal mutable state, so concurrent calls will corrupt state and/or crash.

Issue Context

This is especially problematic for JS projections (JintProjectionStateHandler) which encapsulate a single Jint Engine and mutable _state.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[75-89]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2Config.cs[9-18]
  • src/KurrentDB.Projections.JavaScript/Services/Interpreted/JintProjectionStateHandler.cs[35-70]
[correctness] BiState shared state races
BiState shared state races BiState shared state is tracked per PartitionProcessor, so with multiple partitions the shared state will be updated concurrently from stale snapshots and written multiple times, losing updates and producing incorrect shared aggregates.

Issue description

BiState shared state is handled locally per partition processor, but shared state is conceptually global. With multiple partitions this produces stale reads and lost updates.

Issue Context

Each partition writes to the same shared result stream name ($projections-{name}--result) without coordination.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[94-101]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[127-131]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[75-89]
[correctness] Custom partition routing mismatch
Custom partition routing mismatch Deferred partition-key mode routes events by EventStreamId but later computes the actual custom partition key; custom partitions can group across streams, so the same computed partition can be processed on different processors, splitting/corrupting per-partition state.

Issue description

Deferred partitioning routes by stream id but the real partition key is computed later. Custom partitioning can group across streams (e.g., event.body.region), so per-partition state can be split across processors.

Issue Context

Each processor has its own state cache; there is no cross-processor coordination for a computed partition key.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[52-67]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionDispatcher.cs[86-92]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[70-92]
  • src/KurrentDB.Projections.Core.Tests/Services/Jint/when_partitioning_by_custom_rule.cs[15-33]
[correctness] Checkpoint sequence reset drops
Checkpoint sequence reset drops CheckpointCoordinator discards partially collected buffers when it sees a new markerSequence. Since the read loop can inject marker N+1 before all partitions reported marker N, checkpoint data can be dropped, causing lost state/emits or duplication on restart.

Issue description

Checkpoint collection is single-slot and resets on any new marker sequence. If partitions report out of step, earlier checkpoint buffers are discarded.

Issue Context

The read loop injects markers based on thresholds, independent of whether the prior marker has fully completed across all partitions.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/CheckpointCoordinator.cs[49-65]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2.cs[138-147]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionDispatcher.cs[95-105]
[correctness] Checkpoint write can hang
Checkpoint write can hang CheckpointCoordinator awaits a TaskCompletionSource without cancellation/timeout; if the write response never arrives, checkpointing stalls indefinitely and the semaphore remains held, blocking progress and potentially shutdown.

Issue description

Checkpoint writes can block forever waiting for a reply message; this stalls all subsequent checkpoints and can hang shutdown.

Issue Context

This is a reliability issue under partial failures (dropped messages, node failover, etc.).

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/CheckpointCoordinator.cs[67-112]
[correctness] Null passed to Load
Null passed to Load PartitionProcessor caches null states and later calls _stateHandler.Load(cachedState) even when cachedState is null. This violates the IProjectionStateHandler contract and can crash handlers that don’t accept null.

Issue description

PartitionProcessor may call Load(null) due to null-state caching, but the handler interface does not promise null is accepted.

Issue Context

Null states appear to be part of projection semantics (tests handle null expected states), so this should be made an explicit contract.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[87-92]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[119-125]
  • src/KurrentDB.Projections.Shared/Services/IProjectionStateHandler.cs[18-37]
[correctness] EmitEnabled flag unused
EmitEnabled flag unused ProjectionEngineV2Config exposes EmitEnabled, but emitted events are buffered and written unconditionally; this can allow side-effecting emits even when emit is intended to be disabled by configuration.

Issue description

EmitEnabled is present in the V2 engine config but not enforced, while the engine always writes emitted events.

Issue Context

The management layer already treats emit enablement as a configuration flag, so V2 should provide equivalent enforcement to avoid unexpected side effects.

Fix Focus Areas

  • src/KurrentDB.Projections.V2/Services/Processing/V2/ProjectionEngineV2Config.cs[9-18]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/PartitionProcessor.cs[110-135]
  • src/KurrentDB.Projections.V2/Services/Processing/V2/CheckpointCoordinator.cs[139-150]
  • src/KurrentDB.Projections.Management/Services/Management/ManagedProjection.cs[953-983]
                     PR 5459 (2026-01-14)                    
[possible issue] Prevent crashes from unhandled exceptions

✅ Prevent crashes from unhandled exceptions

Add a try-catch block inside the async void method AuthorizeManyAsync to prevent unhandled exceptions from await accessCheck from crashing the application.

src/KurrentDB.Core/Services/AuthorizationGateway.cs [422-434]

 async void AuthorizeManyAsync<TRequest>(
 	ValueTask<bool> accessCheck,
 	ClaimsPrincipal user,
 	ReadOnlyMemory<Operation> operations,
 	IEnvelope replyTo,
 	IPublisher destination,
 	TRequest request,
 	Func<TRequest, Message> createAccessDenied) where TRequest : Message {
-	if (await accessCheck)
-		AuthorizeMany(user, operations, replyTo, destination, request, createAccessDenied);
-	else
+	try {
+		if (await accessCheck)
+			AuthorizeMany(user, operations, replyTo, destination, request, createAccessDenied);
+		else
+			replyTo.ReplyWith(createAccessDenied(request));
+	} catch (Exception ex) {
+		// It's important to log the exception. Assuming a logger is available.
+		// Log.Error(ex, "Error during asynchronous authorization for multi-stream write.");
 		replyTo.ReplyWith(createAccessDenied(request));
+	}
 }

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that an unhandled exception in an async void method can crash the application and proposes a try-catch block to handle potential exceptions from await accessCheck gracefully.

                     PR 5452 (2026-01-13)                    
[possible issue] Pass correct affinity parameter

✅ Pass correct affinity parameter

Pass message.Affinity instead of the message object to Strategy.GetSynchronizationGroup to ensure correct affinity-based message grouping.

src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.cs [131]

-stateMachine.Schedule(message, Strategy.GetSynchronizationGroup(message));
+stateMachine.Schedule(message, Strategy.GetSynchronizationGroup(message.Affinity));

Suggestion importance[1-10]: 8

__

Why: This is a critical bug fix. The current implementation passes the entire message object to GetSynchronizationGroup, which uses object reference equality, defeating the purpose of affinity-based grouping and causing incorrect synchronization behavior.

[possible issue] Prevent potential unregistration error

✅ Prevent potential unregistration error

Conditionally unregister from the Monitor in RequestStop only if a _queueLengthListener exists to prevent a potential exception.

src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.cs [84-91]

 	public void RequestStop() {
 		if (Interlocked.Exchange(ref _lifetimeSource, null) is { } cts) {
 			cts.Cancel();
-			Monitor.Unregister(this);
-			_queueLengthListener?.Dispose();
-			_queueLengthObserver = null;
+			if (_queueLengthListener is not null) {
+				Monitor.Unregister(this);
+				_queueLengthListener.Dispose();
+				_queueLengthObserver = null;
+			}
 		}
 	}

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential ArgumentException when unregistering from the Monitor if the queue was never registered, which happens with certain strategies.

[high-level] Consider simplifying the custom object pooling

✅ Consider simplifying the custom object pooling

The custom lock-free object pooling for AsyncStateMachine adds complexity. Consider reverting to the simpler, previous implementation that used System.Collections.Concurrent.ConcurrentBag to reduce maintenance overhead and risk.

Examples:

src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.Pooling.cs [9-32]

	private volatile AsyncStateMachine _firstNode;

	private void ReturnToPool(AsyncStateMachine node) {
		AsyncStateMachine current;
		do {
			current = _firstNode;
			node.NextInPool = current;
		} while (Interlocked.CompareExchange(ref _firstNode, node, current) != current);
	}


 ... (clipped 14 lines)

src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.cs [121-123]

		var stateMachine = messageCount > MaxPoolSize
			? new(this)
			: RentFromPool();

Solution Walkthrough:

Before:

// src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.cs
public partial class ThreadPoolMessageScheduler : IQueuedHandler {
    private readonly ConcurrentBag _pool;
    // ...
    public void Publish(Message message) {
        // ...
        AsyncStateMachine stateMachine;
        if (!_pool.TryTake(out stateMachine)) {
            stateMachine = new PoolingAsyncStateMachine(this);
        }
        // ...
    }
}

// src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.StateMachine.cs
private class AsyncStateMachine {
    // ...
    protected void ReturnToPool() => _scheduler._pool.Add(this);
}

After:

// src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.cs
public partial class ThreadPoolMessageScheduler : IQueuedHandler {
    // ...
    public void Publish(Message message) {
        // ...
        var stateMachine = messageCount > MaxPoolSize
            ? new(this)
            : RentFromPool();
        // ...
    }
}

// src/KurrentDB.Core/Bus/ThreadPoolMessageScheduler.Pooling.cs
partial class ThreadPoolMessageScheduler {
    private volatile AsyncStateMachine _firstNode;

    private void ReturnToPool(AsyncStateMachine node) {
        // lock-free push to linked list
        do { ... } while (Interlocked.CompareExchange(...) != ...);
    }

    private AsyncStateMachine RentFromPool() {
        // lock-free pop from linked list
        do { ... } while (Interlocked.CompareExchange(...) != ...);
        return current;
    }
}

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies the replacement of ConcurrentBag with a custom lock-free pooling implementation, raising a valid concern about increased complexity and risk in a critical component for a performance gain that should be justified.

                     PR 5445 (2026-01-12)                    
[general] Improve timeout handling in tests

✅ Improve timeout handling in tests

In ProcessConnectorEvents, replace DateTime.UtcNow with a Stopwatch for more reliable timeout measurement and throw a TimeoutException if the connector is not found, instead of returning silently.

src/Connectors/KurrentDB.Connectors.Tests/Planes/Management/ManagementServerFixture.cs [41-60]

 public async Task ProcessConnectorEvents(string connectorId, CancellationToken cancellationToken = default) {
 	// Wait for the background projection to process events for this connector.
 	// We poll until the connector appears in the snapshot to avoid race conditions
 	// with the background ConnectorsStateProjection service.
 	var timeout = TimeSpan.FromSeconds(5);
-	var start   = DateTime.UtcNow;
+	var stopwatch = System.Diagnostics.Stopwatch.StartNew();
 
-	while (DateTime.UtcNow - start < timeout) {
+	while (stopwatch.Elapsed < timeout) {
 		cancellationToken.ThrowIfCancellationRequested();
 
 		var (snapshot, _, _) = await AssemblyFixture.SnapshotProjectionsStore.LoadSnapshot<ConnectorsSnapshot>(
 			ConnectorQueryConventions.Streams.ConnectorsStateProjectionStream
 		);
 
 		if (snapshot.Connectors.Any(c => c.ConnectorId == connectorId))
 			return;
 
 		await Task.Delay(50, cancellationToken);
 	}
+	
+	throw new TimeoutException($"Connector with ID '{connectorId}' did not appear in the snapshot within the {timeout.TotalSeconds}s timeout.");
 }

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out two significant issues in the test helper method: using DateTime.UtcNow for timeouts and silent failure. The proposed changes to use Stopwatch and throw a TimeoutException substantially improve test reliability and debuggability.

                     PR 5444 (2026-01-12)                    
[possible issue] Avoid converting UTC time to local

✅ Avoid converting UTC time to local

Avoid converting the created timestamp to local time. Instead, format the UTC DateTime directly using the ISO 8601 round-trip format specifier (o) to ensure timezone consistency.

src/KurrentDB.Core/DuckDB/InlineFunctions.cs [47]

-			$"{{ \"data\": {dataString}, \"metadata\": {metaString}, \"stream_id\": \"{stream}\", \"created\": \"{created.ToLocalTime():yyyy-MM-dd'T'HH:mm:ssK}\", \"event_type\": \"{eventType}\" }}";
+			$"{{ \"data\": {dataString}, \"metadata\": {metaString}, \"stream_id\": \"{stream}\", \"created\": \"{created:o}\", \"event_type\": \"{eventType}\" }}";

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that converting UTC time to local server time is not a best practice and can lead to timezone-related issues. Proposing to use the round-trip format specifier (o) on the original UTC DateTime is a robust solution that improves data consistency.

[possible issue] Cast JSON timestamp to TIMESTAMP

✅ Cast JSON timestamp to TIMESTAMP

In the AllCteTemplate query, cast the event->>'created' string value to a TIMESTAMP type to ensure correct sorting and filtering.

src/KurrentDB/Components/Query/QueryService.cs [97]

-event->>'created' as created_at
+CAST(event->>'created' AS TIMESTAMP) as created_at

Suggestion importance[1-10]: 8

__

Why: The PR changes the created_at column to be a string, which would break any sorting or filtering operations that expect a timestamp. This suggestion correctly points out the need to cast this string back to a TIMESTAMP type to maintain query functionality.

                     PR 5437 (2026-01-06)                    
[high-level] Consider a more generic multi-append API

✅ Consider a more generic multi-append API

The new multi-stream append feature uses a low-level, index-based API. It is suggested to create a more abstract, higher-level API to improve safety and ease of use for future implementations.

Examples:

src/KurrentDB.Core/Bus/Extensions/PublisherWriteExtensions.cs [45-52]

	public static async Task WriteEvents(
		this IPublisher publisher,
		LowAllocReadOnlyMemory streams,
		LowAllocReadOnlyMemory expectedRevisions,
		LowAllocReadOnlyMemory events,
		LowAllocReadOnlyMemory eventStreamIndexes,
		CancellationToken cancellationToken = default
	) {

src/KurrentDB.SecondaryIndexing/Indexes/User/Management/UserIndexEventStore.cs [46-102]

		LowAllocReadOnlyMemory streams = duplicate
			? [stream, UserIndexConstants.ManagementAllStream]
			: new(stream);

		LowAllocReadOnlyMemory expectedVersions = duplicate
			? [expectedVersion.Value, ExpectedVersion.Any]
			: new(expectedVersion.Value);

		var totalEventCount = duplicate ? events.Count * 2 : events.Count;
		Event[] processedEvents = new Event[totalEventCount];

 ... (clipped 47 lines)

Solution Walkthrough:

Before:

// In PublisherWriteExtensions.cs
public static async Task WriteEvents(
    this IPublisher publisher,
    LowAllocReadOnlyMemory streams,
    LowAllocReadOnlyMemory expectedRevisions,
    LowAllocReadOnlyMemory events,
    LowAllocReadOnlyMemory eventStreamIndexes,
    CancellationToken cancellationToken = default
) { ... }

// Usage in UserIndexEventStore.cs
// ... build parallel arrays for streams, expectedVersions, events, and eventStreamIndexes
await _client.Writing.WriteEvents(
    streams: streams,
    expectedRevisions: expectedVersions,
    events: processedEvents,
    eventStreamIndexes: eventStreamIndexes,
    cancellationToken);

After:

// Proposed higher-level API
public record MultiStreamWrite(
    string Stream,
    long ExpectedRevision,
    IReadOnlyCollection Events
);

public static async Task<...> WriteEvents(
    this IPublisher publisher,
    IReadOnlyCollection writes,
    CancellationToken cancellationToken = default
) {
    // Internally, this would build the low-level parallel arrays
    // and call the existing implementation.
}

// Simplified usage
var writes = new List { new(stream, expectedVersion, events) };
if (duplicate) {
    writes.Add(new(allStream, ExpectedVersion.Any, events));
}
await _client.Writing.WriteEvents(writes, cancellationToken);

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the new multi-stream write API is low-level and potentially error-prone, proposing a significant architectural improvement for this new core feature that would enhance usability and maintainability.

[possible issue] Fix bug with empty event collections

✅ Fix bug with empty event collections

Add a guard clause to handle empty event collections to prevent a potential crash, and refactor the event processing loop for improved readability.

src/KurrentDB.SecondaryIndexing/Indexes/User/Management/UserIndexEventStore.cs [37-94]

 	public async Task<AppendEventsResult> AppendEvents(
 		StreamName stream,
 		ExpectedStreamVersion expectedVersion,
 		IReadOnlyCollection<NewStreamEvent> events,
 		CancellationToken cancellationToken) {
+
+		if (events.Count == 0) {
+			var streamRevision = await _inner.GetStreamVersion(stream, cancellationToken);
+			return AppendEventsResult.StreamExists(streamRevision.Value);
+		}
 
 		// decide if we are going to duplicate the events to the user index all Stream
 		var duplicate = stream.ToString().StartsWith(UserIndexConstants.Category);
 
 		LowAllocReadOnlyMemory<string> streams = duplicate
 			? [stream, UserIndexConstants.ManagementAllStream]
 			: new(stream);
 
 		LowAllocReadOnlyMemory<long> expectedVersions = duplicate
 			? [expectedVersion.Value, ExpectedVersion.Any]
 			: new(expectedVersion.Value);
 
 		var totalEventCount = duplicate ? events.Count * 2 : events.Count;
-		Event[] processedEvents = new Event[totalEventCount];
-		int[] eventStreamIndexes = new int[totalEventCount];
+		var processedEvents = new Event[totalEventCount];
+		var eventStreamIndexes = new int[totalEventCount];
 
 		var originalStreamIndex = 0;
 		var allStreamIndex = 1;
 
 		int i = 0;
-		int j = events.Count;
 		foreach (var evt in events) {
 			var message = Message.Builder
 				.Value(evt.Payload!)
 				.Headers(new Headers(evt.Metadata.ToHeaders()))
 				.WithSchemaType(SchemaDataFormat.Json)
 				.Create();
 			var data = await _serialize(evt.Payload, message.Headers);
 			var dataArray = data.ToArray();
 			var schema = SchemaInfo.FromHeaders(message.Headers);
 			var isJson = schema.SchemaDataFormat == SchemaDataFormat.Json;
 
 			// process the events into the original stream
 			processedEvents[i] = new Event(
 				eventId: evt.Id,
 				eventType: schema.SchemaName,
 				isJson: isJson,
 				data: dataArray);
 			eventStreamIndexes[i] = originalStreamIndex;
 
 			// process the events into the management stream if necessary
 			if (duplicate) {
-				processedEvents[j] = new Event(
+				var duplicateIndex = i + events.Count;
+				processedEvents[duplicateIndex] = new Event(
 					eventId: Guid.NewGuid(),
 					eventType: schema.SchemaName,
 					isJson: isJson,
 					data: dataArray);
-				eventStreamIndexes[j] = allStreamIndex;
+				eventStreamIndexes[duplicateIndex] = allStreamIndex;
 			}
 
 			i++;
-			j++;
 		}
 ...

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential bug when handling an empty events collection and proposes a valid refactoring to simplify the loop logic, but the provided improved_code for the bug fix is incorrect as it calls a non-existent method _inner.GetStreamVersion.

[general] Fix typo in error message string

✅ Fix typo in error message string

Fix a typo in the SecondaryIndexingDisabled error message by adding a missing closing parenthesis.

src/KurrentDB.Api.V2/Modules/Indexes/ApiErrors.cs [32-34]

 	public static RpcException SecondaryIndexingDisabled() => RpcExceptions.FromError(
 		error: IndexesError.SecondaryIndexingDisabled,
-		message: "Secondary indexing is disabled (configuration key KurrentDB::SecondaryIndexing::Enabled is false");
+		message: "Secondary indexing is disabled (configuration key KurrentDB::SecondaryIndexing::Enabled is false)");

Suggestion importance[1-10]: 2

__

Why: The suggestion correctly identifies and fixes a typo (a missing parenthesis) in an error message string, which is a minor but valid improvement for message clarity.

                     PR 5432 (2026-01-05)                    
[general] Fix typo in error text

✅ Fix typo in error text

Correct the grammatical error "is has" to "has" in the error message string.

src/KurrentDB.Api.V2/Modules/Streams/ApiErrors.cs [75-76]

-var message = $"Stream '{stream}' is has a different group of messages in this session. " +
+var message = $"Stream '{stream}' has a different group of messages in this session. " +
                 $"Appends for the same stream must currently be grouped together and not interleaved with appends for other streams.";

Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies and fixes a grammatical error ("is has") in the new error message, improving its clarity and professionalism.

                     PR 5430 (2026-01-02)                    
[high-level] Refactor logging dependency injection strategy

Refactor logging dependency injection strategy

Refactor the code to inject specific ILogger instances directly into classes instead of injecting ILoggerFactory. This change simplifies constructors and improves testability.

Examples:

src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexEngine.cs [39-54]

	public UserIndexEngine(
		ISystemClient client,
		IPublisher publisher,
		ISubscriber subscriber,
		ISchemaSerializer serializer,
		SecondaryIndexingPluginOptions options,
		DuckDBConnectionPool db,
		IReadIndex index,
		TFChunkDbConfig chunkDbConfig,
		[FromKeyedServices(SecondaryIndexingConstants.InjectionKey)]

 ... (clipped 6 lines)

src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexEngineSubscription.cs [26-41]

public partial class UserIndexEngineSubscription(
	ISystemClient client,
	IPublisher publisher,
	ISchemaSerializer serializer,
	SecondaryIndexingPluginOptions options,
	DuckDBConnectionPool db,
	IReadIndex readIndex,
	Meter meter,
	Func<(long, DateTime)> getLastAppendedRecord,
	ILoggerFactory logFactory,

 ... (clipped 6 lines)

Solution Walkthrough:

Before:

public class UserIndexEngine(ILoggerFactory loggerFactory) {
    private readonly ILogger _log;
    private readonly UserIndexEngineSubscription _subscription;

    public UserIndexEngine(...) {
        _log = loggerFactory.CreateLogger();
        _subscription = new UserIndexEngineSubscription(..., loggerFactory, ...);
    }
    // ...
}

public class UserIndexEngineSubscription(..., ILoggerFactory logFactory, ...) {
    private readonly ILogger _log = logFactory.CreateLogger();
    // ...
}

After:

// Assuming UserIndexEngineSubscription is registered in DI container
public class UserIndexEngine(ILogger log, UserIndexEngineSubscription subscription) {
    private readonly ILogger _log = log;
    private readonly UserIndexEngineSubscription _subscription = subscription;

    // ...
}

public class UserIndexEngineSubscription(..., ILogger log, ...) {
    private readonly ILogger _log = log;
    // ...
}

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a suboptimal dependency injection pattern where ILoggerFactory is used instead of ILogger, impacting multiple new classes and affecting design quality and testability.

[general] Permit uppercase identifiers

✅ Permit uppercase identifiers

Add RegexOptions.IgnoreCase to the GeneratedRegex attribute to allow uppercase characters in index and column identifiers.

src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexSql.cs [40-41]

-[GeneratedRegex("^[a-z][a-z0-9_-]*$", RegexOptions.Compiled)]
+[GeneratedRegex("^[a-z][a-z0-9_-]*$", RegexOptions.Compiled | RegexOptions.IgnoreCase)]
 private static partial Regex ValidationRegex();

Suggestion importance[1-10]: 4

__

Why: This is a reasonable suggestion to make the identifier validation case-insensitive, which could improve flexibility, but it's a minor enhancement and not a bug fix.

                     PR 5425 (2025-12-22)                    
[possible issue] Handle null selector result correctly

✅ Handle null selector result correctly

Modify the CanHandleEvent method to treat a null or undefined result from a selector expression as a signal to skip indexing the event, similar to the skip value.

src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexProcessor.cs [163-172]

 			var fieldValue = _evaluator.Select(_fieldSelectorExpression);
-			if (fieldValue == JsValue.Null)
-				return true;
+			if (fieldValue.IsNull() || fieldValue.IsUndefined())
+				return _fieldSelectorExpression is null;
 
 			if (_skip.Equals(fieldValue))
 				return false;
 
 			field = (TField)TField.ParseFrom(fieldValue);
 
 			return true;

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logical flaw where an event with a null selector result is indexed. The proposed change to skip such events makes the indexing behavior more intuitive and robust.

                     PR 5422 (2025-12-18)                    
[high-level] Refactor JavaScript object creation

✅ Refactor JavaScript object creation

Instead of manually building a JavaScript object using a hierarchy of C# classes that inherit from ObjectInstance, define a simple C# record or class that matches the desired JavaScript structure. Then, use Jint's JsValue.FromObject to automatically convert the C# object to a JsValue, simplifying the code and improving maintainability.

Examples:

src/KurrentDB.SecondaryIndexing/Indexes/User/JavaScript/RecordObject.cs [12-77]

internal sealed class RecordObject : JsObject {
	private readonly PositionObject _position;
	private readonly SchemaInfoObject _schemaInfo;

	public RecordObject(Engine engine, JsonParser parser) : base(engine, parser) {
		_position = new PositionObject(engine, parser);
		SetReadOnlyProperty("position", _position);

		_schemaInfo = new SchemaInfoObject(engine, parser);
		SetReadOnlyProperty("schemaInfo", _schemaInfo);

 ... (clipped 56 lines)

src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexProcessor.cs [157-166]

			_jsRecord.MapFrom(resolvedEvent, ++_sequenceId);

			if (_filter is not null) {
				var passesFilter = _filter.Call(_jsRecord).AsBoolean();
				if (!passesFilter)
					return false;
			}

			if (_fieldSelector is not null) {
				var fieldJsValue = _fieldSelector.Call(_jsRecord);

Solution Walkthrough:

Before:

// KurrentDB.SecondaryIndexing/Indexes/User/UserIndexProcessor.cs
class UserIndexProcessor {
    private readonly RecordObject _jsRecord;

    public UserIndexProcessor(...) {
        _jsRecord = new RecordObject(_engine, parser);
    }

    private bool CanHandleEvent(ResolvedEvent resolvedEvent, out TField? field) {
        _jsRecord.MapFrom(resolvedEvent, ++_sequenceId);
        var passesFilter = _filter.Call(_jsRecord).AsBoolean();
        // ...
    }
}

// KurrentDB.SecondaryIndexing/Indexes/User/JavaScript/RecordObject.cs
class RecordObject : JsObject { // Inherits from Jint's ObjectInstance
    public RecordObject(...) {
        _position = new PositionObject(engine, parser);
        SetReadOnlyProperty("position", _position); // Manual property setting
        // ...
    }

    public void MapFrom(ResolvedEvent resolvedEvent, ulong sequenceId) {
        RecordId = $"{resolvedEvent.OriginalEvent.EventId}"; // Manual mapping
        // ...
        _position.MapFrom(resolvedEvent, sequenceId);
    }
}

After:

// Define simple C# records to represent the JS object structure
record JsApiPosition(string streamId, ulong streamRevision, ulong logPosition);
record JsApiSchemaInfo(string subject, string type);
record JsApiRecord(
    string recordId,
    JsApiPosition position,
    JsApiSchemaInfo schemaInfo,
    object value, // Can be lazily evaluated
    object headers
    // ... other properties
);

// KurrentDB.SecondaryIndexing/Indexes/User/UserIndexProcessor.cs
class UserIndexProcessor {
    private bool CanHandleEvent(ResolvedEvent resolvedEvent, out TField? field) {
        // Create a simple C# object
        var recordPoco = MapToPoco(resolvedEvent, ++_sequenceId);

        // Let Jint handle the conversion to a JS object
        var jsRecord = Jint.Native.JsValue.FromObject(_engine, recordPoco);

        var passesFilter = _filter.Call(jsRecord).AsBoolean();
        // ...
    }
}

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a complex, custom implementation for creating JavaScript objects and proposes a much simpler, more idiomatic approach using Jint's built-in object mapping, which would significantly reduce code complexity and improve maintainability.

                     PR 5421 (2025-12-18)                    
[general] Reintroduce lazy initialization for connection pools

✅ Reintroduce lazy initialization for connection pools

Reintroduce lazy initialization for the DuckDBConnectionPool to avoid creating it for every connection, restoring the previous performance-optimized behavior.

src/KurrentDB/KestrelHelpers.cs [34-41]

 	public static void UseDuckDbConnectionPoolPerConnection(this ListenOptions listenOptions) {
 		listenOptions.Use(next => async connectionContext => {
 			var poolFactory = listenOptions.ApplicationServices.GetRequiredService<DuckDBConnectionPoolLifetime>();
-			using var pool = poolFactory.CreatePool();
-			connectionContext.Items[nameof(DuckDBConnectionPool)] = pool;
-			await next(connectionContext);
+			var lazyPool = new Lazy<DuckDBConnectionPool>(poolFactory.CreatePool);
+			connectionContext.Items[nameof(Lazy<DuckDBConnectionPool>)] = lazyPool;
+
+			try {
+				await next(connectionContext);
+			} finally {
+				if (lazyPool.IsValueCreated) {
+					lazyPool.Value.Dispose();
+				}
+			}
 		});
 	}

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a performance regression where a connection pool is created eagerly for every connection and proposes reintroducing lazy initialization, which is a significant performance improvement.

[general] Update extension method for lazy pool

✅ Update extension method for lazy pool

Update the GetDuckDbConnectionPool extension method to retrieve the Lazy and return its Value, aligning with the lazy initialization pattern.

src/KurrentDB.Core/Services/Transport/Http/HttpContextExtensions.cs [16-25]

 	[CanBeNull]
 	public static DuckDBConnectionPool GetDuckDbConnectionPool(this HttpContext httpContext) {
 		var connectionItemsFeature = httpContext.Features.Get<IConnectionItemsFeature>();
 
 		if (connectionItemsFeature is null ||
-			!connectionItemsFeature.Items.TryGetValue(nameof(DuckDBConnectionPool), out var item))
+		    !connectionItemsFeature.Items.TryGetValue(nameof(Lazy<DuckDBConnectionPool>), out var item) ||
+		    item is not Lazy<DuckDBConnectionPool> lazyPool)
 			return null;
 
-		return item as DuckDBConnectionPool;
+		return lazyPool.Value;
 	}

Suggestion importance[1-10]: 8

__

Why: This suggestion is a necessary follow-up to reintroducing lazy initialization for the connection pool, ensuring the retrieval logic correctly handles the Lazy wrapper to access the pool instance.

                     PR 5409 (2025-12-16)                    
[possible issue] Fix incorrect configuration option skipping

✅ Fix incorrect configuration option skipping

Fix a bug by removing an else block that incorrectly skips processing scalar configuration options if their top-level value is missing in a provider.

src/KurrentDB.Core/Configuration/ClusterVNodeOptions.Framework.cs [104-125]

 				if (!provider.TryGet(option.Value.Key, out var value) && !isDefault) {
 					// Handle options that have been configured as arrays (GossipSeed is currently the only one
 					// where this is possible)
 					if (option.Value.OptionSchema.Value<string>("type") is "array") {
 						var parentPath = option.Value.Key;
 						var childValues = new List<string>();
 
 						foreach (var childKey in provider.GetChildKeys([], parentPath)) {
 							var absoluteChildKey = parentPath + ":" + childKey;
 							if (provider.TryGet(absoluteChildKey, out var childValue) && childValue is not null) {
 								childValues.Add(childValue);
 								sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider);
 							}
 						}
 
-						value = string.Join(", ", childValues);
 						if (childValues.Count is 0)
 							continue; // no child values. skip
+
+						value = string.Join(", ", childValues);
 					} else {
-						continue; // no value and it is an array so don't check for children. skip.
+						continue; // no value for this option in this provider.
 					}
 				}

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where scalar options would be skipped if their top-level key is missing, preventing them from being loaded from other providers.

                     PR 5405 (2025-12-12)                    
[general] Simplify null or whitespace string handling

✅ Simplify null or whitespace string handling

In TryPrettifyJson, simplify the initial check to return string.Empty for any null or whitespace input string, ensuring consistent behavior.

src/KurrentDB/Components/Query/Query.razor [176-186]

 static string TryPrettifyJson(string json) {
-	if (string.IsNullOrWhiteSpace(json)) return json ?? string.Empty;
+	if (string.IsNullOrWhiteSpace(json)) return string.Empty;
 	try {
 		using var doc = JsonDocument.Parse(json);
 		return JsonSerializer.Serialize(doc.RootElement, Indent);
 	}
 	catch (JsonException) {
 		// Not valid JSON; return original (or decide to throw)
 		return json;
 	}
 }

Suggestion importance[1-10]: 4

__

Why: The suggestion improves code clarity and consistency by simplifying the handling of null or whitespace strings, which is a good practice for maintainability.

                     PR 5391 (2025-12-07)                    
[high-level] System memory calculation logic is flawed

✅ System memory calculation logic is flawed

The current method for calculating available system memory is flawed because it incorrectly uses process-specific memory metrics (Process.WorkingSet64) instead of system-wide ones. It is recommended to either make the memory limit a configurable setting or to rely on DuckDB's own default memory management.

Examples:

src/KurrentDB.Core/DuckDB/DuckDBConnectionPoolLifetime.cs [56-63]

		(double Total, double Used) CalculateRam() {
			var process = Process.GetCurrentProcess();
			var processRam = process.WorkingSet64;
			var totalRam = GC.GetGCMemoryInfo().TotalAvailableMemoryBytes;
			var totalGb = (double)totalRam / 1024 / 1024 / 1024;
			var processGb = (double)processRam / 1024 / 1024 / 1024;
			return (totalGb, processGb);
		}

Solution Walkthrough:

Before:

(double Total, double Used) CalculateRam() {
    var process = Process.GetCurrentProcess();
    var processRam = process.WorkingSet64; // This is only this process's memory
    var totalRam = GC.GetGCMemoryInfo().TotalAvailableMemoryBytes;
    // ...
    return (totalGb, processGb);
}

// in constructor...
var (total, used) = CalculateRam();
var availableRam = total - used; // Incorrectly calculates "available" RAM
var duckDbRam = (int)Math.Round(availableRam / 2);
_pool = new ConnectionPoolWithFunctions($"Data Source={path};memory_limit={duckDbRam}GB", ...);

After:

// Suggested Alternative: Make the memory limit configurable
// (This requires adding a property to TFChunkDbConfig)

public DuckDBConnectionPoolLifetime(TFChunkDbConfig config, ...) {
    var path = ...;
    var connectionString = $"Data Source={path}";

    // Check for a configured memory limit
    if (config.MemoryLimitGB.HasValue) {
        connectionString += $";memory_limit={config.MemoryLimitGB.Value}GB";
    }
    // If not configured, DuckDB will use its own default (typically 80% of system RAM)

    _pool = new ConnectionPoolWithFunctions(connectionString, ...);
}

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical flaw where process-specific memory (Process.WorkingSet64) is incorrectly used to calculate system-wide available RAM, leading to a dangerously high and incorrect memory limit for DuckDB.

                     PR 5387 (2025-12-01)                    
[possible issue] Fix a potential race condition

✅ Fix a potential race condition

To prevent a potential race condition in the Subscribe method, assign _cts to a local variable at the start and use that variable for the null check and subsequent accesses.

src/KurrentDB.SecondaryIndexing/Subscriptions/SecondaryIndexSubscription.cs [29-50]

 public void Subscribe() {
-	if (_cts == null) {
+	var cts = _cts;
+	if (cts == null) {
 		log.LogWarning("Subscription already terminated");
 		return;
 	}
 	var position = indexProcessor.GetLastPosition();
 	var startFrom = position == TFPos.Invalid ? Position.Start : Position.FromInt64(position.CommitPosition, position.PreparePosition);
 	log.LogInformation("Starting indexing subscription from {StartFrom}", startFrom);
 
 	_subscription = new(
 		bus: publisher,
 		expiryStrategy: DefaultExpiryStrategy.Instance,
 		checkpoint: startFrom,
 		resolveLinks: false,
 		user: SystemAccounts.System,
 		requiresLeader: false,
 		catchUpBufferSize: options.CommitBatchSize * 2,
-		cancellationToken: _cts!.Token
+		cancellationToken: cts.Token
 	);
 
-	_processingTask = ProcessEvents(_cts.Token);
+	_processingTask = ProcessEvents(cts.Token);
 }

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential race condition where _cts could be nullified after the check, and proposes the standard thread-safe pattern to fix it.

                     PR 5373 (2025-11-19)                    
[general] Improve exception message with context

✅ Improve exception message with context

Improve the exception message when an index is not found by including the indexName to provide better context for debugging.

src/KurrentDB/Components/Query/QueryService.cs [36-43]

 case "index":
 	var indexName = tokens[1];
 	var exists = tryGetUserIndexTableDetails(indexName, out var tableName, out var tableFunctionName, out var hasFields);
 	if (!exists)
-		throw new("Index does not exist");
+		throw new($"Index '{indexName}' does not exist.");
 
 	cte = string.Format(UserIndexCteTemplate, cteName, $"\"{tableName}\"", $"\"{tableFunctionName}\"", hasFields ? ", field" : string.Empty);
 	break;

Suggestion importance[1-10]: 4

__

Why: The suggestion improves the exception message by including the indexName, which enhances debuggability, but it is a minor quality improvement rather than a functional change.

Clone this wiki locally