Harden pre-migration backup retention edge cases - #3112
Chris0Jeky wants to merge 8 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23282f1105
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex address that feedback The regression-first head Please fix the reviewed root cause without broadening scope:
Run the focused backup hardening tests, leave the PR unmerged, and publish the exact new head for hosted qualification and re-review. |
|
@codex review Please re-review exact head |
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74ce65e836
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var snapshotFileName = temporaryFileName[..^TemporaryExtension.Length]; | ||
| if (!currentPattern.IsMatch(snapshotFileName) && !legacyPattern.IsMatch(snapshotFileName)) |
There was a problem hiding this comment.
When a stale candidate contains non-ASCII decimal digits in its timestamp or sequence, .NET's \d in both filename patterns still matches it even with RegexOptions.CultureInvariant, and this branch performs no subsequent parse or canonicalization before deleting the artifact set. BuildFileName only emits invariant ASCII digits, so such a file was not created under the helper's canonical contract but will nevertheless be removed after one day; use ASCII digit classes and validate the canonical timestamp/sequence before treating a staging file or sidecar as managed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 607eefaf77dcc865f033529956b015463516107b after a regression-first run.
RED evidence: test-only head 2606b5b9b662528e58e8b9e17c633d7267dd567c, Required CI run 34977431119, Ubuntu API job 104408910756 failed only Backup_preserves_stale_noncanonical_staging_names (3269 passed / 1 failed / 4 skipped). The first stale noncanonical target used an Arabic-Indic decimal digit and was incorrectly deleted.
The fix makes [0-9] explicit and centralizes canonical parsing for both retention and orphan cleanup: timestamp values must parse and round-trip through the emitted UTC format; sequences must be positive and round-trip through D6, while retaining arbitrary-precision support. The same parser now defines managed identity in both paths. Exact-head CI is running.
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Exact head
Please review the exact current head, especially the shared canonical filename parser and the regression-backed response to the Unicode-digit deletion finding. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607eefaf77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!File.Exists(candidate) && !File.Exists(candidate + TemporaryExtension)) | ||
| { | ||
| return candidate; |
There was a problem hiding this comment.
When a recent sidecar-only staging set has the next sequence (for example, completed snapshots end at 000005 and ...-000006.db.tmp-wal remains), this check ignores the sidecar and reserves 000006. WriteSnapshot then deletes that WAL/SHM before the new age-aware cleanup runs, bypassing the one-day liveness protection and contradicting the new recent-sidecar preservation contract. Include candidate + ".tmp-wal" and candidate + ".tmp-shm" in collision handling so the backup selects another sequence rather than deleting a potentially live sidecar.
Useful? React with 👍 / 👎.
|
@codex address that feedback with a regression-only remote commit first; do not change production code yet. On branch
Commit and push only that test to this PR branch, report the GitHub-resolvable head, and leave the expected RED for hosted API Integration. The production fix must wait until the causal RED is recorded. |
Summary
Testing
|
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh-context review: no merge-blocking correctness finding on this head.
The design is conservative in the right places: deletion eligibility is gated by one canonical parser shared by retention and orphan cleanup; sequence ordering no longer has an exclusion cliff; staging marker/WAL/SHM are treated as one liveness unit; sidecar-only debris remains rediscoverable; and cleanup stays after the durable replacement snapshot and non-fatal. The regression set exercises real SQLite creation plus stale/recent/unrelated, sidecar-only, non-ASCII/impossible names, zero/noncanonical sequences, and values beyond long.MaxValue.
I also rechecked the destructive boundary specifically: the deletion order is WAL, SHM, then marker, so a partial failure leaves a discoverable member; case identity is ordinal while strict parsing prevents case variants becoming managed targets; timestamp and sequence both round-trip before deletion. Exact-head Required CI, CI Extended, and Smart CI are now all green.
Residual risks are pre-existing/accepted rather than introduced here: same-key deployments sharing one absolute backup directory can still interfere, and last-write-time liveness cannot eliminate every cross-process TOCTOU on Unix. Neither is widened by this PR, and both are outside #1856's stated scope.
1 participant
Summary
Closes the three concrete
SqlitePreMigrationBackupresiduals in #1856 and the edge cases surfaced during review:.db.tmp-wal/.db.tmp-shmremnants;BigInteger, so every syntactically valid sequence remains orderable and prunable beyondlong.MaxValue;D6padding are required before Taskdeck will delete a file.Cleanup remains deliberately non-fatal and runs only after the new protective snapshot exists. A one-day age floor prevents reclaiming another live process's staging set.
Regression-first evidence
Sidecar-only orphan discovery
RED head
4bef8d2106bff30a6d9ff294970dd2495f6fe324:34953265511104329316338.db.tmp-walsurvived when the marker file was already absent.The implementation now enumerates strict marker, WAL, and SHM shapes and reconstructs the canonical staging set from any surviving member.
Non-canonical managed-name rejection
RED head
2606b5b9b662528e58e8b9e17c633d7267dd567c:34977431119104408910756Backup_preserves_stale_noncanonical_staging_names(3269 passed / 1 failed / 4 skipped).NET\d, so an impossible Taskdeck filename was deleted.The current parser uses
[0-9]and requires exact timestamp/sequence round-trips before a file becomes a managed deletion target.Verification
Exact head:
607eefaf77dcc865f033529956b015463516107b34979490042: success34979490734: success34979490636: all completed jobs green, including Linux API Integration and E2E Smoke; the long Windows API Integration lane is still running at the time of this evidence refreshScope
This PR does not alter backup creation, migration ordering, the fail-closed snapshot boundary, database locations, or release configuration. It only hardens retention/cleanup identity and crash-debris handling.
Closes #1856