Skip to content

Fence partition logs across lease changes - #327

Merged
PaxMachinaOne merged 4 commits into
KafScale:mainfrom
novatechflow:fix/324-partition-lease-log
Nov 13, 2026
Merged

PaxMachinaOne merged 4 commits into
KafScale:mainfrom
novatechflow:fix/324-partition-lease-log

Conversation

@novatechflow

@novatechflow novatechflow commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #324. Bind each cached partition log to the etcd revision of its lease acquisition, evict it on lease loss, and restore offsets and S3 segments before using a new acquisition. Retained handles and delayed restore/flush callbacks are fenced. Non-owners return NOT_LEADER_OR_FOLLOWER for fetches, and Metadata reports the current lease owner.

Create segments with If-None-Match: * before uploading their indexes. A losing writer cannot overwrite the segment or index, and collision cleanup never deletes the winner's objects. Ambiguous segment PUT failures preserve any orphan and subsequent collisions fail closed. Schema registry retries verify and reuse identical content-addressed schema objects.

Testing

  • Reproduced on unchanged 91f64e9 (the affected log/lease paths match upstream): A -> B -> A returns offsets 0, 1, 1, with a stale fetch and Metadata leader.
  • Regression now returns 0, 1, 2, preserves the acknowledged segment, fences old handles/callbacks, and isolates an in-flight restore from a later acquisition.
  • Added concurrent writer, collision retry, ambiguous upload, and conditional S3 request coverage.
  • go test -race ./cmd/broker ./pkg/metadata ./pkg/storage passed.
  • go test ./... passed on the initial PR head (5aa41b0); affected packages passed with the race detector after the review follow-up.
  • go test -race ./pkg/schemaregistry -run '^(TestLargeSchemaGoesToS3|TestJSONThreeRegistryConcurrency)$' -count=1 passed.
  • MinIO-backed make test-produce-consume unavailable locally because Docker is not running.

Checklist

  • Added regression coverage for the lease handoff and upload races
  • Added license header to the new test file
  • MinIO-backed end-to-end run

Review follow-up

The initial Go CI failure was the published API gate: callback fields made three exported config structs non-comparable. Existing config layouts and constructors are now unchanged; optional callbacks live in separate options types, with compile-time coverage for legacy literals and comparability.

Review regressions reproduced and fixed:

  • A rejected lease-guarded offset commit was still acknowledged; offset publication now returns errors before an append is accepted, and failed logs are evicted.
  • Retrying cleanup after a lost DELETE response could replace another writer's acknowledged segment. Ambiguous cleanup failures permanently fence the old handle.
  • Startup retried the same immutable probe key after a lost successful PUT response; each retry now uses a new key.
  • Earliest-offset discovery now returns the leader retry error for non-owners. Metadata reads one authoritative ownership snapshot per request.

Validation: the API compatibility script passes; race-enabled broker, metadata, storage, and broker-library suites pass; the startup lost-response test passes with the race detector. All three newly reproduced review regressions fail on the initial PR head and pass with these fixes.

The offset publication path also holds the read-state mutex while committing offsets and publishing the corresponding local segment. A regression reproduces the former OFFSET_OUT_OF_RANGE window during a successful append; readers now wait until both are consistent.

Final validation on c26ae73: full go vet ./..., full go test -race ./..., and the published API compatibility gate pass locally. The latest hosted run has passed its API and vet steps and is running the remaining tests. Legacy unkeyed-literal assertions live in their owning packages so vet checks remain enabled.

@PaxMachinaOne
PaxMachinaOne marked this pull request as ready for review November 13, 2026 09:39

@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.

  • Logs reload after lease changes; stale handles and callbacks are fenced.
  • Non-owner fetches redirect; Metadata reports the current owner.
  • Conditional S3 writes prevent overwrites and destructive collision cleanup.

Reproduced offsets 0, 1, 1 before; regression now verifies 0, 1, 2 with acknowledged data preserved.

@PaxMachinaOne
PaxMachinaOne merged commit 8bfe4d7 into KafScale:main Nov 13, 2026
14 checks passed
@novatechflow
novatechflow deleted the fix/324-partition-lease-log branch November 13, 2026 10:26
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: acked records overwritten when a partition lease moves away and back (stale cached partition log)

2 participants