coverage-sweep 5/6: bug-fix wave A (Bug 27–51 + regression tests)#22
Merged
Conversation
#25: Diagnostic packet captures wrote the full pool-buffer (e.g. 8 KiB) instead of the n bytes Recvfrom actually filled. Pcap-like consumers parsed trailing garbage past the real end of every capture. #26: At goroutine exit the pool Put used (*packetBuffer)[:0] — a zero-length slice. A later Get from a freshly spawned netlinker would call syscall.Recvfrom on it, which panics on &p[0] when len(p)==0. iouringPrefillRecvs already restored cap defensively on Get; the syscall producer must Put in usable shape. Both bugs only fired in long-running daemons with namespace add/remove churn — first netlinker exits cleanly, second namespace gets the corrupted buffer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
http.Get uses the DefaultClient with no per-request timeout — a hung or wedged Schema Registry would block daemon startup forever. Add a NewRequestWithContext + 10s hard ceiling so the init-time call observes ctx cancellation and fails cleanly. Also surface non-2xx (other than 404) as an explicit error instead of letting a 401/403/5xx fall through to a confusing JSON-decode failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
franz-go's Close cancels in-flight produces without waiting for broker acks — any records buffered when SIGTERM hit were silently dropped. Add a bounded (5s) Flush before Close so the daemon's last poll cycle is durably delivered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Schema Registries return a JSON error body on non-2xx (e.g.
{\"error_code\":40402,\"message\":\"Subject not found.\"}). The previous code
skipped the status check and decoded that error body straight into
{id int}, producing a silent id:0 success — the CLI printed \"id:0\"
for a missing subject instead of reporting the registry error.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same pattern as bug 29 in cmd/register_schema. Downstream impact here is more harmful: kafka_to_clickhouse stamps every Kafka record's Confluent header with the returned schemaID. A silent id:0 would make every produced record claim schema 0 — ClickHouse's protobuflist input then mis-parses or drops every row. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cmd/clickhouse_http_insert_protobuflist hardwired FORMAT Protobuf for both envelope (Envelope with repeated Rows) and non-envelope (per-row length-prefixed Record) payloads. With the binary's default -envelope=true, the request sent an Envelope-shaped protobuf at a URL declaring the schema as Record — ClickHouse rejected every insert. Switch to FORMAT ProtobufList when useEnvelope is true and Protobuf otherwise. Add TestInsertIntoCHAt_formatSelection so the URL string is asserted in both modes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A broken bidi stream typically returns the same error immediately on every Recv (e.g. "rpc error: stream closed"). The previous loop re-called Recv with zero backoff, pegging a core at 100% until ctx was canceled. Add a ctx-aware 100ms backoff between retries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
drainOnce appends a Result with Buf=nil when the CQE's RequestID isn't in the in-flight map (e.g. post-Close stragglers or, at the >2^32 SQE horizon, a request-ID wrap collision). handleRecvCQE then dereferenced res.Buf without a guard, panicking on (*nil)[:n]. Skip orphan CQEs with a counter for visibility. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The top-of-loop select on ctx.Done only fired between reads. A ReadFromUDP already in flight when ctx was canceled blocked forever (UDP has no read timeout by default), so runMain's wg/defer would never return — the process hung on SIGTERM. Watch ctx in a sibling goroutine and Close the conn on cancel; the blocking Read returns with EBADF / "use of closed network connection", which the loop already converts to a clean nil return via ctx.Err(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
proto.Unmarshal merges into the destination rather than overwriting. xtcpRecord is reused across the consume loop (pool entry), so fields that were SET on record N but UNSET on record N+1 stayed populated in the decoded output — adjacent Kafka records bled into each other. Reset the destination before each Unmarshal. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same pattern as bug 35 in tools/kafka_topic_reader. xtcpRecord is acquired once from a pool and reused for every packet's Unmarshal; without Reset, populated fields from packet N persist into packet N+1. Demo output mixed records. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cpNS) GetNetlinkSocketFDs pulls every fd from nsMap including the xtcpNS fd, but the loop skips xtcpNS (it's xtcp's own bookkeeping namespace, not a target for inet_diag dumps). The function then returned len(socketFDs) — counting the skipped fd as if it had been polled. The Poller used this count to track how many "done" signals to expect, so every poll cycle waited for one extra done that never arrived, falling back to the PollTimeoutTimer instead of advancing on natural signals. Return the actual polled-fd count. Update + extend the existing TestPollAllNetlinkSockets_skipsXtcpNS to assert the correct count, and add a mixed test (one xtcpNS + one regular fd → count=1). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
os.Stat returns (nil, err) on any failure. The previous body only special-cased ErrNotExist (returning false), then fell through to info.IsDir() — which panics on a nil receiver. Permission-denied on a restricted mount or EIO on a flaky filesystem both reach that code path. Treat any Stat error as "no" and only dereference info on the success path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
pkg/xtcp/deserializers.go registered DeserializeCGroupIDXTCP under SockOptEnumValueCst (22) instead of DeserializeSockOptXTCP. The correct SockOpt parser couldn't be plugged in because its signature took *Envelope_XtcpFlatRecord rather than *XtcpFlatRecord, so it didn't match the dispatch map's function-pointer type. Runtime consequence: every INET_DIAG_SOCKOPT attribute (2 bytes wide) was fed to the CGroupID parser, which requires 8 bytes; CGroupID returned ErrCGroupIDSmall every time, SockOpt was never populated in the emitted XtcpFlatRecord, and per-socket TCP socket-option metadata silently disappeared from every record. Fix both: re-type DeserializeSockOptXTCP to take *XtcpFlatRecord, and register it under the SockOpt enum. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
netNamespaceInstance blocked on the parent (daemon-lifetime) ctx, but nsDelete's cancel() reached only the inner nsCtx created inside createNetlinkersAndStore — which the netlinkers used, not the namespace-instance goroutine. Result: deletes stopped the netlinkers but left netNamespaceInstance blocked, so its deferred closeSocket(socketFD) never fired. Each namespace removal leaked one netlink fd + one Go runtime goroutine until daemon shutdown. With namespace churn (short-lived containers), this exhausts fd limits. Lift the nsCtx/nsCancel creation up into netNamespaceInstance, pass both into createNetlinkersAndStore, and block on the nsCtx. nsDelete already calls nsi.cancel() — which is now the same cancel, so both the netlinkers AND the namespace-instance goroutine exit cleanly, firing the deferred close. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When fsnotify misses a namespace removal, reconcileMaps catches it on the next 5-minute tick and Deletes the destMap entry. The previous code Deleted but didn't call the netNSitem's cancel — so the netNamespaceInstance goroutine (post bug 40 fix) and its in-flight netlinkers kept running with no map registration, and the netlink socket fd never got closed. Cancel the netNSitem before deleting. testing=true callers may pass arbitrary value types, so type-assert first and only cancel when the value is actually a netNSitem. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When the server closes the stream cleanly, Recv returns io.EOF.
Subsequent Recv calls on the same closed stream keep returning EOF, so
the `if err == io.EOF { continue }` branch pegged a core forever. The
intended behavior is to break out and let singleStreamingClient's
reconnect-with-sleep loop establish a fresh stream.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
time.Sleep(c.loopsSleep) at the bottom of the loop wasn't ctx-aware, so SIGTERM took up to loopsSleep (default 10s) to be observed. Switch to a select with ctx.Done so shutdown is prompt. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
*c.changePollFrequencyCh <- ... is a blocking send. The channel has a buffer of 2, so the third call without a poller draining (poller stopped, paused, mid-shutdown) blocks forever — pegging the gRPC handler goroutine and never returning to the client. Switch to a select with the request ctx, the server ctx, and a non-blocking default. If the channel is full, drop the coalesced frequency-change and surface a metrics counter; if either ctx fires, return a clean Canceled / Unavailable status. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
*s.pollRequestCh <- struct{}{} same shape as bug 44 — buffered chan of
size 2, third call without a poller drain blocks the streaming-RPC
handler forever. PollFlatRecords clients periodically request polls;
during a quiescent or shutting-down poller, the third request hung
the bidi stream loop.
Switch to a select with the stream ctx, the service ctx, and a
non-blocking default. Drop coalesced pokes (the poller's tick will
re-fire) rather than blocking.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
protowire.AppendVarint returns the appended slice; the previous code called it as if it mutated in place and ignored the return. The length prefix was therefore never written. Non-envelope output was raw protobuf bytes — ClickHouse readers expecting the function's namesake LengthDelimited format misparsed every file. Use the return value the way encodeLengthDelimitedEnvelope already does. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
If pending CQEs hadn't arrived by drainTimeout, Close called QueueExit and threw away r.inFlight. The kernel released its SQEs, but the userspace-side packetBufferPool buffers tracked in inFlight were never handed back to the caller — each ring teardown leaked up to inFlightCap (= sqEntries * 2, default 512) buffers per close. For a daemon with namespace add/remove churn this drains the packet pool. Iterate any remaining in-flight entries before QueueExit and feed them through the onDrain callback with Res=-ETIME so the caller knows they were abandoned at teardown. onRingClosedResult already handles Buf!=nil correctly — the buffers get Put back to packetBufferPool / destBytesPool as usual. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two failure points called log.Fatalf — ring init failure (xio.New) and SQE prefill failure (iouringPrefillRecvs). A single namespace's io_uring setup failure (kernel build doesn't have the required ops, fd quota hit, etc.) would take down the gRPC services, the poller, and every other namespace's netlinkers along with it. Demote both to log.Printf + return so deferred wg.Done + UnlockOSThread + ring.Close (where applicable) fire cleanly. The explicit pre-Fatalf wg.Done() / UnlockOSThread() the previous code did was also incorrect — in mocked-fatalf tests, the function would continue with ring==nil and double-decrement the WaitGroup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Called per-namespace from createNetlinkersAndStore. A single namespace's SO_RCVTIMEO setsockopt failure (rare — maybe fd quota, maybe a closed fd race) used to tear down every gRPC service, every other namespace's netlinkers, and the poller. Demote to a counter + debug log so the affected namespace's netlinker won't observe ctx cancellation between recvs (visible via the counter), but the rest of the daemon stays up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DeserializeCongInfoXTCP matched on the 3-byte "bbr" prefix and always assigned CONGESTION_ALGORITHM_BBR1, even though the XtcpFlatRecord proto defines distinct BBR1 / BBR2 / BBR3 enum values. The original comment hand-waved this as "preserving original behavior" — operators running BBRv2/v3 had no way to distinguish them from BBRv1 in any downstream dashboard or alert. Inspect data[3] (the 4th byte of the null-terminated algorithm name): '2' → BBR2, '3' → BBR3, else → BBR1. Guarded by len(data) >= 4 so the 4-byte minimum guarantee from CongInfoSizeCst still holds. Update the existing bbr2 test to assert BBR2 and add bbr1 / bbr3 round-trip tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Eight near-identical one-off tests collapsed into a single
TestDeserializeCongInfoXTCP_dispatch with a {name, data, wantAlg}
table. The new row "bbr_garbage_byte_falls_back_to_bbr1" also covers
the default-case of the inner BBR sub-discriminator switch (bug 50)
that the previous tests didn't reach.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The for-loop bound was `for r := 1; r < attempts; r++` — one short of the advertised count. attempts=10 ran 9 iterations; attempts=1 ran 0 iterations and returned a confusing "dial X:Y: %!w(<nil>)" formatted error because lastErr was never assigned. attempts<=0 hit the same trap. Fix: loop r := 0..attempts-1 (matches the count contract); reject attempts<=0 up front with an explicit error so the report doesn't contain a `%!w` placeholder. Add table-driven tests covering the zero/negative/one cases that previously slipped through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Convert two scalar helper tests into table-driven form so coverage gaps
are obvious at a glance and edge cases are easy to add. Picks up new
coverage of:
- atoiOr0: whitespace-only, float-like, trailing-garbage inputs
- bytesIndex: empty-both, prefix/suffix matches, needle-longer-than-
haystack
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two new subtests under TestReconcileMaps:
- backstop_delete_calls_netNSitem_cancel: asserts the production
delete path invokes netNSitem.cancel() so orphaned goroutines /
fds wind down (the bug 41 fix).
- backstop_delete_non_netNSitem_value_is_safe: guards the
type-assertion branch — table tests pass raw strings, and the
cancel logic must skip them without panicking.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…in layer) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 15, 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.
Summary
coverage-sweep 5/6 — bug-fix wave A. The first of two bug-fix PRs (commits 201–229): the netlinker syscall pre-fix plus Bug 27–51, each a small, self-contained correctness fix with its regression test. This is the high-value review — please scrutinize.
All applied cleanly onto main (no conflicts).
Fixes (each is its own commit)
Testing
go vet ./...+gofmt -l .— clean (go 1.25; one gofmt-forward for a quality-report test file).go test -ldflags=-checklinkname=0 -tags 'dest_kafka dest_nats dest_nsq dest_valkey' ./...— entire suite green, including the new bug-regression tests.🤖 Generated with Claude Code