Fix retention policy bucket collisions and improve directory pruning - #1
Merged
Merged
Conversation
Correctness fixes in the restic-style keep-* policy computation:
- Period bucket keys are now derived from fixed-width date formats in
FileInfo (Ymd / YmdH / Ym / oW / Y) instead of concatenating unpadded
integers. This fixes silent collisions where different days/hours mapped
to the same key (e.g. 2024-01-15 and 2024-11-05 both became "2024115").
- Weekly buckets now use the ISO-8601 year ("o") paired with the ISO week,
so dates in week 01/53 of a neighbouring year are bucketed correctly
(e.g. 2019-12-30 is ISO week 01 of 2020, not 2019).
- keep-hourly no longer excludes files created at hour 0 (midnight); the
`$hour > 0` guard let midnight files consume a bucket slot without ever
being retained.
- findFiles() sort comparator now returns 0 for equal timestamps instead
of being non-transitive, giving deterministic ordering.
- The "keep at least one" fallback no longer assumes a numeric key, which
is invalid once files have been grouped into an associative array.
Pruning robustness / safety:
- Recursive directory deletion iterates CHILD_FIRST so nested directory
trees (deeper than one level) are fully removed.
- Symlinks are never followed out of the tree; a symlinked entry is
unlinked (the link only), leaving its external target untouched.
- The shallow-path safeguard falls back to the raw path when realpath()
returns false, so it cannot be silently bypassed by a vanished path.
Dependencies / docs:
- Replace the monolog/monolog runtime requirement with psr/log (only the
PSR-3 interfaces are used), and update the README dependency note.
- Bump the phpunit dev dependency to the patched 10.5.62+ line.
- Fix apply() return-type docblock (Result, not array).
Tests: add coverage for the day/hour index collision, midnight hourly,
ISO-week boundaries, weekly counting across a new year, empty/zero/negative
policy config, equal-timestamp sort stability, grouping under a period
policy, nested-directory pruning, and symlink-escape safety.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz
…HPUnit CI failed with "This version of PHPUnit requires PHP >= 8.4.1" on PHP 8.3. The composer install step was fine (locked phpunit/phpunit 10.5.64 installs and runs cleanly on PHP 8.3). The phpunit test step's `version` input was left unset, and php-actions/phpunit@v3 defaults that to "latest" rather than "composer" as its docs suggest, so it downloaded PHPUnit 13.2.4 instead of using our own vendor/bin/phpunit. Pin version: composer so the action uses the project's own PHPUnit install. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz
The version: composer input doesn't run vendor/bin/phpunit directly -- it
reads the version string from composer.lock/composer.json and downloads a
matching PHAR from phar.phpunit.de. That patch version (10.5.64) has no
published PHAR, and the action's curl call doesn't check the HTTP status,
so it silently saved a 404/empty response as phpunit.phar and tried to
execute it ("syntax error: unexpected newline").
vendored_phpunit_path skips version detection and PHAR download entirely,
running the given path directly, so point it at our own composer-installed
vendor/bin/phpunit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz
vendored_phpunit_path correctly points at vendor/bin/phpunit now, but the phpunit step's container failed with "Operation not permitted" trying to chmod +x it. php-actions/composer and php-actions/phpunit each run in their own Docker container mounting the workspace, and vendor/ ends up owned by a UID the second container can't modify. Reset permissions on the runner (outside any container) between the two steps so either container can read/write/execute it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz
vendored_phpunit_path correctly pointed at our own vendor/bin/phpunit, but hit a new failure: "chmod: ... Operation not permitted". php-actions/composer and php-actions/phpunit each run in separate Docker containers mounting the workspace, and the phpunit container's user lacks ownership/CAP_FOWNER over files the composer container wrote -- chmod requires matching ownership or that capability, which permission bits alone can't grant, so a prior "chmod -R a+rwX vendor" step didn't help. This is the third distinct php-actions/* bug hit in a row (wrong default version, broken PHAR download for our patch version, now this). Switch to shivammathur/setup-php, the standard action for PHP CI: it installs PHP 8.3 with xdebug directly on the runner, so composer install and vendor/bin/phpunit run as plain steps under the same user throughout, with no nested-container ownership mismatch to work around. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes critical bugs in the retention policy algorithm where different dates could collide in the same bucket, and improves the directory pruning logic to handle nested directories and symlinks correctly.
Key Changes
Retention Policy Fixes
format('oW')) instead of calendar year, ensuring dates at year boundaries (e.g., 2019-12-30 in ISO week 01 of 2020) are bucketed correctly.$hour > 0guard that prevented files created at midnight (hour 0) from being retained by the hourly policy while still consuming a bucket slot.$day > 0,$week > 0,$month > 0, and$year > 0guards that could exclude valid files.Directory Pruning Improvements
LEAVES_ONLYtoCHILD_FIRSTiteration mode, ensuring nested directories are properly removed before their parents.FilesystemIterator::SKIP_DOTSand explicitly checksisLink()to handle symlinks correctly.realpath()returns false by falling back to the raw path.Code Quality
<=>) for cleaner, more idiomatic comparison.monolog/monologtopsr/logas the only required dependency, reducing bloat.php-actionstoshivammathur/setup-phpfor better control and reliability.Testing
Comprehensive edge-case tests added covering:
https://claude.ai/code/session_01WtQAKxkfQRpobn9ZGSotSz