SEP-2035: Stage each Mydumper dump and publish it atomically - #1509
marcuscruz-percona wants to merge 4 commits into
Conversation
Mydumper was pointed straight at the day-granular output directory with none of --clear, --dirty or --merge, so every run after the first on a given day died on mydumper's own "Directory is not empty" refusal. Each run now dumps into a hidden per-run staging directory beside the day directory and, once the dump is complete and encrypted or hardlinked, renames it into place. An already-published dump is moved aside under a name unique to the run rather than deleted: a failed promotion puts it straight back, and a crash between the two renames leaves it for the reclaim pass the next run performs. A staging directory is abandoned only once the process named in it is gone, so a concurrent run against the same server keeps its own dump. Both scratch names are dot-prefixed, which keeps them out of retention, restore and upload. Hardlinking, checksumming and post-run encryption moved ahead of publication, so a failure in any of them costs only the staged copy. That puts the space saving before the purge, so a previous day this run is about to purge is no longer hardlinked against, where the removal would free nothing. The payload had no executing test coverage; the AST harness now takes a payload path and this adds 36 tests for the staging, publication, reclaim, retention, upload and failure-isolation behaviour.
github-actions
Bot
added
python
app:mysql_backups
There was a problem hiding this comment.
🟡 Changes recommended
Retention-aware incremental hardlinking and concurrent crash recovery can preserve or restore the wrong backup data.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Stages Mydumper backups before atomically publishing them, preserving existing same-day backups on failure.
Changes:
- Adds staging, promotion, crash recovery, and retention-aware hardlinking.
- Generalizes the payload AST test harness.
- Adds comprehensive same-day rerun and failure-isolation tests.
File summaries
| File | Description |
|---|---|
app/sep/apps/mysql_backups/mydumper_payload |
Implements staged backup publication and recovery. |
tests/app/sep/apps/mysql_backups/test_mydumper_same_day_rerun.py |
Tests publication, recovery, retention, and failures. |
tests/app/sep/apps/mysql_backups/payload_harness.py |
Supports multiple payloads and shared logging. |
tests/app/sep/apps/mysql_backups/conftest.py |
Adds generic payload AST loading. |
tests/app/sep/apps/mysql_backups/test_xtrabackup_incremental_base_guard.py |
Reuses the shared recorder. |
changelog.d/SEP-2035.fixed.md |
Documents the same-day rerun fix. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… rule Restore the newest moved-aside dump rather than the one whose run started first. Several runs can each have moved a dump aside for one day; the name carries a start time, which is not the order they published in, so restoring by name could put back the older copy and then delete the newer one against the restored day. A rename leaves the moved-aside directory's own mtime alone, so that is what decides now. Apply the purge-aware retention check to the --updated-since hardlink base as well, not only to the previous-day base. Hardlinking out of a day this run removes leaves the removal freeing nothing, because the staged dump holds those inodes. Reclaim a staging directory as soon as the process named in it is gone. The age grace exists for names that carry no readable pid, but a dead pid was taking it too, so a killed run's leftovers survived for an hour -- contrary to both the helper's docstring and the constant's comment. Pin UTF-8 when the test harness reads a payload's source, so AST extraction does not depend on the runner's locale.
marcuscruz-percona
added
the
qa passed
Coverage report
This PR does not seem to contain any modification to coverable code. |
The hardlink and --updated-since gates decided retention with their own copy of the purge's rule, and the copy disagreed: it counted every Monday, while the purge keeps only the newest few and deletes the rest. A run could still dedup into a Monday it was about to remove, which is the loss the gate exists to avoid. _retained_days now owns the decision and the purge reads it too, which drops the purge's mark-and-sweep bookkeeping. A scratch directory's name is parsed in one place rather than by a positional slice per field, and the tests read the scratch suffixes and the staging grace period out of the payload instead of re-stating them. The same-day rerun test drives run() per execution, so the reclaim, staging, publication and latest ordering under test is the payload's rather than the test's. Two new tests cover a Monday past the weekly allowance: one that the purge drops it, one that the hardlink gate declines it. The stub record that the run and space-saving tests assert against carries attributes instead of string keys, so an assertion that a stub was never reached cannot pass on a misspelled name.
github-actions
Bot
added
the
large-diff
Self-review against the ticket and the repo's standardsRan a two-axis review of this branch against One real defect, in the gate added in
|
yyyyyyyan
left a comment
yyyyyyyan
left a comment
There was a problem hiding this comment.
@marcuscruz-percona — the staging-and-publish protocol is careful work. _publish_backup moves an existing dump aside under a run-unique name rather than deleting it, restores it inline on a failed rename, and leaves the remaining state as the one shape _reclaim_interrupted_publish recovers. The hardlinks survive the rename because work_dir and prev_backup_dir are siblings under backup_server_dir, which is easy to get wrong and is right here. And the test suite turns a module with no executing coverage into 42 tests that drive the real lifted methods over a real filesystem — test_mydumper_error_never_mentions_a_non_empty_directory pinning the log non-empty before its negative assertion is the kind of detail that makes the rest trustworthy.
A hard-killed run's staging directory is stranded permanently once its pid is reused — app/sep/apps/mysql_backups/mydumper_payload:2266. _staging_owner_is_alive returns True whenever os.kill(pid, 0) does not raise ProcessLookupError, and the grace period now applies only to a name with no readable pid. So a run that dies without reaching run()'s cleanup — SIGKILL, OOM, host reboot, container eviction — leaves a .partial directory that is never reclaimed once that pid belongs to an unrelated live process. Nothing else removes them: _purge_old_backups iterates _list_backups, which filters on ^[0-9]{8}$. The staged bytes count against the is_free_space precheck, so this converts into "not enough free space" refusals with nothing in the task log naming the cause, and _reclaim_interrupted_publish's docstring at line 2320 states the guarantee the code does not deliver. MyDumper is constructed inside MyDumperRunner.run() at line 1898 — a forked short-lived child, the pid class most likely to be recycled. Details and the suggested three-line fix are inline.
Five smaller findings are inline — two more on the reclaim and retention paths, two docstring nits, and one asking that AC 4 record the retention gate this PR added.
Happy to re-review once the staging-directory liveness check lands.
| if not pid.isdigit(): | ||
| return None | ||
| try: | ||
| os.kill(int(pid), 0) |
There was a problem hiding this comment.
os.kill(pid, 0) answers "is some process holding this pid", not "is the run that created this directory still going". Since the fresh-dead-pid fix moved the grace period to cover only pid-less names, a recycled pid now means the staging directory is kept forever — and nothing else reclaims it, because _purge_old_backups walks _list_backups, whose ^[0-9]{8}$ filter excludes every dot-prefixed name.
The cost is not just the wasted bytes: they count against the is_free_space precheck at line 2016, so the failure eventually surfaces as a free-space refusal with no line in the task log pointing at the stranded directory. The exposure is near-certain where each allocation gets a fresh PID namespace — pids restart low, and a low pid is almost always live on the next start.
The process's own start time settles it. A process that started after the staging directory was created is a different process holding a recycled pid:
try:
os.kill(int(pid), 0)
started = Path(f"/proc/{pid}").stat().st_ctime
except ProcessLookupError:
return False
except OSError:
# The pid exists but is not ours to signal.
return True
return started <= mtimewith the directory's mtime passed in from _reclaim_scratch_dir, which already stats it. On Linux Path("/proc/<pid>").stat().st_ctime is the process start time as a wall-clock timestamp, so this needs no new import. Worth a regression case alongside test_discards_a_fresh_staging_directory_whose_process_is_gone that builds a name carrying a live pid older than the directory.
Going back to the previous alive or mtime < PARTIAL_MAX_AGE_SECONDS shape would reopen the hole this PR closed — that form deleted a live slow dump after an hour.
| :param scratch_dir: A moved-aside or staging directory from an earlier run. | ||
| :raises OSError: If the restore or the removal fails. | ||
| """ | ||
| if scratch_dir.name.endswith(REPLACED_SUFFIX): |
There was a problem hiding this comment.
Minor. This branch decides purely on whether the day directory exists, while the .partial branch below consults _staging_owner_is_alive — but both names carry the owning pid. A second run starting inside the window between the two renames at lines 2234-2235 sees the day directory missing, restores the first run's moved-aside dump into it, and the first run's work_dir.rename(self.backup_dir) then fails with ENOTEMPTY, discarding its fresh dump.
The window is two adjacent syscalls and the outcome is fail-safe — the published dump survives and the losing run raises — so this is small. It is worth closing because the constant comment at lines 77-80 establishes concurrent runs against one alias as reachable (the lock is per alias in the logging directory, line 1891), so the case is real rather than hypothetical. Consulting _staging_owner_is_alive here too and returning early when the owner is alive is the same call in a second place once the start-time check lands.
| mondays = [ | ||
| day | ||
| for day in days | ||
| if datetime.datetime.strptime(day, "%Y%m%d").date().isoweekday() == 1 |
There was a problem hiding this comment.
Minor. _list_backups matches ^[0-9]{8}$ without validating the date, so 20261345, 20260230 and 00000000 all reach this strptime and raise ValueError. That was survivable while the parse lived only in _purge_old_backups, which runs after publication — the dump shipped and only the purge failed. _retained_days is now also reached from _save_disk_space (line 2155) and _validate_updated_since (line 2356), both inside run()'s try, so a completed dump gets rmtree()'d instead of published.
It needs an operator-created or copied directory to trigger, hence minor. What makes it worth a fix is that the day-name concept is already spelled two ways that neither agree with each other nor validate the date: BACKUP_DIR_REGEX = r"^2[0-9]{7}$" at line 39, used by _collect_paths (line 1433) and now by _reclaim_scratch_dir (line 2295), versus _list_backups's own re.compile("^[0-9]{8}$") at line 2472. One shared predicate that both matches the shape and parses the date, used by all four, makes the "scratch names stay out of retention" invariant hold by construction and removes the crash from both call paths at once.
| self.logger.warning("Leaving %s in place: %s", scratch_dir.name, exc) | ||
|
|
||
| def _validate_updated_since(self) -> None: | ||
| """Validate previous backups that can be used with mydumper's --updated-since option.""" |
There was a problem hiding this comment.
Minor. The body gained a new governing condition at line 2356 — a candidate this run's purge will remove is skipped outright, and the option is switched off when that leaves nothing — but the summary still describes only the mydumper-option validation. _day_is_retained documents the rationale at its own definition (line 2191), so a reader arriving here gets no signal that retention now participates in base selection. The sibling _save_disk_space has the same widening, but its "if conditions allow" summary already admits it.
A sentence covers it, e.g. "A day this run's purge removes is passed over: hardlinking out of it would leave the removal freeing nothing."
| ] | ||
| return days[: max(self.daily_purge, 0)], mondays[: max(self.weekly_purge, 0)] | ||
|
|
||
| def _day_is_retained(self, day_name: str) -> bool: |
There was a problem hiding this comment.
Minor, and about the ticket rather than the code. AC 4 makes a closed preservation promise — prev_backup_dir / valid_prev_backup_dir "continue to resolve to a single day's dump-file directory, so the encryption check, the --updated-since validation, and the hardlinking path keep reading what they already expect." This gate changes what two of those three accept. The PR description records it; SEP-2035 does not, so a later reader reconciling ticket against diff finds a silent delta.
The gate itself is correct and prevents a real cost — amending AC 4 to say the previous-day and --updated-since bases are now additionally required to survive this run's purge is all that is needed.
| """Build the name a run gives one of its scratch directories. | ||
|
|
||
| :param suffix: The scratch suffix, read off the payload. | ||
| :param parts: The dot-separated segments between the day and the suffix -- |
There was a problem hiding this comment.
Minor. -- is standing in for an em dash here; a comma reads the same and is the conventional form for parameter documentation.
| :param parts: The dot-separated segments between the day and the suffix -- | |
| :param parts: The dot-separated segments between the day and the suffix, |
payload_harness.py:296 and :322 carry the same shape on pre-existing lines — worth sweeping while you are in the file.
Summary
--clear,--dirtyor--merge, so every run after the first on a given day died on mydumper's ownDirectory is not emptyrefusal — any sub-daily schedule failed from its second run onward. Each run now dumps into a hidden per-run staging directory beside the day directory and renames it into place once the dump is complete, so the published shape (dump files directly in<BACKUP_DIR>/mydumper/<alias>/<YYYYMMDD>,latestpointing at it) is unchanged.mydumper_payloadhad no executing test coverage. The AST harness now takes a payload path, and this adds 36 tests covering the staging target, publication, crash reclaim, retention and upload blindness to scratch directories, the double-space rule, and failure isolation for the dump, hardlinking and encryption paths.Accepted consequences
latestresolves through a directory name that is absent for the microseconds between the two renames.location, so the older catalog row's location resolves to the newer content.MysqlBackupRun.task_history_idremains the unique key; this is inherent to day-granular retention.--updated-sinceagainst the first run's output — both look at previous days. Only the space and time saving is missed.Tested
<BACKUP_DIR>/mydumper/<alias>/<today>holds dump files directly andlatestpoints at itlateststill resolves, and noDirectory is not emptyline appears in the task logMYDUMPER_EXTRA_ARGS) and confirm the first run's dump is intact and no.partialdirectory survives the next successful run/latestrestore path resolvesAutomated:
pytest tests/app/seppasses (exit 0);tests/app/sep/apps/mysql_backupsis 886 passed / 29 skipped. Payload is 13,796 of the 16,384-byte Nomad limit.make lintandmake run-pre-commitclean.Checklist
make test)make run-pre-commit)make makemigrations)changelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)