Modules::refresh() snapshot fast-path can leave a module upgrade stuck on stale info until a second refresh

[adrianbj]

Summary

After upgrading a module (e.g. via the ProcessWire Upgrade module), the admin reports the upgrade succeeded but the module's info (version, etc.) stays at the old value. It only corrects itself on a later, separate refresh — so the upgrade appears to "not take" on the first try.

Cause

Commit 5bb7f4f1 ("Add mtime snapshot fast-path to Modules::refresh()") added an optimization: ___refresh() builds a [mtime, size] snapshot of every module file and returns early — skipping the module-info rebuild — when the snapshot matches the previously cached one.

The problem is the ordering of events within a single module-upgrade request:

1. The upgrade replaces the module's files on disk, then calls $modules->refresh() in that same request (ModulesDownloader::unzipModule()).
2. For an update of an already-loaded module, the old class is already in memory, so the info rebuilt during this request can still reflect the old code.
3. That same refresh() then saves a file snapshot that already matches the new files on disk.
4. On the next request, refresh() sees the snapshot matches → takes the early return → never rebuilds → the stale info persists.

This is why it takes a second attempt/refresh for the upgrade to appear.

Reproduction

1. Install a module.
2. In one request: load the module, replace its files with a newer version, then call $modules->refresh() (this is what the upgrade flow does).
3. In subsequent requests, $modules->getModuleInfo() keeps reporting the old version; even a fresh-process refresh() returns early and never rebuilds.

Verified against a real install with a throwaway module: the reported version stayed stale across every subsequent request until the snapshot cache was manually cleared.

Expected

A module upgrade should be reflected on the next refresh, not require a second manual refresh.

Affected

• wire/core/Modules/Modules.php — ___refresh()
• Introduced in 5bb7f4f1 (2026-06-25).

A fix + regression test is proposed in processwire/processwire PR (linked below).

[matjazpotocnik]

I already reported this to Ryan a few days ago. I am not at the computer, but could you move $this->info->clearModuleInfoCache($showMessages); at the beginning of the ___refresh method, at least before comparing old and new snapshot version?

[adrianbj]

Thanks Matjaž. I looked at moving clearModuleInfoCache() to the top of ___refresh(), but I think it has an unintended cost: clearModuleInfoCache() isn't just a cache clear — at ModulesInfo.php:1106 it calls saveModuleInfoCache(), which re-reads every module's info from disk. That full rebuild is exactly what the mtime snapshot fast-path in 5bb7f4f1 was added to skip. If it runs before the snapshot comparison, it runs on every refresh(), so the early-return would only save the loadPath()/loaded() step and we'd lose most of the optimization.

It would fix the staleness, but functionally the same way the current patch does — both leave the upgrade request writing stale info (old class still in memory for modules that define info via the static getModuleInfo() rather than an external .info.php/.json), and both rely on the next refresh rebuilding. The committed patch instead invalidates the snapshot only when a module file changed during the current request (mtime ≥ REQUEST_TIME), so the fast-path is preserved for the normal case and the next refresh does a full rebuild.

Happy to go with your approach if you'd prefer the simpler code and think the extra rebuild cost is acceptable — but flagging the tradeoff since it largely reverts the fast-path you added. Let me know what you think.

[matjazpotocnik]

ryan, claude and gpt commited this feature, my version that i handed to ryan didnt have this bug, as far as i know. since i am on vacation and with limited time and internet access, i didnt check the implementation in details and cant help much...

EDIT: my version had the same bug!!!

[matjazpotocnik]

so i asked gpt 5.6 and this is what i got:

Verdict

The PR fixes the reported ProcessWire downloader scenario in the usual case, but I would not merge it as the final core solution without changes. Its central weakness is that it infers “files were replaced during this request” from filesystem timestamps. That is a heuristic, while the upgrade code already knows with certainty that it replaced module files.

Findings

1. Medium–high: correctness depends on destination mtimes

The fix only invalidates the snapshot when at least one module file has:

$mtime >= $_SERVER['REQUEST_TIME']

That works when ProcessWire copies extracted files into the destination and the resulting destination files receive the current timestamp. The current downloader does indeed extract to a temporary directory, copy into the module directory, and then call refresh() in the same request.

But the underlying rule is not reliable for every file-replacement path. It can miss the bug when:

an extraction or deployment preserves archive/source timestamps;

files are atomically moved from a prepared directory while retaining old mtimes;

a deployment tool explicitly restores timestamps;

server and filesystem clocks differ;

an external process replaces files during the request with older timestamps.

In those cases, the new snapshot is considered usable, saved as current, and the original stale-info bug remains possible.

