[tinswzy]

Normal segment rollover waits for completion when the old segment's pending append queue is empty. With embedded MinIO and conditionWrite: disable, fencing sees the live writer's own write.lock and waits 30 seconds, blocking creation of the next segment and foreground writes.

Make the empty-queue path trigger the same background completion lifecycle used after pending appends drain, then return immediately. Existing background failure/takeover notifications, completion metadata guarantees, and explicit close/recovery waits remain in place.

Closes #438.

Related: #437 (normal-rollover report), #434 (recovery timeout involving the same fallback wait), and Milvus discussion #49417 (fallback-mode recovery context).

Regression coverage

• Block fencing in a unit test and verify rollover returns, old-segment appends are rejected, and Completed metadata is published only after completion.
• Verify background fence/finalize failures and metadata revision conflicts still invalidate the writer.
• Add embedded-MinIO integration cases for size, interval, and 20 consecutive rollovers, with conditional writes explicitly enabled/disabled. Assert every append is under 5s, rollover p95 is at most ordinary-write p95 + 1s, and all data/segment finalization checks pass.
• Run this suite in a dedicated PR/nightly/manual CI workflow, exclude it from the component workflow, reject all-skipped fallback results, retain latency artifacts, and integrate CI labels/rerun commands.

Validation

• go test -race ./woodpecker/segment ./woodpecker/log -count=1 -timeout=4m — passed.
• Executed the dedicated workflow's exact gotestsum test step locally against external MinIO/etcd, including -cover -count=1: all six subtests and the result guard passed (~217s).
• golangci-lint run --config .golangci.yml --timeout=10m — passed (0 issues).
• actionlint for all changed workflows and git diff --check — passed.

Local disabled-mode rollover appends improved from 30.27–30.31s to approximately 0.214s. In the 20-roll/42-write CI-command run, ordinary-write p95 was 214ms, rollover-write p95 239ms, and all 42 entries were read back successfully.

Scope

This optimizes foreground writes during normal rollover. The fallback fencing grace period remains: cross-segment reads still exhibited approximately 28–29s waits, and explicit writer close approximately 30s. This PR does not claim to fully resolve the tSafe/strong-consistency failures in #437.

[github-actions]

CI commands

Comment any of these to rerun checks on this pull request:

Command	Reruns
/rerun	every failed workflow on the latest commit
/rerun-lint	Lint
/rerun-mod-tidy	Go Mod Tidy Check
/rerun-unit-test	Unit Test
/rerun-e2e-local	E2E Local
/rerun-e2e-objectstorage	E2E Object Storage
/rerun-e2e-service	E2E Service
/rerun-component-test	Component Integration Test
/rerun-chaos	Chaos Test
/rerun-service-stability-process	Service Stability (Process)
/rerun-service-stability-docker	Service Stability (Docker)
/rerun-operator	Operator Test
/rerun-upgrade-local	Upgrade Compat Local
/rerun-upgrade-objectstorage	Upgrade Compat Object Storage
/rerun-upgrade-service	Upgrade Compat Service

The ci-passed label is applied automatically once every required check passes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.17%. Comparing base (33164f6) to head (f4be9be).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
+ Coverage   84.08%   84.17%   +0.09%     
==========================================
  Files         221      221              
  Lines       31311    31300      -11     
==========================================
+ Hits        26329    26348      +19     
+ Misses       3737     3715      -22     
+ Partials     1245     1237       -8     

Components	Coverage Δ
Server	83.74% <ø> (+0.06%)	⬆️
Client	91.04% <100.00%> (+0.28%)	⬆️
Meta	85.58% <ø> (ø)
Common	88.33% <ø> (+0.06%)	⬆️

Files with missing lines	Coverage Δ
woodpecker/log/log_handle.go	91.43% <ø> (ø)
woodpecker/segment/segment_handle.go	92.40% <100.00%> (-0.06%)	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tinswzy]

/rerun-unit-test

[github-actions]

Rerunning unit-test workflow. Check the Actions tab for progress.

[czs007]

