Opened 2 years ago
Last modified 10 days ago
#61762 new defect (bug)
Options can be incorrectly cached following database errors.
| Reported by: | peterwilsoncc | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | Options, Meta APIs | Version: | |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: |
Description
Various wpdb methods can return falsey values to indicate one of two things:
0: zero effected rowsfalse: a database error
In get_option(), get_network_option(), delete_option() and delete_network_option() the results of the database query are imprecisely checked and can result in populated the notoptions cache incorrectly. Either by adding to the cache following a database error, or not adding to the cache when the zero rows are effected while deleting an option.
In add_network_option(), update_network_option() the notoptions cache is cleared prior to the database query without accounting for database errors. (This simply causes additional queries rather than incorrect data being returned.)
Related #61484, props @bjorsch for picking up.
Change History (7)
This ticket was mentioned in PR #12595 on WordPress/wordpress-develop by @gunjanjaswal.
3 months ago
#1
- Keywords has-patch has-unit-tests added
#2
@
3 months ago
Opened a PR for this: https://github.com/WordPress/wordpress-develop/pull/12595
It gates each notoptions write on the query being known to have succeeded: $wpdb->last_error on the read paths in get_option() and get_network_option(), and false !== $result on the delete paths. A zero-row delete still primes the cache, a failed query no longer does.
delete_network_option() was checking a truthy result, so a successful delete affecting zero rows never primed at all. That's fixed too.
I left the notoptions clearing in add_network_option() and update_network_option() alone for now, since the description notes it only costs extra queries rather than returning incorrect data. Happy to fold it into the same patch if you would rather have it all together.
Unit tests cover the read and delete paths, including the zero-row case.
#3
@
5 weeks ago
Got a bug report about this for my plugin (https://github.com/itmaybejj/editoria11y/issues/80) -- on a multi-site the first write tripped this, and then the fail state meant every subsequent admin page load tried to do it again, resulting in 1.2M duplicate DB rows for the network option because add_network_option() in this fail state does a raw INSERT rather than INSERT ... ON DUPLICATE KEY.
@gunjanjaswal commented on PR #12595:
5 weeks ago
#4
Motivation from the field: Trac #61762 just picked up a report of this hitting a multisite in production. The initial DB error poisoned notoptions for a network option, so every later admin load re-attempted the write, and add_network_option()'s raw INSERT piled up ~1.2M duplicate rows before it was caught (https://core.trac.wordpress.org/ticket/61762#comment:3, plugin report at https://github.com/itmaybejj/editoria11y/issues/80).
This PR fixes that at the root: gating the notoptions write on the query having actually succeeded means a transient error no longer marks a live option as nonexistent, so the recover-never re-write loop never starts. The raw-INSERT idempotency raised on the ticket is worth hardening separately as a follow-up.
#5
@
5 weeks ago
This is the failure mode the ticket is about, and the patch here (also up as a GitHub PR at WordPress/wordpress-develop#12595) fixes it at the root. The runaway writes are downstream of the caching bug: once the initial DB error makes get_network_option() treat the null from a failed query as "missing" and prime notoptions, every later request reads the option as nonexistent and re-attempts the write, so on a persistent object cache the site never recovers on its own.
The patch gates the notoptions write on the query having actually succeeded (last_error on the reads, and it clears notoptions in add_network_option()), so a transient error no longer poisons the cache and the re-write loop doesn't start.
The raw INSERT in add_network_option() that editoria11y flags is worth hardening on its own: making that write idempotent would bound the damage if any other path ever primes notoptions incorrectly. Happy to open a follow-up for that, but it's a distinct fix from the caching root cause this ticket tracks.
@gunjanjaswal commented on PR #12595:
2 weeks ago
#6
Gentle bump — this has been open a couple of months without a review. It's a focused fix so the options and metadata caches don't persist a "nonexistent" marker after a transient database error, which otherwise masks the real value until the cache is cleared. Happy to rebase or adjust anything if that makes it easier to pick up.
@gunjanjaswal commented on PR #12595:
10 days ago
#7
Circling back on this one. Happy to rebase it to refresh CI whenever that's useful — the lone red check is a stale July matrix job showing core's own PHP 8.2 deprecation warnings, not anything from this change. The fix itself is unchanged and focused: don't cache an option as "nonexistent" when the read actually failed on a database error, which otherwise sticks a false negative in the cache until it's flushed. There's since been a field report on Trac #61762 of this biting a multisite, so a first review whenever someone has a window would be much appreciated.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
$wpdb->get_row()returns null both when an option row is missing and when the query itself failed.get_option()andget_network_option()treated the second case as the first and primed thenotoptionscache, so a transient database error could hide an option that is still in the database. With a persistent object cache that state sticks around, and the option keeps returning its default long after the database has recovered.delete_option()had the same problem from the other direction: it primednotoptionsbefore checking the result of$wpdb->delete(), so a failed delete marked a still-present option as nonexistent.Each
notoptionswrite is now gated on the query being known to have succeeded, using$wpdb->last_erroron the read paths andfalse !== $resulton the delete paths.delete_network_option()was guarded on a truthy result, which meant a successful delete affecting zero rows never primed the cache. Zero rows is a successful query, so it primes there now too.Adds regression tests covering the read and delete paths, including the zero-row case.
Clearing
notoptionsinadd_network_option()andupdate_network_option()is left for a follow-up, per the ticket.