The PR description promises detection of “a file changed during this request,” but it actually detects only “a file whose final mtime is no earlier than the request’s integer start time.” Those are not equivalent.

Recommendation: use an explicit invalidation signal from the code that replaces the files rather than infer it from mtime.

---

2. Medium: future-dated files can permanently disable the fast path

This condition:

if($mtimeSize[0] < $requestTime) continue;
$snapshotUsable = false;

also treats files with a future timestamp as having changed in every request until wall-clock time passes that timestamp.

A module file accidentally dated days or years into the future would cause every refresh() to:

1. skip the fast path;

2. clear and rebuild all module information;

3. save an empty snapshot again.

That is not a correctness failure, but it can turn the optimization off indefinitely for the entire installation because of one file.

---

3. Medium: the regression test does not verify the actual regression

The added test checks only:

an ordinary snapshot is stored;

after a mid-request write, the current snapshot is not stored.

It does not test the decisive part:

1. old module class is loaded;

2. module code is replaced;

3. same-request refresh produces potentially stale information;

4. next fresh request/process performs a full rebuild;

5. getModuleInfo() reports the new version;

6. the new snapshot is then saved normally.

Issue #2289 is specifically about stale module info surviving subsequent requests because the snapshot fast path returns early. The new test proves the implementation detail “snapshot becomes empty,” but not the externally observable fix.

At minimum, the test should simulate two requests or two separately bootstrapped ProcessWire processes. A single PHP process cannot unload the old class and therefore cannot prove that the next-request rebuild reads the new class.

---

4. Low–medium: CLI behavior is broader than necessary

The production code initializes:

$requestTime = isset($_SERVER['REQUEST_TIME'])
? (int) $_SERVER['REQUEST_TIME']
: 0;

$snapshotUsable = $requestTime > 0;

Therefore, in an environment without REQUEST_TIME, every refresh does a full rebuild and saves an empty snapshot. The next web request must then rebuild once more.

The PR calls this safe, which is true from a correctness perspective, but it is unnecessary. Absence of REQUEST_TIME does not imply files were replaced. It only means this particular heuristic cannot classify them.

A missing request timestamp should not itself invalidate an otherwise valid snapshot.

---

5. Low: the solution scans all module mtimes to infer information already known upstream

ModulesDownloader::unzipModule() knows exactly when it successfully copied replacement files, and immediately calls:

$this->modules->refresh();

afterward.

That is the strongest place to communicate “do not trust the snapshot produced by this refresh.” It avoids:

timestamp assumptions;

future timestamp problems;

behavior changes for unrelated refresh callers;

unnecessary special handling of CLI requests.

Better fix

I would add an explicit refresh option:

$this->modules->refresh(false, [
'deferSnapshot' => true,
]);

Conceptually:

public function ___refresh($showMessages = false, array $options = array()) {

$deferSnapshot = !empty($options['deferSnapshot']);

// Build module file list and snapshot...

if(!$deferSnapshot) {
	$oldSnapshot = $this->getCache($snapshotKey);

	if(is_array($oldSnapshot) && $oldSnapshot === $newSnapshot) {
		$this->refreshing = false;
		return;
	}
}

$this->installableFiles = array();
$this->info->clearModuleInfoCache($showMessages);

foreach($this->paths as $path) {
	$this->loader->loadPath($path);
}

if($this->duplicates()->numNewDuplicates() > 0) {
	$this->duplicates()->updateDuplicates();
}

$this->loader->loaded();

$this->saveCache(
	$snapshotKey,
	$deferSnapshot ? array() : $newSnapshot
);

$this->refreshing = false;

}

And in the downloader:

$this->modules->refresh(false, [
'deferSnapshot' => true,
]);

An even smaller targeted change would be:

$this->modules->refresh();
$this->modules->saveCache('moduleFileSnapshot', array());

but exposing direct cache manipulation from ModulesDownloader is less clean.

Best long-term design: pending snapshot

The most rigorous solution remains a pending-state mechanism:

same-request upgrade rebuilds module info and saves the new snapshot as pending;

next request sees pending state and must rebuild;

after the fresh rebuild, it promotes the pending snapshot to current.

That is more stateful, but it formally represents what is happening instead of encoding it as an empty array.

Merge recommendation

Request changes.

The current PR is a reasonable tactical patch and probably fixes the standard web-based module-upgrade flow, but I would specifically request:

1. replace the mtime/request-time heuristic with explicit snapshot invalidation from ModulesDownloader;

2. add an end-to-end regression test proving that the next fresh process reports the new module version;

3. avoid globally disabling snapshot caching when REQUEST_TIME is unavailable;

4. add a test for preserved or old destination mtimes.

The key review comment could be:

The stale-snapshot diagnosis is correct, but mtime >= REQUEST_TIME is not a reliable indication that files were replaced in this request. File replacement can preserve older timestamps, while future-dated files would disable the fast path indefinitely. Since ModulesDownloader knows that it has just replaced module files, could it explicitly request a deferred/invalidated snapshot instead? The regression test should also bootstrap a fresh process and verify that the new module version is actually read, rather than only checking that the snapshot was emptied.

[adrianbj]

The review makes a couple of good points that I've now addressed, but the central objection is weaker than it looks.

On mtime reliability (point 1): the scenarios listed (rsync/deploy tools preserving timestamps, atomic moves) generally can't trigger this bug, because the bug requires files to be replaced within the same PHP process that then calls refresh() with the old class already loaded. Out-of-band deployments are handled by the existing snapshot comparison on the next request — the preserved mtimes still won't match what the snapshot recorded, so a full rebuild happens in a fresh process. For the in-process paths, both core install flows (ModulesDownloader and ProcessModuleInstall) extract to a temp dir and $files->copy() into place, and PHP's copy() always stamps destinations with the current time — even on PHP 8.3+, where ZipArchive::extractTo() preserving archive mtimes only affects the temp dir.

On the proposed deferSnapshot alternative (point 5): it has the mirror-image weakness, with narrower coverage. It only protects callers that know to pass the option — the review names ModulesDownloader, but ProcessModuleInstall::uploadModule() has the identical replace-then-refresh pattern and would need the same treatment, as would any third-party module that replaces files and calls $modules->refresh(). The mtime check protects all of those without each caller having to opt in. It also requires adding an options parameter to hookable ___refresh(), which is a public API change beyond the scope of a bugfix — though I'd have no objection if Ryan prefers an explicit signal in addition to the heuristic.

On future-dated files (point 2): valid — a file with a far-future mtime would have disabled the fast path indefinitely (safe, but silently loses the optimization). Fixed: a file's mtime now only invalidates the snapshot if it falls within [REQUEST_TIME, REQUEST_TIME + 86400]; anything further ahead is treated as a bad timestamp rather than "written during this request." Added a test case covering this (far-future mtime must not prevent the snapshot from being saved).

On REQUEST_TIME availability (point 4): PHP sets $_SERVER['REQUEST_TIME'] in all SAPIs including CLI (where it's the process start time), so the "environment without REQUEST_TIME" is essentially hypothetical and the > 0 guard is just a safety fallback for exotic environments. The PR description mis-stated this; corrected.

On end-to-end verification (point 3): agreed that the in-process test can only verify the invalidation mechanism — a single PHP process can't unload the old class, which the review itself notes. So I verified the cross-request behavior against a real installation, using three separately bootstrapped PHP processes: (1) install a test module at version 1, (2) simulate the upgrade request — load the old class, replace the module file with version 2, call refresh() in the same process, exactly as ModulesDownloader does, (3) in a fresh process, call refresh() and read getModuleInfo().

Results:

• Pre-fix core (3.0.268): step 3 reports version 1, and keeps reporting version 1 on every subsequent request — including repeated explicit refresh() calls, since the snapshot matches disk and refresh() returns early. The stale info is stuck permanently. That's this issue reproduced.
• With the fix: the upgrade request saves an invalidated (empty) snapshot, so the next refresh() does a full rebuild in a fresh process and reports version 2. The snapshot is then repopulated normally and stays stable on subsequent refreshes.

One clarification this surfaced, for precision: the fix takes effect at the next refresh() call (Modules > Refresh, or anything else that triggers one) rather than spontaneously on the next page load — but with the pre-fix core even explicit refreshes never recovered, which is the reported bug.

[matjazpotocnik]

Final verdict: Approve.

I no longer think an explicit deferSnapshot option should replace this implementation. The heuristic is well matched to the specific same-process failure mode and provides broader protection without an API change.
The only changes I would still suggest are documentation-level:

• Update the PR description, which currently still appears to state the original unbounded mtime >= REQUEST_TIME behavior and the inaccurate CLI explanation. The code has moved beyond that description.

• Add the three-process manual verification to the testing notes. Avoid claiming that preserved-mtime deployments must always mismatch the snapshot; say they normally do and, more importantly, do not ordinarily produce the same loaded-class condition.

A suitable updated review conclusion would be:

The bounded mtime check is a reasonable generic solution to this specific same-process upgrade problem. It covers both core installation paths and third-party replace-then-refresh callers without requiring a public API change or caller opt-in. The far-future timestamp case is now bounded and tested, and the separate-process verification demonstrates that invalidating the snapshot allows the next request to rebuild the correct module information. Approved.