Must-fix issues introduced by this PR: 0
Merge recommendation: Mergeable as-is — this review found no must-fix issue introduced by this PR.

Adversarial review found no issues requiring changes.

The empty-queue rollover now uses the same async completion path as the non-empty drain path, and the two remaining findings are cleanup items that don't affect runtime behavior.

Verified:

• woodpecker/segment/segment_handle.go:2370-2386: no self-deadlock. TriggerCompletion() is a non-blocking buffered-channel send. The background completion takes s.Lock() only in phase 3, on another goroutine, after the caller has unlocked.
• woodpecker/segment/segment_handle.go:296 / :316: no append can slip in after rolling starts. rollingState is set under the same lock before appendOpsQueue.Len() is read, and AppendAsync checks it again under that lock. An in-flight append therefore cannot race the empty-queue decision.
• completion_manager.go:147 / segment_handle.go:1202-1205: every completion failure invalidates the writer. Fence/complete retry exhaustion and the phase-3 revision conflict both reach NotifyWriterInvalidation. isWriterValid applies to the whole log (internal_log_writer.go:97), so writes to the already-created next segment also stop.
• woodpecker/log/log_handle.go:1201-1216: Close waits for in-flight completion. completeAllSegmentHandlesUnsafe calls ForceCompleteAndClose on every handle, including the segment that was rolled away from, and that call waits via WaitForCompletion.
• pr-ci-label.yaml:100-103: the new required "Embed MinIO Without ConditionWrite" check is skipped when its paths filter doesn't match, so docs-only PRs can't get stuck on ci-passed.
• e2e_truncate_test.go:294-309: the EventuallyWithT wrapper allows for the async completion timing and still asserts that the target segment reaches Truncated, so it does not hide a regression.

Low / nitpick (non-blocking)

Low

• woodpecker/log/log_handle.go:565: the ErrMetadataRevisionInvalid abort branch can no longer run, and its comments and unit test are out of date.

  • Problem: This PR changed segmentHandleImpl.SetRollingReady (segment_handle.go:2370-2386) so that it always returns nil, and it is the only non-mock implementation. As a result, the rollErr != nil && werr.ErrMetadataRevisionInvalid.Is(rollErr) branch is dead code. rollWritableSegmentUnsafe now has one real abort path: ErrMetadataSegmentAlreadyExists at line 591.
  • Out-of-date comments: The function comment at lines 547-549 ("Both abort paths below mean another writer has taken the log over…") and the comment at lines 561-564 still describe a synchronous revision conflict that can no longer happen.
  • Misleading test: log_handle_test.go:2328 (TestLogHandle_GetOrCreateWritableSegmentHandle_RollingMetaRevisionInvalid_AbortsCreation) uses a mock at line 2343 to make SetRollingReady return ErrMetadataRevisionInvalid. That exercises a branch the real implementation never reaches, so it suggests takeover detection is still covered there.
  • Not a regression: Removing the inline fence and complete for an empty queue is intentional. Takeover is now detected when the async completion fails and calls NotifyWriterInvalidation.
  • Suggestion: Remove the dead branch, or change SetRollingReady so it no longer returns error. Update the comments at lines 547-549 and 561-564 so they describe the single remaining abort path. Delete the test at line 2328, or rewrite it to cover the real async invalidation path.

  (raised by xiaocai2333, bigsheeper)

Nitpick

• tests/integration/embed_minio_rollover_condition_write_test.go:170: the p95 helper doesn't handle an empty slice.

  • Problem: The closure indexes latencies[(95*len(latencies)+99)/100-1] without checking the length. With the current parameters both slices are non-empty: line 163 requires segmentCount-1 rolling samples, and each segment adds at least one ordinary sample. If segmentCount is ever set to 1, rollingLatencies will be empty, the index becomes -1, and the test panics instead of failing with a readable assertion.
  • Suggestion: Add require.NotEmpty(t, latencies), or an equivalent early guard, at the start of the closure.

  (raised by xiaocai2333)

[tinswzy]

/rerun-all

[tinswzy]

/rerun

[github-actions]

Rerunning 1 failed workflow(s). Check the Actions tab for progress.