Repository navigation
MTR: preserve a pre-existing component manifest - #823
surendarchandra wants to merge 1 commit into
Conversation
oracle-contributor-agreement
Bot
added
the
OCA Verified
github-actions
Bot
added
Tests
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found three correctness issues in failure recovery: the END cleanup guard is initialized after manifest creation; failed keyring tests can leave test manifests permanently classified as user files; and File::Copy::move can leave a failed restore in a state the next run refuses to recover. Inline comments include suggested fixes and regression coverage. Static review only; PR code and tests were not executed.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 113 AIC · ⌖ 24.4 AIC · ⊞ 13.9K
Comment /codex to run again
67815f0 to
9423e38
Compare
There was a problem hiding this comment.
Found two shutdown defects: parent-only interrupts can restore the original before workers stop, allowing test teardown to destroy it; Windows/Cygwin error exits bypass the END hook and leave the original unrestored. Both are detailed inline.
Reviewed all three changed files and relevant lifecycle code. PR code/tests were not executed, as required. Existing review threads were unavailable because the connector denied access under its integrity policy.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 125.3 AIC · ⌖ 24.2 AIC · ⊞ 13.9K
Comment /codex to run again
9423e38 to
d07d538
Compare
There was a problem hiding this comment.
Found two manifest-preservation issues: concurrent MTR invocations can overwrite the shared backup, and failed --start-and-exit setup skips restoration before any server is detached. Both have inline comments with suggested regression coverage.
Reviewed all four changed files and the surrounding startup/shutdown paths. Validation was static only; no PR code or tests were executed. Existing review comments were unavailable because the read tool denied access under its integrity policy.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 178.4 AIC · ⌖ 24 AIC · ⊞ 13.9K
Comment /codex to run again
d07d538 to
bdb5f70
Compare
There was a problem hiding this comment.
Two findings: failed --start-and-exit startup still disables manifest restoration, and the new Perl regression tests are not registered with main.mtr_unit_tests. Details and suggested fixes are inline.
Reviewed all four changed files and relevant startup/cleanup callers. Static review only: PR code and tests were not executed; git diff --check passed and the working tree is unchanged. Existing review threads could not be read because of the access policy.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 179.3 AIC · ⌖ 24.2 AIC · ⊞ 13.9K
Comment /codex to run again
bdb5f70 to
70816b4
Compare
|
/codex |
|
✅ Codex PR Review completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Found two correctness regressions: skipped test entries can break a successful --start-and-exit handoff, and the persistent lock file can block later runs by another user with directory write permission. Both have inline comments.
Reviewed all six changed files statically; PR code and tests were not executed, as required by this workflow. Existing review threads were inaccessible under the read tool's integrity policy, so I could not check for duplicate findings.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 250.1 AIC · ⌖ 24.4 AIC · ⊞ 14K
Comment /codex to run again
70816b4 to
7117032
Compare
There was a problem hiding this comment.
Found one correctness issue: an all-skipped --start-and-exit run is mistaken for a successful server handoff, leaving the pre-existing manifest unrestored. See the inline comment for the control flow and suggested regression coverage.
Reviewed all six changed files and the surrounding lifecycle code statically; no PR-provided code or tests were executed. Existing review threads were inaccessible under the GitHub tool's access policy, so duplicate findings could not be checked.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 190.4 AIC · ⌖ 23.9 AIC · ⊞ 13.9K
Comment /codex to run again
mysql-test-run.pl treats <basedir>/bin/mysqld.my as scratch space it
owns. create_manifest_file truncates whatever is there (using a
two-arg open in write mode) and writes an MTR redirect manifest
{"read_local_manifest": true}; remove_manifest_file then unlinks
the file. There is no backup and no restore, so a component manifest
that was present before the run is silently destroyed.
The global manifest is the only mechanism that loads components before
InnoDB starts (mysql.component is InnoDB-resident). A distribution
or administrator who installs a manifest — for example to load a
keyring component for tablespace encryption at rest — loses it the
first time MTR runs against that basedir.
Design
------
Move the manifest aside (<path>.mtr_saved) before the first server
start; a same-directory rename preserves mode and ownership and needs
only directory write permission, so a read-only manifest is handled
without changing its permissions. Restore it from the single END hook
inside its parent-only guard, which is armed before the manifest is
touched. The restore is three-state idempotent:
* pending — backup exists, rename it back; clear state only on success.
* done — no-op (double-restore safe).
* (unset) — nothing was saved, nothing to do.
Additional safeguards:
* A run holds an exclusive flock on <path>.mtr_lock from the backup
until the restore, so a second run sharing the basedir dies before
touching anything instead of renaming its stub over the backup.
The lock file is created shared-mode (0666), as mtr_unique.pm does
for build-thread ids, so sequential runs by different users of a
group-writable directory can reuse it.
* The restore is skipped only when --start-and-exit hands off after
the worker reports that the servers are running (and every worker
exited 0): the detached server keeps running and needs MTR's
manifest in place, and the backup stays adjacent for the next MTR
run to adopt and restore. A --start-and-exit run whose setup or
server start fails, or whose tests are all skipped, is restored like
any other run.
* An existing backup from a crashed prior run is adopted only when the
file at the manifest path is MTR's own stub; if both hold different
content, MTR dies before truncating anything rather than guessing
which is current.
* When MTR creates the stub itself it also drops a marker
(<path>.mtr_stub). At the next start, a file beside a marker is
MTR-owned: a leftover stub is unlinked, and anything else (e.g. a
manifest written by an interrupted test) is moved to <path>.mtr_leftover
with a warning rather than being restored into the installation.
* die() is called before truncating if the rename fails.
* Every move is a same-directory rename(), never File::Copy::move: it
is atomic and has no copy fallback, so a failed restore leaves the
stub and the backup untouched for the END-block retry or for
adoption by the next run.
* Restore state is cleared only after the files are actually gone, so
the END-block safety net can retry after a failed unlink or rename.
* On exit the stub is unlinked only if it still holds MTR's own text; a
stub that changed during the run (e.g. a keyring test that failed
before its teardown) is quarantined to <path>.mtr_leftover with a
warning, so the next run never adopts it as a user manifest. If
that rename fails the marker is kept and the next run quarantines it.
* Restore never dies: an unreadable stub is reported and kept for a
later retry.
* A manifest that cannot be read is reported as such, never as a
content conflict.
Abort handling: when MTR is interrupted or dies, its workers may still
be running a test that rewrites the manifest (keyring tests rename their
own backup over it at teardown). The END hook therefore stops and reaps
the workers (stop_workers(), shared with the normal exit path) before it
restores the manifest. On Windows mtr_error() leaves via POSIX::_exit,
which skips END blocks, so the parent installs a pre-exit hook in
mtr_report that stops the workers and restores the manifest first.
The lifecycle logic is extracted into My::Manifest so it can be tested
independently of the MTR harness.
Test coverage
-------------
mysql-test/lib/t/manifest.t exercises the module with Test::More,
following the existing lib/t convention; manifest.t runs under
main.mtr_unit_tests, with test descriptions and skip reasons normalized
so the recorded result does not depend on the platform or user. Cases:
* crashed-prior-run backup adoption
* conflicting backup and newly installed manifest → refuse loudly
* double-restore no-op
* rename failure at create (die before truncate)
* whitespace-path open
* failed restore leaves stub and backup intact; retry restores, and a
next run adopts the backup without a conflict
* failed unlink keeps state for retry
* leftover-stub (marker) detection
* stub-text manifest without marker is preserved
* content written after the stub was created is quarantined, not restored
* interrupted keyring-style test leaves no permanent manifest
* mode/inode preservation across backup and restore
* manifest replaced during the run is quarantined at cleanup and not
adopted by the next run
* stub-write failure after the backup is restored by the END path
* unreadable manifest yields an accurate error
* unreadable stub at restore is kept and retried
* a concurrent run is refused while the first holds the lock; a lock
left by a dead run does not block the next one; the lock file is
mode 0666 regardless of umask and an existing one is reused
* --start-and-exit: a failed setup is restored; a hand-off leaves the
stub and backup for the next run, which restores the original
* root-unreliable chmod injection (SKIPped when running as root)
7117032 to
8057b8e
Compare
There was a problem hiding this comment.
Found two cleanup issues: an aborted --start-and-exit can restore the manifest while directly started servers remain alive, and Windows cleanup can restore the original before workers finish modifying it. Both need lifecycle regression coverage beyond the module-level state tests.
Static review only; PR code and tests were not executed, as required by this workflow.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by Codex PR Review for #823 · codex · gpt60 · 163.9 AIC · ⌖ 24.2 AIC · ⊞ 13.9K
Comment /codex to run again
github-actions
Bot
added
Build Passed
1 participant
What does this change do?
mysql-test-run.plno longer destroys a component manifest that exists before the run. The manifest is moved aside (<path>.mtr_saved) before the first server start and restored from MTR's singleENDhook inside its parent-only guard, so normal and abnormal exits both restore it. The lifecycle logic lives in a new module,mysql-test/lib/My/Manifest.pm, tested bymysql-test/lib/t/manifest.t.Behaviour:
END-hook safety net can retry after a failed unlink or move. Restore never dies; an unreadable file is reported as such, never as a content conflict.<path>.mtr_stub). At the next start a leftover stub is removed, and other content beside a marker (e.g. written by an interrupted test) is moved to<path>.mtr_leftoverwith a warning rather than restored into the installation.--start-and-exitskips only the restore; the next run restores it. Basedirs without a manifest behave as before, apart from the marker file.Why is it needed?
create_manifest_filetruncates whatever is at<basedir>/bin/mysqld.myand writes the redirect{"read_local_manifest": true};remove_manifest_filethen unlinks it. There is no backup and no restore, so a manifest that existed before the run is silently destroyed. The global manifest is the only way to load components before InnoDB starts (mysql.componentis InnoDB-resident), so a distribution or administrator who installs one, for example to load a keyring component for encryption at rest, loses it the first time MTR runs against that basedir. A read-only manifest, as the server itself recommends, makes MTR die at startup.How was it tested?
mysql-test/(Perl unit testmysql-test/lib/t/manifest.t)scripts/ci/mtr.shpasses locallymysql-testsuite, this branch vs the unpatched baseDetails:
mysql-test/lib/t/manifest.t(cd mysql-test && prove -Ilib lib/t/manifest.t): 125 Test::More assertions, all pass. Cases: interrupted-run adoption, conflict refusal, double restore, move failure, whitespace paths, retry after failed restore or unlink, marker/leftover handling, quarantine of post-stub content, mode/inode preservation, manifest replaced during the run, unreadable manifest/stub. Permission-injection cases are skipped when run as root.mysql-testsuite on this branch vs the unpatched base: identical failure sets with and without the patch after rerunning the deltas; zero patch-attributable regressions.Contributor checklist
scripts/ci/format.sh)AI assistance
If AI assistance was used, describe the tool(s) and extent of use:
An AI coding assistant was used to draft parts of the implementation, tests and this description. All changes were reviewed, built and tested by the author, who takes responsibility for them.
Areas touched
mysql-test harness (
mysql-test/mysql-test-run.pl, newmysql-test/lib/My/Manifest.pm), tests (mysql-test/lib/t/manifest.t). No server code.This contribution is submitted under the corporate Oracle Contributor Agreement that covers the author's submissions to the MySQL project.