MDEV-40023 Error on unknown commands in check_expected_crash_and_restart

[FarihaIS]

Description

check_expected_crash_and_restart() in mariadb-test-run.pl silently ignores anything in the expect file that does not start with restart. This let typos like restart_abort fall through to a default restart and made bugs like MDEV-39153 easy to miss.

Fix:

• Anchor the wait / restart / restart_bindir regexes and call mtr_error() on any unrecognized command
• chomp $last_line before matching so error messages don't embed a newline
• Skip empty last lines, since partial writes to the expect file can be observed before the intended command is fully written (same race the existing -z $expect_file check guards against)

restart_bindir is left in place as it is used during development to test binary/data migration between two build trees.

Release Notes

N/A

How can this PR be tested?

Execute the full MTR test suite — no test uses restart_bindir, so removing it should have no effect. The mtr_error() change will prevent any future invalid expect file commands from failing silently.

Basing the PR against the correct MariaDB version

• This is a minor fix, and the PR is based against the main branch.

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

[gemini-code-assist]

Code Review

This pull request simplifies the restart command handling in the MariaDB test suite by removing the unused restart_bindir command and its associated environment variable handling. It also introduces strict error checking for unknown commands in the expect file. The review feedback highlights that the regular expression /^restart/ is too loose and will match any command starting with "restart" (e.g., "restarting"), suggesting it be tightened to /^restart\s*$/ to prevent silently ignoring invalid commands.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[CLAassistant]

All committers have signed the CLA.

[gkodinov]

Thank you for your contribution. This is a preliminary review.

LGTM. Please stand by for the final review.

[Copilot]

Pull request overview

Cleans up the .expect-file crash/restart control path in MTR by removing unused restart_bindir support and surfacing invalid expect-file commands as errors, to avoid silent misbehavior during crash/restart tests.

Changes:

• Remove the restart_bindir handling from check_expected_crash_and_restart() and from start_mysqld.inc.
• Treat unrecognized expect-file commands as fatal via mtr_error() (instead of silently ignoring them).
• Skip empty expect-file lines to mitigate partial-write races.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
mysql-test/mariadb-test-run.pl	Removes restart_bindir logic; adds stricter parsing/erroring for expect-file commands; skips empty lines.
mysql-test/include/start_mysqld.inc	Removes generation of restart_bindir commands in expect files.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[midenok]

This was done for MDEV-25004 and was used to test upgrade between two build trees. I don't think removing such feature unused in main mtr workflow is a good idea. I've used it in the process of development and may use it later in such projects as MDEV-16417. It is useful for any development when you have to check how the binary data migrates the versions.

[midenok]

The code is not dead, it is used in development.

[FarihaIS]

@midenok thank you for your review, I had initially created this PR - and associated Jira MDEV-40023 - due to these two comments on my other PR (comment 1 and 2).

Could you please let me know what the consensus is and whether these changes are still valid? Thank you!

[midenok]

@midenok thank you for your review, I had initially created this PR - and associated Jira MDEV-40023 - due to these two comments on #5221 (comment #5221 (comment) and #5221 (review)).

Could you please let me know what the consensus is and whether these changes are still valid? Thank you!

@FarihaIS Like I said in this PR I disagree with these changes. Is there some different questions by the provided links? Sorry, I just cannot understand. Please solve your task by improving the code, not by removing it. Is it the answer to your questions?

[FarihaIS]

Thanks @midenok, that makes sense - I wasn't aware restart_bindir was actively used in development as the comment on my other PR suggested otherwise.

I've rescoped this PR to keep restart_bindir intact and only address the underlying MDEV-39153 issue: mtr_error() on unrecognized expect-file commands, plus modifying the wait/restart regexes so typos actually reach that branch.

Please let me know if I have missed anything, thank you.

Also, noting here that this change depends on #5221, i.e. main.change_master_default.test still uses restart_abort: (fixed there), which the new mtr_error() correctly catches, so CI here will fail until #5221 is merged.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[midenok]

@FarihaIS I think this one should be rebased on top of #5221. Also please resolve 3 last Copilot comments above. For non-legit ones just comment briefly why.

[FarihaIS]

@midenok I addressed all the Copilot comments, thank you. As for the rebase on #5221, should I do this now (which would retarget this PR to 12.3, since #5221 is against 12.3), or wait for #5221 to merge first?