Keep WordPress auto-updates on for sites created before the setting existed - #4715
Keep WordPress auto-updates on for sites created before the setting existed#4715gcsecsey wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI’s WordPress dependency management so that creating an “auto-updating” (latest) site uses the current WordPress release at creation time, rather than potentially using a stale cached wordpress-versions/latest directory.
Changes:
- Replace the “latest” resolution logic to always fetch the currently reported latest version from wordpress.org and treat missing/failed resolution as an error.
- Reduce the dependency refresh throttle window from 24 hours to 1 hour and avoid persisting the throttle timestamp when the update fails.
- Add a filesystem lock around
wordpress-versions/access and extend test coverage for the new behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/cli/lib/dependency-management/wordpress.ts | Simplifies “latest” version resolution and makes inability to determine latest an explicit error. |
| apps/cli/lib/dependency-management/tests/wordpress.test.ts | Adds focused tests for latest-version resolution and update behaviors. |
| apps/cli/lib/dependency-management/tests/setup.test.ts | Updates throttling expectations and ensures timestamp isn’t persisted on failure. |
| apps/cli/lib/dependency-management/setup.ts | Changes throttle interval to 1 hour and applies locking around WordPress version refresh/setup. |
| apps/cli/lib/dependency-management/lock.ts | Introduces a lockfile-based critical section for wordpress-versions/ operations. |
| apps/cli/commands/site/tests/create.test.ts | Mocks the new lock helper for site creation tests. |
| apps/cli/commands/site/create.ts | Uses the new lock to prevent concurrent reads/writes of cached WordPress versions during site creation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Far more patient than the config-file locks: the guarded work downloads and | ||
| // copies a full WordPress release, which takes minutes on a slow connection. | ||
| const STALE_TIME = 10 * 60 * 1000; | ||
| const WAIT_TIME = 2 * 60 * 1000; |
| // Locked because `wordpress-versions/latest` is shared mutable state: a | ||
| // concurrent refresh rewrites it in place while this copies out of it. | ||
| await withWordPressVersionsLock( async () => { | ||
| if ( options.wpVersion === 'latest' ) { | ||
| const bundledWPPath = path.join( getServerFilesPath(), 'wordpress-versions', 'latest' ); |
📊 Performance Test ResultsComparing 21af72c vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
apps/cli/lib/dependency-management/lock.ts:24
- The lock can become stale while its owner is still legitimately downloading. The guarded operation is explicitly expected to take minutes, but nothing refreshes the lock mtime; after 10 minutes another process may break the lock and mutate/copy
latestconcurrently, recreating the partial-directory race this lock is intended to prevent. Add a heartbeat for the duration ofrun(aswithSessionsMigrationLockdoes inpackages/common/ai/sessions/root-migration.ts:205-220) and clear it before unlocking.
await lockFileAsync( lockfilePath, { stale: STALE_TIME, wait: WAIT_TIME } );
| * by `DEPENDENCY_CHECK_INTERVAL_MS`. Returns true if the check ran and succeeded, | ||
| * false if it was skipped or failed. |
| // Not fatal — the site still gets the cached copy. Reported in production | ||
| // too, or an auto-updating site starts on an older release for no visible | ||
| // reason. | ||
| logger.reportError( new LoggerError( 'Failed to update dependencies', error ), false ); |
| * Serialize access to `wordpress-versions/`. The `latest` directory is shared | ||
| * mutable state: a refresh rewrites it in place, while a site create may be | ||
| * copying files out of it at the same time. Without this, a create started | ||
| * just as a new WordPress release lands can copy a half-written directory. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/cli/commands/site/create.ts:672
updateServerFiles()now catches lookup/update failures and resolvesfalse, so this catch never receives the failure and the new productionlogger.reportErroris not called. The failure is indistinguishable from a throttled skip, contrary to the stated behavior that site creation reports “Failed to update dependencies.” Preserve the no-timestamp behavior, but propagate an error or return a distinct failure result that this caller reports.
logger.reportError( new LoggerError( 'Failed to update dependencies', error ), false );
apps/cli/lib/dependency-management/lock.ts:19
- This lock does not cover every reader of the guarded directory.
apps/cli/commands/import.ts:47-60copieswordpress-versions/latestwithoutwithWordPressVersionsLock, so a concurrent refresh can still rewrite the source while an import copies it. Guard that import path with the same lock and add coverage for it.
* Serialize access to `wordpress-versions/`. The `latest` directory is shared
* mutable state: a refresh rewrites it in place, while a site create may be
* copying files out of it at the same time. Without this, a create started
* just as a new WordPress release lands can copy a half-written directory.
| export async function withWordPressVersionsLock< T >( run: () => Promise< T > ): Promise< T > { | ||
| const lockfilePath = getLockfilePath(); | ||
| await mkdir( path.dirname( lockfilePath ), { recursive: true } ); | ||
| await lockFileAsync( lockfilePath, { stale: STALE_TIME, wait: WAIT_TIME } ); |
Related issues
How AI was used in this PR
I used Opus 5 for implementation and tests.
Proposed Changes
Studio decides whether a site auto-updates from
isWpAutoUpdating. If this was unset, the settings UI read it as auto-updating and showed "latest", while the mu-plugin writer read it as falsy and installed0-disable-auto-updates.php. Sites created before the flag existed have no value, so those sites had auto-updates switched off while the UI showed the opposite.This PR updates this logic so only an explicit
falsedisables auto-updates now.wordpress-versions/latestdirectory rather than downloading a numbered release, so the cache has to be verified before it is used. Pinned versions keep the throttle, since they download by exact version and cannot go stale.Testing Instructions
The fix: an unset flag must mean auto-updating
Make it look like a site created before the flag existed, then restart it so the mu-plugins are rewritten:
Check which auto-update mu-plugin Studio wrote:
Expected
['0-enable-auto-updates.php']. On trunk it prints['0-disable-auto-updates.php'], which is the bug: Settings shows "latest" for that same site.Quit the desktop app before the
cli.jsonedit, and clean up with$S site delete --path ~/Studio/stu2348.Creating a "latest" site must check the cache first
Make the cached copy look stale, record a recent check so the throttle would normally skip, then create a site on "latest":
Expected the current release from https://wordpress.org/download/releases/. On trunk it prints
0.1, because the throttle skipped the check and the site was built from the stale cache.Then confirm a pinned version still respects the throttle, so this only changed the path that needed it:
Expected no
Dependencies updatedline, and a site on 6.9.7.The fake version is
0.1on purpose. The refresh keeps the outgoing release under the version number it reports, so a real-looking number would overwrite a genuine cached directory. Clean up with$S site deletefor both, andrm -rf ~/.studio/server-files/wordpress-versions/0.1. On Linux usesed -iwithout the''.Pre-merge Checklist