[selul]

Requested by Marius · project thread

All Submissions:

• Have you followed the Optimole Contributing guideline?
• Does your code follow the WordPress' coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Part of #1136, which reports two failure modes behind the .1-1.webp files and stale references.

1. Two transfers could take the transfer lock at once. acquire_transfer_lock() read the transient and then wrote it, so two requests that started a few seconds apart (as in the customer logs) could both get a token. The lock is now stored with a write that fails when it already exists:

• persistent object cache: wp_cache_add() in the transient group, which is where set_transient() puts it;
• no object cache: INSERT IGNORE on the _transient_optml_transfer_lock row. The expiry row is written before the insert, so a request that dies right after it can't leave a lock that never expires, and written again after a successful insert (bypassing the cached copy) in case another request's release removed it in between.

Both use the same storage as set_transient(), so renew, release and maybe_reschedule() keep working unchanged. An expired lock can still be taken again, because the get_transient() read deletes it first.

2. A rollback that died after moving the file left it behind. rollback_and_update_images() moves the downloaded file into uploads before it generates sub-sizes. If the request died in between, the next attempt hit the orphaned file and WordPress picked a suffixed name. Now:

• the moved path is stored in _optimole_rollback_pending_file until the rollback of that attachment finishes;
• the retry copies the new download next to that file and renames it over it, instead of sideloading again. The image keeps its name, nothing is deleted, and a copy that fails partway leaves the old file intact. The stored path is only used when it resolves to a file inside the uploads directory (compared with wp_normalize_path(), so Windows paths match);
• the meta is also cleared when the retry finds the local metadata already saved;
• update_attached_file() runs right after the move instead of after the sub-sizes, so a request that dies during the slow part still leaves the attachment pointing at the restored file.

Not covered here: the shared process meta that concurrent move_images() starts overwrite (from the customer log analysis in the issue). Concurrent image moves themselves are already serialized by the GET_LOCK advisory lock from #1173/#1174.

How to test the changes in this Pull Request:

1. Offload some images, start a rollback, and kill PHP while sub-sizes are being generated (or throw from intermediate_image_sizes_advanced).
2. Retry the rollback. The restored file keeps its original name with no -1 suffix, and _wp_attached_file matches the file on disk.
3. Start two transfers in parallel (two quick clicks, or two curl calls to the start endpoint). Only one chain gets a lock token.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

Check	Result
PHPUnit full suite (WP trunk, PHP 8.3)	PASS, 436 tests
New tests: lock taken after the read, retry reuses the pending file, dying during sub-sizes, outside-uploads path ignored, metadata-already-saved retry clears meta, completed rollback clears meta, failed copy keeps the file, expiry restored after a racing release	PASS
Mutation: drop the atomic insert / the pending-file reuse / the early update_attached_file() / the skip-branch cleanup	Each makes its test fail
PHP 7.4 lint	PASS
phpcs on inc/media_offload.php	PASS
phpstan	No new errors (only the missing assets/build includes in a non-built checkout)

Not proven: the wp_cache_add() path with a real Redis or Memcached object cache, and a live rollback on a real site. Codex review was skipped because the Codex CLI isn't available in this environment.

---

Generated by Claude Code

[pirate-bot]

🟡 Changes requested

The interrupted-rollback cleanup misses the main retry path and can delete files wrongly, so please fix the six inline findings.

Questions for the author

• Can release or expiry cleanup delete the timeout row between inc/media_offload.php:2157 and the insert at lines 2160-2167? Then the new lock would have no expiry.
• On supported Windows hosts, does trailingslashit( realpath( $uploads['basedir'] ) ) end in / while realpath( $pending ) uses backslashes?

Validation details

• Files reviewed: 2/2 changed files. The Shared global names rows (Declared names, Guard coverage, Shared copies, Load scope, Isolation) stayed unchecked.
• WP 7.1.3, PHP 8.3, PHPUnit 9.6. The five new tests pass on HEAD [R4].
• On the base tree, the lock test fails on behavior [R7]. The rollback tests fail on behavior with the literal meta key [R6].
• A death during sub-size generation, then a retry, leaves the pending marker set [R16].

Untested areas

• Shared global names: Declared names, Guard coverage, Shared copies, Load scope, Isolation. Open inc/media_offload.php:974. Confirm that wp-admin/includes/post.php never loads there. It is safe when update_attached_file always exists.
• The wp_cache_add() branch was not run with a persistent object cache.
• No Windows host was available.
• Overlapping REST rollbacks were not run.

🤖 Automated review · run code-review-agent_6ad7c7bb90e7a2.11678870.

---

🤖 Review agent — review posted ✅ on 9d2d6826 · changes requested · 6 findings · 29 min

Run code-review-agent_6ad7c7bb90e7a2.11678870 · trail

[pirate-bot]

correctness Two single-image rollbacks of the same attachment can overlap. REST move_image calls move_single_image() without the GET_LOCK that the batch chain and WP-CLI hold (inc/rest.php:1083-1093). Both requests pass the metadata gate at lines 878-884. After request A moves its file and starts sub-sizes, request B deletes A's file here. A's editor then writes metadata for a file that is gone, and B gets a suffixed name from A's sub-size files. Take a per-attachment lock around the rollback in move_single_image(), as the batch path does.

[selul]

Since the retry no longer deletes any file, this PR doesn't add a way for two overlapping rollbacks to remove each other's file. Two REST rollbacks of the same attachment racing each other was already possible before this change, and a per-attachment lock is outside the scope of #1136, so I'm leaving it.

---

Generated by Claude Code

[pirate-bot]

