Repository navigation
fix: harden v1.0.0 trust — MCP ESM crash, concurrency races, CLI/API drift, CI/release - #1
Merged
Merged
Conversation
…drift, CI/release
Verified issues from a deep code analysis (18 findings, all independently confirmed):
- mcp: ship mcp/package.json ("type": "module") — kview-mcp.js crashed on
startup on Node <= 22.6 with SyntaxError: Cannot use import statement
outside a module
- consumer: serialize DynamicConsumerService subscribe/unsubscribe (race
could leak a permanently running container), free per-group buffers after
the last unsubscribe, stop containers on shutdown
- schema: RegistrySettingsStore map is now a ConcurrentHashMap (decode hot
path raced writers); schema-registry cache loads lock per key instead of
one shared lock (a slow registry stalled all decodes cluster-wide)
- connection: KafkaClusterManager get/evict are mutually excluded so a
handle is never handed out while being closed
- test: both integration suites now point kview.data-dir at target/ (the
wrong property name made them rewrite the real ./data on every run)
- cli: kview lag reads partitionsByTopic (was a non-existent byTopic field,
silently printing zero lag rows); kview clusters derives SECURITY from the
masked security object the API actually returns
- docs: correct the reset-offsets example, stop describing the shipped
Schema Registry support as unbuilt, move QA-REPORT/RELEASE-PLAN/
SCHEMA-REGISTRY-PLAN to docs/, remove the divergent legacy kview-cli.js
- ci: smoke-test the MCP server startup on Node 20 (would have caught the
ESM crash)
- release: build with the test suite green; a tag must match the versions
in pom.xml, cli/kview.mjs and mcp/kview-mcp.js
- build: add the Maven wrapper (./mvnw) so a clone builds with just a JDK
This was referenced Sep 26, 2026
Merged
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
1 participant
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.
Quick-win fix batch from a deep code analysis of the repo (18 findings, every one independently reproduced by a second reviewer before being fixed). One PR, small verified changes, no feature work.
What's fixed
MCP server (high)
mcp/kview-mcp.jscrashed on startup on Node <= 22.6 (SyntaxError: Cannot use import statement outside a module) because it is ESM in a.jsfile with nopackage.json. Fixed by shippingmcp/package.jsonwith"type": "module". CI now smoke-tests the MCP server startup on Node 20 — exactly the check that would have caught this.Concurrency (4)
DynamicConsumerService: check-then-act race onsubscribe()could leak a permanently running consumer container. Subscribe/unsubscribe are now serialized, group buffers are freed when the group's last subscription ends, and all containers stop on shutdown (@PreDestroy).RegistrySettingsStore:find()on the decode hot path read a plainLinkedHashMapwhile writers structurally mutated it — now aConcurrentHashMap.SchemaRegistryService: schema-registry cache loads (remote HTTP, 10s timeout) ran inside one shared lock, so a slow or unreachable registry stalled every decode cluster-wide — now per-key locking.KafkaClusterManager:get()could hand out a handle that a concurrentevict()was closing — now mutually excluded.Tests
kafka-wrapper.data-dirvs the code'skview.data-dir), so the scheduled partition-history sampler rewrote the developer's real./datadirectory on every test run. Both now point attarget/.CLI/API drift (3)
kview lag <group>read a non-existentbyTopicfield and silently printed zero lag rows — now readspartitionsByTopicas the API serializes it.kview clusterstabled asecurityProtocolfield the API never returns — now derived from the maskedsecurityobject.reset-offsets my-group latestexample failed (--topic is required) — corrected to the real flag form.Docs / project hygiene
QA-REPORT.md,RELEASE-PLAN.md,SCHEMA-REGISTRY-PLAN.md) moved from the repo root todocs/.kview-cli.js(hardcoded/api/clusters/default/, different command names);cli/kview.mjsis the CLI.Release / build trust
release.ymlbuilt released artifacts with-DskipTestsand extracted aVERSIONno step consumed, while the version was pinned in three places — artifacts are now built test-green, and a guard fails the release unless the tag matchespom.xml,cli/kview.mjsandmcp/kview-mcp.js../mvnw) so a clone builds with just a JDK.Verification
./mvnw -B clean test→ 30/30 tests pass.node mcp/kview-mcp.js < /dev/nullstarts (16 tools) and exits 0 — previously aSyntaxError, exit 1.--version/--helpOK.Not covered here (deliberate follow-ups): API authentication (the high-severity unauthenticated-API finding — that's feature F1, next in the roadmap), the Spring Boot 3.3.4 → 3.5.x bump, and runtime checks against a live broker.