objstore: wait for metadata workers after directory listing errors

[wjhuang2016]

What problem does this PR solve?

Issue Number: close #70807, close #67704

Problem Summary:

When object listing fails, UnmarshalDir closes its result channel without waiting for already scheduled metadata workers. A worker that later sends a result can panic. Also, when the error channel and closed result channel are both ready, iteration can report normal completion instead of the listing error.

What changed and how does it work?

Always join the worker group before closing the result channel, retaining the original listing error when both listing and workers fail. When observing a closed result channel, check for a pending error before returning normal completion.

The worker-lifetime regression blocks a metadata read, fails listing after the read starts, and checks that iteration cannot terminate until the worker completes. Before the fix it terminates prematurely; cleanup cancels the blocked read rather than deliberately crashing the test process with a send on the closed channel. A second repeated regression verifies that a completed failed listing cannot silently become EOF. Both regressions fail before the fix and pass afterward.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Ready validation:

make bazel_prepare
# Before the fix: premature termination and lost-error regressions fail.
./tools/check/failpoint-go-test.sh pkg/objstore -p 4 -run '^TestUnmarshalDir' -count=1
./tools/check/failpoint-go-test.sh pkg/objstore -p 4 -run '^TestUnmarshalDirReturnsWalkError$' -count=1
# After the fix: 10 repeated runs pass with no reported data races.
./tools/check/failpoint-go-test.sh pkg/objstore -p 4 -race -run '^TestUnmarshalDir' -count=10
make lint
git diff --check

Failpoints were disabled automatically after each run. Tests use a controlled storage implementation, not a live cloud account. Full-suite and end-to-end BR/PiTR tests were not run locally.

Related migration-version regression (#67704):

MigrationExt.Load passes migration decoding/version-validation errors through the same UnmarshalDir callback and returns the iterator error. Add coverage for a worker callback error after the producer has time to close the result channel. Temporarily restoring the old closed-channel branch makes this test fail with a nil error; restoring this PR's existing EOF handling makes it pass. No additional production change is needed.

The original TestUnsupportedVersion passed 200 runs both before and after the fix, so its original intermittent schedule was not directly reproduced. The callback-error regression provides the RED/GREEN evidence for the shared root cause.

make bazel_prepare
# New regression with the old closed-channel handling temporarily restored:
./tools/check/failpoint-go-test.sh pkg/objstore -run '^TestUnmarshalDirReturnsWorkerError$' -p 4 -count=1
# Existing fix restored:
./tools/check/failpoint-go-test.sh pkg/objstore -run '^TestUnmarshalDir' -p 4 -count=1
./tools/check/failpoint-go-test.sh br/pkg/stream -run '^TestUnsupportedVersion$' -p 4 -count=200
make lint
git diff --check

Final tests and lint pass; failpoints are disabled and production code is identical to the prior PR head.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

After a listing error, iteration now waits for scheduled workers as required for safe shutdown, instead of reporting an error while they are still running. Successful-listing behavior and storage formats are unchanged. Remote-failure latency was not benchmarked.

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix a possible panic or lost error when BR metadata loading encounters an object-storage listing or metadata decoding failure.

Summary by CodeRabbit

• Bug Fixes

  • Directory listing operations now reliably wait for in-progress file processing before completing.
  • Errors encountered while walking directories are correctly surfaced instead of being reported as a normal end of results.
  • Worker and listing errors are handled more consistently during directory scans.

• Tests

  • Added coverage for directory-walk failures, worker errors, and concurrent processing completion to help prevent regressions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 74be527e-00d6-40d8-8c13-8eb3f6df17fe

📥 Commits

Reviewing files that changed from the base of the PR and between 834664e and d45eba1.

📒 Files selected for processing (1)

• pkg/objstore/storage_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

UnmarshalDir now waits for active workers before closing its result channel and returns walk or worker errors through TryNext. Tests cover delayed workers and repeated error propagation. The Bazel test target adds the iterator dependency.

Changes

UnmarshalDir error handling

Layer / File(s)	Summary
Worker synchronization and error delivery pkg/objstore/helper.go	UnmarshalDir waits for submitted workers before closing results. It checks for pending errors before returning iter.Done.
Error propagation tests and test wiring pkg/objstore/storage_test.go, pkg/objstore/BUILD.bazel	Tests verify delayed worker completion, repeated walk-error propagation, and worker-error propagation. The test target adds the iterator dependency.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to d45eb

No actionable merge risk is established for the worker synchronization or error propagation change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue #70807. UnmarshalDir waits for the worker error group before closing the result channel. The iterator re-checks the error channel after result-channel closure, so it return…
Out of Scope Changes check	✅ Passed	The changes stay within issue #70807. The production change fixes worker lifetime and error propagation in UnmarshalDir. The test storage, regression tests, and Bazel dependency support automated co…
Title check	✅ Passed	The title clearly and concisely describes the main change: waiting for metadata workers after directory listing errors in objstore.
Description check	✅ Passed	The description is complete and follows the repository template. It includes linked issues, the problem, implementation details, unit-test coverage, validation commands, side effects, documentation im…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit saw workers race,
Then closed the channel at the proper pace.
Walk errors now arrive,
Worker errors also survive.
Tests guard the iterator’s trace.}

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.9927%. Comparing base (415c346) to head (d45eba1).
⚠️ Report is 45 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #71139        +/-   ##
================================================
- Coverage   76.3034%   73.9927%   -2.3107%     
================================================
  Files          2041       2133        +92     
  Lines        555380     607096     +51716     
================================================
+ Hits         423774     449207     +25433     
- Misses       130706     154265     +23559     
- Partials        900       3624      +2724     

Flag	Coverage Δ
integration	46.2953% <0.0000%> (+6.6278%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	58.8716% <ø> (ø)
parser	∅ <ø> (∅)
br	63.5030% <ø> (+0.7921%)	⬆️

🚀 New features to boost your workflow:

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

[wjhuang2016]

/check-issue-triage-complete

[flaky-claw]

/retest

[YuJuncen]

Consider make error channel a blocking channel?

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, YuJuncen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/objstore/OWNERS [D3Hunter,YuJuncen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-09-22 07:38:17.503572437 +0000 UTC m=+90422.728793534: ☑️ agreed by YuJuncen.
• 2026-09-22 08:19:13.011104859 +0000 UTC m=+92878.236325966: ☑️ agreed by D3Hunter.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-8.5: #71503.
But this PR has conflicts, please resolve them!

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-nextgen-20251011: #71504.
But this PR has conflicts, please resolve them!

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-nextgen-202603: #71505.