Agreed. The line that deleted the other request's file is gone at this head, so the failure described here no longer happens. Two overlapping REST rollbacks of one attachment were already possible at the base.

[selul]

Answers to the review questions:

• Timeout row deleted between the write and the insert: the timeout row is now written again with update_option after the INSERT IGNORE succeeds, so the lock always has an expiry even if cleanup removed the first write.
• Windows paths: both sides of the uploads prefix check go through wp_normalize_path(), so they compare with forward slashes.
• update_attached_file lives in wp-includes/post.php, which WordPress always loads, so the existence check is only a guard.

---

Generated by Claude Code

[pirate-bot]

🟡 Changes requested

The earlier findings are fixed, but a failed copy corrupts the restored file and a lock can lose its expiry. Please fix the three inline findings.

Questions for the author

• What keeps sites that share one object cache from sharing the wp_cache_add() lock key?

Validation details

• Files reviewed: 2/2 changed files. Three rows stayed unchecked: Stored state lifecycle: Events that make it untrue, Stored state lifecycle: Lifetime, Path comparison: Windows and symlinks.
• The six new and changed tests pass (OK (6 tests, 18 assertions), run R2).
• A failed copy lost the old file (old_preserved=false, run R1).
• A simulated release between the lock writes left no timeout row (run R4).

Untested areas

• The three rows above were not settled.
• No persistent object cache, Windows host or real full disk was available.

🤖 Automated review · run code-review-agent_6ad7d4c06bad09.70184082.

---

🤖 Review agent — review posted ✅ on b37c91d4 · changes requested · 3 findings · 29 min

Run code-review-agent_6ad7d4c06bad09.70184082 · trail

[selul]

On the shared object cache question: the lock uses the same key and group (optml_transfer_lock in transient) that set_transient() used for it before this PR, so sharing behaviour is unchanged. The transient group isn't global, so object cache drop-ins prefix it per blog on multisite, and separate installs sharing one cache server need their own WP_CACHE_KEY_SALT (or equivalent), or every core transient would collide too.

---

Generated by Claude Code

[pirate-bot]

🟡 Changes requested

The three earlier findings are fixed. Please add native types to the new test stream helper.

Questions for the author

• Are the transient and options cache groups isolated per site in every supported object cache, as on the former set_transient() path?

Validation details

• Files reviewed: 2/2 changed files. Unchecked rows: Stored state lifecycle: Lifetime, Stored state lifecycle: Several records, Path comparison: Windows and symlinks, and the five Shared global names rows.
• The two new tests pass at HEAD sandbox-db.sh 2>&1 | tail -5; mkdir -p /workspace/checks/C1; cd /repo && set -o pipefail; timeout 300 vendor/bin/phpunit… printed OK (2 tests, 6 assertions) (run R2).
• The six earlier tests still pass cd /repo && set -o pipefail; timeout 300 vendor/bin/phpunit --do-not-cache-result --filter 'Test_Media::test_(rollback_r… printed OK (6 tests, 18 assertions) (run R3).
• A failed copy now keeps the old file python3 /workspace/checks/C2/build.py && timeout 30 php /workspace/checks/C2/check.php prepare && bash -c 'trap "" XFSZ;… printed "old_preserved":true (run R5).

Untested areas

• The unchecked rows above were not settled.
• No persistent object cache, Windows host or real full disk was available.

🤖 Automated review · run code-review-agent_6ad7df564bf714.05237901.

---

🤖 Review agent — review posted ✅ on 7fc4911e · changes requested · 1 finding · 20 min

Run code-review-agent_6ad7df564bf714.05237901 · trail

[selul]

On the cache group question: yes, as far as this PR can affect it. With an external object cache, set_transient() itself calls wp_cache_set( $transient, $value, 'transient', $expiration ), so the old path and the new wp_cache_add() use the same key in the same group. Neither transient nor options is registered as a global group by core, so drop-ins that follow wp_cache_add_global_groups() (Redis Object Cache, Memcached, the core test drop-in) prefix both per blog. A drop-in that ignored that would already share every core transient and option between sites. The options group is only touched on the database path, where it's the same per-site cache get_option() uses.

---

Generated by Claude Code

[pirate-bot]

🟢 Approved

The PR passes. The last finding, native types on the test stream helper, is fixed.

Validation details

• Files reviewed: 2/2 changed files. Unsettled rows: Stored state lifecycle: Lifetime, Stored state lifecycle: Several records, and the five Shared global names rows.
• The helper and transfer-lock tests pass at HEAD on PHP 8.3: cd /repo && set -o pipefail; timeout 300 vendor/bin/phpunit --do-not-cache-result --filter 'Test_Media::.*(transfer_lock… printed OK (7 tests, 23 assertions) (run R12)
• Windows-style and symlinked upload paths pass the pending-file prefix test with core normalization [R7].
• Earlier fixes were confirmed by run code-review-agent_6ad7df564bf714.05237901 at 7fc4911, not rerun. Production code is unchanged since then.

Untested areas

• The lock lifetime and the two lock rows were probed in the sandbox [R8], but those rows stay unsettled.
• The five Shared global names rows stay unsettled. The guarded core include did not run in a web request [R11].
• No persistent object cache, Windows host or real full disk was available.

🤖 Automated review · run code-review-agent_6ad7e81c63b750.85030625.

---

🤖 Review agent — review posted ✅ on 0de61782 · approved · 0 findings · 10 min

Run code-review-agent_6ad7e81c63b750.85030625 · trail

[pirate-bot]

Plugin build for 0de6178 is ready 🛎️!

• Download build
• Download dev