Skip to content

Stop produce loops for nonexistent partitions - #331

Open
novatechflow wants to merge 3 commits into
KafScale:mainfrom
novatechflow:fix/326-invalid-partition-produce-loop
Open

novatechflow wants to merge 3 commits into
KafScale:mainfrom
novatechflow:fix/326-invalid-partition-produce-loop

Conversation

@novatechflow

@novatechflow novatechflow commented Nov 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #326. Producing to a partition absent from an existing topic could repeatedly auto-create the topic, receive ErrTopicExists, and retry the same failed lookup forever. Allow one auto-creation attempt per partition-log lookup, then return UNKNOWN_TOPIC_OR_PARTITION if the partition is still missing. Requests with acks=0 complete without sending a response.

A lease left by a prior invalid request also caused a second broker to return the wrong ownership error. Failed ownership checks now refresh metadata once per request and distinguish a nonexistent partition from a real partition owned elsewhere. Healthy owned requests skip this work. A failed metadata refresh does not infer that a partition is absent. Topic-level metadata errors are preserved, including INVALID_TOPIC_EXCEPTION, instead of being misclassified as a missing partition. Partition membership is checked only for successful topic entries.

Valid partitions in the same request still succeed. Missing-topic auto-creation continues to work, while concurrent creation or stale metadata cannot cause another unbounded retry. The unreleased notes are updated.

Validation

  • New regressions reproduce the loop on main for acknowledgements -1, 1, and 0, including zero request timeouts. Auto-creation disabled also reproduced the wrong error code.
  • Ten repeated race-detector runs pass for invalid/boundary partition IDs, every acknowledgement mode, mixed valid/invalid partitions, concurrent creation, stale metadata, normal auto-creation, and real embedded-etcd leases.
  • Invalid requests leave existing topic partition counts and valid offsets intact and create no partition log.
  • Hard review reproduced the wrong error on a second broker. Ten repeated race runs cover the fix, stale and unavailable metadata, mixed ownership errors, and 32 concurrent successful producers with unique acknowledged offsets.
  • The linked review finding is reproduced by two broker handlers sharing embedded etcd for every invalid topic name. Additional tests preserve unknown-topic, invalid-topic, authorization, leader, timeout, and server metadata errors. Ten repeated race-detector runs pass.
  • Full go test -race -p 1 ./... and go vet ./... pass on ca429d6. Hosted CI and CodeQL also pass, including authentication coverage and all end-to-end checks.

@novatechflow
novatechflow marked this pull request as ready for review November 14, 2026 12:01

@novatechflow novatechflow left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 808db4c; no remaining blockers.

The original implementation reproduced the busy loop for acknowledgements -1, 1, and 0, including requests with no timeout. It also returned the wrong error when auto-creation was disabled. The fix bounds each partition-log lookup to one auto-creation attempt and one retry, then reports UNKNOWN_TOPIC_OR_PARTITION if the partition is still absent. Requests with acks=0 complete without a response.

Hard review also reproduced the wrong error when a second broker encountered a lease for the nonexistent partition. The fix refreshes metadata once after failed ownership checks; real non-owner partitions retain leader redirection, stale metadata is refreshed, and unavailable metadata does not invent an unknown-partition error.

Ten repeated race-detector runs pass for invalid and boundary partition IDs, mixed valid/invalid requests, concurrent topic creation, metadata remaining stale after creation, normal auto-creation, embedded-etcd leases across two brokers, and 32 concurrent producers with unique acknowledged offsets. Existing partition counts and valid offsets are preserved, no log is initialized for the rejected partition, and valid partitions in the same request still succeed. Full repository race tests, vet, hosted CI, and CodeQL pass on this head.

Release-note draft updated: producing to a nonexistent partition no longer spins or waits for the request timeout; requests requiring a response receive the correct partition error, while acks=0 retains its no-response behavior. No configuration migration is required.

@PaxMachinaOne PaxMachinaOne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 PaxMachina automated review (worker-code)

The loop fix is sound, but lease-error classification mishandles invalid topic names.

  • cmd/broker/main.go:1065: known is initialized before confirming a successful topic metadata entry. For an invalid topic, Metadata returns INVALID_TOPIC_EXCEPTION; the loop skips that entry, leaves known empty, and converts another broker’s ErrNotOwner into ErrUnknownTopic. Thus identical invalid requests can return INVALID_TOPIC_EXCEPTION from the lease owner but UNKNOWN_TOPIC_OR_PARTITION from another broker. Preserve/map the topic-level error instead of treating every nonzero metadata error as proof that the partition is absent.

@novatechflow novatechflow left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the review finding. It was valid and is fixed in ca429d6.

Topic metadata errors are stored separately from partition membership. Only successful topic entries build a partition set; nonzero topic errors retain their Kafka error codes. Invalid names now return INVALID_TOPIC_EXCEPTION from both lease owners and other brokers.

Reproduced the original failure for all six invalid names in the existing test set, plus injected authorization, leader, timeout, and server metadata errors. Ten repeated race-detector runs pass with two broker handlers sharing embedded etcd, alongside the unknown-partition, ownership, stale-metadata, and concurrent auto-creation regressions. Full repository race tests, vet, hosted CI, and CodeQL pass on this head.

Re-review found no remaining blockers. The release-note draft now also notes that topic-level metadata errors are preserved across lease owners.

@PaxMachinaOne PaxMachinaOne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 PaxMachina automated review (worker-code)

The retry cap is sound, but missing-topic auto-creation still permits a partition-ID resource-exhaustion path.

  • High: loadPartitionLog passes the client-controlled partition ID to ensureTopic (cmd/broker/main.go:2329), which calculates NumPartitions = partition + 1 (cmd/broker/main.go:2220). For a missing topic, a request targeting partition 2147483646 attempts to allocate roughly 2.1 billion partition metadata entries, potentially crashing the broker; MaxInt32 also overflows partition+1 and produces incorrect behavior. The boundary test only uses an existing topic, so it never exercises this auto-creation path. Validate/cap requested partition IDs before auto-creation and add a missing-topic boundary regression.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

broker: produce to a nonexistent partition busy-loops in loadPartitionLog and never replies

2 participants