Skip to content

GH-51095: [CI][C++] Fix core file detection in run-test.sh - #51121

Merged
kou merged 8 commits into
apache:mainfrom
hiroyuki-sato:topic/fix-run-test-core-search
Sep 2, 2026
Merged

GH-51095: [CI][C++] Fix core file detection in run-test.sh#51121
kou merged 8 commits into
apache:mainfrom
hiroyuki-sato:topic/fix-run-test-core-search

Conversation

@hiroyuki-sato

@hiroyuki-satohiroyuki-sato commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Rationale for this change

In #50934, find was used instead of (ls /tmp | grep $PATTERN) to find core files in a ShellCheck-safe way, but using find can cause a race condition when files or directories under /tmp are removed by another process while find is scanning the directory.

What changes are included in this PR?

This change uses shell globbing instead of find to avoid this issue.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #51095has been automatically assigned in GitHub to PR creator.

@pitrou

Copy link
Copy Markdown
Member

This seems to fix #51095 for me but I'm not sure why.

If this is just ignoring the error in case of a race condition, it means that in some cases the coredumps might not be detected.

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@pitrou

It looks like I misunderstood which part needed to be fixed after looking at this error and the suggested change below it.
#51095 (comment)

find: ‘/tmp/buffered-test-0gzee19x’: Aucun fichier ou dossier de ce nom

means No such file or directory, so the error occurs because it is trying to find /tmp/buffered-test-0gzee19x.

The part I changed is related to files whose names start with core, so my change is not related to this error.

I'll take another careful look.

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@kou@pitrou Could you take a look again when you get a chance?

Two of the three CI failures produced core files, so in a sense, it seems to be working as expected.
(Although I think this error needs to be fixed separately.)

ARM64 Ubuntu 22.04 C++
https://github.com/apache/arrow/actions/runs/33577007730/job/100083130605?pr=51121#step:8:7779
https://github.com/apache/arrow/actions/runs/33577007730/job/100083130605?pr=51121#step:8:10298

AMD64 Conda C++ AVX2
https://github.com/apache/arrow/actions/runs/33577007730/job/100083130684?pr=51121#step:8:8257
https://github.com/apache/arrow/actions/runs/33577007730/job/100083130684?pr=51121#step:8:10788

Comment threadcpp/build-support/run-test.sh
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 2, 2026
Comment threadcpp/build-support/run-test.sh Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 2, 2026

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you call print_coredumps only when the test is failed?

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 2, 2026
@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@kou Thank you for your review. Applied suggested change

Could you call print_coredumps only when the test is failed?

Does this mean the following?

if [ "$STATUS"-ne 0 ] ;then
print_coredumps
fi

Comment threadcpp/build-support/run-test.sh Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 2, 2026
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 2, 2026
@pitrou

pitrou commented Sep 2, 2026

Copy link
Copy Markdown
Member

Two of the three CI failures produced core files, so in a sense, it seems to be working as expected.
(Although I think this error needs to be fixed separately.)

Wow. We never noticed this before. Do you think those were ignored silently @kou ?

Edit: Ah, it's a ASSERT_DEATH test. Nevermind.

kou
kou approved these changes Sep 2, 2026

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@kou
kou merged commit a62870c into apache:mainSep 2, 2026
55 of 57 checks passed
@koukou removed the awaiting change review Awaiting change review label Sep 2, 2026
@github-actionsgithub-actionsBot added the awaiting merge Awaiting merge label Sep 2, 2026
@hiroyuki-sato
hiroyuki-sato deleted the topic/fix-run-test-core-search branch September 2, 2026 08:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@hiroyuki-sato@pitrou@kou