SPEC-299 CI reliability + SPEC-300 typed GROUP BY aggregations#41
Merged
Conversation
Lands the post-PR#40 work that never reached main: - SPEC-299: root-cause CI fix (timer-leak teardown, remove --forceExit/SIGKILL stopgaps) - SPEC-300: typed GROUP BY field aggregations (SUM/MIN/MAX/AVG) over the wire Surfaces disjoint (client/mcp-server/ci vs core-rust/server-rust query engine). Both reviewed + APPROVED via SpecFlow impl-review (0 critical / 0 major). Co-authored-by: Claude <noreply@anthropic.com>
ec68cf3 to
c5b8acc
Compare
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.
Summary
Lands the work completed on the
sf-297-g1-settled-latchbranch after PR #40(Query Engine Consolidation Phase 1) was squash-merged. PR #40 captured only the
Phase 1 query path; the two specs below were committed afterward and never reached
main. This PR delivers both. Surfaces are disjoint (CI/client/mcp-server vs.core-rust/server-rust query engine), so the diff reviews cleanly.
Please squash-merge — the branch carries a series of throwaway
ci: TEMP diagnosticcommits whose net effect is already reverted; a squash keeps them out of
mainhistory.SPEC-299 — CI reliability (root-cause fix, stopgaps removed)
The chronically-timing-out
Build · Test · Lint(Unit tests) job is fixed at the source;all stopgaps are removed.
maincurrently still carries the stopgaps (--forceExit,timeout -s KILL,forceExit: true) — this PR removes them.SingleServerProvider: clearconnectionTimeoutId+pendingConnectRejectinclose()with an
isClosingreconnect guard (eliminates leaked connection-timeout / reconnect-loop timers).TopGunClient.close()awaits provider close.sync: isTestreleases the SonicBoom fd in both client + mcp-server loggers.MockWebSocket+ HTTP keep-alive drain (closeAllConnections()).node.yml: removed--forceExit/timeout -s KILL/>>>> JEST ENTER;timeout-minutes25→18.mcp-server/jest.config.js: removedforceExit: true.~3150 tests pass locally with no
--forceExit.SPEC-300 — Typed GROUP BY field aggregations (SUM/MIN/MAX/AVG) over the wire
Single-node GROUP BY previously returned correct grouping +
__countbut degenerate__sum/__min/__max(the converter hardcodedaggField = ""). This wires a realaggregation spec end-to-end.
AggFuncenum +Aggregationstruct +Query.aggregationswire type (core-rust).convert_queryserializesquery.aggregations; coordinator parses it back intoVec<Aggregation>;generalized
AggregateProcessoremits__count+ only the requested__<func>_<field>keys(conditional MsgPack Integer/F64) — degenerate-key leak eliminated.
combine-aggregatevertex now emitted only on the multi-node path, leavingCombineProcessoruntouched (distributed combine deferred to TODO-437/442).
groupBy+aggregationsonQuerySchema/QueryFilter; opaqueQueryManagerpassthrough.QUERY_SUB_GROUPBY_AGG,QUERY_RESP_AGG) + WS integration coverageasserting correct non-degenerate values and no key leak.
Test evidence
cargo test --release -p topgun-server --lib— 1291 tests;cargo test -p topgun-core— 12 + 7 doc.--all-targets --all-featuresexit 0 on both crates;cargo fmt --checkclean.@topgunbuild/core+@topgunbuild/clientbuild; cross-lang fixtures + WS integration green.Both specs were reviewed and APPROVED via the SpecFlow impl-review gate (0 critical / 0 major).